From 5ae12040f49a6d78e8ba8b4615ea8cc092b471d0 Mon Sep 17 00:00:00 2001 From: Pavlo Shkrabliuk Date: Sun, 1 Oct 2023 23:03:45 +0300 Subject: [PATCH] Fallback to conflicts when merge is done in non-tty environment (read GUI app). Merge in GUI apps led to silent failure to merge .s7substate properly. It left .s7substate in a visually non-conflict state. Now, if merge happens in non-tty env (we cannot interactively ask user what to do), we fallback to the conflict. --- buildme.sh | 4 +- .../case-cherryPickSingleCommitNeedMerge.sh | 2 +- .../case-mergeConflictInSubrepo-Merge.sh | 17 +- ...mergeConflictInSubrepo-keepLocalVersion.sh | 2 +- ...ergeConflictInSubrepo-keepRemoteVersion.sh | 2 +- system7-tests/mergeTests.m | 134 ++++++++++-- system7/Merge Driver/S7ConfigMergeDriver.m | 201 ++++++++++-------- system7/Utils/Utils.h | 2 - system7/Utils/Utils.m | 7 - 9 files changed, 256 insertions(+), 115 deletions(-) diff --git a/buildme.sh b/buildme.sh index 570dfc4..8ca47b4 100755 --- a/buildme.sh +++ b/buildme.sh @@ -21,13 +21,15 @@ fi # # INSTALL_PATH: # "The directory in which to install the build products. This path is prepended by the DSTROOT." +# +# Starting with Sonoma, xcodebuild install (without clean) stopped replacing existing binary in the DSTROOT. xcodebuild_cmd() { # when executed as part of xcode run script phase, env is populated with settings of current project # these headers are unwanted and may cause module import conflicts unset USER_HEADER_SEARCH_PATHS unset HEADER_SEARCH_PATHS - xcodebuild -target system7 -configuration Release DSTROOT="$HOME" install + xcodebuild -target system7 -configuration Release DSTROOT="$HOME" clean install } if ! xcodebuild_cmd diff --git a/system7-tests/integration/case-cherryPickSingleCommitNeedMerge.sh b/system7-tests/integration/case-cherryPickSingleCommitNeedMerge.sh index d545a05..44ee8a0 100644 --- a/system7-tests/integration/case-cherryPickSingleCommitNeedMerge.sh +++ b/system7-tests/integration/case-cherryPickSingleCommitNeedMerge.sh @@ -48,7 +48,7 @@ echo echo cherry-pick # cherry-pick only last commit from 'release/documents-7.2.4' -echo M | git cherry-pick release/documents-7.2.4 +S7_MERGE_DRIVER_RESPONSE="M" git cherry-pick release/documents-7.2.4 assert test 0 -eq $? assert test plus = `cat Dependencies/ReaddleLib/RDMath.h` diff --git a/system7-tests/integration/case-mergeConflictInSubrepo-Merge.sh b/system7-tests/integration/case-mergeConflictInSubrepo-Merge.sh index 4a32959..75d4277 100644 --- a/system7-tests/integration/case-mergeConflictInSubrepo-Merge.sh +++ b/system7-tests/integration/case-mergeConflictInSubrepo-Merge.sh @@ -52,9 +52,24 @@ popd > /dev/null assert s7 rebind --stage assert git commit -m '"up ReaddleLib"' +# first try non-interactive merge "via GUI app" +echo "emulating GUI Git client that doesn't support interactive stdin" | + git merge --no-edit experiment +assert test 0 -ne $? + +echo +echo "resulting .s7substate:" +cat .s7substate +echo + +assert grep '"<<<"' .s7substate > /dev/null + +assert git merge --abort + + echo echo -echo m | git merge experiment +S7_MERGE_DRIVER_RESPONSE="m" git merge --no-edit experiment assert test 0 -ne $? echo diff --git a/system7-tests/integration/case-mergeConflictInSubrepo-keepLocalVersion.sh b/system7-tests/integration/case-mergeConflictInSubrepo-keepLocalVersion.sh index b8e2a3f..6506a1b 100644 --- a/system7-tests/integration/case-mergeConflictInSubrepo-keepLocalVersion.sh +++ b/system7-tests/integration/case-mergeConflictInSubrepo-keepLocalVersion.sh @@ -45,7 +45,7 @@ assert git commit -m '"up ReaddleLib"' echo echo -echo L | git merge experiment +S7_MERGE_DRIVER_RESPONSE="L" git merge --no-edit experiment assert test 0 -eq $? echo diff --git a/system7-tests/integration/case-mergeConflictInSubrepo-keepRemoteVersion.sh b/system7-tests/integration/case-mergeConflictInSubrepo-keepRemoteVersion.sh index eb3eb2b..052ab94 100644 --- a/system7-tests/integration/case-mergeConflictInSubrepo-keepRemoteVersion.sh +++ b/system7-tests/integration/case-mergeConflictInSubrepo-keepRemoteVersion.sh @@ -45,7 +45,7 @@ assert git commit -m '"up ReaddleLib"' echo echo -echo R | git merge experiment +S7_MERGE_DRIVER_RESPONSE="r" git merge --no-edit experiment assert test 0 -eq $? echo diff --git a/system7-tests/mergeTests.m b/system7-tests/mergeTests.m index 02f7b4b..2b14617 100644 --- a/system7-tests/mergeTests.m +++ b/system7-tests/mergeTests.m @@ -474,19 +474,7 @@ - (void)testBothSideUpdateSubrepoToDifferentRevisionConflictResolution_Merge { ++callNumber; - if (1 == callNumber) { - // play a fool and return an option not in 'possibleOptions' - return S7ConflictResolutionOptionKeepChanged; - } - else if (2 == callNumber) { - // play a fool and return an option not in 'possibleOptions' one more time - return S7ConflictResolutionOptionDelete; - } - else if (3 == callNumber) { - // шоб да, так нет - return S7ConflictResolutionOptionKeepLocal | S7ConflictResolutionOptionKeepRemote; - } - else if (callNumber > 4) { + if (callNumber > 1) { // it keeps asking us? XCTFail(@""); } @@ -502,7 +490,7 @@ - (void)testBothSideUpdateSubrepoToDifferentRevisionConflictResolution_Merge { saveResultToFilePath:S7ConfigFileName]; XCTAssertEqual(0, mergeExitStatus); - XCTAssertEqual(4, callNumber, @"we played a fool several times, but then responded with a valid value – `merge`"); + XCTAssertEqual(1, callNumber, @"we played a fool several times, but then responded with a valid value – `merge`"); S7PrepareCommitMsgHook *prepareCommitHook = [S7PrepareCommitMsgHook new]; XCTAssertEqual(0, [prepareCommitHook runWithArguments:@[ @"merge" ]]); @@ -534,6 +522,124 @@ - (void)testBothSideUpdateSubrepoToDifferentRevisionConflictResolution_Merge { }]; } +- (void)testBothSideUpdateSubrepoToDifferentRevisionConflictResolution_KeepConflict { + __block S7Config *baseConfig = nil; + [self.env.pasteyRd2Repo run:^(GitRepository * _Nonnull repo) { + // ReaddleLib will conflict and two other will have to merge well + s7add(@"Dependencies/RDSFTP", self.env.githubRDSFTPRepo.absolutePath); + s7add(@"Dependencies/RDPDFKit", self.env.githubRDPDFKitRepo.absolutePath); + s7add(@"Dependencies/ReaddleLib", self.env.githubReaddleLibRepo.absolutePath); + + [repo add:@[S7ConfigFileName, @".gitignore"]]; + [repo commitWithMessage:@"add subrepos"]; + + baseConfig = [[S7Config alloc] initWithContentsOfFile:S7ConfigFileName]; + + s7push_currentBranch(repo); + }]; + + __block S7Config *niksConfig = nil; + __block NSString *readdleLib_niks_Revision = nil; + __block NSString *sftp_niks_Revision = nil; + [self.env.nikRd2Repo run:^(GitRepository * _Nonnull repo) { + [repo pull]; + + s7init_deactivateHooks(); + + S7PostMergeHook *postMergeHook = [S7PostMergeHook new]; + const int mergeHookExitStatus = [postMergeHook runWithArguments:@[]]; + XCTAssertEqual(0, mergeHookExitStatus); + + GitRepository *readdleLibSubrepoGit = [GitRepository repoAtPath:@"Dependencies/ReaddleLib"]; + XCTAssertNotNil(readdleLibSubrepoGit); + + readdleLib_niks_Revision = commit(readdleLibSubrepoGit, @"RDGeometry.h", @"xyz", @"some useful math func"); + + GitRepository *sftpSubrepoGit = [GitRepository repoAtPath:@"Dependencies/RDSFTP"]; + XCTAssertNotNil(sftpSubrepoGit); + + sftp_niks_Revision = commit(sftpSubrepoGit, @"RDSFTP.h", @"ssh2 request", @"some usuful code"); + + s7rebind_with_stage(); + [repo commitWithMessage:@"up ReaddleLib and RDSFTP"]; + + niksConfig = [[S7Config alloc] initWithContentsOfFile:S7ConfigFileName]; + + s7push_currentBranch(repo); + }]; + + [self.env.pasteyRd2Repo run:^(GitRepository * _Nonnull repo) { + GitRepository *readdleLibSubrepoGit = [GitRepository repoAtPath:@"Dependencies/ReaddleLib"]; + NSString *readdleLib_pasteys_Revision = + commit(readdleLibSubrepoGit, @"RDSystemInfo.h", @"iPad 11''", @"add support for a new iPad model"); + + GitRepository *pdfkitSubrepoGit = [GitRepository repoAtPath:@"Dependencies/RDPDFKit"]; + NSString *pdfkit_pasteys_Revision = commit(pdfkitSubrepoGit, @"RDPDFAnnotation.h", @"AP/N", @"generate AP/N"); + + s7rebind_with_stage(); + [repo commitWithMessage:@"up ReaddleLib and RDPDFKit"]; + + S7Config *ourConfig = [[S7Config alloc] initWithContentsOfFile:S7ConfigFileName]; + + const int pushExitStatus = s7push_currentBranch(repo); + XCTAssertNotEqual(0, pushExitStatus, @"nik has pushed. I must merge"); + + [repo fetch]; + + S7ConfigMergeDriver *configMergeDriver = [S7ConfigMergeDriver new]; + + [configMergeDriver setResolveConflictBlock:^S7ConflictResolutionOption(S7SubrepoDescription * _Nonnull ourVersion, + S7SubrepoDescription * _Nonnull theirVersion) + { + XCTAssertEqualObjects(ourVersion.path, @"Dependencies/ReaddleLib"); + + return S7ConflictResolutionOptionKeepConflict; + }]; + + const int mergeExitStatus = [configMergeDriver + mergeRepo:repo + baseConfig:baseConfig + ourConfig:ourConfig + theirConfig:niksConfig + saveResultToFilePath:S7ConfigFileName]; + XCTAssertNotEqual(0, mergeExitStatus); + + S7Config *actualConfig = [[S7Config alloc] initWithContentsOfFile:S7ConfigFileName]; + + S7Config *expectedConfig = [[S7Config alloc] initWithSubrepoDescriptions:@[ + [[S7SubrepoDescription alloc] + initWithPath:@"Dependencies/RDSFTP" + url:self.env.githubRDSFTPRepo.absolutePath + revision:sftp_niks_Revision + branch:@"master"], + + [[S7SubrepoDescription alloc] + initWithPath:@"Dependencies/RDPDFKit" + url:self.env.githubRDPDFKitRepo.absolutePath + revision:pdfkit_pasteys_Revision + branch:@"master"], + + [[S7SubrepoDescriptionConflict alloc] + initWithOurVersion:[[S7SubrepoDescription alloc] + initWithPath:@"Dependencies/ReaddleLib" + url:self.env.githubReaddleLibRepo.absolutePath + revision:readdleLib_pasteys_Revision + branch:@"master"] + theirVersion:[[S7SubrepoDescription alloc] + initWithPath:@"Dependencies/ReaddleLib" + url:self.env.githubReaddleLibRepo.absolutePath + revision:readdleLib_niks_Revision + branch:@"master"]] + ]]; + + XCTAssertEqualObjects(actualConfig, expectedConfig); + + S7Config *controlConfig = [[S7Config alloc] initWithContentsOfFile:S7ControlFileName]; + XCTAssertNotNil(controlConfig); + XCTAssertEqualObjects(actualConfig, controlConfig); + }]; +} + - (void)testBothSideUpdateSubrepoToDifferentRevisionSameFileSubrepoConflictResolution_Merge { __block S7Config *baseConfig = nil; __block NSString *readdleLib_initialRevision = nil; diff --git a/system7/Merge Driver/S7ConfigMergeDriver.m b/system7/Merge Driver/S7ConfigMergeDriver.m index 2805799..9bcb9eb 100644 --- a/system7/Merge Driver/S7ConfigMergeDriver.m +++ b/system7/Merge Driver/S7ConfigMergeDriver.m @@ -27,14 +27,14 @@ - (instancetype)init { { void *self __attribute((unused)) __attribute((unavailable)); - const int BUF_LEN = 20; - char buf[BUF_LEN]; - - if (ourVersion && theirVersion) { + NSString *const response = NSProcessInfo.processInfo.environment[@"S7_MERGE_DRIVER_RESPONSE"]; + if (response) { S7ConflictResolutionOption resolution; - - NSString *const response = NSProcessInfo.processInfo.environment[@"S7_MERGE_DRIVER_RESPONSE"]; - if (response.length > 0 && [S7ConfigMergeDriver mergeResolution:&resolution forUserInput:[response characterAtIndex:0]]) { + if (response.length > 0 && + [S7ConfigMergeDriver mergeResolution:&resolution + forUserInput:[response characterAtIndex:0] + allowedResolutions:~0]) + { NSString *resolutionString; switch (resolution) { case S7ConflictResolutionOptionMerge: @@ -46,8 +46,14 @@ - (instancetype)init { case S7ConflictResolutionOptionKeepRemote: resolutionString = @"keep remote"; break; - default: - resolutionString = @""; + case S7ConflictResolutionOptionKeepChanged: + resolutionString = @"keep changed"; + break; + case S7ConflictResolutionOptionDelete: + resolutionString = @"delete"; + break; + case S7ConflictResolutionOptionKeepConflict: + resolutionString = @"keep conflict"; break; } @@ -65,14 +71,32 @@ - (instancetype)init { return resolution; } - else if (response != nil) { + else { fprintf(stderr, "\033[31m" " failed to recognize S7_MERGE_DRIVER_RESPONSE: '%s'\n" "\033[0m", [response cStringUsingEncoding:NSUTF8StringEncoding]); } - + } + + if (!isatty(fileno(stdin))) { + fprintf(stderr, + "\033[31m" + "to run interactive merge of s7 subrepos, please run the following command in Terminal:\n" + "\033[1m" + " git checkout -m .s7substate\n\n" + "\033[m" + "\033[0m"); + return S7ConflictResolutionOptionKeepConflict; + } + + if (ourVersion && theirVersion) { + S7ConflictResolutionOption possibleConflictResolutionOptions = + S7ConflictResolutionOptionKeepLocal | + S7ConflictResolutionOptionKeepRemote | + S7ConflictResolutionOptionMerge; + // should write this to stdout or stderr? fprintf(stdout, "\n" @@ -86,21 +110,17 @@ - (instancetype)init { [theirVersion.humanReadableRevisionAndBranchState cStringUsingEncoding:NSUTF8StringEncoding] ); - do { - char *userInput = fgets(buf, BUF_LEN, stdin); - if (userInput && strlen(userInput) >= 1 && [S7ConfigMergeDriver mergeResolution:&resolution forUserInput:userInput[0]]) { - return resolution; - } - - fprintf(stdout, - "\n sorry?\n" - " (m)erge, keep (l)ocal or keep (r)emote.\n" - " what do you want to do? "); - } - while (1); + return [S7ConfigMergeDriver + readConflictResolutionFromStdinWithAllowedOptions:possibleConflictResolutionOptions + prompt:@"\n sorry?\n" + " (m)erge, keep (l)ocal or keep (r)emote.\n" + " what do you want to do? "]; } else { NSCAssert(ourVersion || theirVersion, @""); + S7ConflictResolutionOption possibleConflictResolutionOptions = + S7ConflictResolutionOptionKeepChanged | + S7ConflictResolutionOptionDelete; if (ourVersion) { fprintf(stdout, @@ -115,22 +135,10 @@ - (instancetype)init { ourVersion.path.fileSystemRepresentation); } - do { - char *userInput = fgets(buf, BUF_LEN, stdin); - if (userInput && strlen(userInput) >= 1) { - if (tolower(userInput[0]) == 'c') { - return S7ConflictResolutionOptionKeepChanged; - } - else if (tolower(userInput[0]) == 'd') { - return S7ConflictResolutionOptionDelete; - } - } - - fprintf(stdout, - "\n sorry?\n" - " use (c)hanged version or (d)elete? "); - } - while (1); + return [S7ConfigMergeDriver + readConflictResolutionFromStdinWithAllowedOptions:possibleConflictResolutionOptions + prompt:@"\n sorry?\n" + " use (c)hanged version or (d)elete? "]; } }]; @@ -440,46 +448,12 @@ - (int)mergeRepo:(GitRepository *)repo S7SubrepoDescriptionConflict *conflict = (S7SubrepoDescriptionConflict *)subrepoDesc; - S7ConflictResolutionOption possibleConflictResolutionOptions = 0; - if (conflict.ourVersion && conflict.theirVersion) { - possibleConflictResolutionOptions = - S7ConflictResolutionOptionKeepLocal | - S7ConflictResolutionOptionKeepRemote | - S7ConflictResolutionOptionMerge; - } - else { - NSAssert(conflict.ourVersion || conflict.theirVersion, @""); - possibleConflictResolutionOptions = - S7ConflictResolutionOptionKeepChanged | - S7ConflictResolutionOptionDelete; - } + const S7ConflictResolutionOption userDecision = self.resolveConflictBlock(conflict.ourVersion, + conflict.theirVersion); - NSUInteger numberOfTries = 0; - S7ConflictResolutionOption userDecision = 0; - do { - ++numberOfTries; - if (numberOfTries > 10) { - fprintf(stderr, "too many attempts – will leave conflict unresolved\n"); - userDecision = S7ConflictResolutionOptionKeepConflict; - conflictResolved = NO; - NSAssert(NO, @""); - break; - } - - userDecision = self.resolveConflictBlock(conflict.ourVersion, - conflict.theirVersion); - - if (NO == isExactlyOneBitSetInNumber(userDecision)) { - continue; - } - - if (0 == (possibleConflictResolutionOptions & userDecision)) { - continue; - } - - break; - - } while (1); + if (S7ConflictResolutionOptionKeepConflict == userDecision) { + conflictResolved = NO; + } switch (userDecision) { case S7ConflictResolutionOptionKeepLocal: @@ -527,11 +501,11 @@ - (int)mergeRepo:(GitRepository *)repo } const int configSaveResult = [mergeResult saveToFileAtPath:resultFilePath]; - if (0 != configSaveResult) { + if (S7ExitCodeSuccess != configSaveResult) { return configSaveResult; } - if (0 != [mergeResult saveToFileAtPath:S7ControlFileName]) { + if (S7ExitCodeSuccess != [mergeResult saveToFileAtPath:S7ControlFileName]) { fprintf(stderr, "failed to save %s to disk.\n", S7ControlFileName.fileSystemRepresentation); @@ -546,21 +520,74 @@ - (int)mergeRepo:(GitRepository *)repo return [S7PostCheckoutHook checkoutSubreposForRepo:repo fromConfig:ourConfig toConfig:mergeResult]; } -+ (BOOL)mergeResolution:(S7ConflictResolutionOption *)resolution forUserInput:(char)userInput { ++ (S7ConflictResolutionOption)readConflictResolutionFromStdinWithAllowedOptions:(S7ConflictResolutionOption)allowedResolutions + prompt:(NSString *)prompt +{ + const int BUF_LEN = 20; + char buf[BUF_LEN]; + + NSUInteger numberOfTries = 0; + do { + ++numberOfTries; + + S7ConflictResolutionOption resolution; + char *userInput = fgets(buf, BUF_LEN, stdin); + if (userInput && + strlen(userInput) >= 1 && + [S7ConfigMergeDriver mergeResolution:&resolution + forUserInput:userInput[0] + allowedResolutions:allowedResolutions]) + { + return resolution; + } + + fprintf(stdout, "%s", [prompt cStringUsingEncoding:NSUTF8StringEncoding]); + } + while (numberOfTries < 5); + + fprintf(stderr, "too many attempts – will leave the conflict unresolved\n"); + + return S7ConflictResolutionOptionKeepConflict; +} + ++ (BOOL)mergeResolution:(S7ConflictResolutionOption *)pResolution + forUserInput:(char)userInput + allowedResolutions:(S7ConflictResolutionOption)allowedResolutions +{ userInput = tolower(userInput); + + S7ConflictResolutionOption userResolution; switch (userInput) { case 'm': - *resolution = S7ConflictResolutionOptionMerge; - return YES; + userResolution = S7ConflictResolutionOptionMerge; + break; + case 'l': - *resolution = S7ConflictResolutionOptionKeepLocal; - return YES; + userResolution = S7ConflictResolutionOptionKeepLocal; + break; + case 'r': - *resolution = S7ConflictResolutionOptionKeepRemote; - return YES; + userResolution = S7ConflictResolutionOptionKeepRemote; + break; + + case 'c': + userResolution = S7ConflictResolutionOptionKeepChanged; + break; + + case 'd': + userResolution = S7ConflictResolutionOptionDelete; + break; + default: return NO; } + + if (0 != (userResolution & allowedResolutions)) { + *pResolution = userResolution; + return YES; + } + + return NO; } @end diff --git a/system7/Utils/Utils.h b/system7/Utils/Utils.h index 7101085..47e27e5 100644 --- a/system7/Utils/Utils.h +++ b/system7/Utils/Utils.h @@ -23,8 +23,6 @@ int removeFilesFromGitattributes(NSSet *filesToRemove); int installHook(NSString *hookName, NSString *commandLine, BOOL forceOverwrite, BOOL installFakeHooks); -BOOL isExactlyOneBitSetInNumber(uint32_t bits); - BOOL isCurrentDirectoryS7RepoRoot(void); BOOL isS7Repo(GitRepository *repo); int s7RepoPreconditionCheck(void); diff --git a/system7/Utils/Utils.m b/system7/Utils/Utils.m index 6c274d7..632689c 100644 --- a/system7/Utils/Utils.m +++ b/system7/Utils/Utils.m @@ -52,13 +52,6 @@ int getConfig(GitRepository *repo, NSString *revision, S7Config * _Nullable __au return 0; } -BOOL isExactlyOneBitSetInNumber(uint32_t bits) -{ - // I was too lazy to do this myself - // taken here https://stackoverflow.com/questions/51094594/how-to-check-if-exactly-one-bit-is-set-in-an-int/51094793 - return bits && !(bits & (bits-1)); -} - int addLineToGitIgnore(NSString *lineToAppend) { static NSString *gitIgnoreFileName = @".gitignore";