merge-ort: make informational messages from recursive merges clearer
PATCH 1/1 merge-ort: make informational messages from recursive merges clearer
From: Elijah Newren <[email protected]>
This is another simple change with a long explanation...
merge-recursive and merge-ort are both based on the same recursive idea:
if there is more than one merge base, merge the merge bases (which may require first merging the merge bases of the merges bases, etc.). The depth of the inner merge is recorded via a variable called "call_depth", which we'll bring up again later. Naturally, the inner merges themselves can have conflicts and various messages generated about those files.
merge-recursive immediately prints to stdout as it goes, at the risk of printing multiple conflict notices for the same path separated far apart from each other with many intervenining conflict notices for other paths between them. And this is true even if there are no inner merges involved. An example of this was given in [1] and apparently caused some confusion:
CONFLICT (rename/add): Rename A->B in HEAD. B added in otherbranch ...dozens of conflicts for OTHER paths...
CONFLICT (content): Merge conflicts in B
In contrast, merge-ort collects messages and stores them by path so that it can print them grouped by path. Thus, the same case handled by merge-ort would have output of the form:
CONFLICT (rename/add): Rename A->B in HEAD. B added in otherbranch CONFLICT (content): Merge conflicts in B
...dozens of conflicts for OTHER paths...
This is generally helpful, but does make a separate bug more problematic. In particular, while merge-recursive might report the following for a recursive merge:
Auto-merging dir.c
Auto-merging midx.c
CONFLICT (content): Merge conflict in midx.c
Auto-merging diff.c
Auto-merging dir.c
CONFLICT (content): Merge conflict in dir.c
merge-ort would instead report:
Auto-merging diff.c
Auto-merging dir.c
Auto-merging dir.c
CONFLICT (content): Merge conflict in dir.c
Auto-merging midx.c
CONFLICT (content): Merge conflict in midx.c
The fact that messages for the same file are together is probably helpful in general, but with the indentation missing for the inner merge it unfortunately serves to confuse. This probably would lead users to wonder:
* Why is Git reporting that "dir.c" is being merged twice?
* If midx.c has conflicts, why do I not see any when I open up the file and why are no conflicts shown in the index?
Fix this output confusion by changing the output to clearly differentiate the messages for outer merges from the ones for inner merges, changing the above output from merge-ort to:
Auto-merging diff.c
From inner merge: Auto-merging dir.c
Auto-merging dir.c
CONFLICT (content): Merge conflict in dir.c
From inner merge: Auto-merging midx.c
From inner merge: CONFLICT (content): Merge conflict in midx.c
(Note: the number of spaces after the 'From inner merge:' is 2*call_depth).
One other thing to note here, that I didn't notice until typing up this commit message, is that merge-recursive does not print any messages from the inner merges by default; the extra verbosity has to be requested. merge-ort currently has no verbosity controls and always prints these. We may also want to change that, but for now, just make the output clearer with these extra markings and indentation.
[1] https://lore.kernel.org/git/CAGyf7-He4in8JWUh9dpAwvoPkQz9hr8nCBpxOxhZEd8+jtqTpg@mail.gmail.com/
---
merge-ort: make informational messages from recursive merges clearer
Sorry for any confusion caused by this bug (though I'm surprised no one else reported it).
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1121%2Fnewren%2Fmerge-ort-clearer-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1121/newren/merge-ort-clearer-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1121
merge-ort.c | 5 +++++ 1 file changed, 5 insertions(+)
merge-ort.c +5 -0
@@ -651,6 +651,11 @@ static void path_msg(struct merge_options *opt, dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);va_start (ap, fmt); +if (opt->priv->call_depth) { +strbuf_addchars (dest,' ' ,2 ); +strbuf_addstr (dest,"From inner merge:" ); +strbuf_addchars (dest,' ' , opt->priv->call_depth *2 ); + }strbuf_vaddf (dest, fmt, ap);va_end (ap);
base-commit: 45fe28c951c3e70666ee4ef8379772851a8e4d32
Signature · 1 line
gitgitgadget
Quoted 71 lines · Elijah Newren via GitGitGadget
From: Elijah Newren <[email protected]>
This is another simple change with a long explanation...
merge-recursive and merge-ort are both based on the same recursive idea:
if there is more than one merge base, merge the merge bases (which may require first merging the merge bases of the merges bases, etc.). The depth of the inner merge is recorded via a variable called "call_depth", which we'll bring up again later. Naturally, the inner merges themselves can have conflicts and various messages generated about those files.merge-recursive immediately prints to stdout as it goes, at the risk of printing multiple conflict notices for the same path separated far apart from each other with many intervenining conflict notices for other paths between them. And this is true even if there are no inner merges involved. An example of this was given in [1] and apparently caused some confusion:
CONFLICT (rename/add): Rename
A->Bin HEAD. B added in otherbranch ...dozens of conflicts for OTHER paths...
CONFLICT (content): Merge conflicts in BIn contrast, merge-ort collects messages and stores them by path so that it can print them grouped by path. Thus, the same case handled by merge-ort would have output of the form:
CONFLICT (rename/add): Rename
A->Bin HEAD. B added in otherbranch CONFLICT (content): Merge conflicts in B
...dozens of conflicts for OTHER paths...This is generally helpful, but does make a separate bug more problematic. In particular, while merge-recursive might report the following for a recursive merge:
Auto-merging
dir.c
Auto-mergingmidx.c
CONFLICT (content): Merge conflict inmidx.c
Auto-mergingdiff.c
Auto-mergingdir.c
CONFLICT (content): Merge conflict indir.cmerge-ort would instead report:
Auto-merging
diff.c
Auto-mergingdir.c
Auto-mergingdir.c
CONFLICT (content): Merge conflict indir.c
Auto-mergingmidx.c
CONFLICT (content): Merge conflict inmidx.cThe fact that messages for the same file are together is probably helpful in general, but with the indentation missing for the inner merge it unfortunately serves to confuse. This probably would lead users to wonder:
* Why is Git reporting that "
dir.c" is being merged twice?
* Ifmidx.chas conflicts, why do I not see any when I open up the file and why are no conflicts shown in the index?Fix this output confusion by changing the output to clearly differentiate the messages for outer merges from the ones for inner merges, changing the above output from merge-ort to:
Auto-merging
diff.c
From inner merge: Auto-mergingdir.c
Auto-mergingdir.c
CONFLICT (content): Merge conflict indir.c
From inner merge: Auto-mergingmidx.c
From inner merge: CONFLICT (content): Merge conflict inmidx.c
Quoted 2 lines
(Note: the number of spaces after the 'From inner merge:' is 2*call_depth).
So "From inner merge: " label is what we get extra, compared to the recursive one that just gives deeper leading-space indentation?
Quoted 6 lines
One other thing to note here, that I didn't notice until typing up this commit message, is that merge-recursive does not print any messages from the inner merges by default; the extra verbosity has to be requested. merge-ort currently has no verbosity controls and always prints these. We may also want to change that, but for now, just make the output clearer with these extra markings and indentation.
Yup, I found that the messages on inner conflicts, especially when they "cancel out" at the outer merge, are mostly noise that carries very little useful information (by being noisy, the user gets a sense of how complex the histories being merged are). Reducing the default messaging level would probably be a good idea.
Quoted 12 lines
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1121%2Fnewren%2Fmerge-ort-clearer-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1121/newren/merge-ort-clearer-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1121merge-ort.c | 5 +++++ 1 file changed, 5 insertions(+)merge-ort.c +0
-0
merge-ort.c static void path_msg(struct merge_options *opt, · line 657
Quoted 7 lines
merge-ort.c +4
-0@@ -651,6 +651,11 @@ static void path_msg(struct merge_options *opt, dest = (opt->record_conflict_msgs_as_headers ? &tmp : sb);va_start (ap, fmt); +if (opt->priv->call_depth) { +strbuf_addchars (dest,' ' ,2 ); +strbuf_addstr (dest,"From inner merge:" ); +strbuf_addchars (dest,' ' , opt->priv->call_depth *2 );
I am unsure if the long label adds much value, but I am perfectly OK tostart from more verbose and see if people complain (or not). This is for human consumption and not meant to be machine parseable so we can iterate.
Will queue. Thanks.
Quoted 7 lines · Junio C Hamano
Quoted 6 lines
One other thing to note here, that I didn't notice until typing up this commit message, is that merge-recursive does not print any messages from the inner merges by default; the extra verbosity has to be requested. merge-ort currently has no verbosity controls and always prints these. We may also want to change that, but for now, just make the output clearer with these extra markings and indentation.
Quoted 5 lines
Yup, I found that the messages on inner conflicts, especially when they "cancel out" at the outer merge, are mostly noise that carries very little useful information (by being noisy, the user gets a sense of how complex the histories being merged are). Reducing the default messaging level would probably be a good idea.
Here is what I just had to scroll through to update 'next' by merging back 'master', only to grab the updates to the release notes. Needless to say, this would have been somewhat baffling if I didn't know to expect it.
It would be good to squelch it before we hear another complaints from old-timer power users ;-)
$ git merge -m 'Sync with master' --no-log master From inner merge: Auto-merging blame.c From inner merge: Auto-merging builtin/am.c From inner merge: Auto-merging builtin/blame.c From inner merge: Auto-merging builtin/clone.c From inner merge: Auto-merging builtin/clone.c From inner merge: Auto-merging builtin/commit.c From inner merge: Auto-merging builtin/fetch.c From inner merge: Auto-merging builtin/fetch.c From inner merge: Auto-merging builtin/grep.c From inner merge: Auto-merging builtin/hash-object.c From inner merge: Auto-merging builtin/log.c From inner merge: Auto-merging builtin/log.c From inner merge: Auto-merging builtin/pack-objects.c From inner merge: Auto-merging builtin/pull.c From inner merge: Auto-merging builtin/pull.c From inner merge: Auto-merging builtin/rebase.c From inner merge: Auto-merging builtin/rebase.c From inner merge: Auto-merging builtin/reflog.c From inner merge: CONFLICT (content): Merge conflict in builtin/reflog.c Auto-merging builtin/reflog.c From inner merge: Auto-merging builtin/reset.c From inner merge: Auto-merging builtin/sparse-checkout.c From inner merge: Auto-merging builtin/sparse-checkout.c From inner merge: Auto-merging builtin/submodule--helper.c From inner merge: Auto-merging builtin/submodule--helper.c From inner merge: CONFLICT (content): Merge conflict in builtin/submodule--helper.c Auto-merging builtin/submodule--helper.c From inner merge: Auto-merging builtin/worktree.c From inner merge: Auto-merging cache.h From inner merge: Auto-merging config.c From inner merge: Auto-merging config.h From inner merge: Auto-merging diff-merges.c From inner merge: Auto-merging diff.c From inner merge: Auto-merging git.c From inner merge: Auto-merging gpg-interface.c From inner merge: Auto-merging grep.c From inner merge: Auto-merging grep.c From inner merge: Auto-merging notes-merge.c From inner merge: Auto-merging object-name.c From inner merge: Auto-merging pack-bitmap-write.c From inner merge: Auto-merging parse-options.c From inner merge: CONFLICT (content): Merge conflict in parse-options.c From inner merge: Auto-merging parse-options.h From inner merge: CONFLICT (content): Merge conflict in parse-options.h From inner merge: Auto-merging refs.c From inner merge: Auto-merging revision.c From inner merge: Auto-merging sequencer.c From inner merge: Auto-merging sequencer.c From inner merge: Auto-merging sparse-index.c From inner merge: Auto-merging submodule-config.c From inner merge: Auto-merging t/t1091-sparse-checkout-builtin.sh From inner merge: CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh Auto-merging t/t1091-sparse-checkout-builtin.sh From inner merge: Auto-merging t/t1512-rev-parse-disambiguation.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t5316-pack-delta-depth.sh From inner merge: Auto-merging t/t6120-describe.sh From inner merge: Auto-merging t/t6120-describe.sh From inner merge: Auto-merging worktree.c Merge made by the 'ort' strategy. Documentation/RelNotes/2.36.0.txt | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Quoted 16 lines · Junio C Hamano
Quoted 7 lines · Junio C Hamano
Quoted 6 lines
One other thing to note here, that I didn't notice until typing up this commit message, is that merge-recursive does not print any messages from the inner merges by default; the extra verbosity has to be requested. merge-ort currently has no verbosity controls and always prints these. We may also want to change that, but for now, just make the output clearer with these extra markings and indentation.
Quoted 5 lines
Yup, I found that the messages on inner conflicts, especially when they "cancel out" at the outer merge, are mostly noise that carries very little useful information (by being noisy, the user gets a sense of how complex the histories being merged are). Reducing the default messaging level would probably be a good idea.
Quoted 7 lines
Here is what I just had to scroll through to update 'next' by merging back 'master', only to grab the updates to the release notes. Needless to say, this would have been somewhat baffling if I didn't know to expect it.
It would be good to squelch it before we hear another complaints from old-timer power users ;-)
I'll submit a patch soon.
Quoted 70 lines
$ git merge -m 'Sync with master' --no-log master From inner merge: Auto-merging blame.c From inner merge: Auto-merging builtin/am.c From inner merge: Auto-merging builtin/blame.c From inner merge: Auto-merging builtin/clone.c From inner merge: Auto-merging builtin/clone.c From inner merge: Auto-merging builtin/commit.c From inner merge: Auto-merging builtin/fetch.c From inner merge: Auto-merging builtin/fetch.c From inner merge: Auto-merging builtin/grep.c From inner merge: Auto-merging builtin/hash-object.c From inner merge: Auto-merging builtin/log.c From inner merge: Auto-merging builtin/log.c From inner merge: Auto-merging builtin/pack-objects.c From inner merge: Auto-merging builtin/pull.c From inner merge: Auto-merging builtin/pull.c From inner merge: Auto-merging builtin/rebase.c From inner merge: Auto-merging builtin/rebase.c From inner merge: Auto-merging builtin/reflog.c From inner merge: CONFLICT (content): Merge conflict in builtin/reflog.c Auto-merging builtin/reflog.c From inner merge: Auto-merging builtin/reset.c From inner merge: Auto-merging builtin/sparse-checkout.c From inner merge: Auto-merging builtin/sparse-checkout.c From inner merge: Auto-merging builtin/submodule--helper.c From inner merge: Auto-merging builtin/submodule--helper.c From inner merge: CONFLICT (content): Merge conflict in builtin/submodule--helper.c Auto-merging builtin/submodule--helper.c From inner merge: Auto-merging builtin/worktree.c From inner merge: Auto-merging cache.h From inner merge: Auto-merging config.c From inner merge: Auto-merging config.h From inner merge: Auto-merging diff-merges.c From inner merge: Auto-merging diff.c From inner merge: Auto-merging git.c From inner merge: Auto-merging gpg-interface.c From inner merge: Auto-merging grep.c From inner merge: Auto-merging grep.c From inner merge: Auto-merging notes-merge.c From inner merge: Auto-merging object-name.c From inner merge: Auto-merging pack-bitmap-write.c From inner merge: Auto-merging parse-options.c From inner merge: CONFLICT (content): Merge conflict in parse-options.c From inner merge: Auto-merging parse-options.h From inner merge: CONFLICT (content): Merge conflict in parse-options.h From inner merge: Auto-merging refs.c From inner merge: Auto-merging revision.c From inner merge: Auto-merging sequencer.c From inner merge: Auto-merging sequencer.c From inner merge: Auto-merging sparse-index.c From inner merge: Auto-merging submodule-config.c From inner merge: Auto-merging t/t1091-sparse-checkout-builtin.sh From inner merge: CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh Auto-merging t/t1091-sparse-checkout-builtin.sh From inner merge: Auto-merging t/t1512-rev-parse-disambiguation.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t5316-pack-delta-depth.sh From inner merge: Auto-merging t/t6120-describe.sh From inner merge: Auto-merging t/t6120-describe.sh From inner merge: Auto-merging worktree.c Merge made by the 'ort' strategy. Documentation/RelNotes/2.36.0.txt | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Quoted 23 lines · Elijah Newren
Quoted 21 lines · Junio C Hamano
Quoted 7 lines · Junio C Hamano
Quoted 6 lines
One other thing to note here, that I didn't notice until typing up this commit message, is that merge-recursive does not print any messages from the inner merges by default; the extra verbosity has to be requested. merge-ort currently has no verbosity controls and always prints these. We may also want to change that, but for now, just make the output clearer with these extra markings and indentation.
Quoted 5 lines
Yup, I found that the messages on inner conflicts, especially when they "cancel out" at the outer merge, are mostly noise that carries very little useful information (by being noisy, the user gets a sense of how complex the histories being merged are). Reducing the default messaging level would probably be a good idea.
Here is what I just had to scroll through to update 'next' by merging back 'master', only to grab the updates to the release notes. Needless to say, this would have been somewhat baffling if I didn't know to expect it.
Quoted 4 lines
Quoted 2 lines
It would be good to squelch it before we hear another complaints from old-timer power users ;-)
I'll submit a patch soon.
https://lore.kernel.org/git/[email protected]/,
which would drop all the "From inner merge:" lines below.
Quoted 71 lines
Quoted 71 lines
$ git merge -m 'Sync with master' --no-log master From inner merge: Auto-merging blame.c From inner merge: Auto-merging builtin/am.c From inner merge: Auto-merging builtin/blame.c From inner merge: Auto-merging builtin/clone.c From inner merge: Auto-merging builtin/clone.c From inner merge: Auto-merging builtin/commit.c From inner merge: Auto-merging builtin/fetch.c From inner merge: Auto-merging builtin/fetch.c From inner merge: Auto-merging builtin/grep.c From inner merge: Auto-merging builtin/hash-object.c From inner merge: Auto-merging builtin/log.c From inner merge: Auto-merging builtin/log.c From inner merge: Auto-merging builtin/pack-objects.c From inner merge: Auto-merging builtin/pull.c From inner merge: Auto-merging builtin/pull.c From inner merge: Auto-merging builtin/rebase.c From inner merge: Auto-merging builtin/rebase.c From inner merge: Auto-merging builtin/reflog.c From inner merge: CONFLICT (content): Merge conflict in builtin/reflog.c Auto-merging builtin/reflog.c From inner merge: Auto-merging builtin/reset.c From inner merge: Auto-merging builtin/sparse-checkout.c From inner merge: Auto-merging builtin/sparse-checkout.c From inner merge: Auto-merging builtin/submodule--helper.c From inner merge: Auto-merging builtin/submodule--helper.c From inner merge: CONFLICT (content): Merge conflict in builtin/submodule--helper.c Auto-merging builtin/submodule--helper.c From inner merge: Auto-merging builtin/worktree.c From inner merge: Auto-merging cache.h From inner merge: Auto-merging config.c From inner merge: Auto-merging config.h From inner merge: Auto-merging diff-merges.c From inner merge: Auto-merging diff.c From inner merge: Auto-merging git.c From inner merge: Auto-merging gpg-interface.c From inner merge: Auto-merging grep.c From inner merge: Auto-merging grep.c From inner merge: Auto-merging notes-merge.c From inner merge: Auto-merging object-name.c From inner merge: Auto-merging pack-bitmap-write.c From inner merge: Auto-merging parse-options.c From inner merge: CONFLICT (content): Merge conflict in parse-options.c From inner merge: Auto-merging parse-options.h From inner merge: CONFLICT (content): Merge conflict in parse-options.h From inner merge: Auto-merging refs.c From inner merge: Auto-merging revision.c From inner merge: Auto-merging sequencer.c From inner merge: Auto-merging sequencer.c From inner merge: Auto-merging sparse-index.c From inner merge: Auto-merging submodule-config.c From inner merge: Auto-merging t/t1091-sparse-checkout-builtin.sh From inner merge: CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh Auto-merging t/t1091-sparse-checkout-builtin.sh From inner merge: Auto-merging t/t1512-rev-parse-disambiguation.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t4202-log.sh From inner merge: Auto-merging t/t5316-pack-delta-depth.sh From inner merge: Auto-merging t/t6120-describe.sh From inner merge: Auto-merging t/t6120-describe.sh From inner merge: Auto-merging worktree.c Merge made by the 'ort' strategy. Documentation/RelNotes/2.36.0.txt | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)
Quoted 11 lines · Elijah Newren
Quoted 11 lines
Quoted 5 lines
Yup, I found that the messages on inner conflicts, especially when they "cancel out" at the outer merge, are mostly noise that carries very little useful information (by being noisy, the user gets a sense of how complex the histories being merged are). Reducing the default messaging level would probably be a good idea.
Here is what I just had to scroll through to update 'next' by merging back 'master', only to grab the updates to the release notes. Needless to say, this would have been somewhat baffling if I didn't know to expect it.
Quoted 27 lines
Quoted 2 lines
It would be good to squelch it before we hear another complaints from old-timer power users ;-)
I'll submit a patch soon.
Quoted 22 lines
$ git merge -m 'Sync with master' --no-log master From inner merge: Auto-merging blame.c From inner merge: Auto-merging builtin/am.c From inner merge: Auto-merging builtin/blame.c From inner merge: Auto-merging builtin/clone.c From inner merge: Auto-merging builtin/clone.c From inner merge: Auto-merging builtin/commit.c From inner merge: Auto-merging builtin/fetch.c From inner merge: Auto-merging builtin/fetch.c From inner merge: Auto-merging builtin/grep.c From inner merge: Auto-merging builtin/hash-object.c From inner merge: Auto-merging builtin/log.c From inner merge: Auto-merging builtin/log.c From inner merge: Auto-merging builtin/pack-objects.c From inner merge: Auto-merging builtin/pull.c From inner merge: Auto-merging builtin/pull.c From inner merge: Auto-merging builtin/rebase.c From inner merge: Auto-merging builtin/rebase.c From inner merge: Auto-merging builtin/reflog.c From inner merge: CONFLICT (content): Merge conflict in builtin/reflog.c Auto-merging builtin/reflog.c
Thanks. I think "From inner merge: " can be removed even when we emit these messages, as the indentation would make it clear that the indented ones are different from the outermost ones.