{"thread":{"id":"49053","subject":"Two RFC/WIP series for facilitating merge conflict resolution","startedAt":"2018-08-06T22:44:16Z","lastAt":"2018-08-09T21:27:48Z","messageCount":13,"participants":["Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"354689","messageId":"CABPp-BHxJyWsAQ3FkfdC-5Vqe3d7wWZm-hVYd0-afNY9dEgMeQ@mail.gmail.com","threadId":"49053","inReplyTo":null,"subject":"Two RFC/WIP series for facilitating merge conflict resolution","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T22:44:02Z","receivedAt":"2018-08-06T22:44:16Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi everyone,\n\nLast week, Duy posted an interesting RFC patch for improving merge\nconflict resolution by coloring the \"CONFLICT' messages that are\noutput during a merge. I have two more ideas (complementary to his)\nfor improving merge conflict resolution experience.  My two ideas are\nmostly independent of each other but with a couple important ties;\nI'll shortly respond with a short RFC/WIP series for each.\n\nSome notes applicable to both:\n  - Each has a very long cover letter, due to trying to anticipate\nquestions/surprises and responding to each.  It may be simpler to skip\nthe cover letter (or just read the up-front summary) and see if the\npatches make sense,\n  - I am planning to imminently start looking into my merge-recursive\nrewrite[1].  One possibility is just doing these changes as part of\nthe new strategy...thoughts?\n\n\nElijah\n\n[1] See part E from\nhttps://public-inbox.org/git/CABPp-BFQJZHfCJZ1qvhvVcMd-_sOfi0Fkm5PexEwzzN+Zw2akw@mail.gmail.com/;\nalso the two RFCs I'm about to post are part C from that email.\n"},{"id":"354690","messageId":"20180806224547.8619-1-newren@gmail.com","threadId":"49053","inReplyTo":"CABPp-BHxJyWsAQ3FkfdC-5Vqe3d7wWZm-hVYd0-afNY9dEgMeQ@mail.gmail.com","subject":"[RFC/WIP PATCH 0/1] Simplify handling of directory/file conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T22:45:46Z","receivedAt":"2018-08-06T22:45:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Directory/file conflicts are more difficult than they need to be for users\nto resolve (and to un-resolve).  Simplify that process by doing to the\nindex what we do to the working tree: renaming the file in the\ndirectory/file conflict to a different location.  This also avoids leaving\ncruft untracked files around if the user decides to abort the merge.\n\nFrom one angle this proposal might appear surprising, so extended rationale\nfor this change can be found below (and if it seems surprising, some of the\nbelow may need to be moved into the commit message for the patch).\n\n\n== What git does, prior to this series ==\n\nLet's say there's a directory/file conflict for path 'foo'.  One might see\ngit report:\n  CONFLICT (file/directory): There is a directory with name foo in\n  BRANCH2. Adding foo as foo~BRANCH\nFurther, at this point, git will record:\n  * foo~BRANCH in the working tree\n  * foo/ in the working tree\n  * foo at higher order stage in the index\n  * foo/* entries found in the index (at stage 0)\n\n== User experience resolving directory/file conflicts ==\n\nLet's say the user wants to resolve by just moving 'foo' (the file) to\nsomewhere else.  Here's five different things a user might try at this\npoint (not sequentially, but rather competing ideas they might try),\nalong with commentary about how each fails to resolve the conflict:\n\n$ git mv foo other\n  # Moves the directory instead, oops\n\n$ mv foo~BRANCH other\n$ git add other\n  # Still leaves 'foo' conflicted in the index\n\n$ git mv foo~BRANCH other\n  # Error: \"Not under source control\"\n\n$ git add -u\n  # Removes conflict entry for 'foo' from index, but doesn't add\n  # new one and leaves foo~BRANCH around untracked\n\n$ git rm foo\n  # Doesn't work (\"foo: needs merge...fatal: git rm: 'foo': Is a directory)\n\n== User experience un-resolving directory/file conflict ==\n\nIf the user decides they don't like the merge and run 'git merge --abort',\nthe abort fails due to a separate bug being fixed here:\n\n  https://public-inbox.org/git/20180713163331.22446-1-newren@gmail.com/\n\nHowever, even once the fixes there are part of git, a 'git merge --abort'\nwill leave behind new untracked files that were created by the merge\nattempt (in the case above, one named foo~BRANCH).  This is suboptimal.\n\n== Correct solution; old and new ==\n\nCurrently, this is what a user needs to run to resolve this conflict:\n\n$ mv foo~BRANCH other\n$ git add other\n$ git rm --cached foo\n\nIf git would record foo~BRANCH at a higher stage in the index instead\nof recording foo there, then we could shorten this to:\n\n$ git add foo~BRANCH\n$ git mv foo~BRANCH other\n\nIf we could also teach git-mv to quit reporting \"not under version\ncontrol\" for index entries with higher order stages (and instead rename\nthem while keeping them as higher order stages), then we could also allow\nthose two commands to be reversed:\n\n$ git mv foo~BRANCH other\n$ git add other\n\nWhile this change to what git records in the index might feel like a lie,\nit does make it easier for the user and we already have a precedent for\ntreating user convience as more important than trying to represent how we\ngot to the current state, as shown in the following analogy:\n\n== Rename analogy ==\n\nIf one side of history renames A->B but there are content conflicts, one\nchoice for the contents for the index would be:\n  <mode> <original sha> 1    A\n  <mode> <side-one sha> 2    A\n  <mode> <side-two sha> 3    B\nHowever, that's not what git does.  In particular, this choice would require\nthe user to run both 'git add B' and 'git rm --cached A' to resolve the\nconflict.  Further, it prevents the user from running commands such as\n  git checkout [--ours|--theirs|--conflict|-m] B\n  git diff [--ours|--theirs|--base] B\nThis would also make it harder to pair up entries from 'git ls-files -u'\n(especially if there are many entries between A and B), since nothing\nmarks them as related anymore.  So, instead, git records the following in\nthe index:\n  <mode> <original sha> 1    B\n  <mode> <side-one sha> 2    B\n  <mode> <side-two sha> 3    B\nThis might seem like a lie if you view the index as a place to record how\nwe got to the current state rather than a way to help users resolve\nconflicts and update state, but it is certainly far more convenient for\nthe user to work with.  Follow suit with directory/file conflicts.\n\n\nElijah Newren (1):\n  merge-recursive: make file/directory conflicts easier to resolve\n\n merge-recursive.c                    | 38 ++++++++++++++++++++++------\n t/t3030-merge-recursive.sh           | 16 ++++++------\n t/t6020-merge-df.sh                  |  4 +--\n t/t6022-merge-rename.sh              |  4 +--\n t/t6036-recursive-corner-cases.sh    |  5 ++--\n t/t6042-merge-rename-corner-cases.sh |  4 +--\n t/t6043-merge-rename-directories.sh  |  4 +--\n 7 files changed, 49 insertions(+), 26 deletions(-)\n\n-- \n2.18.0.550.g44d6daf40a.dirty\n"},{"id":"354691","messageId":"20180806224547.8619-2-newren@gmail.com","threadId":"49053","inReplyTo":"20180806224547.8619-1-newren@gmail.com","subject":"[RFC/WIP PATCH 1/1] merge-recursive: make file/directory conflicts easier to resolve","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T22:45:47Z","receivedAt":"2018-08-06T22:45:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"File/directory conflicts are somewhat difficult to resolve; by way of\ncontrast, renames are much easier to handle.  For file/directory\nconflicts, we already have to record the file under a different name in\nthe working copy due to the directory being in the way; if we also record\nthe file under a different name in the index then it simplifies matters\nfor the user, and ensures that 'git reset --hard' and 'git merge --abort'\nwill clean up files created by the merge operation.\n\nNote that there are multiple ways to get a file/directory conflict:\n  * add/add (one side adds a file, the other a directory)\n  * modify/delete (the side that deletes puts a directory in the way)\n  * rename vs add-directory (directory added on one side in way of rename)\nAs such, there are multiple code paths that need updating.\n\nFIXME: Several testcases were updated to show how this affected the\n  testsuite in general, but there are still 11 more tests across five\n  testfiles that need fixing.  As I said, this is just WIP/RFC.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c                    | 38 ++++++++++++++++++++++------\n t/t3030-merge-recursive.sh           | 16 ++++++------\n t/t6020-merge-df.sh                  |  4 +--\n t/t6022-merge-rename.sh              |  4 +--\n t/t6036-recursive-corner-cases.sh    |  5 ++--\n t/t6042-merge-rename-corner-cases.sh |  4 +--\n t/t6043-merge-rename-directories.sh  |  4 +--\n 7 files changed, 49 insertions(+), 26 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1446e92bea..34906b0f90 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1527,6 +1527,20 @@ static int handle_change_delete(struct merge_options *o,\n \t\t * path.  We could call update_file_flags() with update_cache=0\n \t\t * and update_wd=0, but that's a no-op.\n \t\t */\n+\t\tif (!o->call_depth && alt_path) {\n+\t\t\tstruct diff_filespec orig, new;\n+\t\t\tint stage = (change_branch == o->branch1) ? 2 : 3;\n+\n+\t\t\tremove_file_from_cache(path);\n+\t\t\torig.mode = o_mode;\n+\t\t\toidcpy(&orig.oid, o_oid);\n+\t\t\tnew.mode = changed_mode;\n+\t\t\toidcpy(&new.oid, changed_oid);\n+\t\t\tif (update_stages(o, alt_path, &orig,\n+\t\t\t\t\t  stage == 2 ? &new : NULL,\n+\t\t\t\t\t  stage == 3 ? &new : NULL))\n+\t\t\t\tret = -1;\n+\t\t}\n \t\tif (change_branch != o->branch1 || alt_path)\n \t\t\tret = update_file(o, 0, changed_oid, changed_mode, update_path);\n \t}\n@@ -3089,11 +3103,11 @@ static int merge_content(struct merge_options *o,\n \n \tif (df_conflict_remains || is_dirty) {\n \t\tchar *new_path;\n-\t\tif (o->call_depth) {\n-\t\t\tremove_file_from_cache(path);\n-\t\t} else {\n+\t\tnew_path = unique_path(o, path, rename_conflict_info->branch1);\n+\t\tremove_file_from_cache(path);\n+\t\tif (!o->call_depth) {\n \t\t\tif (!mfi.clean) {\n-\t\t\t\tif (update_stages(o, path, &one, &a, &b))\n+\t\t\t\tif (update_stages(o, new_path, &one, &a, &b))\n \t\t\t\t\treturn -1;\n \t\t\t} else {\n \t\t\t\tint file_from_stage2 = was_tracked(o, path);\n@@ -3101,14 +3115,13 @@ static int merge_content(struct merge_options *o,\n \t\t\t\toidcpy(&merged.oid, &mfi.oid);\n \t\t\t\tmerged.mode = mfi.mode;\n \n-\t\t\t\tif (update_stages(o, path, NULL,\n+\t\t\t\tif (update_stages(o, new_path, NULL,\n \t\t\t\t\t\t  file_from_stage2 ? &merged : NULL,\n \t\t\t\t\t\t  file_from_stage2 ? NULL : &merged))\n \t\t\t\t\treturn -1;\n \t\t\t}\n \n \t\t}\n-\t\tnew_path = unique_path(o, path, rename_conflict_info->branch1);\n \t\tif (is_dirty) {\n \t\t\toutput(o, 1, _(\"Refusing to lose dirty file at %s\"),\n \t\t\t       path);\n@@ -3244,10 +3257,19 @@ static int process_entry(struct merge_options *o,\n \t\t\toutput(o, 1, _(\"CONFLICT (%s): There is a directory with name %s in %s. \"\n \t\t\t       \"Adding %s as %s\"),\n \t\t\t       conf, path, other_branch, path, new_path);\n+\t\t\tremove_file_from_cache(path);\n+\t\t\tif (!o->call_depth) {\n+\t\t\t\tstruct diff_filespec dfs;\n+\n+\t\t\t\tdfs.mode = mode;\n+\t\t\t\toidcpy(&dfs.oid, oid);\n+\t\t\t\tif (update_stages(o, new_path, NULL,\n+\t\t\t\t\t\t  a_oid ? &dfs : NULL,\n+\t\t\t\t\t\t  a_oid ? NULL : &dfs))\n+\t\t\t\t\tclean_merge = -1;\n+\t\t\t}\n \t\t\tif (update_file(o, 0, oid, mode, new_path))\n \t\t\t\tclean_merge = -1;\n-\t\t\telse if (o->call_depth)\n-\t\t\t\tremove_file_from_cache(path);\n \t\t\tfree(new_path);\n \t\t} else {\n \t\t\toutput(o, 2, _(\"Adding %s\"), path);\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex ff641b348a..6d456da001 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -369,9 +369,9 @@ test_expect_success 'merge-recursive d/f conflict result' '\n \n \tgit ls-files -s >actual &&\n \t(\n-\t\techo \"100644 $o0 1\ta\" &&\n-\t\techo \"100644 $o1 2\ta\" &&\n \t\techo \"100644 $o4 0\ta/c\" &&\n+\t\techo \"100644 $o0 1\ta~$c1\" &&\n+\t\techo \"100644 $o1 2\ta~$c1\" &&\n \t\techo \"100644 $o0 0\tb\" &&\n \t\techo \"100644 $o0 0\tc\" &&\n \t\techo \"100644 $o1 0\td/e\"\n@@ -393,9 +393,9 @@ test_expect_success 'merge-recursive d/f conflict result the other way' '\n \n \tgit ls-files -s >actual &&\n \t(\n-\t\techo \"100644 $o0 1\ta\" &&\n-\t\techo \"100644 $o1 3\ta\" &&\n \t\techo \"100644 $o4 0\ta/c\" &&\n+\t\techo \"100644 $o0 1\ta~$c1\" &&\n+\t\techo \"100644 $o1 3\ta~$c1\" &&\n \t\techo \"100644 $o0 0\tb\" &&\n \t\techo \"100644 $o0 0\tc\" &&\n \t\techo \"100644 $o1 0\td/e\"\n@@ -420,9 +420,9 @@ test_expect_success 'merge-recursive d/f conflict result' '\n \t\techo \"100644 $o1 0\ta\" &&\n \t\techo \"100644 $o0 0\tb\" &&\n \t\techo \"100644 $o0 0\tc\" &&\n-\t\techo \"100644 $o6 3\td\" &&\n \t\techo \"100644 $o0 1\td/e\" &&\n-\t\techo \"100644 $o1 2\td/e\"\n+\t\techo \"100644 $o1 2\td/e\" &&\n+\t\techo \"100644 $o6 3\td~$c6\"\n \t) >expected &&\n \ttest_cmp expected actual\n \n@@ -444,9 +444,9 @@ test_expect_success 'merge-recursive d/f conflict result' '\n \t\techo \"100644 $o1 0\ta\" &&\n \t\techo \"100644 $o0 0\tb\" &&\n \t\techo \"100644 $o0 0\tc\" &&\n-\t\techo \"100644 $o6 2\td\" &&\n \t\techo \"100644 $o0 1\td/e\" &&\n-\t\techo \"100644 $o1 3\td/e\"\n+\t\techo \"100644 $o1 3\td/e\" &&\n+\t\techo \"100644 $o6 2\td~$c6\"\n \t) >expected &&\n \ttest_cmp expected actual\n \ndiff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh\nindex 2af1beec5f..f4ea318ec8 100755\n--- a/t/t6020-merge-df.sh\n+++ b/t/t6020-merge-df.sh\n@@ -81,7 +81,7 @@ test_expect_success 'modify/delete + directory/file conflict' '\n \n \ttest 5 -eq $(git ls-files -s | wc -l) &&\n \ttest 4 -eq $(git ls-files -u | wc -l) &&\n-\ttest 1 -eq $(git ls-files -o | wc -l) &&\n+\ttest 0 -eq $(git ls-files -o | wc -l) &&\n \n \ttest -f letters/file &&\n \ttest -f letters.txt &&\n@@ -100,7 +100,7 @@ test_expect_success 'modify/delete + directory/file conflict; other way' '\n \n \ttest 5 -eq $(git ls-files -s | wc -l) &&\n \ttest 4 -eq $(git ls-files -u | wc -l) &&\n-\ttest 1 -eq $(git ls-files -o | wc -l) &&\n+\ttest 0 -eq $(git ls-files -o | wc -l) &&\n \n \ttest -f letters/file &&\n \ttest -f letters.txt &&\ndiff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh\nindex b760c223c6..0ea3760265 100755\n--- a/t/t6022-merge-rename.sh\n+++ b/t/t6022-merge-rename.sh\n@@ -385,7 +385,7 @@ test_expect_success 'Rename+D/F conflict; renamed file cannot merge and dir in t\n \ttest_must_fail git merge --strategy=recursive dir-in-way &&\n \n \ttest 5 -eq \"$(git ls-files -u | wc -l)\" &&\n-\ttest 3 -eq \"$(git ls-files -u dir | grep -v file-in-the-way | wc -l)\" &&\n+\ttest 3 -eq \"$(git ls-files -u dir~HEAD | wc -l)\" &&\n \ttest 2 -eq \"$(git ls-files -u dir/file-in-the-way | wc -l)\" &&\n \n \ttest_must_fail git diff --quiet &&\n@@ -421,7 +421,7 @@ test_expect_success 'Same as previous, but merged other way' '\n \ttest_must_fail git merge --strategy=recursive renamed-file-has-conflicts &&\n \n \ttest 5 -eq \"$(git ls-files -u | wc -l)\" &&\n-\ttest 3 -eq \"$(git ls-files -u dir | grep -v file-in-the-way | wc -l)\" &&\n+\ttest 3 -eq \"$(git ls-files -u dir~renamed-file-has-conflicts | wc -l)\" &&\n \ttest 2 -eq \"$(git ls-files -u dir/file-in-the-way | wc -l)\" &&\n \n \ttest_must_fail git diff --quiet &&\ndiff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh\nindex 59e52c5a09..f8790e6975 100755\n--- a/t/t6036-recursive-corner-cases.sh\n+++ b/t/t6036-recursive-corner-cases.sh\n@@ -471,7 +471,7 @@ test_expect_success 'setup differently handled merges of directory/file conflict\n \t\ttest_must_fail git merge C^0 &&\n \t\tgit clean -fd &&\n \t\tgit rm -rf a/ &&\n-\t\tgit rm a &&\n+\t\tgit rm a~HEAD &&\n \t\tgit cat-file -p B:a >a2 &&\n \t\tgit add a2 &&\n \t\tgit commit -m D2 &&\n@@ -492,6 +492,7 @@ test_expect_success 'setup differently handled merges of directory/file conflict\n \t\ttest_must_fail git merge B^0 &&\n \t\tgit clean -fd &&\n \t\tgit rm -rf a/ &&\n+\t\tgit rm a~B^0 &&\n \t\ttest_write_lines 1 2 3 4 5 6 7 8 >a &&\n \t\tgit add a &&\n \t\tgit commit -m E3 &&\n@@ -501,7 +502,7 @@ test_expect_success 'setup differently handled merges of directory/file conflict\n \t\ttest_must_fail git merge B^0 &&\n \t\tgit clean -fd &&\n \t\tgit rm -rf a/ &&\n-\t\tgit rm a &&\n+\t\tgit rm a~B^0 &&\n \t\ttest_write_lines 1 2 3 4 5 6 7 8 >a2 &&\n \t\tgit add a2 &&\n \t\tgit commit -m E4 &&\ndiff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh\nindex 07dd09d985..25cb50478c 100755\n--- a/t/t6042-merge-rename-corner-cases.sh\n+++ b/t/t6042-merge-rename-corner-cases.sh\n@@ -305,14 +305,14 @@ test_expect_success 'rename/directory conflict + clean content merge' '\n \t\tgit ls-files -u >out &&\n \t\ttest_line_count = 1 out &&\n \t\tgit ls-files -o >out &&\n-\t\ttest_line_count = 2 out &&\n+\t\ttest_line_count = 1 out &&\n \n \t\techo 0 >expect &&\n \t\tgit cat-file -p base:file >>expect &&\n \t\techo 7 >>expect &&\n \t\ttest_cmp expect newfile~HEAD &&\n \n-\t\ttest $(git rev-parse :2:newfile) = $(git hash-object expect) &&\n+\t\ttest $(git rev-parse :2:newfile~HEAD) = $(git hash-object expect) &&\n \n \t\ttest_path_is_file newfile/realfile &&\n \t\ttest_path_is_file newfile~HEAD\ndiff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh\nindex 4a71f17edd..12047a3309 100755\n--- a/t/t6043-merge-rename-directories.sh\n+++ b/t/t6043-merge-rename-directories.sh\n@@ -1160,10 +1160,10 @@ test_expect_success '5d-check: Directory/file/file conflict due to directory ren\n \t\tgit ls-files -u >out &&\n \t\ttest_line_count = 1 out &&\n \t\tgit ls-files -o >out &&\n-\t\ttest_line_count = 2 out &&\n+\t\ttest_line_count = 1 out &&\n \n \t\tgit rev-parse >actual \\\n-\t\t\t:0:y/b :0:y/c :0:z/d :0:y/f :2:y/d :0:y/d/e &&\n+\t\t\t:0:y/b :0:y/c :0:z/d :0:y/f :2:y/d~HEAD :0:y/d/e &&\n \t\tgit rev-parse >expect \\\n \t\t\t O:z/b  O:z/c  B:z/d  B:z/f  A:y/d  B:y/d/e &&\n \t\ttest_cmp expect actual &&\n-- \n2.18.0.550.g44d6daf40a.dirty\n\n"},{"id":"354692","messageId":"20180806224745.8681-4-newren@gmail.com","threadId":"49053","inReplyTo":"20180806224745.8681-1-newren@gmail.com","subject":"[RFC/WIP PATCH 3/3] merge-recursive: provide more conflict hints for non-textual conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T22:47:45Z","receivedAt":"2018-08-06T22:47:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"For non-textual conflicts, provide additional information in the working\ncopy in the form of additional conflict markers and explanatory text\nstating what type of non-textual conflict was involved.  In particular,\nregular files involved in path-based conflicts would have something like\nthe following at the beginning of the file:\n\n    <<<<<<<< HEAD\n    Conflict hint: This block of text was not part of the original\n    branch; it serves instead to hint about non-textual conflicts:\n      MODIFY/DELETE: path foo modified in HEAD and deleted in BRANCH\n    ========\n    Conflict hint: This block of text was not part of the original\n    branch; it serves instead to hint about non-textual conflicts:\n      MODIFY/DELETE: path foo modified in HEAD and deleted in BRANCH\n    >>>>>>>> BRANCH\n\nWhen non regular files (binaries, symlinks, etc.) are involved in\nconflicts, we instead put this information in a separate file that only\ncontains this conflict information.\n\nThe goals of providing this extra information are:\n  * Make it clearer to users what conflicts they are dealing with and why\n  * Enable new features like Thomas Rast' old remerge-diff proposal[1]\n\n[1] https://public-inbox.org/git/cover.1409860234.git.tr@thomasrast.ch/\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n merge-recursive.c                   | 133 +++++++++++++++++++++++++++-\n t/t3031-merge-criscross.sh          |   2 +\n t/t6022-merge-rename.sh             |  39 ++------\n t/t6043-merge-rename-directories.sh |   4 +-\n t/t7610-mergetool.sh                |   4 +\n 5 files changed, 144 insertions(+), 38 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 4832234073..cdfe9824d2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -313,6 +313,96 @@ static void output_commit_title(struct merge_options *o, struct commit *commit)\n \tflush_output(o);\n }\n \n+static void write_conflict_notice(struct strbuf *buf,\n+\t\t\t\t  struct strbuf *notice,\n+\t\t\t\t  int use_crlf)\n+{\n+\tint marker_size = 8;\n+\n+\tstrbuf_addchars(buf, '<', marker_size);\n+\tif (use_crlf)\n+\t\tstrbuf_addch(buf, '\\r');\n+\tstrbuf_addch(buf, '\\n');\n+\n+\tstrbuf_addbuf(buf, notice);\n+\tif (use_crlf)\n+\t\tstrbuf_addch(buf, '\\r');\n+\tstrbuf_addch(buf, '\\n');\n+\n+\tstrbuf_addchars(buf, '=', marker_size);\n+\tif (use_crlf)\n+\t\tstrbuf_addch(buf, '\\r');\n+\tstrbuf_addch(buf, '\\n');\n+\n+\tstrbuf_addbuf(buf, notice);\n+\tif (use_crlf)\n+\t\tstrbuf_addch(buf, '\\r');\n+\tstrbuf_addch(buf, '\\n');\n+\n+\tstrbuf_addchars(buf, '>', marker_size);\n+\tif (use_crlf)\n+\t\tstrbuf_addch(buf, '\\r');\n+\tstrbuf_addch(buf, '\\n');\n+}\n+\n+static int create_non_textual_conflict_file(struct merge_options *o,\n+\t\t\t\t\t    struct strbuf *notice,\n+\t\t\t\t\t    const char *path,\n+\t\t\t\t\t    struct object_id *oid)\n+{\n+\tstruct strbuf contents = STRBUF_INIT;\n+\tint ret = 0;\n+\tint use_crlf = 0; /* FIXME: Determine platform default?? */\n+\n+\twrite_conflict_notice(&contents, notice, use_crlf);\n+\n+\tif (write_object_file(contents.buf, contents.len, \"blob\", oid))\n+\t\tret = err(o, _(\"Unable to add %s to database\"), path);\n+\n+\tstrbuf_release(&contents);\n+\treturn ret;\n+}\n+\n+static int insert_non_textual_conflict_header(struct merge_options *o,\n+\t\t\t\t\t      struct strbuf *notice,\n+\t\t\t\t\t      const char *path,\n+\t\t\t\t\t      struct object_id *oid,\n+\t\t\t\t\t      unsigned int mode)\n+{\n+\tstruct strbuf header = STRBUF_INIT;\n+\tstruct strbuf dst_buf = STRBUF_INIT;\n+\tenum object_type type;\n+\tchar *buf;\n+\tunsigned long size;\n+\tchar *end;\n+\tint use_crlf;\n+\tint ret = 0;\n+\n+\tif (!S_ISREG(mode))\n+\t\tBUG(\"insert_non_textual_conflict_header called on file with wrong mode: %0d\", mode);\n+\n+\tbuf = read_object_file(oid, &type, &size);\n+\tif (!buf)\n+\t\treturn err(o, _(\"cannot read object %s '%s'\"), oid_to_hex(oid), path);\n+\tif (type != OBJ_BLOB) {\n+\t\treturn err(o, _(\"blob expected for %s '%s'\"), oid_to_hex(oid), path);\n+\t}\n+\n+\tend = strchrnul(buf, '\\n');\n+\tuse_crlf = (end > buf && end[-1] == '\\r');\n+\twrite_conflict_notice(&header, notice, use_crlf);\n+\n+\tstrbuf_addbuf(&dst_buf, &header);\n+\tstrbuf_add(&dst_buf, buf, size);\n+\n+\tif (write_object_file(dst_buf.buf, dst_buf.len, type_name(type), oid))\n+\t\tret = err(o, _(\"Unable to add %s to database\"), path);\n+\n+\tstrbuf_release(&header);\n+\tstrbuf_release(&dst_buf);\n+\treturn ret;\n+}\n+\n static int add_cacheinfo(struct merge_options *o,\n \t\t\t unsigned int mode, const struct object_id *oid,\n \t\t\t const char *path, int stage, int refresh, int options)\n@@ -1464,6 +1554,9 @@ static int handle_change_delete(struct merge_options *o,\n {\n \tchar *alt_path = NULL;\n \tconst char *update_path = path;\n+\tstruct object_id new_oid;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint is_binary;\n \tint ret = 0;\n \n \tif (dir_in_way(path, !o->call_depth && !S_ISGITLINK(changed_mode), 0) ||\n@@ -1527,8 +1620,44 @@ static int handle_change_delete(struct merge_options *o,\n \t\t * path.  We could call update_file_flags() with update_cache=0\n \t\t * and update_wd=0, but that's a no-op.\n \t\t */\n-\t\tif (change_branch != o->branch1 || alt_path)\n-\t\t\tret = update_file(o, 0, changed_oid, changed_mode, update_path);\n+\t\toidcpy(&new_oid, changed_oid);\n+\t\tstrbuf_addf(&sb, _(\"CONFLICT (%s/delete): %s deleted in %s and %s in %s.\"),\n+\t\t\t    change, path, delete_branch, change_past, change_branch);\n+\t\t /*\n+\t\t  * FIXME: figure out if update_path's contents are binary;\n+\t\t  * buffer_is_binary() may help, though in the case of e.g.\n+\t\t  * add/add conflicts it'd be nice to avoid calling that\n+\t\t  * multiple times per buffer.\n+\t\t  */\n+\t\tis_binary = 0;\n+\t\tif (S_ISREG(changed_mode) && !is_binary) {\n+\t\t\tinsert_non_textual_conflict_header(\n+\t\t\t    o,\n+\t\t\t    &sb,\n+\t\t\t    update_path,\n+\t\t\t    &new_oid,\n+\t\t\t    changed_mode);\n+\t\t\tret = update_file_flags(o, &new_oid, changed_mode,\n+\t\t\t\t\t\tupdate_path,\n+\t\t\t\t\t\t/* update_cache */ 0,\n+\t\t\t\t\t\t/* update_wd */    1);\n+\t\t} else {\n+\t\t\tstruct diff_filespec conflict_file;\n+\t\t\tchar *conflict_path;\n+\t\t\tconflict_path = unique_path(o, update_path, \"conflicts\");\n+\n+\t\t\tconflict_file.mode = S_IFREG | 0644;\n+\t\t\tcreate_non_textual_conflict_file(o, &sb, conflict_path,\n+\t\t\t\t\t\t\t &conflict_file.oid);\n+\t\t\tret = update_file_flags(o, &conflict_file.oid,\n+\t\t\t\t\t\tconflict_file.mode,\n+\t\t\t\t\t\tconflict_path,\n+\t\t\t\t\t\t/* update_cache */ 0,\n+\t\t\t\t\t\t/* update_wd */    1);\n+\t\t\tret |= update_stages(o, conflict_path, &conflict_file,\n+\t\t\t\t\t     NULL, NULL);\n+\t\t\tfree(conflict_path);\n+\t\t}\n \t}\n \tfree(alt_path);\n \ndiff --git a/t/t3031-merge-criscross.sh b/t/t3031-merge-criscross.sh\nindex e59b0a32d6..5c39339809 100755\n--- a/t/t3031-merge-criscross.sh\n+++ b/t/t3031-merge-criscross.sh\n@@ -75,6 +75,7 @@ test_expect_success 'setup repo with criss-cross history' '\n \tgit checkout B &&\n \ttest_must_fail git merge E &&\n \t# force-resolve\n+\techo 9 >data/new-9 &&\n \tgit add data &&\n \tgit commit -m F &&\n \tgit branch F &&\n@@ -83,6 +84,7 @@ test_expect_success 'setup repo with criss-cross history' '\n \tgit checkout C &&\n \ttest_must_fail git merge D &&\n \t# force-resolve\n+\techo 9 >data/new-9 &&\n \tgit add data &&\n \tgit commit -m G &&\n \tgit branch G\ndiff --git a/t/t6022-merge-rename.sh b/t/t6022-merge-rename.sh\nindex b760c223c6..a065d79049 100755\n--- a/t/t6022-merge-rename.sh\n+++ b/t/t6022-merge-rename.sh\n@@ -467,7 +467,7 @@ test_expect_success 'both rename source and destination involved in D/F conflict\n \ttest -f destdir/foo &&\n \ttest -f one &&\n \ttest -f destdir~HEAD &&\n-\ttest \"stuff\" = \"$(cat destdir~HEAD)\"\n+\tgrep \"stuff\" destdir~HEAD\n '\n \n test_expect_success 'setup pair rename to parent of other (D/F conflicts)' '\n@@ -510,8 +510,8 @@ test_expect_success 'pair rename to parent of other (D/F conflicts) w/ untracked\n \ttest -d one &&\n \ttest -f one~rename-two &&\n \ttest -f two &&\n-\ttest \"other\" = $(cat one~rename-two) &&\n-\ttest \"stuff\" = $(cat two)\n+\tgrep \"other\" one~rename-two &&\n+\tgrep \"stuff\" two\n '\n \n test_expect_success 'pair rename to parent of other (D/F conflicts) w/ clean start' '\n@@ -529,8 +529,8 @@ test_expect_success 'pair rename to parent of other (D/F conflicts) w/ clean sta\n \n \ttest -f one &&\n \ttest -f two &&\n-\ttest \"other\" = $(cat one) &&\n-\ttest \"stuff\" = $(cat two)\n+\tgrep \"other\" one &&\n+\tgrep \"stuff\" two\n '\n \n test_expect_success 'setup rename of one file to two, with directories in the way' '\n@@ -704,35 +704,6 @@ test_expect_success 'avoid unnecessary update, dir->(file,nothing)' '\n \ttest_cmp expect actual # \"df\" should have stayed intact\n '\n \n-test_expect_success 'setup avoid unnecessary update, modify/delete' '\n-\tgit rm -rf . &&\n-\tgit clean -fdqx &&\n-\trm -rf .git &&\n-\tgit init &&\n-\n-\t>irrelevant &&\n-\t>file &&\n-\tgit add -A &&\n-\tgit commit -mA &&\n-\n-\tgit checkout -b side &&\n-\tgit rm -f file &&\n-\tgit commit -m \"Delete file\" &&\n-\n-\tgit checkout master &&\n-\techo bla >file &&\n-\tgit add -A &&\n-\tgit commit -m \"Modify file\"\n-'\n-\n-test_expect_success 'avoid unnecessary update, modify/delete' '\n-\tgit checkout -q master^0 &&\n-\ttest-tool chmtime --get =1000000000 file >expect &&\n-\ttest_must_fail git merge side &&\n-\ttest-tool chmtime --get file >actual &&\n-\ttest_cmp expect actual # \"file\" should have stayed intact\n-'\n-\n test_expect_success 'setup avoid unnecessary update, rename/add-dest' '\n \tgit rm -rf . &&\n \tgit clean -fdqx &&\ndiff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh\nindex 4a71f17edd..c5e1874df1 100755\n--- a/t/t6043-merge-rename-directories.sh\n+++ b/t/t6043-merge-rename-directories.sh\n@@ -1908,8 +1908,8 @@ test_expect_success '7e-check: transitive rename in rename/delete AND dirs in th\n \t\t\t A:x/d/f  A:y/d/g  O:z/b  O:z/c  O:x/d &&\n \t\ttest_cmp expect actual &&\n \n-\t\tgit hash-object y/d~B^0 >actual &&\n-\t\tgit rev-parse O:x/d >expect &&\n+\t\ttail -n +6 y/d~B^0 >actual &&\n+\t\tgit cat-file -p O:x/d >expect &&\n \t\ttest_cmp expect actual\n \t)\n '\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 047156e9d5..10c0c903c6 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -374,6 +374,7 @@ test_expect_success 'deleted vs modified submodule' '\n \t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n+\t( yes \"d\" | git mergetool submod~conflicts ) &&\n \trmdir submod && mv submod-movedaside submod &&\n \ttest \"$(cat submod/bar)\" = \"branch1 submodule\" &&\n \tgit submodule update -N &&\n@@ -391,6 +392,7 @@ test_expect_success 'deleted vs modified submodule' '\n \t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n+\t( yes \"d\" | git mergetool submod~conflicts ) &&\n \ttest ! -e submod &&\n \toutput=\"$(git mergetool --no-prompt)\" &&\n \ttest \"$output\" = \"No files need merging\" &&\n@@ -405,6 +407,7 @@ test_expect_success 'deleted vs modified submodule' '\n \t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n+\t( yes \"d\" | git mergetool submod~conflicts ) &&\n \ttest ! -e submod &&\n \ttest -d submod.orig &&\n \tgit submodule update -N &&\n@@ -421,6 +424,7 @@ test_expect_success 'deleted vs modified submodule' '\n \t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n+\t( yes \"d\" | git mergetool submod~conflicts ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n \tgit submodule update -N &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n-- \n2.18.0.550.g44d6daf40a.dirty\n\n"},{"id":"354693","messageId":"20180806224745.8681-1-newren@gmail.com","threadId":"49053","inReplyTo":"CABPp-BHxJyWsAQ3FkfdC-5Vqe3d7wWZm-hVYd0-afNY9dEgMeQ@mail.gmail.com","subject":"[RFC/WIP PATCH 0/3] Modifications to handling of non-textual file merge conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T22:47:42Z","receivedAt":"2018-08-06T22:47:51Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"== Summary ==\n\nFor non-textual conflicts, I would like to provide additional information\nin the working copy in the form of additional conflict markers and\nexplanatory text stating what type of non-textual conflict was involved.\nThis should\n  * Make it clearer to users what conflicts they are dealing with and why\n  * Enable new features like Thomas Rast' old remerge-diff proposal[1]\n\n[1] https://public-inbox.org/git/cover.1409860234.git.tr@thomasrast.ch/\n\nIf this sounds rather imprecise, concrete examples are provided in the\nnext section of this email.  If this change sounds surprising or\nnon-intuitive, more detailed rationale motivating this change (which is\nadmittedly slightly non-obvious) can be found in the remainder of this\nemail.  It may be the more of the information below needs to be moved into\nthe commit message for patch 3.\n\n== Examples of Proposal ==\n\nThere are two basic types of changes at play here, each best shown with a\nrepresentative example:\n\n1) Representative example: A modify/delete conflict; the path in question\nin the working tree would have conflict information at the top of the file\nfollowed by the normal file contents; thus it could be of the form:\n\n    <<<<<<<< HEAD\n    Conflict hint: This block of text was not part of the original\n    branch; it serves instead to hint about non-textual conflicts:\n      MODIFY/DELETE: path foo modified in HEAD and deleted in BRANCH\n    ========\n    Conflict hint: This block of text was not part of the original\n    branch; it serves instead to hint about non-textual conflicts:\n      MODIFY/DELETE: path foo modified in HEAD and deleted in BRANCH\n    >>>>>>>> BRANCH\n    Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n    sed diam nonumy eirmod tempor invidunt ut labore et dolore\n    magna aliquyam erat, sed diam voluptua. At vero eos et\n    accusam et justo duo dolores et ea rebum. Stet clita kasd\n    gubergren, no sea takimata sanctus est Lorem ipsum dolor\n    sit amet.\n\nAlternative ideas for handling the explanatory text here are welcome.  I\nchose to use identical text on both sides of the conflict in an attempt\nto highlight that this isn't a normal textual conflict and the text isn't\nmeant to be part of the file.\n\nThis type of example could apply for each of the following types of\nconflicts:\n  * modify/delete\n  * rename/delete\n  * directory/file\n  * submodule/file\n  * symlink/file\n  * rename/rename(1to2)\n  * executable mode conflict (i.e. 100644 vs. 100755 mode; could come\n    from add/add or modify/delete or rename/delete)\n\nIt could also be used for the following types of conflicts to help\ndifferentiate between it and other conflict types:\n  * add/add\n  * rename/add[/delete]\n  * rename/rename(2to1)[/delete[/delete]]\n  * rename/rename(1to2)/add[/add]\n\nHowever, any of the types above would be inappropriate if the regular\nfile(s) in question were binary; in those cases, they'd actually fall\ninto category two:\n\n\n2) Representative example: A binary edit/edit conflict.  In this case,\nit would be inappropriate to put the conflict markers inside the\nbinary file.  Instead, we create another file (e.g. path~CONFLICTS)\nand put conflict markers in it:\n\n    <<<<<<<< HEAD\n    Conflict hint: This block of text was not part of the original\n    branch; it serves instead to hint about non-textual conflicts:\n      BINARY conflict: path foo modified in both branches\n    ========\n    Conflict hint: This block of text was not part of the original\n    branch; it serves instead to hint about non-textual conflicts:\n      BINARY conflict: path foo modified in both branches\n    >>>>>>>> BRANCH\n\nThis file would also be added to the index at stage 1 (so that 'git merge\n--abort' would clean this file out instead of leaving it around untracked,\nand also because 'git status' would report \"deleted in both\" which seems\nreasonable).\n\nThis type of example could apply for each of the following types of\nconflicts:\n  * binary edit/edit\n  * any of the conflicts from type 1 when binary files are involved\n  * symlink edit/edit (or add/add)\n  * symlink/submodule\n  * symlink/directory\n  * directory/submodule\n  * submodule/submodule\n\nIt could also apply to the following new corner case conflict types from\ndirectory rename detection:\n  * N-way colliding paths (N>=2) due to directory renames\n  * directory rename split; half renamed to one directory and half to another\n\n\n== Motivation, part 1: Problem statement ==\n\nWhen conflicts arise we need ways to inform the user of the existence of\nthe conflicts and their nature.  For textual conflicts with regular files,\nwe have a simple way of doing this: inserting conflict markers into the\nrelevant region of the file with both conflicting versions present.\nImportantly, this representation of the conflict is present in the working\ncopy.\n\nFor other types of conflicts (path-based or non-regular files), we often\nprovide no hint in the working copy about either the existence or the\nnature of the conflict.  I think this is suboptimal from a users'\npoint-of-view, and is also limiting some feature development.\n\n== Motivation, part 2: Current non-textual conflict hints ==\n\nFor non-textual conflicts, the hints git currently gives the user come in\ntwo forms: messages printed during the merge, and higher order stages in\nthe index.  Both have some downsides.\n\nFor large repos, conflict messages (\"e.g. CONFLICT(modify/delete): ...\")\nprinted during the merge can easily be \"lost in the noise\" and might even\nbe inaccessible depending on the terminal scrollback buffer size.\nFurther, as the user begins resolving conflicts in that terminal, it\nbecomes harder and harder to find the original conflict messages for the\nremaining paths.\n\nWhile higher order stages in the index can be helpful, there are many more\nconflict types than there are permutations of higher order stages.  To\nname just one example, if all three higher order stages exist, what type\nof conflict is it?  It could be an edit/edit conflict, or a\nrename/add/delete conflict, or even a file from a directory/file conflict\nif that file was involved in a rename.\n\n== Motivation, part 3: Disappearing conflict hints ==\n\nI want to revive Thomas Rast' remerge-diff feature proposal.  To implement\nthat feature, he essentially does an auto-merge of the parent commits and\nrecords a resulting tree.  That tree includes conflict information, namely\nin the form of files that have conflict markers in them.  He then diffs\nthis auto-merged tree to the actual tree of the merge commit.\n\nI like the idea of an auto-merge tree with conflict information, but note\nthat this means printed conflict messages and higher order index entries\nwill be _completely_ lost, making it important that there be a way of\nstoring hints about conflicts in the working tree.\n\n(Side note: Thomas' old proposal partially address this; he takes paths\nthat only had either a stage 2 or 3 entry and does a two-way diff with an\nempty file.  That is a very reasonable first cut, but it misses lots of\ninformation.  For example, binary conflicts and mode conflicts would\nessentially be ignored.  Differentation between conflict types -- which\nmay be important or helpful to users trying to understand what happened --\nwould be lost.)\n\n\nElijah Newren (3):\n  rerere: avoid buffer overrun\n  merge-recursive: fix handling of submodules in modify/delete conflicts\n  merge-recursive: provide more conflict hints for non-textual conflicts\n\n merge-recursive.c                   | 135 +++++++++++++++++++++++++++-\n rerere.c                            |   2 +-\n t/t3031-merge-criscross.sh          |   2 +\n t/t6022-merge-rename.sh             |  39 ++------\n t/t6043-merge-rename-directories.sh |   4 +-\n t/t7610-mergetool.sh                |   4 +\n 6 files changed, 146 insertions(+), 40 deletions(-)\n\n-- \n2.18.0.550.g44d6daf40a.dirty\n\n"},{"id":"354694","messageId":"20180806224745.8681-3-newren@gmail.com","threadId":"49053","inReplyTo":"20180806224745.8681-1-newren@gmail.com","subject":"[RFC/WIP PATCH 2/3] merge-recursive: fix handling of submodules in modify/delete conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T22:47:44Z","receivedAt":"2018-08-06T22:47:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Similar to commit c641ca670729 (\"merge-recursive: handle addition of\nsubmodule on our side of history\", 2017-11-14) a submodule can be\nconfused for a D/F conflict for modify/delete and rename/delete\nconflicts.  (To the code, it appears there is a directory in the way of\nus writing our expected path to the working tree, because our path is a\nsubmodule; i.e. the submodule is itself the directory and any directory\nis assumed to be a D/F conflict that is in the way.)  So, when we are\ndealing with a submodule, avoid checking the working copy for a\ndirectory being in the way.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n\nIt might be better to first check that the submodule existed on the HEAD\nside of the merge, because there could be a directory in the way of the\nsubmodule.  But that's starting to get ugly quick, and the real fix to\nmake this cleaner is the rewrite of merge-recursive that does an index-only\nmerge first, then updates the working copy later...\n\n merge-recursive.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1446e92bea..4832234073 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1466,7 +1466,7 @@ static int handle_change_delete(struct merge_options *o,\n \tconst char *update_path = path;\n \tint ret = 0;\n \n-\tif (dir_in_way(path, !o->call_depth, 0) ||\n+\tif (dir_in_way(path, !o->call_depth && !S_ISGITLINK(changed_mode), 0) ||\n \t    (!o->call_depth && would_lose_untracked(path))) {\n \t\tupdate_path = alt_path = unique_path(o, path, change_branch);\n \t}\n-- \n2.18.0.550.g44d6daf40a.dirty\n\n"},{"id":"354695","messageId":"20180806224745.8681-2-newren@gmail.com","threadId":"49053","inReplyTo":"20180806224745.8681-1-newren@gmail.com","subject":"[RFC/WIP PATCH 1/3] rerere: avoid buffer overrun","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-06T22:47:43Z","receivedAt":"2018-08-06T22:47:53Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"check_one_conflict() compares `i` to `active_nr` in two places to avoid\nbuffer overruns, but left out an important third location.  This has\nnot previously been a problem, because existing merge strategies have\ntended to not create entries at stage #1 that do not have a\ncorresponding entry at either stage #2 or stage #3.  However, this is\nnot guaranteed, so add a check to avoid segfaults.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n rerere.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 16c8aac621..7d22fb08c7 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -533,7 +533,7 @@ static int check_one_conflict(int i, int *type)\n \t}\n \n \t*type = PUNTED;\n-\twhile (ce_stage(active_cache[i]) == 1)\n+\twhile (i < active_nr && ce_stage(active_cache[i]) == 1)\n \t\ti++;\n \n \t/* Only handle regular files with both stages #2 and #3 */\n-- \n2.18.0.550.g44d6daf40a.dirty\n"},{"id":"355018","messageId":"xmqqbmabcuhf.fsf@gitster-ct.c.googlers.com","threadId":"49053","inReplyTo":"20180806224547.8619-2-newren@gmail.com","subject":"Re: [RFC/WIP PATCH 1/1] merge-recursive: make file/directory conflicts easier to resolve","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T17:36:44Z","receivedAt":"2018-08-09T17:36:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> File/directory conflicts are somewhat difficult to resolve; by way of\n> contrast, renames are much easier to handle.  For file/directory\n> conflicts, we already have to record the file under a different name in\n> the working copy due to the directory being in the way; if we also record\n> the file under a different name in the index then it simplifies matters\n> for the user, and ensures that 'git reset --hard' and 'git merge --abort'\n> will clean up files created by the merge operation.\n\nYeah, and then our file \"path\" renamed to \"path~2\" to make room for\ndirectory \"path\" they introduced, can be relocated to its final\nplace in the merge resolution, e.g. \"git mv path~2 path/ours\" or\n\"git mv path path.theirs && git mv path~2 path\".\n\nOf course, \"git rm\" and \"git mv\" must work sensibly, if we want this\nchange to be truly helpful--but if not, they need to be fixed ;-)\n\n"},{"id":"355020","messageId":"xmqq600jctr1.fsf@gitster-ct.c.googlers.com","threadId":"49053","inReplyTo":"20180806224745.8681-1-newren@gmail.com","subject":"Re: [RFC/WIP PATCH 0/3] Modifications to handling of non-textual file merge conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T17:52:34Z","receivedAt":"2018-08-09T17:52:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> 1) Representative example: A modify/delete conflict; the path in question\n> in the working tree would have conflict information at the top of the file\n> followed by the normal file contents; thus it could be of the form:\n>\n>     <<<<<<<< HEAD\n>     Conflict hint: This block of text was not part of the original\n>     branch; it serves instead to hint about non-textual conflicts:\n>       MODIFY/DELETE: path foo modified in HEAD and deleted in BRANCH\n>     ========\n>     Conflict hint: This block of text was not part of the original\n>     branch; it serves instead to hint about non-textual conflicts:\n>       MODIFY/DELETE: path foo modified in HEAD and deleted in BRANCH\n>     >>>>>>>> BRANCH\n>     Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n>     sed diam nonumy eirmod tempor invidunt ut labore et dolore\n>     magna aliquyam erat, sed diam voluptua. At vero eos et\n>     accusam et justo duo dolores et ea rebum. Stet clita kasd\n>     gubergren, no sea takimata sanctus est Lorem ipsum dolor\n>     sit amet.\n>\n> Alternative ideas for handling the explanatory text here are welcome.\n\nIn a modify/delete conflict, we currently do not leave any in-file\nclue, so smudging the modified side like this might be a change that\nhelps those who \"grep e '<<<<<<<'\" to find the set of paths that\nneed to be examined.  I personally do not feel it would be all that\nuseful, as \"ls-files -u\" is how I'd learn about these paths.\n\nWhat I would want to see when faced to a modify/delete conflict is\nhow the modification side changed the contents, as the change, or\nits moral equivalent, would need to be ported to other locations in\nthe context of the deleting side.  But I am not sure if it makes\nsense to attempt to somehow include \"diff HEAD...MERGE_HEAD\" (or the\nother way around) in the file to show the contents change on the\nmodification side.\n\n> 2) Representative example: A binary edit/edit conflict.  In this case,\n> it would be inappropriate to put the conflict markers inside the\n> binary file.  Instead, we create another file (e.g. path~CONFLICTS)\n> and put conflict markers in it:\n>\n>     <<<<<<<< HEAD\n>     Conflict hint: This block of text was not part of the original\n>     branch; it serves instead to hint about non-textual conflicts:\n>       BINARY conflict: path foo modified in both branches\n>     ========\n>     Conflict hint: This block of text was not part of the original\n>     branch; it serves instead to hint about non-textual conflicts:\n>       BINARY conflict: path foo modified in both branches\n>     >>>>>>>> BRANCH\n>\n> This file would also be added to the index at stage 1 (so that 'git merge\n> --abort' would clean this file out instead of leaving it around untracked,\n> and also because 'git status' would report \"deleted in both\" which seems\n> reasonable).\n>\n> This type of example could apply for each of the following types of\n> conflicts:\n>   * binary edit/edit\n>   * any of the conflicts from type 1 when binary files are involved\n>   * symlink edit/edit (or add/add)\n>   * symlink/submodule\n>   * symlink/directory\n>   * directory/submodule\n>   * submodule/submodule\n>\n> It could also apply to the following new corner case conflict types from\n> directory rename detection:\n>   * N-way colliding paths (N>=2) due to directory renames\n>   * directory rename split; half renamed to one directory and half to another\n\nHmph, I am starting to wonder if it may be easier to access if\ninstead you did not touch any working tree file to do any of the\nabove, and instead write a single file in $GIT_DIR/ to explain what\nkind of conflicts these paths are involved in.  That would probably\ngive a better and easier-to-read summary than \"ls-files -u\" output.\n\nOr do we have _enough_ information in the \"ls-files -u\" already to\ninfer \"Ah, we are in symlink edit/edit conflict.\", etc.?  If so,\nperhaps \"git status\" can be extended to show what kind of conflict\nthese paths are in by reading the higher-stage index entries (and\nlack of stages thereof, when dealing with a conflict with deletion\ninvolved)?\n"},{"id":"355023","messageId":"CABPp-BE23fzBPuR94Ukrn4Va_Aw2wsZ0DjjxRxbSYJLmCrvP6w@mail.gmail.com","threadId":"49053","inReplyTo":"xmqq600jctr1.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC/WIP PATCH 0/3] Modifications to handling of non-textual file merge conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-09T18:51:24Z","receivedAt":"2018-08-09T18:51:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 9, 2018 at 10:52 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > 1) Representative example: A modify/delete conflict; the path in question\n> > in the working tree would have conflict information at the top of the file\n> > followed by the normal file contents; thus it could be of the form:\n> >\n> >     <<<<<<<< HEAD\n> >     Conflict hint: This block of text was not part of the original\n> >     branch; it serves instead to hint about non-textual conflicts:\n> >       MODIFY/DELETE: path foo modified in HEAD and deleted in BRANCH\n> >     ========\n> >     Conflict hint: This block of text was not part of the original\n> >     branch; it serves instead to hint about non-textual conflicts:\n> >       MODIFY/DELETE: path foo modified in HEAD and deleted in BRANCH\n> >     >>>>>>>> BRANCH\n> >     Lorem ipsum dolor sit amet, consectetuer sadipscing elitr,\n> >     sed diam nonumy eirmod tempor invidunt ut labore et dolore\n> >     magna aliquyam erat, sed diam voluptua. At vero eos et\n> >     accusam et justo duo dolores et ea rebum. Stet clita kasd\n> >     gubergren, no sea takimata sanctus est Lorem ipsum dolor\n> >     sit amet.\n> >\n> > Alternative ideas for handling the explanatory text here are welcome.\n>\n> In a modify/delete conflict, we currently do not leave any in-file\n> clue, so smudging the modified side like this might be a change that\n> helps those who \"grep e '<<<<<<<'\" to find the set of paths that\n> need to be examined.\n\nYes, that's one of the things I'm hoping for with this change.\n\n>  I personally do not feel it would be all that\n> useful, as \"ls-files -u\" is how I'd learn about these paths.\n\n\"ls-files -u\" is really nice; I love it and try to show it to others,\nbut keep getting surprised by how many people are surprised to learn\nof its existence.\n\nFurther, \"ls-files -u\" may be insufficient even for those who know\nabout it, for two reasons (and admittedly, I care a lot more about the\nsecond than the first):\n\n* There are more conflict types than the number of permutations of\nstages.  For example, how do you know if a file was from a\nrename/delete or a modify/delete conflict?  And if from a\nrename/delete, how do you determine the original filename?  The index\ndoesn't store this information.  Granted those two conflict types are\nat least similar, but other stage combinations might be more\nconfusing.  For example, if all three stages are present, is the\nconflict in question an edit/edit, a rename/add/delete, or a D/F\nconflict where the file came from a rename?\n\n* Future feature: Thomas Rast' proposed remerge-diff[1] (which I want\nto resurrect and remove the edge/corner cases from).  This worked by\ncreating what I call an auto-merged tree, where you just accept all\nconflicts and commit.  Then you diff the merge commit to what the\nauto-merge tree was to see how the user edited conflicts to create the\nmerge commit.  Problem is, for the auto-merge tree we won't have an\nindex anymore so how do we represent conflicts that aren't edit/edit\nof normal text files?  My proposal here is an answer for that; I'm\nopen to others, but it's the best I came up with.\n\n[1] https://public-inbox.org/git/cover.1409860234.git.tr@thomasrast.ch/\n\n(I'll also note here that others have requested the ability to create\nan as-merged-as-possibly tree including conflicts for other purposes;\nsee \"API suggestion\" of\nhttps://public-inbox.org/git/CAFAcib-2fxiVxtVWcbvafY3-Br7Y70HMiHFZoT0VfK6JU0Dp9A@mail.gmail.com/\nand my response noting the various difficulties with non-textual\nconflicts.  So it may not be just the remerge-diff ability here.)\n\n> What I would want to see when faced to a modify/delete conflict is\n> how the modification side changed the contents, as the change, or\n> its moral equivalent, would need to be ported to other locations in\n> the context of the deleting side.  But I am not sure if it makes\n> sense to attempt to somehow include \"diff HEAD...MERGE_HEAD\" (or the\n> other way around) in the file to show the contents change on the\n> modification side.\n\nThat's a good point; I'm not sure how to include that either.\nSomething to think about.\n\n> > 2) Representative example: A binary edit/edit conflict.  In this case,\n> > it would be inappropriate to put the conflict markers inside the\n> > binary file.  Instead, we create another file (e.g. path~CONFLICTS)\n> > and put conflict markers in it:\n> >\n> >     <<<<<<<< HEAD\n> >     Conflict hint: This block of text was not part of the original\n> >     branch; it serves instead to hint about non-textual conflicts:\n> >       BINARY conflict: path foo modified in both branches\n> >     ========\n> >     Conflict hint: This block of text was not part of the original\n> >     branch; it serves instead to hint about non-textual conflicts:\n> >       BINARY conflict: path foo modified in both branches\n> >     >>>>>>>> BRANCH\n> >\n> > This file would also be added to the index at stage 1 (so that 'git merge\n> > --abort' would clean this file out instead of leaving it around untracked,\n> > and also because 'git status' would report \"deleted in both\" which seems\n> > reasonable).\n> >\n> > This type of example could apply for each of the following types of\n> > conflicts:\n> >   * binary edit/edit\n> >   * any of the conflicts from type 1 when binary files are involved\n> >   * symlink edit/edit (or add/add)\n> >   * symlink/submodule\n> >   * symlink/directory\n> >   * directory/submodule\n> >   * submodule/submodule\n> >\n> > It could also apply to the following new corner case conflict types from\n> > directory rename detection:\n> >   * N-way colliding paths (N>=2) due to directory renames\n> >   * directory rename split; half renamed to one directory and half to another\n>\n> Hmph, I am starting to wonder if it may be easier to access if\n> instead you did not touch any working tree file to do any of the\n> above, and instead write a single file in $GIT_DIR/ to explain what\n> kind of conflicts these paths are involved in.  That would probably\n> give a better and easier-to-read summary than \"ls-files -u\" output.\n\nThat's interesting, and would be an alternate way to help some users.\nI think it's less discoverable than sticking the info in files in the\nworking tree for the user to see (particularly for the users who are\nallergic to git commands and just stay in their IDE as much as\npossible), but there might be something interesting here.\n\nHowever, I don't see how this would cover the remerge-diff usecase (or\nmore general create-as-merged-as-possible-tree-with-conflicts-in-it\nusecases) at all, which was my biggest motivation for these changes.\nIs there a good solution there?\n\n> Or do we have _enough_ information in the \"ls-files -u\" already to\n> infer \"Ah, we are in symlink edit/edit conflict.\", etc.?  If so,\n> perhaps \"git status\" can be extended to show what kind of conflict\n> these paths are in by reading the higher-stage index entries (and\n> lack of stages thereof, when dealing with a conflict with deletion\n> involved)?\n\nWe have enough information in _some_ cases to determine which type of\nconflict it is from the index (e.g. because we don't detect renames\nfor symlinks or submodules and thus have fewer conflict types possible\ninvolving those).  However, we again run into problems (at least for\nbinaries) about there being more conflict types than permutations of\nstage entries, and for all conflicts, we run into the same problem of\nnot having an index at all for the remerge-diff capability.\n"},{"id":"355027","messageId":"CABPp-BH8oiJ17K6OHCikAxk8Jy9M7mSA=WCVX9KosTC+p4rE4w@mail.gmail.com","threadId":"49053","inReplyTo":"xmqqbmabcuhf.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC/WIP PATCH 1/1] merge-recursive: make file/directory conflicts easier to resolve","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-09T19:26:44Z","receivedAt":"2018-08-09T19:26:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 9, 2018 at 10:36 AM Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > File/directory conflicts are somewhat difficult to resolve; by way of\n> > contrast, renames are much easier to handle.  For file/directory\n> > conflicts, we already have to record the file under a different name in\n> > the working copy due to the directory being in the way; if we also record\n> > the file under a different name in the index then it simplifies matters\n> > for the user, and ensures that 'git reset --hard' and 'git merge --abort'\n> > will clean up files created by the merge operation.\n>\n> Yeah, and then our file \"path\" renamed to \"path~2\" to make room for\n> directory \"path\" they introduced, can be relocated to its final\n> place in the merge resolution, e.g. \"git mv path~2 path/ours\" or\n> \"git mv path path.theirs && git mv path~2 path\".\n\nYes.  :-)\n\n> Of course, \"git rm\" and \"git mv\" must work sensibly, if we want this\n> change to be truly helpful--but if not, they need to be fixed ;-)\n\nThat actually brings up an interesting question.  Right now, if given\na path that appears in the index but at a stage greater than 0, git mv\nwill fail with \"not under version control\".  Obviously, that error\nmessage is a lie in such a case, but what should it do?  Print a more\naccurate error message (\"Refusing to rename file until you remove\nconflicts and git add it\"?)  Or, what I'd prefer, rename the entry (or\nentries) and keep the higher stage number(s) -- is there an unseen\ndanger with doing this?\n\n(Alternatively, if there is only one entry with stage greater than 0\nand it has no other conflicts, one could envision git mv doing the\nrename and dropping to stage 0 at the same time, but that sounds a bit\ndangerous to me.)\n"},{"id":"355035","messageId":"xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com","threadId":"49053","inReplyTo":"CABPp-BH8oiJ17K6OHCikAxk8Jy9M7mSA=WCVX9KosTC+p4rE4w@mail.gmail.com","subject":"Re: [RFC/WIP PATCH 1/1] merge-recursive: make file/directory conflicts easier to resolve","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-09T20:54:45Z","receivedAt":"2018-08-09T20:54:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n>> Of course, \"git rm\" and \"git mv\" must work sensibly, if we want this\n>> change to be truly helpful--but if not, they need to be fixed ;-)\n>\n> That actually brings up an interesting question.  Right now, if given\n> a path that appears in the index but at a stage greater than 0, git mv\n> will fail with \"not under version control\".  Obviously, that error\n> message is a lie in such a case, but what should it do?\n>\n> (Alternatively, if there is only one entry with stage greater than 0\n> and it has no other conflicts, one could envision git mv doing the\n> rename and dropping to stage 0 at the same time, but that sounds a bit\n> dangerous to me.)\n\nI do not particularly think it is \"dangerous\".  In fact, that sort\nof behaviour was what I had in mind when I said \"work sensibly\".\n\nWhen resolving a conflict that they added a new path at stage #3 to\nremove that path, I can say \"git rm $that_path\", which removes all\nstages of that path and make the index closer to the next commit.\nOr I may decide to keep that path by \"git add $that_path\", which\nadds that path at stage #0.  I think \"git mv $that_path $over_there\"\nthat drops that path at stage #3 to stage #0 of another path would\nbe in line with these two.\n\n"},{"id":"355041","messageId":"CABPp-BG1t3hJsVWDKwy-dMM-CTY8voQfN=ebQ+BpJyJHSJ=gvg@mail.gmail.com","threadId":"49053","inReplyTo":"xmqqk1ozb6qy.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC/WIP PATCH 1/1] merge-recursive: make file/directory conflicts easier to resolve","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-08-09T21:27:34Z","receivedAt":"2018-08-09T21:27:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 9, 2018 at 1:54 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Elijah Newren <newren@gmail.com> writes:\n>\n> >> Of course, \"git rm\" and \"git mv\" must work sensibly, if we want this\n> >> change to be truly helpful--but if not, they need to be fixed ;-)\n> >\n> > That actually brings up an interesting question.  Right now, if given\n> > a path that appears in the index but at a stage greater than 0, git mv\n> > will fail with \"not under version control\".  Obviously, that error\n> > message is a lie in such a case, but what should it do?\n> >\n> > (Alternatively, if there is only one entry with stage greater than 0\n> > and it has no other conflicts, one could envision git mv doing the\n> > rename and dropping to stage 0 at the same time, but that sounds a bit\n> > dangerous to me.)\n>\n> I do not particularly think it is \"dangerous\".  In fact, that sort\n> of behaviour was what I had in mind when I said \"work sensibly\".\n>\n> When resolving a conflict that they added a new path at stage #3 to\n> remove that path, I can say \"git rm $that_path\", which removes all\n> stages of that path and make the index closer to the next commit.\n> Or I may decide to keep that path by \"git add $that_path\", which\n> adds that path at stage #0.  I think \"git mv $that_path $over_there\"\n> that drops that path at stage #3 to stage #0 of another path would\n> be in line with these two.\n\nThis argument makes sense to me *IF* there's no possibility for\ninternal textual conflicts.  But if there are textual conflicts, I\ndon't see how it works.  So either I'm misunderstanding part of what\nyou're suggesting or you may have overlooked that case.\n\nLet's say we did want to drop to stage #0 when the user runs git mv.\nI'm assuming you agree that'd be bad to do if there were still\nconflict markers left in that file (which can happen when the file of\na D/F conflict came from a renamed file that had edits on both sides\nof history, for example).  That suggests we have to open and parse the\nfile and look for conflict markers before dropping to stage #0 and\nonly proceeding when none are found.  That feels a bit magic; this\nauto-resolving-upon-mv seems to risk surprising someone to me.  In\nparticular, I'm imagining a scenario where someone edits some file\nenough to remove conflict markers but isn't satisfied that everything\nis semantically resolved yet, then runs git mv on the file, then\nstarts working on other files, and then tries to come back to the\noriginal file only to discover that they can't find it in the list of\nunmerged files because we marked it as resolved for them.\n\nAm I missing something here?\n"}]}