| Message ID | df6e2774f1a5560a598dd8b46131bc6b0a261d4a.1630376800.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a new --remerge-diff capability to show & log | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > path_msg() has the ability to mark messages as omittable when recording > conflict messages in an in-tree file. This conflict message file will > then be shown by diffing the merge-created tree to the actual merge > commit that is created. While all the messages touched in this commit > are very useful when trying to create a merge initially, early use with > the --remerge-diff feature (the only user of this omittable conflict > message capability), suggests that the particular messages marked in > this commit are just noise when trying to see what changes users made to > create a merge commit. Mark them as omittable. Sorry for asking something that may be obvious, but if you recreate an automated and potentially conflicting merge in-core, in order to then compare the recorded merge result, is there *any* message that is worth showing while doing the first step, and where in the output do users see them?
On Tue, Aug 31, 2021 at 2:06 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > path_msg() has the ability to mark messages as omittable when recording > > conflict messages in an in-tree file. This conflict message file will > > then be shown by diffing the merge-created tree to the actual merge > > commit that is created. While all the messages touched in this commit > > are very useful when trying to create a merge initially, early use with > > the --remerge-diff feature (the only user of this omittable conflict > > message capability), suggests that the particular messages marked in > > this commit are just noise when trying to see what changes users made to > > create a merge commit. Mark them as omittable. > > Sorry for asking something that may be obvious No, it's not obvious. I had a hard time figuring out the right way to split up this series and order the patches in part because so many little pieces are needed before I can show the solution. So some of this comes from the later patches, but let me see if I can motivate the issue and solution I picked a bit more right here. > but if you recreate > an automated and potentially conflicting merge in-core, in order to > then compare the recorded merge result, is there *any* message that > is worth showing while doing the first step, Yes, absolutely. While three-way *content* conflicts are easily representable within a file in the working tree using conflict markers, *non-content* conflicts are usually not easily representable in such a fashion. For example, a rename/delete conflict will not result in any conflict markers, and will result in the renamed version of the file being left in the working tree; the only way you know there is a conflict for that file is either because of the stage in the index or the the message that is printed to the terminal: CONFLICT (rename/delete): %s renamed to %s in %s, but deleted in %s. But relying on higher order stages in the index and messages printed to the terminal present a bit of a problem for --remerge-diff; as described, it ignores both those sources of input. Here's a few other conflict types that face similar issues: * modify/delete conflicts * failure to merge submodule * directory rename detection (i.e. new file added to old directory that other side renamed; which directory should file end up in?) * distinct types of files (e.g. regular file and symlink) located at the same path * rename/rename conflicts In all these cases (and some others), relying on a diff of "what the working directory looks like at the end of a merge" to "the tree recorded in the merge commit" does not convey enough (if any) information about the above types of conflicts. > and where in the output do users see them? For --remerge-diff, I chose to handle the fact that the working directory didn't naturally have enough information, by augmenting the working directory with additional information. So, for example, if there was a file named `dir/my.file` that had a modify/delete conflict (and the user who did the real merge edited that file a bit as part of conflict resolution), then I would also add a `dir/my.fil e.conflict_msg` file whose contents are """ == Conflict notices for my.file == CONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT SUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2 (SHORT SUMMARY2) of dir/my.file left in tree. """ Creating this file means you see something like this in the remerge-diff (note there are diffs for two files, with the synthetic file appearing just before the file it has messages about): diff --git a/dir/my.fil e.conflict_msg b/dir/my.fil e.conflict_msg deleted file mode 100644 index 2bd215a32f06..000000000000 --- a/dir/my.fil e.conflict_msg +++ /dev/null @@ -1,2 +0,0 @@ -== Conflict notices for my.file == -CONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT SUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2 (SHORT SUMMARY2) of dir/my.file left in tree. diff --git a/dir/my.file b/dir/my.file index 09c78c725a3b..79f9d1fb7611 100644 --- a/dir/my.file +++ b/dir/my.file @@ -950,15 +912,15 @@ int my_func(struct stuff *data, rq->timeline, rq); - old_func(data, 1); + new_func("for funsies", data, VALID); obj->value = 8; obj->read_attempts = 0; } else { - err = old_func(data, 0); + err = new_func("for funsies", data, INVALID); if (unlikely(err)) return err; - another_old_func(data, obj->value); + another_new_func(data, obj->value, INVALID); obj->value++; } obj->read_attempts += 1; The `dir/my.fil e.conflict_msg` file is definitely slightly weird, but any solution here would be. I think it's fairly intuitive what is meant. No one has commented on this choice in the 9+ months it's been in use internally by ~50 users (even with -p implying --remerge-diff automatically to make it more likely people have used this option), so either it really is intuitive, or it doesn't come up much. It could well be the latter.
Elijah Newren <newren@gmail.com> writes: > Creating this file means you see something like this in the > remerge-diff (note there are diffs for two files, with the synthetic > file appearing just before the file it has messages about): > > diff --git a/dir/my.fil e.conflict_msg b/dir/my.fil e.conflict_msg > deleted file mode 100644 > index 2bd215a32f06..000000000000 > --- a/dir/my.fil e.conflict_msg > +++ /dev/null > @@ -1,2 +0,0 @@ > -== Conflict notices for my.file == > -CONFLICT (modify/delete): dir/my.file deleted in HASH1 (SHORT > SUMMARY1) and modified in HASH2 (SHORT SUMMARY 2). Version HASH2 > (SHORT SUMMARY2) of dir/my.file left in tree. I do not know if my reaction should be "Cute" or "Yuck" ;-) Hopefully nobody uses .conflict_msg as a suffix for their files.
diff --git a/merge-ort.c b/merge-ort.c index 515dc39b7f6..4dbf0a477af 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -2409,7 +2409,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt, */ ci->path_conflict = 1; if (pair->status == 'A') - path_msg(opt, new_path, 0, + path_msg(opt, new_path, 1, _("CONFLICT (file location): %s added in %s " "inside a directory that was renamed in %s, " "suggesting it should perhaps be moved to " @@ -2417,7 +2417,7 @@ static void apply_directory_rename_modifications(struct merge_options *opt, old_path, branch_with_new_path, branch_with_dir_rename, new_path); else - path_msg(opt, new_path, 0, + path_msg(opt, new_path, 1, _("CONFLICT (file location): %s renamed to %s " "in %s, inside a directory that was renamed " "in %s, suggesting it should perhaps be " @@ -3814,7 +3814,7 @@ static void process_entry(struct merge_options *opt, reason = _("add/add"); if (S_ISGITLINK(merged_file.mode)) reason = _("submodule"); - path_msg(opt, path, 0, + path_msg(opt, path, 1, _("CONFLICT (%s): Merge conflict in %s"), reason, path); }