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

Commit Message

Michael Lohmann Jan. 17, 2024, 8:14 a.m. UTC

Co-authored-by: Johannes Sixt <[e-mail]>
Signed-off-by: Michael Lohmann <[e-mail]>
---

On 12. Jan 2024, at 21:18, Junio C Hamano <[e-mail]> 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(-)

Comments

Michael Lohmann Jan. 17, 2024, 9:19 a.m. UTC | #1

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/

Christian Couder Jan. 17, 2024, 9:58 a.m. UTC | #2

On Wed, Jan 17, 2024 at 10:20 AM Michael Lohmann
<[e-mail]> 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/
>

Michael Lohmann Jan. 17, 2024, 5:41 p.m. UTC | #3

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

Junio C Hamano Jan. 17, 2024, 6:33 p.m. UTC | #4

Michael Lohmann <[e-mail]> 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.

Ruben Safir Jan. 21, 2024, 12:41 a.m. UTC | #5

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

Elijah Newren Jan. 24, 2024, 7:06 a.m. UTC | #6

On Wed, Jan 17, 2024 at 12:14 AM Michael Lohmann
<[e-mail]> wrote:
>
> Co-authored-by: Johannes Sixt <[e-mail]>
> Signed-off-by: Michael Lohmann <[e-mail]>
> ---
>
> On 12. Jan 2024, at 21:18, Junio C Hamano <[e-mail]> 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/[e-mail]/,
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?

Johannes Sixt Jan. 24, 2024, 5:19 p.m. UTC | #7

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

Junio C Hamano Jan. 24, 2024, 5:34 p.m. UTC | #8

Elijah Newren <[e-mail]> 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/[e-mail]/,
> 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?

;-)

Junio C Hamano Jan. 24, 2024, 7:46 p.m. UTC | #9

Johannes Sixt <[e-mail]> 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.

Johannes Sixt Jan. 24, 2024, 10:06 p.m. UTC | #10

Am 24.01.24 um 20:46 schrieb Junio C Hamano:
> Johannes Sixt <[e-mail]> 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

Junio C Hamano Jan. 24, 2024, 10:13 p.m. UTC | #11

Johannes Sixt <[e-mail]> writes:

> Am 24.01.24 um 20:46 schrieb Junio C Hamano:
>> Johannes Sixt <[e-mail]> 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.

Junio C Hamano Feb. 9, 2024, 11:54 p.m. UTC | #12

Johannes Sixt <[e-mail]> 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 <[e-mail]>
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 <[e-mail]>
Signed-off-by: Michael Lohmann <[e-mail]>
[jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]
Signed-off-by: Junio C Hamano <[e-mail]>
---
 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);

Patch

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