From: Philippe Blain Date: Tue, 13 Feb 2024 13:27:20 GMT Subject: Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert Message-ID: <790a3f11-5a8c-42f2-7a35-f2900c0299b4@gmail.com> In-Reply-To: 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 >> >> '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 . 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.