From 05666399ce48bdde6c442a58df5e8ec4a9ac9197 Mon Sep 17 00:00:00 2001 From: Dan Leehr Date: Mon, 17 Aug 2015 22:00:19 -0400 Subject: [PATCH 1/6] Fix checkout methods utilizing notifyBlock/GTCheckoutStrategySafe Currently, the performCheckout methods move HEAD to the new ref/commit before calling git_checkout_head. This is incorrect behavior. When git_checkout_head is called, it reports changes between the workdir and HEAD per-file to the notifyBlock, allowing the block to abort the checkout based on a reason (dirty/conflict/etc). However, since HEAD has already been moved to a different ref/commit, the notifyBlock is notified about changes between the current workdir and the requested ref/commit. This results in a dirty state for any file that differs between the old ref and the new ref, regardless of whether or not any change was made in the workdir. The correct behavior (e.g. with GTCheckoutStrategySafe) is to notify of changes that will cause data loss in the workdir. This change leaves HEAD in position until after the checkout, correcting the checkout behavior. When GTCheckoutStrategySafe is used, notifyBlock is only invoked as expected (e.g. dirty files that would be overwritten on checkout). Also, HEAD is only moved if the checkout succeeds, preventing other confusing behavior. --- ObjectiveGit/GTRepository.m | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/ObjectiveGit/GTRepository.m b/ObjectiveGit/GTRepository.m index 12eb3a315..fc8474e1c 100644 --- a/ObjectiveGit/GTRepository.m +++ b/ObjectiveGit/GTRepository.m @@ -830,17 +830,15 @@ - (BOOL)performCheckoutWithStrategy:(GTCheckoutStrategyType)strategy notifyFlags } - (BOOL)checkoutCommit:(GTCommit *)targetCommit strategy:(GTCheckoutStrategyType)strategy notifyFlags:(GTCheckoutNotifyFlags)notifyFlags error:(NSError **)error progressBlock:(GTCheckoutProgressBlock)progressBlock notifyBlock:(GTCheckoutNotifyBlock)notifyBlock { - BOOL success = [self moveHEADToCommit:targetCommit error:error]; + BOOL success = [self performCheckoutWithStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; if (success == NO) return NO; - - return [self performCheckoutWithStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; + return [self moveHEADToCommit:targetCommit error:error]; } - (BOOL)checkoutReference:(GTReference *)targetReference strategy:(GTCheckoutStrategyType)strategy notifyFlags:(GTCheckoutNotifyFlags)notifyFlags error:(NSError **)error progressBlock:(GTCheckoutProgressBlock)progressBlock notifyBlock:(GTCheckoutNotifyBlock)notifyBlock { - BOOL success = [self moveHEADToReference:targetReference error:error]; + BOOL success = [self performCheckoutWithStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; if (success == NO) return NO; - - return [self performCheckoutWithStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; + return [self moveHEADToReference:targetReference error:error]; } - (BOOL)checkoutCommit:(GTCommit *)target strategy:(GTCheckoutStrategyType)strategy error:(NSError **)error progressBlock:(GTCheckoutProgressBlock)progressBlock { From 0e2ba1089c4f9ff96d52075003bbc383d3c1ce19 Mon Sep 17 00:00:00 2001 From: Dan Leehr Date: Tue, 18 Aug 2015 21:58:37 -0400 Subject: [PATCH 2/6] Add tests for checkout+notify --- ObjectiveGitTests/GTRepositorySpec.m | 63 ++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/ObjectiveGitTests/GTRepositorySpec.m b/ObjectiveGitTests/GTRepositorySpec.m index 82b528f40..6c14c27ba 100644 --- a/ObjectiveGitTests/GTRepositorySpec.m +++ b/ObjectiveGitTests/GTRepositorySpec.m @@ -14,6 +14,10 @@ QuickSpecBegin(GTRepositorySpec) +static NSString * const mainFile = @"main.m"; +static NSString * const readmeFile = @"README1.txt"; + + __block GTRepository *repository; beforeEach(^{ @@ -376,6 +380,65 @@ }); }); +describe(@"-checkout:strategy:notifyFlags:error:notifyBlock:progressBlock:", ^{ + it(@"should notify dirty file and stop ref checkout", ^{ + NSError *error = nil; + GTReference *ref = [repository lookUpReferenceWithName:@"refs/heads/other-branch" error:&error]; + expect(ref).notTo(beNil()); + expect(error.localizedDescription).to(beNil()); + // Write something to a file in the repo + BOOL writeResult = [@"Replacement data in main.m\n" writeToURL:[repository.fileURL URLByAppendingPathComponent:mainFile] atomically:YES encoding:NSUTF8StringEncoding error:&error]; + expect(@(writeResult)).to(beTruthy()); + // Now attempt to checkout the other-branch + __block NSUInteger notifyCount = 0; + __block BOOL mainFileDirty = NO; + int (^notifyBlock)(GTCheckoutNotifyFlags, NSString *, GTDiffFile *, GTDiffFile *, GTDiffFile *); + notifyBlock = ^(GTCheckoutNotifyFlags why, NSString *path, GTDiffFile *baseline, GTDiffFile *target, GTDiffFile *workdir) { + notifyCount++; + if([path isEqualToString:mainFile] && (why & GTCheckoutNotifyDirty)) { + mainFileDirty = YES; + return GIT_EUSER; + } else { + return 0; + } + }; + + BOOL result = [repository checkoutReference:ref strategy:GTCheckoutStrategySafe notifyFlags:GTCheckoutNotifyDirty error:&error progressBlock:nil notifyBlock:notifyBlock]; + expect(@(notifyCount)).to(equal(@(1))); + expect(@(mainFileDirty)).to(beTruthy()); + expect(@(result)).to(beFalsy()); + expect(@(error.code)).to(equal(@(GIT_EUSER))); + }); + + it(@"should notify dirty file and stop commit checkout", ^{ + NSError *error = nil; + GTCommit *commit = [repository lookUpObjectBySHA:@"1d69f3c0aeaf0d62e25591987b93b8ffc53abd77" objectType:GTObjectTypeCommit error:&error]; + expect(commit).notTo(beNil()); + expect(error.localizedDescription).to(beNil()); + BOOL writeResult = [@"Replacement data in README\n" writeToURL:[repository.fileURL URLByAppendingPathComponent:readmeFile] atomically:YES encoding:NSUTF8StringEncoding error:&error]; + expect(@(writeResult)).to(beTruthy()); + // Now attempt to checkout the other-branch + __block NSUInteger notifyCount = 0; + __block BOOL readmeFileDirty = NO; + int (^notifyBlock)(GTCheckoutNotifyFlags, NSString *, GTDiffFile *, GTDiffFile *, GTDiffFile *); + notifyBlock = ^(GTCheckoutNotifyFlags why, NSString *path, GTDiffFile *baseline, GTDiffFile *target, GTDiffFile *workdir) { + notifyCount++; + if([path isEqualToString:readmeFile] && (why & GTCheckoutNotifyDirty)) { + readmeFileDirty = YES; + return GIT_EUSER; + } else { + return 0; + } + }; + + BOOL result = [repository checkoutCommit:commit strategy:GTCheckoutStrategySafe notifyFlags:GTCheckoutNotifyDirty error:&error progressBlock:nil notifyBlock:notifyBlock]; + expect(@(notifyCount)).to(equal(@(1))); + expect(@(readmeFileDirty)).to(beTruthy()); + expect(@(result)).to(beFalsy()); + expect(@(error.code)).to(equal(@(GIT_EUSER))); + }); +}); + describe(@"-remoteNamesWithError:", ^{ it(@"allows access to remote names", ^{ NSError *error = nil; From 3c173056086265f6092447607cf9c2c714fd1d7f Mon Sep 17 00:00:00 2001 From: Dan Leehr Date: Tue, 18 Aug 2015 22:31:18 -0400 Subject: [PATCH 3/6] Cleanup checkout+notify tests --- ObjectiveGitTests/GTRepositorySpec.m | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/ObjectiveGitTests/GTRepositorySpec.m b/ObjectiveGitTests/GTRepositorySpec.m index 6c14c27ba..32d8bbef9 100644 --- a/ObjectiveGitTests/GTRepositorySpec.m +++ b/ObjectiveGitTests/GTRepositorySpec.m @@ -381,15 +381,13 @@ }); describe(@"-checkout:strategy:notifyFlags:error:notifyBlock:progressBlock:", ^{ - it(@"should notify dirty file and stop ref checkout", ^{ + it(@"should fail ref checkout with dirty file and notify", ^{ NSError *error = nil; GTReference *ref = [repository lookUpReferenceWithName:@"refs/heads/other-branch" error:&error]; expect(ref).notTo(beNil()); expect(error.localizedDescription).to(beNil()); - // Write something to a file in the repo BOOL writeResult = [@"Replacement data in main.m\n" writeToURL:[repository.fileURL URLByAppendingPathComponent:mainFile] atomically:YES encoding:NSUTF8StringEncoding error:&error]; expect(@(writeResult)).to(beTruthy()); - // Now attempt to checkout the other-branch __block NSUInteger notifyCount = 0; __block BOOL mainFileDirty = NO; int (^notifyBlock)(GTCheckoutNotifyFlags, NSString *, GTDiffFile *, GTDiffFile *, GTDiffFile *); @@ -410,14 +408,13 @@ expect(@(error.code)).to(equal(@(GIT_EUSER))); }); - it(@"should notify dirty file and stop commit checkout", ^{ + it(@"should fail commit checkout with dirty file and notify", ^{ NSError *error = nil; GTCommit *commit = [repository lookUpObjectBySHA:@"1d69f3c0aeaf0d62e25591987b93b8ffc53abd77" objectType:GTObjectTypeCommit error:&error]; expect(commit).notTo(beNil()); expect(error.localizedDescription).to(beNil()); BOOL writeResult = [@"Replacement data in README\n" writeToURL:[repository.fileURL URLByAppendingPathComponent:readmeFile] atomically:YES encoding:NSUTF8StringEncoding error:&error]; expect(@(writeResult)).to(beTruthy()); - // Now attempt to checkout the other-branch __block NSUInteger notifyCount = 0; __block BOOL readmeFileDirty = NO; int (^notifyBlock)(GTCheckoutNotifyFlags, NSString *, GTDiffFile *, GTDiffFile *, GTDiffFile *); From c1e58a64c6206608c6d5c27ebeb88f8f75a41af0 Mon Sep 17 00:00:00 2001 From: Dan Leehr Date: Wed, 19 Aug 2015 13:45:41 -0400 Subject: [PATCH 4/6] Rework checkout methods to checkout the target treeish instead of head --- ObjectiveGit/GTRepository.m | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/ObjectiveGit/GTRepository.m b/ObjectiveGit/GTRepository.m index fc8474e1c..f15a1de98 100644 --- a/ObjectiveGit/GTRepository.m +++ b/ObjectiveGit/GTRepository.m @@ -809,7 +809,7 @@ - (BOOL)moveHEADToCommit:(GTCommit *)commit error:(NSError **)error { return gitError == GIT_OK; } -- (BOOL)performCheckoutWithStrategy:(GTCheckoutStrategyType)strategy notifyFlags:(GTCheckoutNotifyFlags)notifyFlags error:(NSError **)error progressBlock:(GTCheckoutProgressBlock)progressBlock notifyBlock:(GTCheckoutNotifyBlock)notifyBlock { +- (BOOL)performCheckout:(GTObject *)target withStrategy:(GTCheckoutStrategyType)strategy notifyFlags:(GTCheckoutNotifyFlags)notifyFlags error:(NSError **)error progressBlock:(GTCheckoutProgressBlock)progressBlock notifyBlock:(GTCheckoutNotifyBlock)notifyBlock { git_checkout_options checkoutOptions = GIT_CHECKOUT_OPTIONS_INIT; @@ -821,7 +821,7 @@ - (BOOL)performCheckoutWithStrategy:(GTCheckoutStrategyType)strategy notifyFlags checkoutOptions.notify_flags = notifyFlags; checkoutOptions.notify_payload = (__bridge void *)notifyBlock; - int gitError = git_checkout_head(self.git_repository, &checkoutOptions); + int gitError = git_checkout_tree(self.git_repository, target.git_object, &checkoutOptions); if (gitError < GIT_OK) { if (error != NULL) *error = [NSError git_errorFor:gitError description:@"Failed to checkout tree."]; } @@ -830,13 +830,18 @@ - (BOOL)performCheckoutWithStrategy:(GTCheckoutStrategyType)strategy notifyFlags } - (BOOL)checkoutCommit:(GTCommit *)targetCommit strategy:(GTCheckoutStrategyType)strategy notifyFlags:(GTCheckoutNotifyFlags)notifyFlags error:(NSError **)error progressBlock:(GTCheckoutProgressBlock)progressBlock notifyBlock:(GTCheckoutNotifyBlock)notifyBlock { - BOOL success = [self performCheckoutWithStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; + BOOL success = [self performCheckout:targetCommit withStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; if (success == NO) return NO; return [self moveHEADToCommit:targetCommit error:error]; } - (BOOL)checkoutReference:(GTReference *)targetReference strategy:(GTCheckoutStrategyType)strategy notifyFlags:(GTCheckoutNotifyFlags)notifyFlags error:(NSError **)error progressBlock:(GTCheckoutProgressBlock)progressBlock notifyBlock:(GTCheckoutNotifyBlock)notifyBlock { - BOOL success = [self performCheckoutWithStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; + GTOID *targetOID = [targetReference targetOID]; + GTObject *target = [self lookUpObjectByOID:targetOID error:error]; + if (target == nil) { + return NO; + } + BOOL success = [self performCheckout:target withStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; if (success == NO) return NO; return [self moveHEADToReference:targetReference error:error]; } From 8e6c27c87f98ada92dcda9b7900de50fe1c7c317 Mon Sep 17 00:00:00 2001 From: Dan Leehr Date: Wed, 19 Aug 2015 14:14:47 -0400 Subject: [PATCH 5/6] Rework checkout+notify tests to clarify expected behavior --- ObjectiveGitTests/GTRepositorySpec.m | 44 +++++++++++++--------------- 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/ObjectiveGitTests/GTRepositorySpec.m b/ObjectiveGitTests/GTRepositorySpec.m index 32d8bbef9..2f2983023 100644 --- a/ObjectiveGitTests/GTRepositorySpec.m +++ b/ObjectiveGitTests/GTRepositorySpec.m @@ -14,8 +14,8 @@ QuickSpecBegin(GTRepositorySpec) -static NSString * const mainFile = @"main.m"; -static NSString * const readmeFile = @"README1.txt"; +static NSString * const readmeFile = @"README.md"; +static NSString * const readme1File = @"README1.txt"; __block GTRepository *repository; @@ -381,58 +381,54 @@ }); describe(@"-checkout:strategy:notifyFlags:error:notifyBlock:progressBlock:", ^{ - it(@"should fail ref checkout with dirty file and notify", ^{ + it(@"should fail ref checkout with conflict and notify", ^{ NSError *error = nil; GTReference *ref = [repository lookUpReferenceWithName:@"refs/heads/other-branch" error:&error]; expect(ref).notTo(beNil()); expect(error.localizedDescription).to(beNil()); - BOOL writeResult = [@"Replacement data in main.m\n" writeToURL:[repository.fileURL URLByAppendingPathComponent:mainFile] atomically:YES encoding:NSUTF8StringEncoding error:&error]; + BOOL writeResult = [@"Conflicting data in README.md\n" writeToURL:[repository.fileURL URLByAppendingPathComponent:readmeFile] atomically:YES encoding:NSUTF8StringEncoding error:&error]; expect(@(writeResult)).to(beTruthy()); __block NSUInteger notifyCount = 0; - __block BOOL mainFileDirty = NO; + __block BOOL readmeFileConflicted = NO; int (^notifyBlock)(GTCheckoutNotifyFlags, NSString *, GTDiffFile *, GTDiffFile *, GTDiffFile *); notifyBlock = ^(GTCheckoutNotifyFlags why, NSString *path, GTDiffFile *baseline, GTDiffFile *target, GTDiffFile *workdir) { notifyCount++; - if([path isEqualToString:mainFile] && (why & GTCheckoutNotifyDirty)) { - mainFileDirty = YES; - return GIT_EUSER; - } else { - return 0; + if([path isEqualToString:readmeFile] && (why & GTCheckoutNotifyConflict)) { + readmeFileConflicted = YES; } + return 0; }; - BOOL result = [repository checkoutReference:ref strategy:GTCheckoutStrategySafe notifyFlags:GTCheckoutNotifyDirty error:&error progressBlock:nil notifyBlock:notifyBlock]; + BOOL result = [repository checkoutReference:ref strategy:GTCheckoutStrategySafe notifyFlags:GTCheckoutNotifyConflict error:&error progressBlock:nil notifyBlock:notifyBlock]; expect(@(notifyCount)).to(equal(@(1))); - expect(@(mainFileDirty)).to(beTruthy()); + expect(@(readmeFileConflicted)).to(beTruthy()); expect(@(result)).to(beFalsy()); - expect(@(error.code)).to(equal(@(GIT_EUSER))); + expect(@(error.code)).to(equal(@(GIT_ECONFLICT))); }); - it(@"should fail commit checkout with dirty file and notify", ^{ + it(@"should fail commit checkout with conflict and notify", ^{ NSError *error = nil; GTCommit *commit = [repository lookUpObjectBySHA:@"1d69f3c0aeaf0d62e25591987b93b8ffc53abd77" objectType:GTObjectTypeCommit error:&error]; expect(commit).notTo(beNil()); expect(error.localizedDescription).to(beNil()); - BOOL writeResult = [@"Replacement data in README\n" writeToURL:[repository.fileURL URLByAppendingPathComponent:readmeFile] atomically:YES encoding:NSUTF8StringEncoding error:&error]; + BOOL writeResult = [@"Conflicting data in README1.txt\n" writeToURL:[repository.fileURL URLByAppendingPathComponent:readme1File] atomically:YES encoding:NSUTF8StringEncoding error:&error]; expect(@(writeResult)).to(beTruthy()); __block NSUInteger notifyCount = 0; - __block BOOL readmeFileDirty = NO; + __block BOOL readme1FileConflicted = NO; int (^notifyBlock)(GTCheckoutNotifyFlags, NSString *, GTDiffFile *, GTDiffFile *, GTDiffFile *); notifyBlock = ^(GTCheckoutNotifyFlags why, NSString *path, GTDiffFile *baseline, GTDiffFile *target, GTDiffFile *workdir) { notifyCount++; - if([path isEqualToString:readmeFile] && (why & GTCheckoutNotifyDirty)) { - readmeFileDirty = YES; - return GIT_EUSER; - } else { - return 0; + if([path isEqualToString:readme1File] && (why & GTCheckoutNotifyConflict)) { + readme1FileConflicted = YES; } + return 0; }; - BOOL result = [repository checkoutCommit:commit strategy:GTCheckoutStrategySafe notifyFlags:GTCheckoutNotifyDirty error:&error progressBlock:nil notifyBlock:notifyBlock]; + BOOL result = [repository checkoutCommit:commit strategy:GTCheckoutStrategySafe notifyFlags:GTCheckoutNotifyConflict error:&error progressBlock:nil notifyBlock:notifyBlock]; expect(@(notifyCount)).to(equal(@(1))); - expect(@(readmeFileDirty)).to(beTruthy()); + expect(@(readme1FileConflicted)).to(beTruthy()); expect(@(result)).to(beFalsy()); - expect(@(error.code)).to(equal(@(GIT_EUSER))); + expect(@(error.code)).to(equal(@(GIT_ECONFLICT))); }); }); From 5fa70c915e3acbd64bddbdbd20981c10e2c71740 Mon Sep 17 00:00:00 2001 From: Dan Leehr Date: Wed, 19 Aug 2015 14:17:14 -0400 Subject: [PATCH 6/6] Convert nil check+return to single line --- ObjectiveGit/GTRepository.m | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ObjectiveGit/GTRepository.m b/ObjectiveGit/GTRepository.m index f15a1de98..b609dc90d 100644 --- a/ObjectiveGit/GTRepository.m +++ b/ObjectiveGit/GTRepository.m @@ -838,9 +838,7 @@ - (BOOL)checkoutCommit:(GTCommit *)targetCommit strategy:(GTCheckoutStrategyType - (BOOL)checkoutReference:(GTReference *)targetReference strategy:(GTCheckoutStrategyType)strategy notifyFlags:(GTCheckoutNotifyFlags)notifyFlags error:(NSError **)error progressBlock:(GTCheckoutProgressBlock)progressBlock notifyBlock:(GTCheckoutNotifyBlock)notifyBlock { GTOID *targetOID = [targetReference targetOID]; GTObject *target = [self lookUpObjectByOID:targetOID error:error]; - if (target == nil) { - return NO; - } + if (target == nil) return NO; BOOL success = [self performCheckout:target withStrategy:strategy notifyFlags:notifyFlags error:error progressBlock:progressBlock notifyBlock:notifyBlock]; if (success == NO) return NO; return [self moveHEADToReference:targetReference error:error];