| Message ID | 4cc53c55a6ea1531342b23bc9343890a576d9f1c.1640419160.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a new --remerge-diff capability to show & log | expand |
On Sat, Dec 25, 2021 at 07:59:19AM +0000, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > Conflicts such as modify/delete, rename/rename, or file/directory are > not representable via content conflict markers, and the normal output > messages notifying users about these were dropped with --remerge-diff. > While we don't want these messages randomly shown before the commit > and diff headers, we do want them to still be shown; include them as > part of the diff headers instead. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > log-tree.c | 3 ++ > merge-ort.c | 1 + > merge-ort.h | 10 +++++ > t/t4069-remerge-diff.sh | 86 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 100 insertions(+) > > diff --git a/log-tree.c b/log-tree.c > index 33c28f537a6..97fbb756d21 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -922,6 +922,7 @@ static int do_remerge_diff(struct rev_info *opt, > /* Setup merge options */ > init_merge_options(&o, the_repository); > o.show_rename_progress = 0; > + o.record_conflict_msgs_as_headers = 1; > > ctx.abbrev = DEFAULT_ABBREV; > format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx); > @@ -938,10 +939,12 @@ static int do_remerge_diff(struct rev_info *opt, > merge_incore_recursive(&o, bases, parent1, parent2, &res); > > /* Show the diff */ > + opt->diffopt.additional_path_headers = res.path_messages; > diff_tree_oid(&res.tree->object.oid, oid, "", &opt->diffopt); > log_tree_diff_flush(opt); > > /* Cleanup */ > + opt->diffopt.additional_path_headers = NULL; > strbuf_release(&parent1_desc); > strbuf_release(&parent2_desc); > merge_finalize(&o, &res); > diff --git a/merge-ort.c b/merge-ort.c > index 9142d56e0ad..07e53083cbd 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -4579,6 +4579,7 @@ redo: > trace2_region_leave("merge", "process_entries", opt->repo); > > /* Set return values */ > + result->path_messages = &opt->priv->output; > result->tree = parse_tree_indirect(&working_tree_oid); > /* existence of conflicted entries implies unclean */ > result->clean &= strmap_empty(&opt->priv->conflicted); > diff --git a/merge-ort.h b/merge-ort.h > index c011864ffeb..fe599b87868 100644 > --- a/merge-ort.h > +++ b/merge-ort.h > @@ -5,6 +5,7 @@ > > struct commit; > struct tree; > +struct strmap; > > struct merge_result { > /* > @@ -23,6 +24,15 @@ struct merge_result { > */ > struct tree *tree; > > + /* > + * Special messages and conflict notices for various paths > + * > + * This is a map of pathnames to strbufs. It contains various > + * warning/conflict/notice messages (possibly multiple per path) > + * that callers may want to use. > + */ > + struct strmap *path_messages; > + > /* > * Additional metadata used by merge_switch_to_result() or future calls > * to merge_incore_*(). Includes data needed to update the index (if > diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh > index 192dbce2bfe..a040d3bcd91 100755 > --- a/t/t4069-remerge-diff.sh > +++ b/t/t4069-remerge-diff.sh > @@ -4,6 +4,15 @@ test_description='remerge-diff handling' > > . ./test-lib.sh > > +# --remerge-diff uses ort under the hood regardless of setting. However, > +# we set up a file/directory conflict beforehand, and the different backends > +# handle the conflict differently, which would require separate code paths > +# to resolve. There's not much point in making the code uglier to do that, > +# though, when the real thing we are testing (--remerge-diff) will hardcode > +# calls directly into the merge-ort API anyway. So just force the use of > +# ort on the setup too. > +GIT_TEST_MERGE_ALGORITHM=ort > + > test_expect_success 'setup basic merges' ' > test_write_lines 1 2 3 4 5 6 7 8 9 >numbers && > git add numbers && > @@ -55,6 +64,7 @@ test_expect_success 'remerge-diff with both a resolved conflict and an unrelated > git log -1 --oneline ab_resolution >tmp && > cat <<-EOF >>tmp && > diff --git a/numbers b/numbers > + CONFLICT (content): Merge conflict in numbers > index a1fb731..6875544 100644 > --- a/numbers > +++ b/numbers > @@ -83,4 +93,80 @@ test_expect_success 'remerge-diff with both a resolved conflict and an unrelated > test_cmp expect actual > ' > > +test_expect_success 'setup non-content conflicts' ' > + git switch --orphan base && > + > + test_write_lines 1 2 3 4 5 6 7 8 9 >numbers && > + test_write_lines a b c d e f g h i >letters && > + test_write_lines in the way >content && > + git add numbers letters content && > + git commit -m base && > + > + git branch side1 && > + git branch side2 && > + > + git checkout side1 && > + test_write_lines 1 2 three 4 5 6 7 8 9 >numbers && > + git mv letters letters_side1 && > + git mv content file_or_directory && > + git add numbers && > + git commit -m side1 && > + > + git checkout side2 && > + git rm numbers && > + git mv letters letters_side2 && > + mkdir file_or_directory && > + echo hello >file_or_directory/world && > + git add file_or_directory/world && > + git commit -m side2 && > + > + git checkout -b resolution side1 && > + test_must_fail git merge side2 && > + test_write_lines 1 2 three 4 5 6 7 8 9 >numbers && > + git add numbers && > + git add letters_side1 && > + git rm letters && > + git rm letters_side2 && > + git add file_or_directory~HEAD && > + git mv file_or_directory~HEAD wanted_content && > + git commit -m resolved > +' > + > +test_expect_success 'remerge-diff with non-content conflicts' ' > + git log -1 --oneline resolution >tmp && > + cat <<-EOF >>tmp && > + diff --git a/file_or_directory~HASH (side1) b/wanted_content the "~HASH (side1)" suffix will probably mess with some programs that extract the filename from the diff. I don't know what programs are supposed to expect. I can see arguments for either dropping the suffix or including only "~HASH" since that's part of the actual filename that's left in the worktree. Maybe it's not so important. The file/link typechange conflict test I'll add below exposes what looks like an accidental interaction with the trailing tab characters that we emit on --- and +++ lines if the "filename" contains a space (since 1a9eb3b9d5 (git-diff/git-apply: make diff output a bit friendlier to GNU patch (part 2), 2006-09-22)). index 70885e4..0000000 --- a/typechange~738109f (side1) <-- git diff adds a trailing tab! +++ /dev/null I haven't formed an opinion yet, but since Tig uses the --- and +++ lines to extract file names, I'd drop the " (side1)" suffix from at least the --- and +++ lines. Maybe also the ^diff lines, I'm not sure > + similarity index 100% > + rename from file_or_directory~HASH (side1) > + rename to wanted_content > + CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead. I wonder if it's better to have this line further up, before the "rename" resolution, to correct the temporal order. > + diff --git a/letters b/letters > + CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2). > + diff --git a/letters_side2 b/letters_side2 > + deleted file mode 100644 > + index b236ae5..0000000 > + --- a/letters_side2 > + +++ /dev/null > + @@ -1,9 +0,0 @@ > + -a > + -b > + -c > + -d > + -e > + -f > + -g > + -h > + -i > + diff --git a/numbers b/numbers > + CONFLICT (modify/delete): numbers deleted in HASH (side2) and modified in HASH (side1). Version HASH (side1) of numbers left in tree. > + EOF Took me some time to grok these but the output makes sense (it's loud and ugly but that's okay since these are serious conflicts). > + # We still have some sha1 hashes above; rip them out so test works > + # with sha256 > + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect && > + > + git show --oneline --remerge-diff resolution >tmp && > + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual && > + test_cmp expect actual > +' > + > test_done > -- > gitgitgadget We're missing a test case for typechange. Here's is a quick draft I've been playing around with. Seems ugly that the "diff --git a/typechange b/typechange" is doubled but okay. Maybe a rename/delete conflict is interesting as well, I'm not sure. (Also I wonder if switching the order of parents will give any interesting difference, I guess not) test_expect_success 'remerge-diff with file/link conflict' ' git branch -d base side1 side2 && git switch --orphan base && echo base >typechange && git add typechange && git commit -m base && git branch side1 && git branch side2 && git checkout side1 && echo orig-file-contents >typechange && git commit -a -m side1 && git checkout side2 && ln -sf . typechange && git add typechange && git commit -m side2 && git checkout -b resolution2 side1 && test_must_fail git merge side2 && rm typechange && mv typechange~HEAD typechange && echo resolved >>typechange && git add typechange~HEAD typechange && git merge --continue && git show --oneline --remerge-diff resolution2 >tmp && sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual && cat <<-EOF >tmp && 7759b27 Merge branch ${SQ}side2${SQ} into resolution2 diff --git a/typechange b/typechange deleted file mode 120000 CONFLICT (distinct types): typechange had different types on each side; renamed one of them so each can be recorded somewhere. index 945c9b4..0000000 --- a/typechange +++ /dev/null @@ -1 +0,0 @@ -. \ No newline at end of file diff --git a/typechange b/typechange new file mode 100644 CONFLICT (distinct types): typechange had different types on each side; renamed one of them so each can be recorded somewhere. index 0000000..70885e4 --- /dev/null +++ b/typechange @@ -0,0 +1,2 @@ +orig-file-contents +resolved diff --git a/typechange~738109f (side1) b/typechange~738109f (side1) deleted file mode 100644 index 70885e4..0000000 --- a/typechange~738109f (side1) +++ /dev/null @@ -1 +0,0 @@ -orig-file-contents EOF # We still have some sha1 hashes above; rip them out so test works # with sha256 sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect && test_cmp expect actual '
On Tue, Dec 28, 2021 at 2:57 AM Johannes Altmanninger <aclopte@gmail.com> wrote: > > On Sat, Dec 25, 2021 at 07:59:19AM +0000, Elijah Newren via GitGitGadget wrote: ... > > +test_expect_success 'remerge-diff with non-content conflicts' ' > > + git log -1 --oneline resolution >tmp && > > + cat <<-EOF >>tmp && > > + diff --git a/file_or_directory~HASH (side1) b/wanted_content > > the "~HASH (side1)" suffix will probably mess with some programs that extract > the filename from the diff. "~HASH (side1)" is part of the filename, so this won't mess those programs up at all (unless those programs can't deal with filenames containing spaces or parentheses or something). > I don't know what programs are supposed to expect. I can see arguments for > either dropping the suffix or including only "~HASH" since that's part of > the actual filename that's left in the worktree. When there are conflicts that prevent the file from being recorded in the tree, such as file/directory conflicts, we have to rename the file elsewhere. We want the new name to be something that the user can find and reason about. So, both merge-recursive and merge-ort use ${filename}~${branchname}, where ${branchname} is defined in the struct merge_options (opt->branch1 or opt->branch2) that were passed in to the function. For a regular `git merge` we just use opt->branch1 = "HEAD" and opt->branch2 = <name of branch/commit typed on command line to merge>. Neither of those strings make sense for remerge-diff. We could just use hashes, but why are users going to be familiar with the hashes of the parents of a merge commit when looking at --remerge-diff output? Parents are not part of the output by default. So, log-tree.c uses this bit of logic format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx); format_commit_message(parent2, "%h (%s)", &parent2_desc, &ctx); o.branch1 = parent1_desc.buf; o.branch2 = parent2_desc.buf; which means that the branch name is of the form "$HASH ($EXTRA_DESCRIPTION)", and thus that files in file/directory conflicts (or add/add + file/symlink conflicts or file/submodule conflicts) will get renamed to a file of the form "$FILENAME~$HASH ($EXTRA_DESCRIPTION)" Note that these branch names also appear in CONFLICT messages, in conflict markers, etc., and in fact are used much more frequently in those locations. In those places it's perhaps even more important to attempt to provide meaningful names, so dropping the extra description doesn't make sense. > The file/link typechange conflict test I'll add below exposes what looks > like an accidental interaction with the trailing tab characters that we emit > on --- and +++ lines if the "filename" contains a space (since 1a9eb3b9d5 > (git-diff/git-apply: make diff output a bit friendlier to GNU patch (part > 2), 2006-09-22)). > > index 70885e4..0000000 > --- a/typechange~738109f (side1) <-- git diff adds a trailing tab! > +++ /dev/null > > I haven't formed an opinion yet, but since Tig uses the --- and +++ lines > to extract file names, I'd drop the " (side1)" suffix from at least the --- > and +++ lines. Maybe also the ^diff lines, I'm not sure As above, " (side1)" is part of the filename and thus belongs here. > > + similarity index 100% > > + rename from file_or_directory~HASH (side1) > > + rename to wanted_content > > + CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead. > > I wonder if it's better to have this line further up, before the "rename" > resolution, to correct the temporal order. Yeah, I've gone back and forth about where these would best be placed. You make a good point, even if the code is slightly uglier to move earlier. However, I do really like having the CONFLICT notices being close to the file text being shown, which makes me conflicted (no pun intended) about moving it earlier. Hmm.... > > + diff --git a/letters b/letters > > + CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2). > > + diff --git a/letters_side2 b/letters_side2 > > + deleted file mode 100644 > > + index b236ae5..0000000 > > + --- a/letters_side2 > > + +++ /dev/null > > + @@ -1,9 +0,0 @@ > > + -a > > + -b > > + -c > > + -d > > + -e > > + -f > > + -g > > + -h > > + -i > > + diff --git a/numbers b/numbers > > + CONFLICT (modify/delete): numbers deleted in HASH (side2) and modified in HASH (side1). Version HASH (side1) of numbers left in tree. > > + EOF > > Took me some time to grok these but the output makes sense (it's loud and > ugly but that's okay since these are serious conflicts). > > > + # We still have some sha1 hashes above; rip them out so test works > > + # with sha256 > > + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect && > > + > > + git show --oneline --remerge-diff resolution >tmp && > > + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual && > > + test_cmp expect actual > > +' > > + > > test_done > > -- > > gitgitgadget > > We're missing a test case for typechange. <grin> ...and a testcase for an add/add conflict, and a file/submodule conflict, and file/symlink, and symlink/submodule, and submodule/submodule (different submodules both added at same path), and binary merge conflict, and a variety of failure-to-merge-submodule-updates, and at least half a dozen regular rename-based conflict types, and several directory-rename-based conflict types...and that's just beginning to scratch the surface once you start dreaming of combinations of the different conflict types occurring for the same path (in particular, I'm thinking of examples from the testcases found in t6416, t6422, t6423 -- or at least sections 7, 9, & 12 of t6423). I don't think providing a comprehensive set of possible conflicts is useful here; we just need a representative sample. I was curious whether that was best served with just two examples or three, but ultimately decided on 3. I would have been more likely to pick 2 than 4, though. However, while I fail to see how typechange stresses --remerge-diff in ways the other conflict types don't, or how it might help clarify the output for users, I might be overlooking something. Is there a particular reason you wanted to see the typechange conflict included? > Here's is a quick draft I've been > playing around with. Seems ugly that the "diff --git a/typechange b/typechange" > is doubled but okay. > > Maybe a rename/delete conflict is interesting as well, I'm not sure. (Also I > wonder if switching the order of parents will give any interesting difference, > I guess not) > > test_expect_success 'remerge-diff with file/link conflict' ' > git branch -d base side1 side2 && > git switch --orphan base && I'd rather have subdirectories with git repositories (much like t6416, t6422, and t6423) if we're going to be adding many more tests here. > echo base >typechange && > git add typechange && > git commit -m base && > > git branch side1 && > git branch side2 && > > git checkout side1 && > echo orig-file-contents >typechange && > git commit -a -m side1 && > > git checkout side2 && > ln -sf . typechange && > git add typechange && > git commit -m side2 && > > git checkout -b resolution2 side1 && > test_must_fail git merge side2 && > rm typechange && > mv typechange~HEAD typechange && > echo resolved >>typechange && > git add typechange~HEAD typechange && > git merge --continue && > > git show --oneline --remerge-diff resolution2 >tmp && > sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual && > > cat <<-EOF >tmp && > 7759b27 Merge branch ${SQ}side2${SQ} into resolution2 > diff --git a/typechange b/typechange > deleted file mode 120000 > CONFLICT (distinct types): typechange had different types on each side; renamed one of them so each can be recorded somewhere. > index 945c9b4..0000000 > --- a/typechange > +++ /dev/null > @@ -1 +0,0 @@ > -. > \ No newline at end of file > diff --git a/typechange b/typechange > new file mode 100644 > CONFLICT (distinct types): typechange had different types on each side; renamed one of them so each can be recorded somewhere. > index 0000000..70885e4 > --- /dev/null > +++ b/typechange > @@ -0,0 +1,2 @@ > +orig-file-contents > +resolved > diff --git a/typechange~738109f (side1) b/typechange~738109f (side1) > deleted file mode 100644 > index 70885e4..0000000 > --- a/typechange~738109f (side1) > +++ /dev/null > @@ -1 +0,0 @@ > -orig-file-contents > EOF > # We still have some sha1 hashes above; rip them out so test works > # with sha256 > sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect && > > test_cmp expect actual > ' but otherwise the testcase looks good, if we want it.
diff --git a/log-tree.c b/log-tree.c index 33c28f537a6..97fbb756d21 100644 --- a/log-tree.c +++ b/log-tree.c @@ -922,6 +922,7 @@ static int do_remerge_diff(struct rev_info *opt, /* Setup merge options */ init_merge_options(&o, the_repository); o.show_rename_progress = 0; + o.record_conflict_msgs_as_headers = 1; ctx.abbrev = DEFAULT_ABBREV; format_commit_message(parent1, "%h (%s)", &parent1_desc, &ctx); @@ -938,10 +939,12 @@ static int do_remerge_diff(struct rev_info *opt, merge_incore_recursive(&o, bases, parent1, parent2, &res); /* Show the diff */ + opt->diffopt.additional_path_headers = res.path_messages; diff_tree_oid(&res.tree->object.oid, oid, "", &opt->diffopt); log_tree_diff_flush(opt); /* Cleanup */ + opt->diffopt.additional_path_headers = NULL; strbuf_release(&parent1_desc); strbuf_release(&parent2_desc); merge_finalize(&o, &res); diff --git a/merge-ort.c b/merge-ort.c index 9142d56e0ad..07e53083cbd 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -4579,6 +4579,7 @@ redo: trace2_region_leave("merge", "process_entries", opt->repo); /* Set return values */ + result->path_messages = &opt->priv->output; result->tree = parse_tree_indirect(&working_tree_oid); /* existence of conflicted entries implies unclean */ result->clean &= strmap_empty(&opt->priv->conflicted); diff --git a/merge-ort.h b/merge-ort.h index c011864ffeb..fe599b87868 100644 --- a/merge-ort.h +++ b/merge-ort.h @@ -5,6 +5,7 @@ struct commit; struct tree; +struct strmap; struct merge_result { /* @@ -23,6 +24,15 @@ struct merge_result { */ struct tree *tree; + /* + * Special messages and conflict notices for various paths + * + * This is a map of pathnames to strbufs. It contains various + * warning/conflict/notice messages (possibly multiple per path) + * that callers may want to use. + */ + struct strmap *path_messages; + /* * Additional metadata used by merge_switch_to_result() or future calls * to merge_incore_*(). Includes data needed to update the index (if diff --git a/t/t4069-remerge-diff.sh b/t/t4069-remerge-diff.sh index 192dbce2bfe..a040d3bcd91 100755 --- a/t/t4069-remerge-diff.sh +++ b/t/t4069-remerge-diff.sh @@ -4,6 +4,15 @@ test_description='remerge-diff handling' . ./test-lib.sh +# --remerge-diff uses ort under the hood regardless of setting. However, +# we set up a file/directory conflict beforehand, and the different backends +# handle the conflict differently, which would require separate code paths +# to resolve. There's not much point in making the code uglier to do that, +# though, when the real thing we are testing (--remerge-diff) will hardcode +# calls directly into the merge-ort API anyway. So just force the use of +# ort on the setup too. +GIT_TEST_MERGE_ALGORITHM=ort + test_expect_success 'setup basic merges' ' test_write_lines 1 2 3 4 5 6 7 8 9 >numbers && git add numbers && @@ -55,6 +64,7 @@ test_expect_success 'remerge-diff with both a resolved conflict and an unrelated git log -1 --oneline ab_resolution >tmp && cat <<-EOF >>tmp && diff --git a/numbers b/numbers + CONFLICT (content): Merge conflict in numbers index a1fb731..6875544 100644 --- a/numbers +++ b/numbers @@ -83,4 +93,80 @@ test_expect_success 'remerge-diff with both a resolved conflict and an unrelated test_cmp expect actual ' +test_expect_success 'setup non-content conflicts' ' + git switch --orphan base && + + test_write_lines 1 2 3 4 5 6 7 8 9 >numbers && + test_write_lines a b c d e f g h i >letters && + test_write_lines in the way >content && + git add numbers letters content && + git commit -m base && + + git branch side1 && + git branch side2 && + + git checkout side1 && + test_write_lines 1 2 three 4 5 6 7 8 9 >numbers && + git mv letters letters_side1 && + git mv content file_or_directory && + git add numbers && + git commit -m side1 && + + git checkout side2 && + git rm numbers && + git mv letters letters_side2 && + mkdir file_or_directory && + echo hello >file_or_directory/world && + git add file_or_directory/world && + git commit -m side2 && + + git checkout -b resolution side1 && + test_must_fail git merge side2 && + test_write_lines 1 2 three 4 5 6 7 8 9 >numbers && + git add numbers && + git add letters_side1 && + git rm letters && + git rm letters_side2 && + git add file_or_directory~HEAD && + git mv file_or_directory~HEAD wanted_content && + git commit -m resolved +' + +test_expect_success 'remerge-diff with non-content conflicts' ' + git log -1 --oneline resolution >tmp && + cat <<-EOF >>tmp && + diff --git a/file_or_directory~HASH (side1) b/wanted_content + similarity index 100% + rename from file_or_directory~HASH (side1) + rename to wanted_content + CONFLICT (file/directory): directory in the way of file_or_directory from HASH (side1); moving it to file_or_directory~HASH (side1) instead. + diff --git a/letters b/letters + CONFLICT (rename/rename): letters renamed to letters_side1 in HASH (side1) and to letters_side2 in HASH (side2). + diff --git a/letters_side2 b/letters_side2 + deleted file mode 100644 + index b236ae5..0000000 + --- a/letters_side2 + +++ /dev/null + @@ -1,9 +0,0 @@ + -a + -b + -c + -d + -e + -f + -g + -h + -i + diff --git a/numbers b/numbers + CONFLICT (modify/delete): numbers deleted in HASH (side2) and modified in HASH (side1). Version HASH (side1) of numbers left in tree. + EOF + # We still have some sha1 hashes above; rip them out so test works + # with sha256 + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >expect && + + git show --oneline --remerge-diff resolution >tmp && + sed -e "s/[0-9a-f]\{7,\}/HASH/g" tmp >actual && + test_cmp expect actual +' + test_done