Message ID | pull.1103.git.1640109948.gitgitgadget@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Add a new --remerge-diff capability to show & log | expand |
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > Here are some patches to add a --remerge-diff capability to show & log, > which works by comparing merge commits to an automatic remerge (note that > the automatic remerge tree can contain files with conflict markers). > > Changes since original submission[1]: > > * Rebased on top of the version of ns/tmp-objdir that Neeraj submitted > (Neeraj's patches were based on v2.34, but ns/tmp-objdir got applied on > an old commit and does not even build because of that). Oh, that's bad. I wish people do not rebase their updates on top of newer 'master' only for the sake of it, once an older version is queued. > * Modify ll-merge API to return a status, instead of printing "Cannot merge > binary files" on stdout[2] (as suggested by Peff) I wondered if we want to do the same for other error messages to give callers greater control, but this change by itself already looks quite good. > * Make conflict messages and other such warnings into diff headers of the > subsequent remerge-diff rather than appearing in the diff as file content > of some funny looking filenames (as suggested by Peff[3] and Junio[4]) OK. > * Sergey ack'ed the diff-merges.c portion of the patches, but that wasn't > limited to one patch so not sure where to record that ack. On that single patch?
On Tue, Dec 21, 2021 at 3:20 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > Here are some patches to add a --remerge-diff capability to show & log, > > which works by comparing merge commits to an automatic remerge (note that > > the automatic remerge tree can contain files with conflict markers). > > > > Changes since original submission[1]: > > > > * Rebased on top of the version of ns/tmp-objdir that Neeraj submitted > > (Neeraj's patches were based on v2.34, but ns/tmp-objdir got applied on > > an old commit and does not even build because of that). > > Oh, that's bad. I wish people do not rebase their updates on top of > newer 'master' only for the sake of it, once an older version is > queued. > > > * Modify ll-merge API to return a status, instead of printing "Cannot merge > > binary files" on stdout[2] (as suggested by Peff) > > I wondered if we want to do the same for other error messages to > give callers greater control, but this change by itself already > looks quite good. > > > * Make conflict messages and other such warnings into diff headers of the > > subsequent remerge-diff rather than appearing in the diff as file content > > of some funny looking filenames (as suggested by Peff[3] and Junio[4]) > > OK. > > > * Sergey ack'ed the diff-merges.c portion of the patches, but that wasn't > > limited to one patch so not sure where to record that ack. > > On that single patch? Yeah, the main patch near the end that changed 4 files; he acked the changes to just one of those files in that patch.
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > Documentation/diff-options.txt | 8 ++++ > apply.c | 5 ++- > builtin/checkout.c | 12 ++++-- > builtin/log.c | 16 ++++++++ > diff-merges.c | 12 ++++++ > diff.c | 34 ++++++++++++++++- > diff.h | 1 + > ll-merge.c | 40 ++++++++++--------- > ll-merge.h | 9 ++++- > log-tree.c | 70 ++++++++++++++++++++++++++++++++++ > merge-blobs.c | 5 ++- > merge-ort.c | 49 +++++++++++++++++++++--- > merge-ort.h | 10 +++++ > merge-recursive.c | 8 +++- > merge-recursive.h | 1 + > notes-merge.c | 5 ++- > rerere.c | 10 +++-- > revision.h | 6 ++- > t/t6404-recursive-merge.sh | 9 ++++- > t/t6406-merge-attr.sh | 9 ++++- > tmp-objdir.c | 5 +++ > tmp-objdir.h | 6 +++ > 22 files changed, 288 insertions(+), 42 deletions(-) It is somewhat disappointing that there is no test or documentation update that show how a typical remerge-diff output should look like. I was specifically interested in finding out how the "conflict hint messages from merge backend in the diff header" output would look. I left some messages here and there on the patches I read carefully, and they looked mostly good. I only skimmed 7 and 8 and did not find anything glaringly wrong, but that wouldn't count as a review. Thanks.