Re: [PATCH v4 2/2] revision: implement `git log --merge` also for rebase/cherry-pick/revert

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 12, 2024, 11:02 UTC
Message-ID: <c5d60b5b-3181-4bb7-a7f8-eb97474526d7@gmail.com>
In-Reply-To: <20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v4-2-3bc9e62808f4@gmail.com>

Hi Philippe

On 10/02/2024 23:35, Philippe Blain wrote:

Show 21 quoted lines

> 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.

Show 11 quoted lines

> 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.

Show 14 quoted lines

> 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.

Show 23 quoted lines

>   --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