GTIndex improvements and GTRepository+Status tweaks #622

slavikus · 2017-03-16T13:59:03Z

(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

pietbrauer · 2017-03-27T18:20:10Z

ObjectiveGit/GTIndex.m

@@ -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)


Could you add braces here?

pietbrauer · 2017-03-27T18:20:43Z

ObjectiveGit/GTIndex.m

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

-		GTIndexEntry *blockAncestor;
+		GTIndexEntry *blockAncestor = nil;


This is default behavior and could be omitted.

pietbrauer · 2017-03-27T18:20:56Z

ObjectiveGit/GTIndex.m

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

-		GTIndexEntry *blockOurs;
+		GTIndexEntry *blockOurs = nil;


This is default behavior and could be omitted.

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

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.

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.".

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

pietbrauer · 2017-03-27T18:21:02Z

ObjectiveGit/GTIndex.m

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

-		GTIndexEntry *blockTheirs;
+		GTIndexEntry *blockTheirs = nil;
 		if (theirs != NULL) {


This is default behavior and could be omitted.

pietbrauer · 2017-03-27T18:22:44Z

ObjectiveGit/GTRepository+Status.h

-/// 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) {


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

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.

I see, cool.

pietbrauer · 2017-03-27T18:24:13Z

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

tiennou · 2017-03-27T21:01:03Z

ObjectiveGit/GTIndex.h

+
+/// 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) {


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

tiennou · 2017-03-27T21:06:05Z

ObjectiveGit/GTIndex.h

+/// 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


"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"

tiennou · 2017-03-27T21:10:14Z

ObjectiveGit/GTIndex.m

@@ -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.");


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 ?

tiennou · 2017-03-27T21:19:12Z

ObjectiveGit/GTIndex.m

+		.shouldAbortImmediately = NO,
+	};
+
+	int returnCode = git_index_add_all(self.git_index, &strarray, (unsigned int)flags, (block != nil ? GTIndexPathspecMatchFound : NULL), &payload);


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 · 2017-03-27T21:22:22Z

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

…pecs

tiennou

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 !

tiennou · 2018-08-17T20:56:19Z

ObjectiveGit/GTIndex.h

@@ -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;


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

slavikus added 2 commits March 16, 2017 16:55

Numerous GTIndex improvements

8a68584

- Added GTIndex.checksum accessor - Added addPathspecs/removePathspaces calls. - Fixed proper nullability for enumerateConflictedFiles

GTRepository+Status improvements

c7fcb63

- Converted GTRepositoryStatusFlags into NS_OPTIONS for proper handling from Swift - Added missing flags for GTFileStatusFlags

pietbrauer reviewed Mar 27, 2017

View reviewed changes

tiennou approved these changes Mar 27, 2017

View reviewed changes

slavikus added 2 commits March 29, 2017 11:30

Merge remote-tracking branch 'upstream/master' into gtindex-add-paths…

6088ef6

…pecs

Code review feedback fixes for PR libgit2#622

d497586

tiennou requested changes Aug 17, 2018

View reviewed changes

tiennou added test needed enhancement labels Mar 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GTIndex improvements and GTRepository+Status tweaks #622

GTIndex improvements and GTRepository+Status tweaks #622

slavikus commented Mar 16, 2017

pietbrauer Mar 27, 2017

slavikus Mar 27, 2017

pietbrauer Mar 27, 2017

pietbrauer Mar 27, 2017

slavikus Mar 27, 2017

mcyril Mar 27, 2017

pietbrauer Mar 27, 2017

mcyril Mar 27, 2017

pietbrauer Mar 27, 2017

pietbrauer Mar 27, 2017

slavikus Mar 27, 2017

pietbrauer Mar 27, 2017

pietbrauer commented Mar 27, 2017

tiennou Mar 27, 2017

tiennou Mar 27, 2017

tiennou Mar 27, 2017

tiennou Mar 27, 2017

tiennou commented Mar 27, 2017

tiennou left a comment

tiennou Aug 17, 2018

GTIndex improvements and GTRepository+Status tweaks #622

Are you sure you want to change the base?

GTIndex improvements and GTRepository+Status tweaks #622

Conversation

slavikus commented Mar 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietbrauer commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Mar 27, 2017

tiennou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment