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

[Phillip Wood <phillip.wood123@gmail.com>]

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