Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GTIndex improvements and GTRepository+Status tweaks #622

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

slavikus
Copy link
Contributor

(Credit for this pull request goes to @mcyril)

  • Added GTIndex.checksum accessor
  • Added addPathspecs/removePathspaces calls.
  • Fixed proper nullability for enumerateConflictedFiles
  • Converted GTRepositoryStatusFlags into NS_OPTIONS for proper handling from Swift
  • Added missing flags for GTFileStatusFlags

- Added GTIndex.checksum accessor
- Added addPathspecs/removePathspaces calls.
- Fixed proper nullability for enumerateConflictedFiles
- Converted GTRepositoryStatusFlags into NS_OPTIONS for proper handling from Swift
- Added missing flags for GTFileStatusFlags
@@ -121,6 +121,14 @@ - (id)initWithGitIndex:(git_index *)index repository:(GTRepository *)repository

#pragma mark Entries

- (GTOID *)checksum {
const git_oid *oid = git_index_checksum(self.git_index);
if (oid != NULL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add braces here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@@ -308,17 +316,17 @@ - (BOOL)enumerateConflictedFilesWithError:(NSError **)error usingBlock:(void (^)
return NO;
}

GTIndexEntry *blockAncestor;
GTIndexEntry *blockAncestor = nil;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is default behavior and could be omitted.

if (ancestor != NULL) {
blockAncestor = [[GTIndexEntry alloc] initWithGitIndexEntry:ancestor];
}

GTIndexEntry *blockOurs;
GTIndexEntry *blockOurs = nil;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is default behavior and could be omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the compiler makes it easier by auto-nulling new declarations, I don't think an implicit = nil makes any harm. :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slava, that's unrelated. the rationale is pretty simple (kindergarten rule) — compiler doesn't nullify local (stack allocated) scalars in C-derived languages, there is no such a thing in C as "default behaviour" which zeroes locals. explicit zeroing will prevent propagating of rubbish in case when the next condition is false. that's must have.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't care too much about this, but in the ARC transitioning release notes it reads "Using ARC, strong, weak, and autoreleasing stack variables are now implicitly initialized with nil.".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, nice, I always forget about ARC. then it could be omitted, but explicit nullify looks more semantically clean

if (ours != NULL) {
blockOurs = [[GTIndexEntry alloc] initWithGitIndexEntry:ours];
}

GTIndexEntry *blockTheirs;
GTIndexEntry *blockTheirs = nil;
if (theirs != NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is default behavior and could be omitted.

/// See status.h for documentation of each individual flag.
typedef enum {
/// See status.h for documentation of each individual git_status_opt_t flag.
typedef NS_OPTIONS(NSInteger, GTRepositoryStatusFlags) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be NS_ENUM, if it was a enum before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, NS_ENUM implies a single enumeration whereas NS_OPTIONS allow multiple values, especially when it bridges stuff to Swift, so you can do stuff like [ .includeUntracked, .includeIgnored ] natively. By the meaning of the enumeration, it's really an options bitfield, hence the choice of NS_OPTIONS over NS_ENUM.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, cool.

@pietbrauer
Copy link
Member

Tests for this would be really nice. @tiennou do you have any additional thoughts?


/// An enum for use with addPathspecs:flags:error:passingTest: below
/// See index.h for documentation of each individual git_index_add_option_t flag.
typedef NS_OPTIONS(NSInteger, GTIndexAddOptionFlags) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Maybe just GTIndexAddOptions ? That would make for more consistent wordings (e.g. "A combination of GTIndexAddOptionFlags flags" below).

/// pathspecs - An `NSString` array of path patterns. (E.g: *.c)
/// If nil is passed in, all index entries will be updated.
/// flags - A combination of GTIndexAddOptionFlags flags
/// block - A block run each time a pathspec is matched; before the index is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"A block that will be called on each match, before the index is changed."
"what the pathspec" => "which pathspec".
"If you pass in NULL nil"

@@ -336,10 +344,58 @@ - (BOOL)enumerateConflictedFilesWithError:(NSError **)error usingBlock:(void (^)
BOOL shouldAbortImmediately;
};

- (BOOL)addPathspecs:(NSArray *)pathspecs flags:(GTIndexAddOptionFlags)flags error:(NSError **)error passingTest:(GTIndexPathspecMatchedBlock)block {
NSAssert(self.repository.isBare == NO, @"This method only works with non-bare repositories.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is up for debate, but I feel like an assert is too much ? As someone who writes a GUI (where people can open mostly anything), I'd prefer to get an error back instead of exit.

What does libgit2 do in that case, return GIT_EBARE ?

.shouldAbortImmediately = NO,
};

int returnCode = git_index_add_all(self.git_index, &strarray, (unsigned int)flags, (block != nil ? GTIndexPathspecMatchFound : NULL), &payload);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it's not from here, but GTIndexPathspecMatchFound sounds strange (I was wondering if it was a snuck-in flag or something).

Can you add Callback to the function name ?

@tiennou
Copy link
Contributor

tiennou commented Mar 27, 2017

Thanks for the PR, a few nitpicks but 👍 overall. Some tests would be ✨ though.

Copy link
Contributor

@tiennou tiennou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long wait @slavikus. I'd be interested to merge that in but I've just found a minor issue. You should be able to merge master in, and this would be good to go !

@@ -45,6 +47,9 @@ NS_ASSUME_NONNULL_BEGIN
/// The file URL for the index if it exists on disk; nil otherwise.
@property (nonatomic, readonly, copy) NSURL * _Nullable fileURL;

/// The index checksum
@property (nonatomic, readonly, strong) GTOID * _Nullable checksum;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Nullable is not necessary here : the underlying git_oid is part of the git_index struct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants