[RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Michael Lohmann]

This extends the functionality of `git log --merge` to also work with
conflicts for rebase, cherry pick and revert.

Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Hi everybody!

When Phillip Wood gave me some nice hints on his workflow concerning
conflicts [1] (we discussed if `--show-current-patch` would make sense
for cherry pick/revert). When I learned about `git log --merge` I was a
bit sad to discover that those do not exist for rebase/revert/cherry
pick since they look really valuable for getting an understanding on
what was changed. It is basically the counterpart to
`git show ${ACTION}_HEAD` for understanding the source of the conflict.

I am curious if also other people would be interested in having an easy
way to get a log of only the relevant commits touching conflicting files
for said actions.

With this patch I think the functionality is there, just hijacking the
`--merge` code - maybe an alias like `git log --conflict` or similar
might be more descriptive now?

What do you think about this idea? (Or am I maybe missing an easy way to
achieve it with the existing code as well?)

Michael


[1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/

 revision.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/revision.c b/revision.c
index 2424c9bd67..2e5c00dabd 100644
--- a/revision.c
+++ b/revision.c
@@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,
 	}
 }
 
+static char* get_action_head_name(struct object_id* oid)
+{
+	if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) {
+		return "MERGE_HEAD";
+	} else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) {
+		return "REBASE_HEAD";
+	} else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) {
+		return "CHERRY_PICK_HEAD";
+	} else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) {
+		return "REVERT_HEAD";
+	} else
+		die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
+}
+
 static void prepare_show_merge(struct rev_info *revs)
 {
 	struct commit_list *bases;
 	struct commit *head, *other;
 	struct object_id oid;
 	const char **prune = NULL;
+	const char *action_head_name;
 	int i, prune_num = 1; /* counting terminating NULL */
 	struct index_state *istate = revs->repo->index;
 
 	if (repo_get_oid(the_repository, "HEAD", &oid))
 		die("--merge without HEAD?");
 	head = lookup_commit_or_die(&oid, "HEAD");
-	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
-		die("--merge without MERGE_HEAD?");
-	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+	action_head_name = get_action_head_name(&oid);
+	other = lookup_commit_or_die(&oid, action_head_name);
 	add_pending_object(revs, &head->object, "HEAD");
-	add_pending_object(revs, &other->object, "MERGE_HEAD");
+	add_pending_object(revs, &other->object, action_head_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);
-- 
2.39.3 (Apple Git-145)

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Junio C Hamano]

Michael Lohmann <mi.al.lohmann@gmail.com> writes:

> This extends the functionality of `git log --merge` to also work with
> conflicts for rebase, cherry pick and revert.
>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
>  ... It is basically the counterpart to
> `git show ${ACTION}_HEAD` for understanding the source of the conflict.

I do not know about the validity of that approach to use *_HEAD, but
we may want to tighten the original's use of repo_get_oid() here ...

> -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> -		die("--merge without MERGE_HEAD?");
> -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");

... so that we won't be confused in a repository that has a tag
whose name happens to be MERGE_HEAD.  We probably should be using
refs.c:refs_resolve_ref_unsafe() instead to 

 (1) ensure MERGE_HEAD is a ref, 
 (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref(),
     and optionally
 (3) error out when MERGE_HEAD is a symref.

As your patch makes the problem even worse, if we were to do such a
tightening (and I do not see a reason not to), it may want to be
done before, not after, this patch.

I won't comment on the coding style violations in the patch below in
this message.

Thanks.

> diff --git a/revision.c b/revision.c
> index 2424c9bd67..2e5c00dabd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,
>  	}
>  }
>  
> +static char* get_action_head_name(struct object_id* oid)
> +{
> +	if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) {
> +		return "MERGE_HEAD";
> +	} else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) {
> +		return "REBASE_HEAD";
> +	} else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) {
> +		return "CHERRY_PICK_HEAD";
> +	} else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) {
> +		return "REVERT_HEAD";
> +	} else
> +		die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
> +}
> +
>  static void prepare_show_merge(struct rev_info *revs)
>  {
>  	struct commit_list *bases;
>  	struct commit *head, *other;
>  	struct object_id oid;
>  	const char **prune = NULL;
> +	const char *action_head_name;
>  	int i, prune_num = 1; /* counting terminating NULL */
>  	struct index_state *istate = revs->repo->index;
>  
>  	if (repo_get_oid(the_repository, "HEAD", &oid))
>  		die("--merge without HEAD?");
>  	head = lookup_commit_or_die(&oid, "HEAD");
> -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> -		die("--merge without MERGE_HEAD?");
> -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> +	action_head_name = get_action_head_name(&oid);
> +	other = lookup_commit_or_die(&oid, action_head_name);
>  	add_pending_object(revs, &head->object, "HEAD");
> -	add_pending_object(revs, &other->object, "MERGE_HEAD");
> +	add_pending_object(revs, &other->object, action_head_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);

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Johannes Sixt]

Am 12.01.24 um 00:33 schrieb Michael Lohmann:
> This extends the functionality of `git log --merge` to also work with
> conflicts for rebase, cherry pick and revert.
> 
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
> Hi everybody!
> 
> When Phillip Wood gave me some nice hints on his workflow concerning
> conflicts [1] (we discussed if `--show-current-patch` would make sense
> for cherry pick/revert). When I learned about `git log --merge` I was a
> bit sad to discover that those do not exist for rebase/revert/cherry
> pick since they look really valuable for getting an understanding on
> what was changed. It is basically the counterpart to
> `git show ${ACTION}_HEAD` for understanding the source of the conflict.
> 
> I am curious if also other people would be interested in having an easy
> way to get a log of only the relevant commits touching conflicting files
> for said actions.
> 
> With this patch I think the functionality is there, just hijacking the
> `--merge` code - maybe an alias like `git log --conflict` or similar
> might be more descriptive now?
> 
> What do you think about this idea? (Or am I maybe missing an easy way to
> achieve it with the existing code as well?)

Ha! Very nice patch. For comparison, have a look at my patch to achieve
the same that I never submitted (in particular, the author date):
https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0

This implementation is more complete than mine. I like it.

> 
> Michael
> 
> 
> [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/
> 
>  revision.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..2e5c00dabd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,
>  	}
>  }
>  
> +static char* get_action_head_name(struct object_id* oid)
> +{
> +	if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) {
> +		return "MERGE_HEAD";
> +	} else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) {
> +		return "REBASE_HEAD";
> +	} else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) {
> +		return "CHERRY_PICK_HEAD";
> +	} else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) {
> +		return "REVERT_HEAD";
> +	} else
> +		die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
> +}
> +
>  static void prepare_show_merge(struct rev_info *revs)
>  {
>  	struct commit_list *bases;
>  	struct commit *head, *other;
>  	struct object_id oid;
>  	const char **prune = NULL;
> +	const char *action_head_name;
>  	int i, prune_num = 1; /* counting terminating NULL */
>  	struct index_state *istate = revs->repo->index;
>  
>  	if (repo_get_oid(the_repository, "HEAD", &oid))
>  		die("--merge without HEAD?");
>  	head = lookup_commit_or_die(&oid, "HEAD");
> -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> -		die("--merge without MERGE_HEAD?");
> -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> +	action_head_name = get_action_head_name(&oid);
> +	other = lookup_commit_or_die(&oid, action_head_name);
>  	add_pending_object(revs, &head->object, "HEAD");
> -	add_pending_object(revs, &other->object, "MERGE_HEAD");
> +	add_pending_object(revs, &other->object, action_head_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);

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Johannes Sixt]

Am 12.01.24 um 08:35 schrieb Johannes Sixt:
> ... (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0

The Github UI is a bit unhelpful here. The author date is Nov 11, 2014.

-- Hannes

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[phillip.wood123]

Hi Michael

On 11/01/2024 23:33, Michael Lohmann wrote:
> This extends the functionality of `git log --merge` to also work with
> conflicts for rebase, cherry pick and revert.
> 
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
> Hi everybody!
> 
> When Phillip Wood gave me some nice hints on his workflow concerning
> conflicts [1] (we discussed if `--show-current-patch` would make sense
> for cherry pick/revert). When I learned about `git log --merge` I was a
> bit sad to discover that those do not exist for rebase/revert/cherry
> pick since they look really valuable for getting an understanding on
> what was changed. It is basically the counterpart to
> `git show ${ACTION}_HEAD` for understanding the source of the conflict.
> 
> I am curious if also other people would be interested in having an easy
> way to get a log of only the relevant commits touching conflicting files
> for said actions.
> 
> With this patch I think the functionality is there, just hijacking the
> `--merge` code - maybe an alias like `git log --conflict` or similar
> might be more descriptive now?
> 
> What do you think about this idea? (Or am I maybe missing an easy way to
> achieve it with the existing code as well?)

I should start by saying that I didn't know "git log --merge" existed 
before I saw this message so please correct me if I've misunderstood 
what this patch is doing. If I understand correctly it shows the commits 
from each side of the merge and is equivalent to

     git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)

When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ 
[*] so I'm not sure what

     git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)

tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a common 
ancestor. As I say I've not used "git log --merge" so maybe I'm missing 
the reason why it would be useful when cherry-picking or rebasing.

Best Wishes

Phillip

[*] assuming we're not picking a merge commit

> Michael
> 
> 
> [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/
> 
>   revision.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..2e5c00dabd 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,
>   	}
>   }
>   
> +static char* get_action_head_name(struct object_id* oid)
> +{
> +	if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) {
> +		return "MERGE_HEAD";
> +	} else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) {
> +		return "REBASE_HEAD";
> +	} else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) {
> +		return "CHERRY_PICK_HEAD";
> +	} else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) {
> +		return "REVERT_HEAD";
> +	} else
> +		die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
> +}
> +
>   static void prepare_show_merge(struct rev_info *revs)
>   {
>   	struct commit_list *bases;
>   	struct commit *head, *other;
>   	struct object_id oid;
>   	const char **prune = NULL;
> +	const char *action_head_name;
>   	int i, prune_num = 1; /* counting terminating NULL */
>   	struct index_state *istate = revs->repo->index;
>   
>   	if (repo_get_oid(the_repository, "HEAD", &oid))
>   		die("--merge without HEAD?");
>   	head = lookup_commit_or_die(&oid, "HEAD");
> -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> -		die("--merge without MERGE_HEAD?");
> -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> +	action_head_name = get_action_head_name(&oid);
> +	other = lookup_commit_or_die(&oid, action_head_name);
>   	add_pending_object(revs, &head->object, "HEAD");
> -	add_pending_object(revs, &other->object, "MERGE_HEAD");
> +	add_pending_object(revs, &other->object, action_head_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);

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Michael Lohmann]

Hi Phillip,

On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote:
> I should start by saying that I didn't know "git log --merge" existed before
> I saw this message
I also just found it and it looked very useful...

> so please correct me if I've misunderstood what this patch is doing. If I
> understand correctly it shows the commits from each side of the merge and is
> equivalent to
> 
>    git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
> 
> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*]
> so I'm not sure what
> 
>    git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)

Almost, but not quite: "git log —merge" only shows the commits touching the
conflict, so it would be equivalent to (I think):

   git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)

(or replace CHERRY_PICK with one of the other actions)

> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.

True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I
have to confess I did not check how it would behave under those circumstances.
It could either error, or (more helpful) show the log touching the file until
the root commit.

Best wishes
Michael

[PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Michael Lohmann]

This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---
Hi!

On 12. Jan 2024, at 01:15, Junio C Hamano <gitster@pobox.com> wrote:
> > This extends the functionality of `git log --merge` to also work with
> > conflicts for rebase, cherry pick and revert.
> > 
> > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> > ---
> > ... It is basically the counterpart to
> > `git show ${ACTION}_HEAD` for understanding the source of the conflict.
> 
> I do not know about the validity of that approach to use *_HEAD,

What I wanted to convey is that e.g. "git show CHERRY_PICK_HEAD" will
tell you about the conflict from the perspective of the commit that is
currently to be applied while "git log --merge" tells the story from the
perspective of HEAD. So they are by no means the same, but can
complement each other in getting an understanding about the conflict.

> but we may want to tighten the original's use of repo_get
> > -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> > -		die("--merge without MERGE_HEAD?");
> > -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> 
> ... so that we won't be confused in a repository that has a tag
> whose name happens to be MERGE_HEAD.  We probably should be using
> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...

Here I am really at the limit of my understanding of the core functions.
Is this roughly what you had in mind? From my testing it seems to do the
job, but I don't understand the details "refs_resolve_ref_unsafe"...

 revision.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index 2424c9bd67..786e1a3e89 100644
--- a/revision.c
+++ b/revision.c
@@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs)
 	struct commit *head, *other;
 	struct object_id oid;
 	const char **prune = NULL;
+	const char *other_head;
 	int i, prune_num = 1; /* counting terminating NULL */
 	struct index_state *istate = revs->repo->index;
 
 	if (repo_get_oid(the_repository, "HEAD", &oid))
 		die("--merge without HEAD?");
 	head = lookup_commit_or_die(&oid, "HEAD");
-	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+	other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
+					     "MERGE_HEAD",
+					     RESOLVE_REF_READING,
+					     &oid,
+					     NULL);
+	if (!other_head)
 		die("--merge without MERGE_HEAD?");
-	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
+	other = lookup_commit_or_die(&oid, other_head);
 	add_pending_object(revs, &head->object, "HEAD");
-	add_pending_object(revs, &other->object, "MERGE_HEAD");
+	add_pending_object(revs, &other->object, other_head);
 	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);
-- 
2.43.0.284.g6c31128a96

[PATCH v2 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Michael Lohmann]

Co-authored-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---

> I won't comment on the coding style violations in the patch below in
> this message.

I hope this one is better. The other one was just a proof of the general
concept and meant as a starting point for a discussion if this is wanted
at all. But I should still have taken more care.

On 12. Jan 2024, at 08:35, Johannes Sixt <j6t@kdbg.org> wrote:
> Ha! Very nice patch. For comparison, have a look at my patch to achieve
> the same that I never submitted (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0
> 
> This implementation is more complete than mine. I like it.

Ha! Nice one! I took a few of your changes as an inspiration and put you
as a co-author.

Cheers
Michael

Difference compared to v1: Basically complete rewrite using
"refs_resolve_ref_unsafe". Has to be applied after [PATCH v2 1/2] to
avoid conflict.

 revision.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/revision.c b/revision.c
index 786e1a3e89..b0b47dd241 100644
--- a/revision.c
+++ b/revision.c
@@ -1961,6 +1961,25 @@ static void add_pending_commit_list(struct rev_info *revs,
 	}
 }
 
+static const char *lookup_other_head(struct object_id *oid)
+{
+	struct ref_store *refs = get_main_ref_store(the_repository);
+	const char *name;
+	int i;
+	static const char *const other_head[] = {
+		"MERGE_HEAD", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD"
+	};
+
+	for (i = 0; i < ARRAY_SIZE(other_head); i++) {
+		name = refs_resolve_ref_unsafe(refs, other_head[i],
+					       RESOLVE_REF_READING, oid, NULL);
+		if (name)
+			return name;
+	}
+
+	die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
+}
+
 static void prepare_show_merge(struct rev_info *revs)
 {
 	struct commit_list *bases;
@@ -1974,13 +1993,7 @@ 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");
-	other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
-					     "MERGE_HEAD",
-					     RESOLVE_REF_READING,
-					     &oid,
-					     NULL);
-	if (!other_head)
-		die("--merge without MERGE_HEAD?");
+	other_head = lookup_other_head(&oid);
 	other = lookup_commit_or_die(&oid, other_head);
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, other_head);
-- 
2.43.0.284.g6c31128a96

Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Junio C Hamano]

Michael Lohmann <mi.al.lohmann@gmail.com> writes:

>> but we may want to tighten the original's use of repo_get
>> > -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
>> > -		die("--merge without MERGE_HEAD?");
>> > -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
>> 
>> ... so that we won't be confused in a repository that has a tag
>> whose name happens to be MERGE_HEAD.  We probably should be using
>> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
>
> Here I am really at the limit of my understanding of the core functions.
> Is this roughly what you had in mind? From my testing it seems to do the
> job, but I don't understand the details "refs_resolve_ref_unsafe"...

Perhaps there are others who are more familiar with the ref API than
I am, but I was just looking at refs.h today and wonder if something
along the lines of this ...

    if (read_ref_full("MERGE_HEAD",
    		      RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
		      &oid, NULL))
	die("no MERGE_HEAD");
    if (is_null_oid(&oid))
	die("MERGE_HEAD is a symbolic ref???");

... would be simpler.

With the above, we are discarding the refname read_ref_full()
obtains from its refs_resolve_ref_unsafe(), but I think that is
totally fine.  We expect it to be "MERGE_HEAD" in the good case.
The returned value can be different from "MERGE_HEAD" if it is
a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be
trusted, we should catch that case with the NULL-ness check on oid.

Which would mean that we do not need a separate "other_head"
variable, and instead can use "MERGE_HEAD".


>  revision.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..786e1a3e89 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs)
>  	struct commit *head, *other;
>  	struct object_id oid;
>  	const char **prune = NULL;
> +	const char *other_head;
>  	int i, prune_num = 1; /* counting terminating NULL */
>  	struct index_state *istate = revs->repo->index;
>  
>  	if (repo_get_oid(the_repository, "HEAD", &oid))
>  		die("--merge without HEAD?");
>  	head = lookup_commit_or_die(&oid, "HEAD");
> -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> +	other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> +					     "MERGE_HEAD",
> +					     RESOLVE_REF_READING,
> +					     &oid,
> +					     NULL);
> +	if (!other_head)
>  		die("--merge without MERGE_HEAD?");
> -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> +	other = lookup_commit_or_die(&oid, other_head);
>  	add_pending_object(revs, &head->object, "HEAD");
> -	add_pending_object(revs, &other->object, "MERGE_HEAD");
> +	add_pending_object(revs, &other->object, other_head);
>  	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);

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> What do you think about this idea? (Or am I maybe missing an easy way to
>> achieve it with the existing code as well?)
>
> Ha! Very nice patch. For comparison, have a look at my patch to achieve
> the same that I never submitted (in particular, the author date):
> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0
>
> This implementation is more complete than mine. I like it.

I like the way your other_merge_candidate() loops over an array of
possible candidates, which makes it a lot easier to extend, though,
admittedly the "die()" message needs auto-generating if we really
wanted to make it maintenance free ;-)

Thanks.

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Junio C Hamano]

phillip.wood123@gmail.com writes:

> I should start by saying that I didn't know "git log --merge" existed
> before I saw this message so please correct me if I've misunderstood
> what this patch is doing. If I understand correctly it shows the
> commits from each side of the merge and is equivalent to
>
>     git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
>
> When a commit is cherry-picked the merge base used is
> CHERRY_PICK_HEAD^ [*] so I'm not sure what
>
>     git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)
>
> tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a
> common ancestor. As I say I've not used "git log --merge" so maybe I'm
> missing the reason why it would be useful when cherry-picking or
> rebasing.

Good question.  It is the whole point of cherry-pick that
CHERRY_PICK_HEAD and the current HEAD may not have any meaningful
ancestry relationship, so yes, it is questionable to use the same
logic for "log --merge" between HEAD...CHERRY_PICK_HEAD, while using
HEAD...MERGE_HEAD does make perfect sense.

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Michael Lohmann]

> > tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a
> > common ancestor. As I say I've not used "git log --merge" so maybe I'm
> > missing the reason why it would be useful when cherry-picking or
> > rebasing.
> 
> Good question.  It is the whole point of cherry-pick that
> CHERRY_PICK_HEAD and the current HEAD may not have any meaningful
> ancestry relationship, so yes, it is questionable to use the same
> logic for "log --merge" between HEAD...CHERRY_PICK_HEAD, while using
> HEAD...MERGE_HEAD does make perfect sense.

I tried to say it in [1]: a merge also does not need a common ancestor! Try it:

   git init test
   cd test
   echo something > README.md
   git add --all
   git commit -m "initial commit"
   git remote add origin_git git://git.kernel.org/pub/scm/git/git.git
   git fetch origin_git master     # obviously no common ancestor
   git merge --allow-unrelated-histories origin_git/master
   # merge conflict
   git log --merge    # Still loggs all the conflicting commits

So indeed the command that Phillip wrote is not equivalent and the existing
logic already can deal with unrelated histories. Or am I misunderstanding?

Michael

[1] https://lore.kernel.org/git/20240112150346.73735-1-mi.al.lohmann@gmail.com/

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Junio C Hamano]

Michael Lohmann <mi.al.lohmann@gmail.com> writes:

> Almost, but not quite: "git log —merge" only shows the commits touching the
> conflict, so it would be equivalent to (I think):
>
>    git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)
>
> (or replace CHERRY_PICK with one of the other actions)

It can certainly _reduce_ the noise, but I am not sure if it works
over the right history segment.  Let me think aloud a bit.

Let's imagine that in a history forked long time ago,

 O----O----O----O----X HEAD
  \
   Z---Z---Z---Z---A---B CHERRY_PICK_HEAD

where all commits modified the same path in question that you saw
conflicts in when your "git cherry-pick B" stopped.

I do not know what to think about the changes to that path by the
commits denoted by 'O', but the changes done to the path by commits
denoted by 'Z' should have absolutely no relevance, as the whole
point of cherry-picking B relative to A is because we do not care
about what Zs did, no?  For that matter, given that how we structure
the 3-way merge for such a cherry-pick to "move from X the same way
as you would move from A to B", how you got to X (i.e. Os) should
not matter, either.

On the other hand, such a conflict may arise from the fact that Os
and Zs made changes differently to cause the contents of the path at
X and A differ significantly.  So, OK, I can buy your argument that
what Os and Zs to the conflicted path did can be interesting when
understanding the conflict during such a cherry-pick.

>> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.
>
> True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I

But that is very different, isn't it?  Merging two unrelated
histories is like merging two histories where the common ancestor
had an empty tree, i.e.

      o---o---o---X HEAD
     /
   (v) an imaginary ancestor with an empty tree
     \
      o---o---o---O MERGE_HEAD

so it is a reasonable degenerated behaviour to consider what all
commits on both sides did to the paths in question.

Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert

[Phillip Wood]

Hi Michael

On 12/01/2024 15:03, Michael Lohmann wrote:
> Hi Phillip,
> 
> On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote:
>> I should start by saying that I didn't know "git log --merge" existed before
>> I saw this message
> I also just found it and it looked very useful...
> 
>> so please correct me if I've misunderstood what this patch is doing. If I
>> understand correctly it shows the commits from each side of the merge and is
>> equivalent to
>>
>>     git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)
>>
>> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*]
>> so I'm not sure what
>>
>>     git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)
> 
> Almost, but not quite: "git log —merge" only shows the commits touching the
> conflict, so it would be equivalent to (I think):
> 
>     git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)
> 
> (or replace CHERRY_PICK with one of the other actions)

Thanks for clarifying that.

>> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.
> 
> True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I
> have to confess I did not check how it would behave under those circumstances.
> It could either error, or (more helpful) show the log touching the file until
> the root commit.

What I was trying to get at was that with "git merge" "git log --merge" 
will show commits that are part of the merge. With "git cherry-pick" 
that's not the case because we're selecting the commits to show using 
the merge base of HEAD and CHERRY_PICK_HEAD while cherry-pick uses 
CHERRY_PICK_HEAD^ as the base of the merge. I think Junio explains why 
it is still useful to show those commits in [1] i.e. they help the user 
understand the conflicts even though they are not part of the merge. It 
might be worth expanding the commit message to explain that.

Best Wishes

Phillip

[1] https://lore.kernel.org/git/xmqqil3y9rvm.fsf@gitster.g/

Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2319 bytes --]

On Fri, Jan 12, 2024 at 12:10:54PM -0800, Junio C Hamano wrote:
> Michael Lohmann <mi.al.lohmann@gmail.com> writes:
> 
> >> but we may want to tighten the original's use of repo_get
> >> > -	if (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> >> > -		die("--merge without MERGE_HEAD?");
> >> > -	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
> >> 
> >> ... so that we won't be confused in a repository that has a tag
> >> whose name happens to be MERGE_HEAD.  We probably should be using
> >> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...
> >
> > Here I am really at the limit of my understanding of the core functions.
> > Is this roughly what you had in mind? From my testing it seems to do the
> > job, but I don't understand the details "refs_resolve_ref_unsafe"...
> 
> Perhaps there are others who are more familiar with the ref API than
> I am, but I was just looking at refs.h today and wonder if something
> along the lines of this ...
> 
>     if (read_ref_full("MERGE_HEAD",
>     		      RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> 		      &oid, NULL))
> 	die("no MERGE_HEAD");
>     if (is_null_oid(&oid))
> 	die("MERGE_HEAD is a symbolic ref???");
> 
> ... would be simpler.
> 
> With the above, we are discarding the refname read_ref_full()
> obtains from its refs_resolve_ref_unsafe(), but I think that is
> totally fine.  We expect it to be "MERGE_HEAD" in the good case.
> The returned value can be different from "MERGE_HEAD" if it is
> a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be
> trusted, we should catch that case with the NULL-ness check on oid.

Yeah, this should be fine.

Even though I have stared at our refs API for extended periods of time
during the last months I still have to look up this part of the API
almost every time. I find it to be hard to use because you not only have
to pay attention to the return value, but also to the flags in some edge
cases. I wouldn't be surprised if there were many callsites that get
this wrong.

> Which would mean that we do not need a separate "other_head"
> variable, and instead can use "MERGE_HEAD".

There is no need for this, is there? We have already resolved the ref to
an object ID, so why not use that via `add_pending_oid()`?

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Which would mean that we do not need a separate "other_head"
>> variable, and instead can use "MERGE_HEAD".
>
> There is no need for this, is there? We have already resolved the ref to
> an object ID, so why not use that via `add_pending_oid()`?

add_pending_oid() and its callees use the name only for human
consumption (e.g., in error messages), as they all already have the
object name.

[PATCH v3 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Michael Lohmann]

This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
(3) error out when MERGE_HEAD is a symref.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---

Changes with respect to v2:
- use read_ref_full instead of refs_resolve_ref_unsafe
- don't use "other_head" variable to store MERGE_HEAD
- check for symbolic ref

 revision.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 2424c9bd67..aa4c4dc778 100644
--- a/revision.c
+++ b/revision.c
@@ -1973,8 +1973,12 @@ 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 (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+	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");
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");
-- 
2.39.3 (Apple Git-145)

[PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Michael Lohmann]

Co-authored-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
---

On 12. Jan 2024, at 21:18, Junio C Hamano <gitster@pobox.com> wrote:
> I like the way your other_merge_candidate() loops over an array of
> possible candidates, which makes it a lot easier to extend, though,
> admittedly the "die()" message needs auto-generating if we really
> wanted to make it maintenance free ;-)

I would certainly like that but I did not find an easy way of achieving
this in C. Help wanted.

Changes with respect to v2:
- use read_ref_full instead of refs_resolve_ref_unsafe
- check for symbolic ref
- extract "other_name" in this patch, instead of patch 1

 revision.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index aa4c4dc778..c778413c7d 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", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_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, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_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);
-- 
2.39.3 (Apple Git-145)

Full disclosure

[Michael Lohmann]

Just as a disclosure: I was told that my contributions are not welcome [1]
(even though I have to say that I don't fully agree with the reasoning), but I
did not want to leave these patches alone.

@Junio C Hamano: Please take this into account when deciding if you want to
accept the patches. This is just for transparancy and I will not do any more
contributions than potentially finishing this one. If you do not want these
patches from me, but it was still deemed to be an interesting feature: could
someone else take over?

Michael

[1]: https://lore.kernel.org/git/xmqqil3ybets.fsf@gitster.g/

Re: Full disclosure

[Christian Couder]

On Wed, Jan 17, 2024 at 10:20 AM Michael Lohmann
<mi.al.lohmann@gmail.com> wrote:
>
> Just as a disclosure: I was told that my contributions are not welcome [1]
> (even though I have to say that I don't fully agree with the reasoning), but I
> did not want to leave these patches alone.

I might have missed something, but as far as I see in the email you
mention, you weren't told that your contributions are not welcome.
Junio reviewed your patch and agreed with some of Peff's comments
about your patch. It just means that they both think your patch could
be further improved. They didn't reject your patch, nor your
contributions in general.

> @Junio C Hamano: Please take this into account when deciding if you want to
> accept the patches. This is just for transparancy and I will not do any more
> contributions than potentially finishing this one. If you do not want these
> patches from me, but it was still deemed to be an interesting feature: could
> someone else take over?
>
> Michael
>
> [1]: https://lore.kernel.org/git/xmqqil3ybets.fsf@gitster.g/
>

Full disclosure

[Michael Lohmann]

Hi Christian!

Yes: I agree that the current state of the last submitted patch in that
discussion is far from optimal.
After Jeff Kings explanation I had a much better understanding for the
situation and the reasoning (and his suggestion was definitely better than
mine).

I had already prepared a new version which tackled (I think) pretty much all of
the criticisms. But then the above mentioned message came in and when I read
this:

> [...] they are trying to be different for the sake of being different, which
> is not a good sign.  I'd want our contributors to be original where being
> original matters more.

I am reading:

1) I am "trying to be different for the sake of being different"
2) Contributors like this are not wanted

So yes, I do understand this as a general statement on me as a contributor
without any proposal for me at least to explain the situation from my side.

I teach my colleges not to name variables with how they are initialized, but
with what information they actually convey and I found the "_NONE" one at least
misleading in its name.

So in the initial discussion I was a bit stubborn, because Philip wrote "I
don't have a strong opinion" and from my perspective the only argument was
"over there we also do it that way" (which _can_ 100% be a valid reason), but
Junio C Hamano did not even acknowledge my criticisms of the other the variable
name. I am totally fine with a decision like this if done consciously, but if I
don't even get an acknowledgement, let alone an explanation, my demands I place
on my code quality are that I write the best code I can. And with all the info
I had (prior to Jeffs message) I did still favour my first suggestion.

In my eyes it would be helpful to at least tell me what your (in my eyes not
obvious) preferences are on naming things, because otherwise I will rather
stick to my standards than blindly follow a single instance of other code where
I don't even know if that was a concious decision or it just happened by
accident.

So no, I don't agree with the assessment of point 1), but I still read the
message like that. I will accept it and instead use my skills in different
projects where they are indeed valued.

Cheers
Michael

Re: Full disclosure

[Junio C Hamano]

Michael Lohmann <mi.al.lohmann@gmail.com> writes:

> Just as a disclosure: I was told that my contributions are not welcome [1]
> (even though I have to say that I don't fully agree with the reasoning), but I
> did not want to leave these patches alone.

Rereading the message I agree that the message came out in a wrong
way that I did not intend.  It is sure that I do not want to see
certain attitude and behaviour in the patches in our contributors,
but that is totally different from "contribution from a specific
contributor is not welcome".  The phrasing was making of my
incompetence in communication skills, not from malice.

So I am sorry that I wrote something that it is fair to read as
saying "you are unwelcome".  I didn't mean it that way.

Re: Full disclosure

[Ruben Safir]

https://www.zazzle.com/idit_aharons_tzfat_original_backpack_design-256246398431855710

On 1/17/24 12:41, Michael Lohmann wrote:
> Hi Christian!
> 
> Yes: I agree that the current state of the last submitted patch in that
> discussion is far from optimal.
> After Jeff Kings explanation I had a much better understanding for the
> situation and the reasoning (and his suggestion was definitely better than
> mine).
> 
> I had already prepared a new version which tackled (I think) pretty much all of
> the criticisms. But then the above mentioned message came in and when I read
> this:
> 
>> [...] they are trying to be different for the sake of being different, which
>> is not a good sign.  I'd want our contributors to be original where being
>> original matters more.
> 
> I am reading:
> 
> 1) I am "trying to be different for the sake of being different"
> 2) Contributors like this are not wanted
> 
> So yes, I do understand this as a general statement on me as a contributor
> without any proposal for me at least to explain the situation from my side.
> 
> I teach my colleges not to name variables with how they are initialized, but
> with what information they actually convey and I found the "_NONE" one at least
> misleading in its name.
> 
> So in the initial discussion I was a bit stubborn, because Philip wrote "I
> don't have a strong opinion" and from my perspective the only argument was
> "over there we also do it that way" (which _can_ 100% be a valid reason), but
> Junio C Hamano did not even acknowledge my criticisms of the other the variable
> name. I am totally fine with a decision like this if done consciously, but if I
> don't even get an acknowledgement, let alone an explanation, my demands I place
> on my code quality are that I write the best code I can. And with all the info
> I had (prior to Jeffs message) I did still favour my first suggestion.
> 
> In my eyes it would be helpful to at least tell me what your (in my eyes not
> obvious) preferences are on naming things, because otherwise I will rather
> stick to my standards than blindly follow a single instance of other code where
> I don't even know if that was a concious decision or it just happened by
> accident.
> 
> So no, I don't agree with the assessment of point 1), but I still read the
> message like that. I will accept it and instead use my skills in different
> projects where they are indeed valued.
> 
> Cheers
> Michael

-- 
So many immigrant groups have swept through our town
that Brooklyn, like Atlantis, reaches mythological
proportions in the mind of the world - RI Safir 1998
http://www.mrbrklyn.com
DRM is THEFT - We are the STAKEHOLDERS - RI Safir 2002

http://www.nylxs.com - Leadership Development in Free Software
http://www.brooklyn-living.com

Being so tracked is for FARM ANIMALS and extermination camps,
but incompatible with living as a free human being. -RI Safir 2013

Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Elijah Newren]

On Wed, Jan 17, 2024 at 12:14 AM Michael Lohmann
<mi.al.lohmann@gmail.com> wrote:
>
> Co-authored-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> ---
>
> On 12. Jan 2024, at 21:18, Junio C Hamano <gitster@pobox.com> wrote:
> > I like the way your other_merge_candidate() loops over an array of
> > possible candidates, which makes it a lot easier to extend, though,
> > admittedly the "die()" message needs auto-generating if we really
> > wanted to make it maintenance free ;-)
>
> I would certainly like that but I did not find an easy way of achieving
> this in C. Help wanted.
>
> Changes with respect to v2:
> - use read_ref_full instead of refs_resolve_ref_unsafe
> - check for symbolic ref
> - extract "other_name" in this patch, instead of patch 1
>
>  revision.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index aa4c4dc778..c778413c7d 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", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_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, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_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);
> --
> 2.39.3 (Apple Git-145)

I had to go look up previous versions to see the discussion of why
this was useful for things other than merge.  I agree with Phillip
from https://lore.kernel.org/git/648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com/,
that the commit message _needs_ to explain this, likely using some of
Junio's explanation.

Also, what about cases where users do a cherry-pick in the middle of a
rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist?  What
then?

Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Johannes Sixt]

Am 24.01.24 um 08:06 schrieb Elijah Newren:
> On Wed, Jan 17, 2024 at 12:14 AM Michael Lohmann
>> +static const char *lookup_other_head(struct object_id *oid)
>> +{
>> +       int i;
>> +       static const char *const other_head[] = {
>> +               "MERGE_HEAD", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_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, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?");
>> +}
...
> Also, what about cases where users do a cherry-pick in the middle of a
> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist?  What
> then?

Good point! IMO, REBASE_HEAD should have lower precedence than all the
other *_HEADs. It would mean to reorder the entries:

	static const char *const other_head[] = {
		"MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
	};

(and perhaps adjust the error message, too).

-- Hannes

Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> I had to go look up previous versions to see the discussion of why
> this was useful for things other than merge.  I agree with Phillip
> from https://lore.kernel.org/git/648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com/,
> that the commit message _needs_ to explain this, likely using some of
> Junio's explanation.

Please note that I am not very happy with that "explanation" myself.
The only thing I can still agree to in that message myself is that
it is sensible for "log --merge" to go down all the way to the root
of the histories leading to MERGE_HEAD and HEAD in the "merging two
unrelated histories" scenario.  Treating CHERRY_PICK_HEAD and others
the same way, to me, almost sounds as if we are saying "all the
commits behind the commits involved in the conflicted operation are
worth looking at", which is not a very useful or productive thing.

> Also, what about cases where users do a cherry-pick in the middle of a
> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist?  What
> then?

;-)

Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Good point! IMO, REBASE_HEAD should have lower precedence than all the
> other *_HEADs. It would mean to reorder the entries:
>
> 	static const char *const other_head[] = {
> 		"MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
> 	};
>
> (and perhaps adjust the error message, too).

And probably give a warning saying that we noticed you are rebasing
and cherry-picking and we chose to show the --merge based on the
relationship between cherry-pick-head and head, ignoring your rebase
status, or something.

Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Johannes Sixt]

Am 24.01.24 um 20:46 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Good point! IMO, REBASE_HEAD should have lower precedence than all the
>> other *_HEADs. It would mean to reorder the entries:
>>
>> 	static const char *const other_head[] = {
>> 		"MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
>> 	};
>>
>> (and perhaps adjust the error message, too).
> 
> And probably give a warning saying that we noticed you are rebasing
> and cherry-picking and we chose to show the --merge based on the
> relationship between cherry-pick-head and head, ignoring your rebase
> status, or something.

I don't think that's necessary. When rebase stopped with a merge
conflict, neither of the other commands can begin their work until the
conflicted state is removed. That should be a concious act, just like
when thereafter one of these other commands is used and leads to a
conflict. At least I would certainly not need a reminder.

-- Hannes

Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 24.01.24 um 20:46 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>> 
>>> Good point! IMO, REBASE_HEAD should have lower precedence than all the
>>> other *_HEADs. It would mean to reorder the entries:
>>>
>>> 	static const char *const other_head[] = {
>>> 		"MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
>>> 	};
>>>
>>> (and perhaps adjust the error message, too).
>> 
>> And probably give a warning saying that we noticed you are rebasing
>> and cherry-picking and we chose to show the --merge based on the
>> relationship between cherry-pick-head and head, ignoring your rebase
>> status, or something.
>
> I don't think that's necessary. When rebase stopped with a merge
> conflict, neither of the other commands can begin their work until the
> conflicted state is removed. That should be a concious act, just like
> when thereafter one of these other commands is used and leads to a
> conflict. At least I would certainly not need a reminder.

OK.

Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> Also, what about cases where users do a cherry-pick in the middle of a
>> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist?  What
>> then?
>
> Good point! IMO, REBASE_HEAD should have lower precedence than all the
> other *_HEADs. It would mean to reorder the entries:
>
> 	static const char *const other_head[] = {
> 		"MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
> 	};
>
> (and perhaps adjust the error message, too).

I've tweaked this change into the commit.

Thanks, all.

------- >8 ------------- >8 ------------- >8 ------------- >8 -------
From: Michael Lohmann <mi.al.lohmann@gmail.com>
Date: Wed, 17 Jan 2024 09:14:05 +0100
Subject: [PATCH] revision: implement `git log --merge` also for
 rebase/cherry_pick/revert

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>
---
 revision.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

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);
-- 
2.44.0-rc0

[PATCH v4 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert

[Philippe Blain]

Changes in v4:
- Added a commit message for 2/2 detailing the use case and summarizing the discussion in the thread
- Adjusted the documentation of the option

Link to v3: https://lore.kernel.org/r/20240117081405.14012-1-mi.al.lohmann@gmail.com

Range-diff vs v3:

1:  37405be1a3 = 1:  37405be1a3 revision: ensure MERGE_HEAD is a ref in prepare_show_merge
2:  de080a628c ! 2:  6ac1608809 revision: implement `git log --merge` also for rebase/cherry_pick/revert
    @@ Metadata
     Author: Michael Lohmann <mi.al.lohmann@gmail.com>

      ## Commit message ##
    -    revision: implement `git log --merge` also for rebase/cherry_pick/revert
    +    revision: implement `git log --merge` also for rebase/cherry-pick/revert

    +    '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 ##
    +@@ Documentation/gitk.txt: 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::
    +
    +
    + ## Documentation/rev-list-options.txt ##
    +@@ Documentation/rev-list-options.txt: 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
    +
      ## revision.c ##
     @@ revision.c: static void add_pending_commit_list(struct rev_info *revs,
      	}

---
Michael Lohmann (2):
      revision: ensure MERGE_HEAD is a ref in prepare_show_merge
      revision: implement `git log --merge` also for rebase/cherry-pick/revert

 Documentation/gitk.txt             |  8 ++++----
 Documentation/rev-list-options.txt |  6 ++++--
 revision.c                         | 27 +++++++++++++++++++++++----
 3 files changed, 31 insertions(+), 10 deletions(-)
---
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
change-id: 20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-05bd8e8797db

[PATCH v4 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Philippe Blain]

From: Michael Lohmann <mi.al.lohmann@gmail.com>

This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
(3) error out when MERGE_HEAD is a symref.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 2424c9bd67..aa4c4dc778 100644
--- a/revision.c
+++ b/revision.c
@@ -1973,8 +1973,12 @@ 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 (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+	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");
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");

-- 
2.39.1

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

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

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`,
+	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);

-- 
2.39.1

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

[Johannes Sixt]

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

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

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

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.

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

[Johannes Sixt]

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

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

[Phillip Wood]

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

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

[Junio C Hamano]

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>

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

[Jean-Noël Avila]

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

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

[Philippe Blain]

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.

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

[Philippe Blain]

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.

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

[Phillip Wood]

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

[PATCH v5 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert

[Philippe Blain]

Changes in v5:
- Marked error messages for translation and tweaked them as suggested by Phillip
- Reworded the message of 2/2 as suggested by Phillip
- Removed the change to gitk's doc in 2/2 as pointed out by Johannes
- Fixed the trailers in 2/2
- Improved the doc in 2/2 as suggested by Phillip and Jean-Noël

Changes in v4:
- Added a commit message for 2/2 detailing the use case and summarizing the discussion in the thread
- Adjusted the documentation of the option

---
Michael Lohmann (2):
      revision: ensure MERGE_HEAD is a ref in prepare_show_merge
      revision: implement `git log --merge` also for rebase/cherry-pick/revert

 Documentation/rev-list-options.txt |  7 +++++--
 revision.c                         | 27 +++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 6 deletions(-)
---
base-commit: 186b115d3062e6230ee296d1ddaa0c4b72a464b5
change-id: 20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-05bd8e8797db

Range-diff versus v4:

1:  37405be1a3 ! 1:  c9536431d1 revision: ensure MERGE_HEAD is a ref in prepare_show_merge
    @@ Commit message
         Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    +    Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
     
      ## revision.c ##
     @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
    @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
     +			&oid, NULL))
      		die("--merge without MERGE_HEAD?");
     +	if (is_null_oid(&oid))
    -+		die("MERGE_HEAD is a symbolic ref???");
    ++		die(_("MERGE_HEAD is a symbolic ref?"));
      	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
      	add_pending_object(revs, &head->object, "HEAD");
      	add_pending_object(revs, &other->object, "MERGE_HEAD");
2:  6ac1608809 ! 2:  1641c4be81 revision: implement `git log --merge` also for rebase/cherry-pick/revert
    @@ Commit message
         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 rebases and cherry-picks, an interesting range to look at is
    +    HEAD...{REBASE_HEAD,CHERRY_PICK_HEAD}, since even if all the commits
    +    included in that range are not directly part of the 3-way merge,
    +    conflicts encountered during these operations can indeed be caused by
    +    changes introduced in preceding commits on both sides of the history.
     
    -    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.
    +    For revert, as we are (most likely) reversing changes from a previous
    +    commit, an appropriate range is REVERT_HEAD..HEAD, which is equivalent
    +    to REVERT_HEAD...HEAD and to HEAD...REVERT_HEAD, if we keep HEAD and its
    +    parents on the left side of the range.
     
    -    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.
    +    As such, adjust the code in prepare_show_merge so it constructs the
    +    range HEAD...$OTHER for 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>
    +    Co-authored-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 ##
    -@@ Documentation/gitk.txt: 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::
    - 
    +    Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
     
      ## Documentation/rev-list-options.txt ##
     @@ Documentation/rev-list-options.txt: See also linkgit:git-reflog[1].
    @@ Documentation/rev-list-options.txt: See also linkgit:git-reflog[1].
      --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`,
    ++	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.
    ++	when the index has unmerged entries. This option can be used to show
    ++	relevant commits when resolving conflicts from a 3-way merge.
      
      --boundary::
      	Output excluded boundary commits. Boundary commits are
    @@ revision.c: static void add_pending_commit_list(struct rev_info *revs,
     +				RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
     +				oid, NULL)) {
     +			if (is_null_oid(oid))
    -+				die("%s is a symbolic ref???", other_head[i]);
    ++				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?");
    ++	die(_("--merge requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD"));
     +}
     +
      static void prepare_show_merge(struct rev_info *revs)
    @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
     -			&oid, NULL))
     -		die("--merge without MERGE_HEAD?");
     -	if (is_null_oid(&oid))
    --		die("MERGE_HEAD is a symbolic ref???");
    +-		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);

[PATCH v5 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Philippe Blain]

From: Michael Lohmann <mi.al.lohmann@gmail.com>

This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
(3) error out when MERGE_HEAD is a symref.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 revision.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 2424c9bd67..ee26988cc6 100644
--- a/revision.c
+++ b/revision.c
@@ -1973,8 +1973,12 @@ 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 (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+	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");
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");

-- 
2.39.1

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

[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 and cherry-picks, an interesting range to look at is
HEAD...{REBASE_HEAD,CHERRY_PICK_HEAD}, since even if all the commits
included in that range are not directly part of the 3-way merge,
conflicts encountered during these operations can indeed be caused by
changes introduced in preceding commits on both sides of the history.

For revert, as we are (most likely) reversing changes from a previous
commit, an appropriate range is REVERT_HEAD..HEAD, which is equivalent
to REVERT_HEAD...HEAD and to HEAD...REVERT_HEAD, if we keep HEAD and its
parents on the left side of the range.

As such, adjust the code in prepare_show_merge so it constructs the
range HEAD...$OTHER for 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: Johannes Sixt <j6t@kdbg.org>
Co-authored-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: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/rev-list-options.txt |  7 +++++--
 revision.c                         | 31 +++++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 2bf239ff03..9ce7a5eedc 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -341,8 +341,11 @@ 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. This option can be used to show
+	relevant commits when resolving conflicts from a 3-way merge.
 
 --boundary::
 	Output excluded boundary commits. Boundary commits are
diff --git a/revision.c b/revision.c
index ee26988cc6..a90a6f861b 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 requires one of the pseudorefs 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);

-- 
2.39.1

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

[Junio C Hamano]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> +	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 requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD"));
> +}

Just a minor nit, but reacting to recent "passive-aggressive"
message change in another thread, perhaps we should stop asking a
rhetorical question like the new message and instead state what we
detected and what we consider is an error condition as a fact in
them.

The last die() in the above helper function used to be such a
rhetorical question "--merge without HEAD?" but now it reads much
better.  The one about symbolic ref is new in this series, and we
can avoid making it rhetorical from the get go.  Perhaps "%s exists
but it is a symbolic ref" or something?

Re: [PATCH v5 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Jean-Noël Avila]

Hello,

Le 25/02/2024 à 22:56, Philippe Blain a écrit :
> From: Michael Lohmann <mi.al.lohmann@gmail.com>
> 
> This is done to
> (1) ensure MERGE_HEAD is a ref,
> (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
> (3) error out when MERGE_HEAD is a symref.
> 
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>  revision.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/revision.c b/revision.c
> index 2424c9bd67..ee26988cc6 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1973,8 +1973,12 @@ 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 (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
> +	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?"));

Following the thread about being less passive-aggressive, maybe this
could be rephrased in an assertive mood.

By the way, this string is translatable, but not the one 2 lines above.
What is the policy around translation?

>  	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
>  	add_pending_object(revs, &head->object, "HEAD");
>  	add_pending_object(revs, &other->object, "MERGE_HEAD");
> 

Thanks.

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

[Philippe Blain]

Hi Junio,

Le 2024-02-25 à 23:35, Junio C Hamano a écrit :
> Philippe Blain <levraiphilippeblain@gmail.com> writes:
> 
>> +	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 requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD"));
>> +}
> 
> Just a minor nit, but reacting to recent "passive-aggressive"
> message change in another thread, perhaps we should stop asking a
> rhetorical question like the new message and instead state what we
> detected and what we consider is an error condition as a fact in
> them.
> 
> The last die() in the above helper function used to be such a
> rhetorical question "--merge without HEAD?" but now it reads much
> better.  The one about symbolic ref is new in this series, and we
> can avoid making it rhetorical from the get go.  Perhaps "%s exists
> but it is a symbolic ref" or something?

Ok, I can make that change. I agree we should maybe keep these rethorical 
questions to 'BUG' calls...

Thanks,
Philippe.

Re: [PATCH v5 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Philippe Blain]

Hi Jean-Noël,

Le 2024-02-26 à 12:22, Jean-Noël Avila a écrit :
> Hello,
> 
> Le 25/02/2024 à 22:56, Philippe Blain a écrit :
>> From: Michael Lohmann <mi.al.lohmann@gmail.com>
>>
>> This is done to
>> (1) ensure MERGE_HEAD is a ref,
>> (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
>> (3) error out when MERGE_HEAD is a symref.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>>  revision.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/revision.c b/revision.c
>> index 2424c9bd67..ee26988cc6 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1973,8 +1973,12 @@ 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 (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
>> +	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?"));
> 
> Following the thread about being less passive-aggressive, maybe this
> could be rephrased in an assertive mood.

Yes, Junio suggested the same in <xmqqa5nnj10v.fsf@gitster.g>.

> 
> By the way, this string is translatable, but not the one 2 lines above.
> What is the policy around translation?

My understanding is that new error messages should be translated, but here the patch
is not touching the message "--merge without HEAD?" so I would think
it is OK to avoid changing these lines to mark it for translation.
But, I could make that change in a preparatory patch (and rephrase it 
at the same time). 

Thanks, 
Philippe.

Re: [PATCH v5 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert

[Phillip Wood]

On 25/02/2024 21:56, Philippe Blain wrote:
> Changes in v5:
> - Marked error messages for translation and tweaked them as suggested by Phillip
> - Reworded the message of 2/2 as suggested by Phillip
> - Removed the change to gitk's doc in 2/2 as pointed out by Johannes
> - Fixed the trailers in 2/2
> - Improved the doc in 2/2 as suggested by Phillip and Jean-Noël

These changes look good, thanks for making them. I agree with the other 
reviewers that it would be nice to improve the wording of the error 
message when we find a symbolic ref. Everything else looks good to me.

Thanks

Phillip

Re: [PATCH v5 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 25/02/2024 21:56, Philippe Blain wrote:
>> Changes in v5:
>> - Marked error messages for translation and tweaked them as suggested by Phillip
>> - Reworded the message of 2/2 as suggested by Phillip
>> - Removed the change to gitk's doc in 2/2 as pointed out by Johannes
>> - Fixed the trailers in 2/2
>> - Improved the doc in 2/2 as suggested by Phillip and Jean-Noël
>
> These changes look good, thanks for making them. I agree with the
> other reviewers that it would be nice to improve the wording of the
> error message when we find a symbolic ref. Everything else looks good
> to me.
>
> Thanks
>
> Phillip

Thanks for a review.  Queued.

[PATCH v6 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert

[Philippe Blain]

Changes in v6:
- Changed the error message added in 1/2 and adjusted in 2/2 to avoid a rhetorical
question

Changes in v5:
- Marked error messages for translation and tweaked them as suggested by Phillip
- Reworded the message of 2/2 as suggested by Phillip
- Removed the change to gitk's doc in 2/2 as pointed out by Johannes
- Fixed the trailers in 2/2
- Improved the doc in 2/2 as suggested by Phillip and Jean-Noël

Changes in v4:
- Added a commit message for 2/2 detailing the use case and summarizing the discussion in the thread
- Adjusted the documentation of the option

---
Michael Lohmann (2):
      revision: ensure MERGE_HEAD is a ref in prepare_show_merge
      revision: implement `git log --merge` also for rebase/cherry-pick/revert

 Documentation/rev-list-options.txt |  7 +++++--
 revision.c                         | 27 +++++++++++++++++++++++----
 2 files changed, 28 insertions(+), 6 deletions(-)
---
base-commit: 3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0
change-id: 20240210-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-05bd8e8797db

Range-diff versus v5:

1:  c9536431d1 ! 1:  363657561c revision: ensure MERGE_HEAD is a ref in prepare_show_merge
    @@ Commit message
         (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
         (3) error out when MERGE_HEAD is a symref.
     
    +    Note that we avoid marking the new error message for translation as it
    +    will be done in the next commit when the message is generalized to other
    +    special refs.
    +
         Helped-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
    @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
     +			&oid, NULL))
      		die("--merge without MERGE_HEAD?");
     +	if (is_null_oid(&oid))
    -+		die(_("MERGE_HEAD is a symbolic ref?"));
    ++		die(_("MERGE_HEAD exists but is a symbolic ref"));
      	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
      	add_pending_object(revs, &head->object, "HEAD");
      	add_pending_object(revs, &other->object, "MERGE_HEAD");
2:  1641c4be81 ! 2:  749abadc04 revision: implement `git log --merge` also for rebase/cherry-pick/revert
    @@ revision.c: static void add_pending_commit_list(struct rev_info *revs,
     +				RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
     +				oid, NULL)) {
     +			if (is_null_oid(oid))
    -+				die(_("%s is a symbolic ref?"), other_head[i]);
    ++				die(_("%s exists but is a symbolic ref"), other_head[i]);
     +			return other_head[i];
     +		}
     +
    @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
     -			&oid, NULL))
     -		die("--merge without MERGE_HEAD?");
     -	if (is_null_oid(&oid))
    --		die(_("MERGE_HEAD is a symbolic ref?"));
    +-		die(_("MERGE_HEAD exists but 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);

[PATCH v6 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge

[Philippe Blain]

From: Michael Lohmann <mi.al.lohmann@gmail.com>

This is done to
(1) ensure MERGE_HEAD is a ref,
(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
(3) error out when MERGE_HEAD is a symref.

Note that we avoid marking the new error message for translation as it
will be done in the next commit when the message is generalized to other
special refs.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 revision.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 2424c9bd67..df775f74d0 100644
--- a/revision.c
+++ b/revision.c
@@ -1973,8 +1973,12 @@ 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 (repo_get_oid(the_repository, "MERGE_HEAD", &oid))
+	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 exists but is a symbolic ref"));
 	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
 	add_pending_object(revs, &head->object, "HEAD");
 	add_pending_object(revs, &other->object, "MERGE_HEAD");

-- 
2.39.1

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

[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 and cherry-picks, an interesting range to look at is
HEAD...{REBASE_HEAD,CHERRY_PICK_HEAD}, since even if all the commits
included in that range are not directly part of the 3-way merge,
conflicts encountered during these operations can indeed be caused by
changes introduced in preceding commits on both sides of the history.

For revert, as we are (most likely) reversing changes from a previous
commit, an appropriate range is REVERT_HEAD..HEAD, which is equivalent
to REVERT_HEAD...HEAD and to HEAD...REVERT_HEAD, if we keep HEAD and its
parents on the left side of the range.

As such, adjust the code in prepare_show_merge so it constructs the
range HEAD...$OTHER for 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: Johannes Sixt <j6t@kdbg.org>
Co-authored-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: Philippe Blain <levraiphilippeblain@gmail.com>
---
 Documentation/rev-list-options.txt |  7 +++++--
 revision.c                         | 31 +++++++++++++++++++++++--------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a583b52c61..29e7b50bcf 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -341,8 +341,11 @@ 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. This option can be used to show
+	relevant commits when resolving conflicts from a 3-way merge.
 
 --boundary::
 	Output excluded boundary commits. Boundary commits are
diff --git a/revision.c b/revision.c
index df775f74d0..d2ebdd045a 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 exists but is a symbolic ref"), other_head[i]);
+			return other_head[i];
+		}
+
+	die(_("--merge requires one of the pseudorefs 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 exists but 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);

-- 
2.39.1

Re: [PATCH v6 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert

[phillip.wood123]

Hi Philippe

On 28/02/2024 13:54, Philippe Blain wrote:
> Range-diff versus v5:
> 
> 1:  c9536431d1 ! 1:  363657561c revision: ensure MERGE_HEAD is a ref in prepare_show_merge
>      @@ Commit message
>           (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
>           (3) error out when MERGE_HEAD is a symref.
>       
>      +    Note that we avoid marking the new error message for translation as it
>      +    will be done in the next commit when the message is generalized to other
>      +    special refs.

Looking at the change below, the new message is in fact marked for 
translation. I don't think this matters (other than the commit message 
being confusing) as the translators will only see the final version of 
the massage.

>           Helped-by: Junio C Hamano <gitster@pobox.com>
>           Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com>
>           Signed-off-by: Junio C Hamano <gitster@pobox.com>
>      @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
>       +			&oid, NULL))
>        		die("--merge without MERGE_HEAD?");
>       +	if (is_null_oid(&oid))
>      -+		die(_("MERGE_HEAD is a symbolic ref?"));
>      ++		die(_("MERGE_HEAD exists but is a symbolic ref"));

The new message is marked with _(...) so will be translated.

>        	other = lookup_commit_or_die(&oid, "MERGE_HEAD");
>        	add_pending_object(revs, &head->object, "HEAD");
>        	add_pending_object(revs, &other->object, "MERGE_HEAD");
> 2:  1641c4be81 ! 2:  749abadc04 revision: implement `git log --merge` also for rebase/cherry-pick/revert
>      @@ revision.c: static void add_pending_commit_list(struct rev_info *revs,
>       +				RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>       +				oid, NULL)) {
>       +			if (is_null_oid(oid))
>      -+				die(_("%s is a symbolic ref?"), other_head[i]);
>      ++				die(_("%s exists but is a symbolic ref"), other_head[i]);

The second patch updates the message and this new version retains the _(...)

Best Wishes

Phillip

>       +			return other_head[i];
>       +		}
>       +
>      @@ revision.c: static void prepare_show_merge(struct rev_info *revs)
>       -			&oid, NULL))
>       -		die("--merge without MERGE_HEAD?");
>       -	if (is_null_oid(&oid))
>      --		die(_("MERGE_HEAD is a symbolic ref?"));
>      +-		die(_("MERGE_HEAD exists but 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);
> 

Re: [PATCH v6 0/2] Implement `git log --merge` also for rebase/cherry-pick/revert

[Philippe Blain]

Hi Phillip,

Le 2024-02-28 à 09:40, phillip.wood123@gmail.com a écrit :
> Hi Philippe
> 
> On 28/02/2024 13:54, Philippe Blain wrote:
>> Range-diff versus v5:
>>
>> 1:  c9536431d1 ! 1:  363657561c revision: ensure MERGE_HEAD is a ref in prepare_show_merge
>>      @@ Commit message
>>           (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()
>>           (3) error out when MERGE_HEAD is a symref.
>>            +    Note that we avoid marking the new error message for translation as it
>>      +    will be done in the next commit when the message is generalized to other
>>      +    special refs.
> 
> Looking at the change below, the new message is in fact marked for translation. I don't think this matters (other than the commit message being confusing) as the translators will only see the final version of the massage.

Oops, you are right, I wanted to do what I wrote but ended up marking it also in 1/2.
Junio already merged it to next, so I guess it's gonna stay that way.

Thanks for your review,
Philippe.