| Message ID | 20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v4-2-3bc9e62808f4@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement `git log --merge` also for rebase/cherry-pick/revert | expand |
Thank you for stepping in and resubmitting with an extended commit message and documentation! Am 11.02.24 um 00:35 schrieb Philippe Blain: > From: Michael Lohmann <mi.al.lohmann@gmail.com> > > 'git log' learned in ae3e5e1ef2 (git log -p --merge [[--] paths...], > 2006-07-03) to show commits touching conflicted files in the range > HEAD...MERGE_HEAD, an addition documented in d249b45547 (Document > rev-list's option --merge, 2006-08-04). > > It can be useful to look at the commit history to understand what lead > to merge conflicts also for other mergy operations besides merges, like > cherry-pick, revert and rebase. > > For rebases, an interesting range to look at is HEAD...REBASE_HEAD, > since the conflicts are usually caused by how the code changed > differently on HEAD since REBASE_HEAD forked from it. > > For cherry-picks and revert, it is less clear that > HEAD...CHERRY_PICK_HEAD and HEAD...REVERT_HEAD are indeed interesting > ranges, since these commands are about applying or unapplying a single > (or a few, for cherry-pick) commit(s) on top of HEAD. However, conflicts > encountered during these operations can indeed be caused by changes > introduced in preceding commits on both sides of the history. I very much agree. Thank you for spelling it out! > Adjust the code in prepare_show_merge so it constructs the range > HEAD...$OTHER for each of OTHER={MERGE_HEAD, CHERRY_PICK_HEAD, > REVERT_HEAD or REBASE_HEAD}. Note that we try these pseudorefs in order, > so keep REBASE_HEAD last since the three other operations can be > performed during a rebase. Note also that in the uncommon case where > $OTHER and HEAD do not share a common ancestor, this will show the > complete histories of both sides since their root commits, which is the > same behaviour as currently happens in that case for HEAD and > MERGE_HEAD. Well explained! > > Adjust the documentation of this option accordingly. > > Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com> > Co-authored-by: Johannes Sixt <j6t@kdbg.org> > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> > [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last] > Signed-off-by: Junio C Hamano <gitster@pobox.com> Signed-off-by trailers should occur in temporal order. Therefore, when you pick up a commit and resend it, you should keep existing Signed-off-by and add yours last. > --- > Documentation/gitk.txt | 8 ++++---- > Documentation/rev-list-options.txt | 6 ++++-- > revision.c | 31 +++++++++++++++++++++++-------- > 3 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt > index c2213bb77b..80ff4e149a 100644 > --- a/Documentation/gitk.txt > +++ b/Documentation/gitk.txt > @@ -63,10 +63,10 @@ linkgit:git-rev-list[1] for a complete list. > > --merge:: > > - After an attempt to merge stops with conflicts, show the commits on > - the history between two branches (i.e. the HEAD and the MERGE_HEAD) > - that modify the conflicted files and do not exist on all the heads > - being merged. > + Show commits touching conflicted paths in the range `HEAD...$OTHER`, > + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, > + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works > + when the index has unmerged entries. Unfortunately, this patch does not help gitk. Gitk has its own logic to treat --merge and needs its own patch. This hunk should not be part of this patch. > > --left-right:: > > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt > index 2bf239ff03..5b4672c346 100644 > --- a/Documentation/rev-list-options.txt > +++ b/Documentation/rev-list-options.txt > @@ -341,8 +341,10 @@ See also linkgit:git-reflog[1]. > Under `--pretty=reference`, this information will not be shown at all. > > --merge:: > - After a failed merge, show refs that touch files having a > - conflict and don't exist on all heads to merge. > + Show commits touching conflicted paths in the range `HEAD...$OTHER`, > + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, > + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works > + when the index has unmerged entries. Good. I used --left-right to check that the direction is indeed HEAD...$OTHER and not $OTHER...HEAD. -- Hannes
Hi Johannes, Le 2024-02-11 à 03:34, Johannes Sixt a écrit : >> Adjust the documentation of this option accordingly. >> >> Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com> >> Co-authored-by: Johannes Sixt <j6t@kdbg.org> >> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> >> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> >> [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last] >> Signed-off-by: Junio C Hamano <gitster@pobox.com> > > Signed-off-by trailers should occur in temporal order. Therefore, when > you pick up a commit and resend it, you should keep existing > Signed-off-by and add yours last. Thank you, I did not know that. I guess Junio should be kept last though ? Or maybe I should remove Junio's sign-off if I send a new version of the patch ? I'll resend with corrected order. By the way, Michael put you as co-author but did not add your signed-off-by... >> --- >> Documentation/gitk.txt | 8 ++++---- >> Documentation/rev-list-options.txt | 6 ++++-- >> revision.c | 31 +++++++++++++++++++++++-------- >> 3 files changed, 31 insertions(+), 14 deletions(-) >> >> diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt >> index c2213bb77b..80ff4e149a 100644 >> --- a/Documentation/gitk.txt >> +++ b/Documentation/gitk.txt >> @@ -63,10 +63,10 @@ linkgit:git-rev-list[1] for a complete list. >> >> --merge:: >> >> - After an attempt to merge stops with conflicts, show the commits on >> - the history between two branches (i.e. the HEAD and the MERGE_HEAD) >> - that modify the conflicted files and do not exist on all the heads >> - being merged. >> + Show commits touching conflicted paths in the range `HEAD...$OTHER`, >> + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, >> + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works >> + when the index has unmerged entries. > > Unfortunately, this patch does not help gitk. Gitk has its own logic to > treat --merge and needs its own patch. This hunk should not be part of > this patch. Ah, you are right. I assumed it just used rev-list under the hood, but it's not the case for this flag. I'll remove that hunk. Thanks, Philippe.
Am 11.02.24 um 17:43 schrieb Philippe Blain: > Hi Johannes, > > Le 2024-02-11 à 03:34, Johannes Sixt a écrit : > >>> Adjust the documentation of this option accordingly. >>> >>> Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com> >>> Co-authored-by: Johannes Sixt <j6t@kdbg.org> >>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> >>> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> >>> [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last] >>> Signed-off-by: Junio C Hamano <gitster@pobox.com> >> >> Signed-off-by trailers should occur in temporal order. Therefore, when >> you pick up a commit and resend it, you should keep existing >> Signed-off-by and add yours last. > > Thank you, I did not know that. I guess Junio should be kept last though ? > Or maybe I should remove Junio's sign-off if I send a new version of the > patch ? You should *not* remove Junio's Signed-off-by, because the patch went through his hands before you picked it up. Then you add your own sign-off below. Later, Junio will sign it off again. > I'll resend with corrected order. > > By the way, Michael put you as co-author but did not add your signed-off-by... This is fine and sufficient. Micheal used some of my ideas, but I didn't take part in the patch submission process. -- Hannes
Hi Philippe On 10/02/2024 23:35, Philippe Blain wrote: > From: Michael Lohmann <mi.al.lohmann@gmail.com> > > 'git log' learned in ae3e5e1ef2 (git log -p --merge [[--] paths...], > 2006-07-03) to show commits touching conflicted files in the range > HEAD...MERGE_HEAD, an addition documented in d249b45547 (Document > rev-list's option --merge, 2006-08-04). > > It can be useful to look at the commit history to understand what lead > to merge conflicts also for other mergy operations besides merges, like > cherry-pick, revert and rebase. > > For rebases, an interesting range to look at is HEAD...REBASE_HEAD, > since the conflicts are usually caused by how the code changed > differently on HEAD since REBASE_HEAD forked from it. > > For cherry-picks and revert, it is less clear that > HEAD...CHERRY_PICK_HEAD and HEAD...REVERT_HEAD are indeed interesting > ranges, since these commands are about applying or unapplying a single > (or a few, for cherry-pick) commit(s) on top of HEAD. However, conflicts > encountered during these operations can indeed be caused by changes > introduced in preceding commits on both sides of the history. I tend to think that there isn't much difference between rebase and cherry-pick here - they are both cherry-picking commits and it is perfectly possible to rebase a branch onto an unrelated upstream. The important part for me is that we're showing these commits because even though they aren't part of the 3-way merge they are relevant for investigating where any merge conflicts come from. For revert I'd argue that the only sane use is reverting an ancestor of HEAD but maybe I'm missing something. In that case REVERT_HEAD...HEAD is the same as REVERT_HEAD..HEAD so it shows the changes since the commit that is being reverted which will be the ones causing the conflict. > Adjust the code in prepare_show_merge so it constructs the range > HEAD...$OTHER for each of OTHER={MERGE_HEAD, CHERRY_PICK_HEAD, > REVERT_HEAD or REBASE_HEAD}. Note that we try these pseudorefs in order, > so keep REBASE_HEAD last since the three other operations can be > performed during a rebase. Note also that in the uncommon case where > $OTHER and HEAD do not share a common ancestor, this will show the > complete histories of both sides since their root commits, which is the > same behaviour as currently happens in that case for HEAD and > MERGE_HEAD. > > Adjust the documentation of this option accordingly. Thanks for the comprehensive commit message. > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt > index 2bf239ff03..5b4672c346 100644 > --- a/Documentation/rev-list-options.txt > +++ b/Documentation/rev-list-options.txt > @@ -341,8 +341,10 @@ See also linkgit:git-reflog[1]. > Under `--pretty=reference`, this information will not be shown at all. > > --merge:: > - After a failed merge, show refs that touch files having a > - conflict and don't exist on all heads to merge. > + Show commits touching conflicted paths in the range `HEAD...$OTHER`, > + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, > + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works > + when the index has unmerged entries. Do you know what "and don't exist on all heads to merge" in the original is referring to? The new text doesn't mention anything that sounds like that but I don't understand what the original was trying to say. It might be worth adding a sentence explaining when this option is useful. This option can be used to show the commits that are relevant when resolving conflicts from a 3-way merge or something like that. > --boundary:: > Output excluded boundary commits. Boundary commits are > diff --git a/revision.c b/revision.c > index aa4c4dc778..36dc2f94f7 100644 > --- a/revision.c > +++ b/revision.c > @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs, > } > } > > +static const char *lookup_other_head(struct object_id *oid) > +{ > + int i; > + static const char *const other_head[] = { > + "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD" > + }; > + > + for (i = 0; i < ARRAY_SIZE(other_head); i++) > + if (!read_ref_full(other_head[i], > + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > + oid, NULL)) { > + if (is_null_oid(oid)) > + die("%s is a symbolic ref???", other_head[i]); This would benefit from being translated and I think one '?' would suffice (I'm not sure we even need that - are there other possible causes of a null oid here?) > + return other_head[i]; > + } > + > + die("--merge without MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD?"); This is not a question and would also benefit from translation. It might be more helpful to say that "--merge" requires one of those pseudorefs. Thanks for pick this series up and polishing it Phillip
Johannes Sixt <j6t@kdbg.org> writes: > Am 11.02.24 um 17:43 schrieb Philippe Blain: >> Hi Johannes, >> >> Le 2024-02-11 à 03:34, Johannes Sixt a écrit : >> >>>> Adjust the documentation of this option accordingly. >>>> >>>> Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com> >>>> Co-authored-by: Johannes Sixt <j6t@kdbg.org> >>>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> >>>> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> >>>> [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last] >>>> Signed-off-by: Junio C Hamano <gitster@pobox.com> >>> >>> Signed-off-by trailers should occur in temporal order. Therefore, when >>> you pick up a commit and resend it, you should keep existing >>> Signed-off-by and add yours last. >> >> Thank you, I did not know that. I guess Junio should be kept last though ? >> Or maybe I should remove Junio's sign-off if I send a new version of the >> patch ? > > You should *not* remove Junio's Signed-off-by, because the patch went > through his hands before you picked it up. Then you add your own > sign-off below. Later, Junio will sign it off again. In the meantime, this is how I tweaked while queuing. Co-authored-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last] Signed-off-by: Junio C Hamano <gitster@pobox.com> [pb: greatly enhanced the log message] Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Le 11/02/2024 à 00:35, Philippe Blain a écrit : > From: Michael Lohmann <mi.al.lohmann@gmail.com> > > 'git log' learned in ae3e5e1ef2 (git log -p --merge [[--] paths...], > 2006-07-03) to show commits touching conflicted files in the range > HEAD...MERGE_HEAD, an addition documented in d249b45547 (Document > rev-list's option --merge, 2006-08-04). > > It can be useful to look at the commit history to understand what lead > to merge conflicts also for other mergy operations besides merges, like > cherry-pick, revert and rebase. > > For rebases, an interesting range to look at is HEAD...REBASE_HEAD, > since the conflicts are usually caused by how the code changed > differently on HEAD since REBASE_HEAD forked from it. > > For cherry-picks and revert, it is less clear that > HEAD...CHERRY_PICK_HEAD and HEAD...REVERT_HEAD are indeed interesting > ranges, since these commands are about applying or unapplying a single > (or a few, for cherry-pick) commit(s) on top of HEAD. However, conflicts > encountered during these operations can indeed be caused by changes > introduced in preceding commits on both sides of the history. > > Adjust the code in prepare_show_merge so it constructs the range > HEAD...$OTHER for each of OTHER={MERGE_HEAD, CHERRY_PICK_HEAD, > REVERT_HEAD or REBASE_HEAD}. Note that we try these pseudorefs in order, > so keep REBASE_HEAD last since the three other operations can be > performed during a rebase. Note also that in the uncommon case where > $OTHER and HEAD do not share a common ancestor, this will show the > complete histories of both sides since their root commits, which is the > same behaviour as currently happens in that case for HEAD and > MERGE_HEAD. > > Adjust the documentation of this option accordingly. > > Co-authored-by: Philippe Blain <levraiphilippeblain@gmail.com> > Co-authored-by: Johannes Sixt <j6t@kdbg.org> > Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> > [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last] > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Documentation/gitk.txt | 8 ++++---- > Documentation/rev-list-options.txt | 6 ++++-- > revision.c | 31 +++++++++++++++++++++++-------- > 3 files changed, 31 insertions(+), 14 deletions(-) > > diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt > index c2213bb77b..80ff4e149a 100644 > --- a/Documentation/gitk.txt > +++ b/Documentation/gitk.txt > @@ -63,10 +63,10 @@ linkgit:git-rev-list[1] for a complete list. > > --merge:: > > - After an attempt to merge stops with conflicts, show the commits on > - the history between two branches (i.e. the HEAD and the MERGE_HEAD) > - that modify the conflicted files and do not exist on all the heads > - being merged. > + Show commits touching conflicted paths in the range `HEAD...$OTHER`, if $OTHER is a placeholder, why not use the placeholder notation <other> instead of a notation that could deceive the reader into thinking that this is an actual environment variable? > + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, > + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works > + when the index has unmerged entries. > Thanks
Hi Jean-Nöel, Le 2024-02-13 à 03:33, Jean-Noël Avila a écrit : > Le 11/02/2024 à 00:35, Philippe Blain a écrit : >> From: Michael Lohmann <mi.al.lohmann@gmail.com> >> >> >> diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt >> index c2213bb77b..80ff4e149a 100644 >> --- a/Documentation/gitk.txt >> +++ b/Documentation/gitk.txt >> @@ -63,10 +63,10 @@ linkgit:git-rev-list[1] for a complete list. >> --merge:: >> - After an attempt to merge stops with conflicts, show the commits on >> - the history between two branches (i.e. the HEAD and the MERGE_HEAD) >> - that modify the conflicted files and do not exist on all the heads >> - being merged. >> + Show commits touching conflicted paths in the range `HEAD...$OTHER`, > > if $OTHER is a placeholder, why not use the placeholder notation <other> > instead of a notation that could deceive the reader into thinking that > this is an actual environment variable? Good point, I'll make that change. Thanks! Philippe.
Hi Phillip, Le 2024-02-12 à 06:02, Phillip Wood a écrit : > Hi Philippe > > On 10/02/2024 23:35, Philippe Blain wrote: >> From: Michael Lohmann <mi.al.lohmann@gmail.com> >> >> 'git log' learned in ae3e5e1ef2 (git log -p --merge [[--] paths...], >> 2006-07-03) to show commits touching conflicted files in the range >> HEAD...MERGE_HEAD, an addition documented in d249b45547 (Document >> rev-list's option --merge, 2006-08-04). >> >> It can be useful to look at the commit history to understand what lead >> to merge conflicts also for other mergy operations besides merges, like >> cherry-pick, revert and rebase. >> >> For rebases, an interesting range to look at is HEAD...REBASE_HEAD, >> since the conflicts are usually caused by how the code changed >> differently on HEAD since REBASE_HEAD forked from it. >> >> For cherry-picks and revert, it is less clear that >> HEAD...CHERRY_PICK_HEAD and HEAD...REVERT_HEAD are indeed interesting >> ranges, since these commands are about applying or unapplying a single >> (or a few, for cherry-pick) commit(s) on top of HEAD. However, conflicts >> encountered during these operations can indeed be caused by changes >> introduced in preceding commits on both sides of the history. > > I tend to think that there isn't much difference between rebase and cherry-pick here - they are both cherry-picking commits and it is perfectly possible to rebase a branch onto an unrelated upstream. The important part for me is that we're showing these commits because even though they aren't part of the 3-way merge they are relevant for investigating where any merge conflicts come from. > > For revert I'd argue that the only sane use is reverting an ancestor of HEAD but maybe I'm missing something. In that case REVERT_HEAD...HEAD is the same as REVERT_HEAD..HEAD so it shows the changes since the commit that is being reverted which will be the ones causing the conflict. Thanks, I can rework the wording from that angle. >> Adjust the code in prepare_show_merge so it constructs the range >> HEAD...$OTHER for each of OTHER={MERGE_HEAD, CHERRY_PICK_HEAD, >> REVERT_HEAD or REBASE_HEAD}. Note that we try these pseudorefs in order, >> so keep REBASE_HEAD last since the three other operations can be >> performed during a rebase. Note also that in the uncommon case where >> $OTHER and HEAD do not share a common ancestor, this will show the >> complete histories of both sides since their root commits, which is the >> same behaviour as currently happens in that case for HEAD and >> MERGE_HEAD. >> >> Adjust the documentation of this option accordingly. > > Thanks for the comprehensive commit message. > >> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt >> index 2bf239ff03..5b4672c346 100644 >> --- a/Documentation/rev-list-options.txt >> +++ b/Documentation/rev-list-options.txt >> @@ -341,8 +341,10 @@ See also linkgit:git-reflog[1]. >> Under `--pretty=reference`, this information will not be shown at all. >> --merge:: >> - After a failed merge, show refs that touch files having a >> - conflict and don't exist on all heads to merge. >> + Show commits touching conflicted paths in the range `HEAD...$OTHER`, >> + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, >> + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works >> + when the index has unmerged entries. > > Do you know what "and don't exist on all heads to merge" in the original is referring to? The new text doesn't mention anything that sounds like that but I don't understand what the original was trying to say. Yes, it took me a while to understand what that meant. I think it is simply describing the range of commits shown. If we substitute "refs" for "commits" and switch the order of the sentence, it reads: After a failed merge, show commits that don't exist on all heads to merge and that touch files having a conflict. So it's just describing (a bit awkwardly) the HEAD...MERGE_HEAD range. > It might be worth adding a sentence explaining when this option is useful. > > This option can be used to show the commits that are relevant > when resolving conflicts from a 3-way merge > > or something like that. Nice idea, I'll add that. > >> --boundary:: >> Output excluded boundary commits. Boundary commits are >> diff --git a/revision.c b/revision.c >> index aa4c4dc778..36dc2f94f7 100644 >> --- a/revision.c >> +++ b/revision.c >> @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs, >> } >> } >> +static const char *lookup_other_head(struct object_id *oid) >> +{ >> + int i; >> + static const char *const other_head[] = { >> + "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD" >> + }; >> + >> + for (i = 0; i < ARRAY_SIZE(other_head); i++) >> + if (!read_ref_full(other_head[i], >> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, >> + oid, NULL)) { >> + if (is_null_oid(oid)) >> + die("%s is a symbolic ref???", other_head[i]); > > This would benefit from being translated and I think one '?' would suffice (I'm not sure we even need that - are there other possible causes of a null oid here?) This bit was suggested by Junio upthread in <xmqqzfxa9usx.fsf@gitster.g>. I'm not sure if the are other causes of null oid, as I don't know well this part of the code. I agree that a single '?' would be enough, but I'm not sure about marking this for translation, I think maybe this situation would be best handled with BUG() ? >> + return other_head[i]; >> + } >> + >> + die("--merge without MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD?"); > > This is not a question and would also benefit from translation. It might be more helpful to say that "--merge" requires one of those pseudorefs. Yes, I agree. I'll tweak that. > Thanks for pick this series up and polishing it > > Phillip > Thanks, Philippe.
Hi Philippe On 13/02/2024 13:27, Philippe Blain wrote: > Le 2024-02-12 à 06:02, Phillip Wood a écrit : >> Hi Philippe >> On 10/02/2024 23:35, Philippe Blain wrote: >>> From: Michael Lohmann <mi.al.lohmann@gmail.com> >>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt >>> index 2bf239ff03..5b4672c346 100644 >>> --- a/Documentation/rev-list-options.txt >>> +++ b/Documentation/rev-list-options.txt >>> @@ -341,8 +341,10 @@ See also linkgit:git-reflog[1]. >>> Under `--pretty=reference`, this information will not be shown at all. >>> --merge:: >>> - After a failed merge, show refs that touch files having a >>> - conflict and don't exist on all heads to merge. >>> + Show commits touching conflicted paths in the range `HEAD...$OTHER`, >>> + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, >>> + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works >>> + when the index has unmerged entries. >> >> Do you know what "and don't exist on all heads to merge" in the original > is referring to? The new text doesn't mention anything that sounds like >that but I don't understand what the original was trying to say. > > Yes, it took me a while to understand what that meant. I think it is simply > describing the range of commits shown. If we substitute "refs" for "commits" > and switch the order of the sentence, it reads: > > After a failed merge, show commits that don't exist on all heads to merge > and that touch files having a conflict. > > So it's just describing (a bit awkwardly) the HEAD...MERGE_HEAD range. Ah, that makes sense, thanks for explaining >>> + for (i = 0; i < ARRAY_SIZE(other_head); i++) >>> + if (!read_ref_full(other_head[i], >>> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, >>> + oid, NULL)) { >>> + if (is_null_oid(oid)) >>> + die("%s is a symbolic ref???", other_head[i]); >> >> This would benefit from being translated and I think one '?' would suffice >> (I'm not sure we even need that - are there other possible causes of a null >> oid here?) > > This bit was suggested by Junio upthread in <xmqqzfxa9usx.fsf@gitster.g>. > I'm not sure if the are other causes of null oid, as I don't know well this > part of the code. > I agree that a single '?' would be enough, but I'm not sure about marking > this for translation, I think maybe this situation would be best handled with > BUG() ? I think it would be a bug for git to create MERGE_HEAD as a symbolic ref but when we read MERGE_HEAD and find it is a symbolic ref we don't know if git created it or some third-party script so I think we should just report an error. Best Wishes Phillip
diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt index c2213bb77b..80ff4e149a 100644 --- a/Documentation/gitk.txt +++ b/Documentation/gitk.txt @@ -63,10 +63,10 @@ linkgit:git-rev-list[1] for a complete list. --merge:: - After an attempt to merge stops with conflicts, show the commits on - the history between two branches (i.e. the HEAD and the MERGE_HEAD) - that modify the conflicted files and do not exist on all the heads - being merged. + Show commits touching conflicted paths in the range `HEAD...$OTHER`, + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works + when the index has unmerged entries. --left-right:: diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 2bf239ff03..5b4672c346 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -341,8 +341,10 @@ See also linkgit:git-reflog[1]. Under `--pretty=reference`, this information will not be shown at all. --merge:: - After a failed merge, show refs that touch files having a - conflict and don't exist on all heads to merge. + Show commits touching conflicted paths in the range `HEAD...$OTHER`, + where `$OTHER` is the first existing pseudoref in `MERGE_HEAD`, + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works + when the index has unmerged entries. --boundary:: Output excluded boundary commits. Boundary commits are diff --git a/revision.c b/revision.c index aa4c4dc778..36dc2f94f7 100644 --- a/revision.c +++ b/revision.c @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs, } } +static const char *lookup_other_head(struct object_id *oid) +{ + int i; + static const char *const other_head[] = { + "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD" + }; + + for (i = 0; i < ARRAY_SIZE(other_head); i++) + if (!read_ref_full(other_head[i], + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, + oid, NULL)) { + if (is_null_oid(oid)) + die("%s is a symbolic ref???", other_head[i]); + return other_head[i]; + } + + die("--merge without MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD?"); +} + static void prepare_show_merge(struct rev_info *revs) { struct commit_list *bases; struct commit *head, *other; struct object_id oid; + const char *other_name; const char **prune = NULL; int i, prune_num = 1; /* counting terminating NULL */ struct index_state *istate = revs->repo->index; @@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs) if (repo_get_oid(the_repository, "HEAD", &oid)) die("--merge without HEAD?"); head = lookup_commit_or_die(&oid, "HEAD"); - if (read_ref_full("MERGE_HEAD", - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - &oid, NULL)) - die("--merge without MERGE_HEAD?"); - if (is_null_oid(&oid)) - die("MERGE_HEAD is a symbolic ref???"); - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); + other_name = lookup_other_head(&oid); + other = lookup_commit_or_die(&oid, other_name); add_pending_object(revs, &head->object, "HEAD"); - add_pending_object(revs, &other->object, "MERGE_HEAD"); + add_pending_object(revs, &other->object, other_name); bases = repo_get_merge_bases(the_repository, head, other); add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);