{"thread":{"id":"60730","subject":"[RFC PATCH] `log --merge` also for rebase/cherry pick/revert","startedAt":"2024-01-11T23:33:47Z","lastAt":"2024-02-09T23:54:48Z","messageCount":30,"participants":["Michael Lohmann","Junio C Hamano","Johannes Sixt","phillip.wood123@gmail.com","Phillip Wood","Patrick Steinhardt","Christian Couder","Ruben Safir","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486673","messageId":"20240111233311.64734-1-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":null,"subject":"[RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-11T23:33:11Z","receivedAt":"2024-01-11T23:33:47Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"This extends the functionality of `git log --merge` to also work with\nconflicts for rebase, cherry pick and revert.\n\nSigned-off-by: Michael Lohmann \n---\nHi everybody!\n\nWhen Phillip Wood gave me some nice hints on his workflow concerning\nconflicts [1] (we discussed if `--show-current-patch` would make sense\nfor cherry pick/revert). When I learned about `git log --merge` I was a\nbit sad to discover that those do not exist for rebase/revert/cherry\npick since they look really valuable for getting an understanding on\nwhat was changed. It is basically the counterpart to\n`git show ${ACTION}_HEAD` for understanding the source of the conflict.\n\nI am curious if also other people would be interested in having an easy\nway to get a log of only the relevant commits touching conflicting files\nfor said actions.\n\nWith this patch I think the functionality is there, just hijacking the\n`--merge` code - maybe an alias like `git log --conflict` or similar\nmight be more descriptive now?\n\nWhat do you think about this idea? (Or am I maybe missing an easy way to\nachieve it with the existing code as well?)\n\nMichael\n\n\n[1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/\n\n revision.c | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 2424c9bd67..2e5c00dabd 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,\n \t}\n }\n \n+static char* get_action_head_name(struct object_id* oid)\n+{\n+\tif (!repo_get_oid(the_repository, \"MERGE_HEAD\", oid)) {\n+\t\treturn \"MERGE_HEAD\";\n+\t} else if (!repo_get_oid(the_repository, \"REBASE_HEAD\", oid)) {\n+\t\treturn \"REBASE_HEAD\";\n+\t} else if (!repo_get_oid(the_repository, \"CHERRY_PICK_HEAD\", oid)) {\n+\t\treturn \"CHERRY_PICK_HEAD\";\n+\t} else if (!repo_get_oid(the_repository, \"REVERT_HEAD\", oid)) {\n+\t\treturn \"REVERT_HEAD\";\n+\t} else\n+\t\tdie(\"--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?\");\n+}\n+\n static void prepare_show_merge(struct rev_info *revs)\n {\n \tstruct commit_list *bases;\n \tstruct commit *head, *other;\n \tstruct object_id oid;\n \tconst char **prune = NULL;\n+\tconst char *action_head_name;\n \tint i, prune_num = 1; /* counting terminating NULL */\n \tstruct index_state *istate = revs->repo->index;\n \n \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n \t\tdie(\"--merge without HEAD?\");\n \thead = lookup_commit_or_die(&oid, \"HEAD\");\n-\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n-\t\tdie(\"--merge without MERGE_HEAD?\");\n-\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n+\taction_head_name = get_action_head_name(&oid);\n+\tother = lookup_commit_or_die(&oid, action_head_name);\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n-\tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n+\tadd_pending_object(revs, &other->object, action_head_name);\n \tbases = repo_get_merge_bases(the_repository, head, other);\n \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n-- \n2.39.3 (Apple Git-145)\n\n"},{"id":"486676","messageId":"xmqqy1cvcsp3.fsf@gitster.g","threadId":"60730","inReplyTo":"20240111233311.64734-1-mi.al.lohmann@gmail.com","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T00:15:52Z","receivedAt":"2024-01-12T00:16:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lohmann writes:\n\n> This extends the functionality of `git log --merge` to also work with\n> conflicts for rebase, cherry pick and revert.\n>\n> Signed-off-by: Michael Lohmann \n> ---\n> ... It is basically the counterpart to\n> `git show ${ACTION}_HEAD` for understanding the source of the conflict.\n\nI do not know about the validity of that approach to use *_HEAD, but\nwe may want to tighten the original's use of repo_get_oid() here ...\n\n> -\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n> -\t\tdie(\"--merge without MERGE_HEAD?\");\n> -\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n\n... so that we won't be confused in a repository that has a tag\nwhose name happens to be MERGE_HEAD. We probably should be using\nrefs.c:refs_resolve_ref_unsafe() instead to \n\n (1) ensure MERGE_HEAD is a ref, \n (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref(),\n and optionally\n (3) error out when MERGE_HEAD is a symref.\n\nAs your patch makes the problem even worse, if we were to do such a\ntightening (and I do not see a reason not to), it may want to be\ndone before, not after, this patch.\n\nI won't comment on the coding style violations in the patch below in\nthis message.\n\nThanks.\n\n> diff --git a/revision.c b/revision.c\n> index 2424c9bd67..2e5c00dabd 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,\n> \t}\n> }\n> \n> +static char* get_action_head_name(struct object_id* oid)\n> +{\n> +\tif (!repo_get_oid(the_repository, \"MERGE_HEAD\", oid)) {\n> +\t\treturn \"MERGE_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"REBASE_HEAD\", oid)) {\n> +\t\treturn \"REBASE_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"CHERRY_PICK_HEAD\", oid)) {\n> +\t\treturn \"CHERRY_PICK_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"REVERT_HEAD\", oid)) {\n> +\t\treturn \"REVERT_HEAD\";\n> +\t} else\n> +\t\tdie(\"--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?\");\n> +}\n> +\n> static void prepare_show_merge(struct rev_info *revs)\n> {\n> \tstruct commit_list *bases;\n> \tstruct commit *head, *other;\n> \tstruct object_id oid;\n> \tconst char **prune = NULL;\n> +\tconst char *action_head_name;\n> \tint i, prune_num = 1; /* counting terminating NULL */\n> \tstruct index_state *istate = revs->repo->index;\n> \n> \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n> \t\tdie(\"--merge without HEAD?\");\n> \thead = lookup_commit_or_die(&oid, \"HEAD\");\n> -\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n> -\t\tdie(\"--merge without MERGE_HEAD?\");\n> -\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n> +\taction_head_name = get_action_head_name(&oid);\n> +\tother = lookup_commit_or_die(&oid, action_head_name);\n> \tadd_pending_object(revs, &head->object, \"HEAD\");\n> -\tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n> +\tadd_pending_object(revs, &other->object, action_head_name);\n> \tbases = repo_get_merge_bases(the_repository, head, other);\n> \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n> \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n"},{"id":"486692","messageId":"ce46229d-8964-4445-9a17-cff40aca1cb4@kdbg.org","threadId":"60730","inReplyTo":"20240111233311.64734-1-mi.al.lohmann@gmail.com","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-01-12T07:35:50Z","receivedAt":"2024-01-12T07:35:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.01.24 um 00:33 schrieb Michael Lohmann:\n> This extends the functionality of `git log --merge` to also work with\n> conflicts for rebase, cherry pick and revert.\n> \n> Signed-off-by: Michael Lohmann \n> ---\n> Hi everybody!\n> \n> When Phillip Wood gave me some nice hints on his workflow concerning\n> conflicts [1] (we discussed if `--show-current-patch` would make sense\n> for cherry pick/revert). When I learned about `git log --merge` I was a\n> bit sad to discover that those do not exist for rebase/revert/cherry\n> pick since they look really valuable for getting an understanding on\n> what was changed. It is basically the counterpart to\n> `git show ${ACTION}_HEAD` for understanding the source of the conflict.\n> \n> I am curious if also other people would be interested in having an easy\n> way to get a log of only the relevant commits touching conflicting files\n> for said actions.\n> \n> With this patch I think the functionality is there, just hijacking the\n> `--merge` code - maybe an alias like `git log --conflict` or similar\n> might be more descriptive now?\n> \n> What do you think about this idea? (Or am I maybe missing an easy way to\n> achieve it with the existing code as well?)\n\nHa! Very nice patch. For comparison, have a look at my patch to achieve\nthe same that I never submitted (in particular, the author date):\nhttps://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0\n\nThis implementation is more complete than mine. I like it.\n\n> \n> Michael\n> \n> \n> [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/\n> \n> revision.c | 22 ++++++++++++++++++----\n> 1 file changed, 18 insertions(+), 4 deletions(-)\n> \n> diff --git a/revision.c b/revision.c\n> index 2424c9bd67..2e5c00dabd 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,\n> \t}\n> }\n> \n> +static char* get_action_head_name(struct object_id* oid)\n> +{\n> +\tif (!repo_get_oid(the_repository, \"MERGE_HEAD\", oid)) {\n> +\t\treturn \"MERGE_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"REBASE_HEAD\", oid)) {\n> +\t\treturn \"REBASE_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"CHERRY_PICK_HEAD\", oid)) {\n> +\t\treturn \"CHERRY_PICK_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"REVERT_HEAD\", oid)) {\n> +\t\treturn \"REVERT_HEAD\";\n> +\t} else\n> +\t\tdie(\"--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?\");\n> +}\n> +\n> static void prepare_show_merge(struct rev_info *revs)\n> {\n> \tstruct commit_list *bases;\n> \tstruct commit *head, *other;\n> \tstruct object_id oid;\n> \tconst char **prune = NULL;\n> +\tconst char *action_head_name;\n> \tint i, prune_num = 1; /* counting terminating NULL */\n> \tstruct index_state *istate = revs->repo->index;\n> \n> \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n> \t\tdie(\"--merge without HEAD?\");\n> \thead = lookup_commit_or_die(&oid, \"HEAD\");\n> -\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n> -\t\tdie(\"--merge without MERGE_HEAD?\");\n> -\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n> +\taction_head_name = get_action_head_name(&oid);\n> +\tother = lookup_commit_or_die(&oid, action_head_name);\n> \tadd_pending_object(revs, &head->object, \"HEAD\");\n> -\tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n> +\tadd_pending_object(revs, &other->object, action_head_name);\n> \tbases = repo_get_merge_bases(the_repository, head, other);\n> \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n> \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n\n"},{"id":"486697","messageId":"ff7a568d-dcf7-4230-ba08-5234b505ab8f@kdbg.org","threadId":"60730","inReplyTo":"ce46229d-8964-4445-9a17-cff40aca1cb4@kdbg.org","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-01-12T07:59:33Z","receivedAt":"2024-01-12T07:59:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12.01.24 um 08:35 schrieb Johannes Sixt:\n> ... (in particular, the author date):\n> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0\n\nThe Github UI is a bit unhelpful here. The author date is Nov 11, 2014.\n\n-- Hannes\n\n"},{"id":"486707","messageId":"47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com","threadId":"60730","inReplyTo":"20240111233311.64734-1-mi.al.lohmann@gmail.com","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-12T11:01:21Z","receivedAt":"2024-01-12T11:01:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michael\n\nOn 11/01/2024 23:33, Michael Lohmann wrote:\n> This extends the functionality of `git log --merge` to also work with\n> conflicts for rebase, cherry pick and revert.\n> \n> Signed-off-by: Michael Lohmann \n> ---\n> Hi everybody!\n> \n> When Phillip Wood gave me some nice hints on his workflow concerning\n> conflicts [1] (we discussed if `--show-current-patch` would make sense\n> for cherry pick/revert). When I learned about `git log --merge` I was a\n> bit sad to discover that those do not exist for rebase/revert/cherry\n> pick since they look really valuable for getting an understanding on\n> what was changed. It is basically the counterpart to\n> `git show ${ACTION}_HEAD` for understanding the source of the conflict.\n> \n> I am curious if also other people would be interested in having an easy\n> way to get a log of only the relevant commits touching conflicting files\n> for said actions.\n> \n> With this patch I think the functionality is there, just hijacking the\n> `--merge` code - maybe an alias like `git log --conflict` or similar\n> might be more descriptive now?\n> \n> What do you think about this idea? (Or am I maybe missing an easy way to\n> achieve it with the existing code as well?)\n\nI should start by saying that I didn't know \"git log --merge\" existed \nbefore I saw this message so please correct me if I've misunderstood \nwhat this patch is doing. If I understand correctly it shows the commits \nfrom each side of the merge and is equivalent to\n\n git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)\n\nWhen a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ \n[*] so I'm not sure what\n\n git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)\n\ntells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a common \nancestor. As I say I've not used \"git log --merge\" so maybe I'm missing \nthe reason why it would be useful when cherry-picking or rebasing.\n\nBest Wishes\n\nPhillip\n\n[*] assuming we're not picking a merge commit\n\n> Michael\n> \n> \n> [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/\n> \n> revision.c | 22 ++++++++++++++++++----\n> 1 file changed, 18 insertions(+), 4 deletions(-)\n> \n> diff --git a/revision.c b/revision.c\n> index 2424c9bd67..2e5c00dabd 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs,\n> \t}\n> }\n> \n> +static char* get_action_head_name(struct object_id* oid)\n> +{\n> +\tif (!repo_get_oid(the_repository, \"MERGE_HEAD\", oid)) {\n> +\t\treturn \"MERGE_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"REBASE_HEAD\", oid)) {\n> +\t\treturn \"REBASE_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"CHERRY_PICK_HEAD\", oid)) {\n> +\t\treturn \"CHERRY_PICK_HEAD\";\n> +\t} else if (!repo_get_oid(the_repository, \"REVERT_HEAD\", oid)) {\n> +\t\treturn \"REVERT_HEAD\";\n> +\t} else\n> +\t\tdie(\"--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?\");\n> +}\n> +\n> static void prepare_show_merge(struct rev_info *revs)\n> {\n> \tstruct commit_list *bases;\n> \tstruct commit *head, *other;\n> \tstruct object_id oid;\n> \tconst char **prune = NULL;\n> +\tconst char *action_head_name;\n> \tint i, prune_num = 1; /* counting terminating NULL */\n> \tstruct index_state *istate = revs->repo->index;\n> \n> \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n> \t\tdie(\"--merge without HEAD?\");\n> \thead = lookup_commit_or_die(&oid, \"HEAD\");\n> -\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n> -\t\tdie(\"--merge without MERGE_HEAD?\");\n> -\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n> +\taction_head_name = get_action_head_name(&oid);\n> +\tother = lookup_commit_or_die(&oid, action_head_name);\n> \tadd_pending_object(revs, &head->object, \"HEAD\");\n> -\tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n> +\tadd_pending_object(revs, &other->object, action_head_name);\n> \tbases = repo_get_merge_bases(the_repository, head, other);\n> \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n> \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n"},{"id":"486711","messageId":"20240112150346.73735-1-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":"47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-12T15:03:46Z","receivedAt":"2024-01-12T15:03:57Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Hi Phillip,\n\nOn 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote:\n> I should start by saying that I didn't know \"git log --merge\" existed before\n> I saw this message\nI also just found it and it looked very useful...\n\n> so please correct me if I've misunderstood what this patch is doing. If I\n> understand correctly it shows the commits from each side of the merge and is\n> equivalent to\n> \n> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)\n> \n> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*]\n> so I'm not sure what\n> \n> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)\n\nAlmost, but not quite: \"git log —merge\" only shows the commits touching the\nconflict, so it would be equivalent to (I think):\n\n git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)\n\n(or replace CHERRY_PICK with one of the other actions)\n\n> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.\n\nTrue - but same for MERGE_HEAD (\"git merge --allow-unrelated-histories\"). I\nhave to confess I did not check how it would behave under those circumstances.\nIt could either error, or (more helpful) show the log touching the file until\nthe root commit.\n\nBest wishes\nMichael\n"},{"id":"486713","messageId":"20240112155033.77204-1-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":"xmqqy1cvcsp3.fsf@gitster.g","subject":"[PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-12T15:50:32Z","receivedAt":"2024-01-12T15:50:58Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"This is done to\n(1) ensure MERGE_HEAD is a ref,\n(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()\n\nHelped-by: Junio C Hamano \nSigned-off-by: Michael Lohmann \n---\nHi!\n\nOn 12. Jan 2024, at 01:15, Junio C Hamano wrote:\n> > This extends the functionality of `git log --merge` to also work with\n> > conflicts for rebase, cherry pick and revert.\n> > \n> > Signed-off-by: Michael Lohmann \n> > ---\n> > ... It is basically the counterpart to\n> > `git show ${ACTION}_HEAD` for understanding the source of the conflict.\n> \n> I do not know about the validity of that approach to use *_HEAD,\n\nWhat I wanted to convey is that e.g. \"git show CHERRY_PICK_HEAD\" will\ntell you about the conflict from the perspective of the commit that is\ncurrently to be applied while \"git log --merge\" tells the story from the\nperspective of HEAD. So they are by no means the same, but can\ncomplement each other in getting an understanding about the conflict.\n\n> but we may want to tighten the original's use of repo_get\n> > -\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n> > -\t\tdie(\"--merge without MERGE_HEAD?\");\n> > -\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n> \n> ... so that we won't be confused in a repository that has a tag\n> whose name happens to be MERGE_HEAD. We probably should be using\n> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...\n\nHere I am really at the limit of my understanding of the core functions.\nIs this roughly what you had in mind? From my testing it seems to do the\njob, but I don't understand the details \"refs_resolve_ref_unsafe\"...\n\n revision.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 2424c9bd67..786e1a3e89 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs)\n \tstruct commit *head, *other;\n \tstruct object_id oid;\n \tconst char **prune = NULL;\n+\tconst char *other_head;\n \tint i, prune_num = 1; /* counting terminating NULL */\n \tstruct index_state *istate = revs->repo->index;\n \n \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n \t\tdie(\"--merge without HEAD?\");\n \thead = lookup_commit_or_die(&oid, \"HEAD\");\n-\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n+\tother_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n+\t\t\t\t\t \"MERGE_HEAD\",\n+\t\t\t\t\t RESOLVE_REF_READING,\n+\t\t\t\t\t &oid,\n+\t\t\t\t\t NULL);\n+\tif (!other_head)\n \t\tdie(\"--merge without MERGE_HEAD?\");\n-\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n+\tother = lookup_commit_or_die(&oid, other_head);\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n-\tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n+\tadd_pending_object(revs, &other->object, other_head);\n \tbases = repo_get_merge_bases(the_repository, head, other);\n \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n-- \n2.43.0.284.g6c31128a96\n\n"},{"id":"486714","messageId":"20240112155033.77204-2-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":"20240112155033.77204-1-mi.al.lohmann@gmail.com","subject":"[PATCH v2 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-12T15:50:33Z","receivedAt":"2024-01-12T15:50:58Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Co-authored-by: Johannes Sixt \nSigned-off-by: Michael Lohmann \n---\n\n> I won't comment on the coding style violations in the patch below in\n> this message.\n\nI hope this one is better. The other one was just a proof of the general\nconcept and meant as a starting point for a discussion if this is wanted\nat all. But I should still have taken more care.\n\nOn 12. Jan 2024, at 08:35, Johannes Sixt wrote:\n> Ha! Very nice patch. For comparison, have a look at my patch to achieve\n> the same that I never submitted (in particular, the author date):\n> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0\n> \n> This implementation is more complete than mine. I like it.\n\nHa! Nice one! I took a few of your changes as an inspiration and put you\nas a co-author.\n\nCheers\nMichael\n\nDifference compared to v1: Basically complete rewrite using\n\"refs_resolve_ref_unsafe\". Has to be applied after [PATCH v2 1/2] to\navoid conflict.\n\n revision.c | 27 ++++++++++++++++++++-------\n 1 file changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 786e1a3e89..b0b47dd241 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1961,6 +1961,25 @@ static void add_pending_commit_list(struct rev_info *revs,\n \t}\n }\n \n+static const char *lookup_other_head(struct object_id *oid)\n+{\n+\tstruct ref_store *refs = get_main_ref_store(the_repository);\n+\tconst char *name;\n+\tint i;\n+\tstatic const char *const other_head[] = {\n+\t\t\"MERGE_HEAD\", \"REBASE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\"\n+\t};\n+\n+\tfor (i = 0; i < ARRAY_SIZE(other_head); i++) {\n+\t\tname = refs_resolve_ref_unsafe(refs, other_head[i],\n+\t\t\t\t\t RESOLVE_REF_READING, oid, NULL);\n+\t\tif (name)\n+\t\t\treturn name;\n+\t}\n+\n+\tdie(\"--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?\");\n+}\n+\n static void prepare_show_merge(struct rev_info *revs)\n {\n \tstruct commit_list *bases;\n@@ -1974,13 +1993,7 @@ static void prepare_show_merge(struct rev_info *revs)\n \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n \t\tdie(\"--merge without HEAD?\");\n \thead = lookup_commit_or_die(&oid, \"HEAD\");\n-\tother_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n-\t\t\t\t\t \"MERGE_HEAD\",\n-\t\t\t\t\t RESOLVE_REF_READING,\n-\t\t\t\t\t &oid,\n-\t\t\t\t\t NULL);\n-\tif (!other_head)\n-\t\tdie(\"--merge without MERGE_HEAD?\");\n+\tother_head = lookup_other_head(&oid);\n \tother = lookup_commit_or_die(&oid, other_head);\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n \tadd_pending_object(revs, &other->object, other_head);\n-- \n2.43.0.284.g6c31128a96\n\n"},{"id":"486727","messageId":"xmqqzfxa9usx.fsf@gitster.g","threadId":"60730","inReplyTo":"20240112155033.77204-1-mi.al.lohmann@gmail.com","subject":"Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T20:10:54Z","receivedAt":"2024-01-12T20:11:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lohmann writes:\n\n>> but we may want to tighten the original's use of repo_get\n>> > -\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n>> > -\t\tdie(\"--merge without MERGE_HEAD?\");\n>> > -\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n>> \n>> ... so that we won't be confused in a repository that has a tag\n>> whose name happens to be MERGE_HEAD. We probably should be using\n>> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...\n>\n> Here I am really at the limit of my understanding of the core functions.\n> Is this roughly what you had in mind? From my testing it seems to do the\n> job, but I don't understand the details \"refs_resolve_ref_unsafe\"...\n\nPerhaps there are others who are more familiar with the ref API than\nI am, but I was just looking at refs.h today and wonder if something\nalong the lines of this ...\n\n if (read_ref_full(\"MERGE_HEAD\",\n \t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n\t\t &oid, NULL))\n\tdie(\"no MERGE_HEAD\");\n if (is_null_oid(&oid))\n\tdie(\"MERGE_HEAD is a symbolic ref???\");\n\n... would be simpler.\n\nWith the above, we are discarding the refname read_ref_full()\nobtains from its refs_resolve_ref_unsafe(), but I think that is\ntotally fine. We expect it to be \"MERGE_HEAD\" in the good case.\nThe returned value can be different from \"MERGE_HEAD\" if it is\na symbolic ref, but if the comment in refs.h on NO_RECURSE is to be\ntrusted, we should catch that case with the NULL-ness check on oid.\n\nWhich would mean that we do not need a separate \"other_head\"\nvariable, and instead can use \"MERGE_HEAD\".\n\n\n> revision.c | 12 +++++++++---\n> 1 file changed, 9 insertions(+), 3 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 2424c9bd67..786e1a3e89 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs)\n> \tstruct commit *head, *other;\n> \tstruct object_id oid;\n> \tconst char **prune = NULL;\n> +\tconst char *other_head;\n> \tint i, prune_num = 1; /* counting terminating NULL */\n> \tstruct index_state *istate = revs->repo->index;\n> \n> \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n> \t\tdie(\"--merge without HEAD?\");\n> \thead = lookup_commit_or_die(&oid, \"HEAD\");\n> -\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n> +\tother_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n> +\t\t\t\t\t \"MERGE_HEAD\",\n> +\t\t\t\t\t RESOLVE_REF_READING,\n> +\t\t\t\t\t &oid,\n> +\t\t\t\t\t NULL);\n> +\tif (!other_head)\n> \t\tdie(\"--merge without MERGE_HEAD?\");\n> -\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n> +\tother = lookup_commit_or_die(&oid, other_head);\n> \tadd_pending_object(revs, &head->object, \"HEAD\");\n> -\tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n> +\tadd_pending_object(revs, &other->object, other_head);\n> \tbases = repo_get_merge_bases(the_repository, head, other);\n> \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n> \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n"},{"id":"486728","messageId":"xmqqv87y9ufq.fsf@gitster.g","threadId":"60730","inReplyTo":"ce46229d-8964-4445-9a17-cff40aca1cb4@kdbg.org","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T20:18:49Z","receivedAt":"2024-01-12T20:18:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> What do you think about this idea? (Or am I maybe missing an easy way to\n>> achieve it with the existing code as well?)\n>\n> Ha! Very nice patch. For comparison, have a look at my patch to achieve\n> the same that I never submitted (in particular, the author date):\n> https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0\n>\n> This implementation is more complete than mine. I like it.\n\nI like the way your other_merge_candidate() loops over an array of\npossible candidates, which makes it a lot easier to extend, though,\nadmittedly the \"die()\" message needs auto-generating if we really\nwanted to make it maintenance free ;-)\n\nThanks.\n"},{"id":"486729","messageId":"xmqqr0im9ub9.fsf@gitster.g","threadId":"60730","inReplyTo":"47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T20:21:30Z","receivedAt":"2024-01-12T20:21:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"phillip.wood123@gmail.com writes:\n\n> I should start by saying that I didn't know \"git log --merge\" existed\n> before I saw this message so please correct me if I've misunderstood\n> what this patch is doing. If I understand correctly it shows the\n> commits from each side of the merge and is equivalent to\n>\n> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)\n>\n> When a commit is cherry-picked the merge base used is\n> CHERRY_PICK_HEAD^ [*] so I'm not sure what\n>\n> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)\n>\n> tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a\n> common ancestor. As I say I've not used \"git log --merge\" so maybe I'm\n> missing the reason why it would be useful when cherry-picking or\n> rebasing.\n\nGood question. It is the whole point of cherry-pick that\nCHERRY_PICK_HEAD and the current HEAD may not have any meaningful\nancestry relationship, so yes, it is questionable to use the same\nlogic for \"log --merge\" between HEAD...CHERRY_PICK_HEAD, while using\nHEAD...MERGE_HEAD does make perfect sense.\n"},{"id":"486731","messageId":"20240112210650.5455-1-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":"xmqqr0im9ub9.fsf@gitster.g","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-12T21:06:50Z","receivedAt":"2024-01-12T21:07:10Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"> > tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a\n> > common ancestor. As I say I've not used \"git log --merge\" so maybe I'm\n> > missing the reason why it would be useful when cherry-picking or\n> > rebasing.\n> \n> Good question. It is the whole point of cherry-pick that\n> CHERRY_PICK_HEAD and the current HEAD may not have any meaningful\n> ancestry relationship, so yes, it is questionable to use the same\n> logic for \"log --merge\" between HEAD...CHERRY_PICK_HEAD, while using\n> HEAD...MERGE_HEAD does make perfect sense.\n\nI tried to say it in [1]: a merge also does not need a common ancestor! Try it:\n\n git init test\n cd test\n echo something > README.md\n git add --all\n git commit -m \"initial commit\"\n git remote add origin_git git://git.kernel.org/pub/scm/git/git.git\n git fetch origin_git master # obviously no common ancestor\n git merge --allow-unrelated-histories origin_git/master\n # merge conflict\n git log --merge # Still loggs all the conflicting commits\n\nSo indeed the command that Phillip wrote is not equivalent and the existing\nlogic already can deal with unrelated histories. Or am I misunderstanding?\n\nMichael\n\n[1] https://lore.kernel.org/git/20240112150346.73735-1-mi.al.lohmann@gmail.com/\n"},{"id":"486732","messageId":"xmqqil3y9rvm.fsf@gitster.g","threadId":"60730","inReplyTo":"20240112150346.73735-1-mi.al.lohmann@gmail.com","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T21:14:05Z","receivedAt":"2024-01-12T21:14:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lohmann writes:\n\n> Almost, but not quite: \"git log —merge\" only shows the commits touching the\n> conflict, so it would be equivalent to (I think):\n>\n> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)\n>\n> (or replace CHERRY_PICK with one of the other actions)\n\nIt can certainly _reduce_ the noise, but I am not sure if it works\nover the right history segment. Let me think aloud a bit.\n\nLet's imagine that in a history forked long time ago,\n\n O----O----O----O----X HEAD\n \\\n Z---Z---Z---Z---A---B CHERRY_PICK_HEAD\n\nwhere all commits modified the same path in question that you saw\nconflicts in when your \"git cherry-pick B\" stopped.\n\nI do not know what to think about the changes to that path by the\ncommits denoted by 'O', but the changes done to the path by commits\ndenoted by 'Z' should have absolutely no relevance, as the whole\npoint of cherry-picking B relative to A is because we do not care\nabout what Zs did, no? For that matter, given that how we structure\nthe 3-way merge for such a cherry-pick to \"move from X the same way\nas you would move from A to B\", how you got to X (i.e. Os) should\nnot matter, either.\n\nOn the other hand, such a conflict may arise from the fact that Os\nand Zs made changes differently to cause the contents of the path at\nX and A differ significantly. So, OK, I can buy your argument that\nwhat Os and Zs to the conflicted path did can be interesting when\nunderstanding the conflict during such a cherry-pick.\n\n>> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.\n>\n> True - but same for MERGE_HEAD (\"git merge --allow-unrelated-histories\"). I\n\nBut that is very different, isn't it? Merging two unrelated\nhistories is like merging two histories where the common ancestor\nhad an empty tree, i.e.\n\n o---o---o---X HEAD\n /\n (v) an imaginary ancestor with an empty tree\n \\\n o---o---o---O MERGE_HEAD\n\nso it is a reasonable degenerated behaviour to consider what all\ncommits on both sides did to the paths in question.\n"},{"id":"486803","messageId":"648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com","threadId":"60730","inReplyTo":"20240112150346.73735-1-mi.al.lohmann@gmail.com","subject":"Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-01-15T10:48:38Z","receivedAt":"2024-01-15T10:48:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michael\n\nOn 12/01/2024 15:03, Michael Lohmann wrote:\n> Hi Phillip,\n> \n> On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote:\n>> I should start by saying that I didn't know \"git log --merge\" existed before\n>> I saw this message\n> I also just found it and it looked very useful...\n> \n>> so please correct me if I've misunderstood what this patch is doing. If I\n>> understand correctly it shows the commits from each side of the merge and is\n>> equivalent to\n>>\n>> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD)\n>>\n>> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*]\n>> so I'm not sure what\n>>\n>> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD)\n> \n> Almost, but not quite: \"git log —merge\" only shows the commits touching the\n> conflict, so it would be equivalent to (I think):\n> \n> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative)\n> \n> (or replace CHERRY_PICK with one of the other actions)\n\nThanks for clarifying that.\n\n>> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor.\n> \n> True - but same for MERGE_HEAD (\"git merge --allow-unrelated-histories\"). I\n> have to confess I did not check how it would behave under those circumstances.\n> It could either error, or (more helpful) show the log touching the file until\n> the root commit.\n\nWhat I was trying to get at was that with \"git merge\" \"git log --merge\" \nwill show commits that are part of the merge. With \"git cherry-pick\" \nthat's not the case because we're selecting the commits to show using \nthe merge base of HEAD and CHERRY_PICK_HEAD while cherry-pick uses \nCHERRY_PICK_HEAD^ as the base of the merge. I think Junio explains why \nit is still useful to show those commits in [1] i.e. they help the user \nunderstand the conflicts even though they are not part of the merge. It \nmight be worth expanding the commit message to explain that.\n\nBest Wishes\n\nPhillip\n\n[1] https://lore.kernel.org/git/xmqqil3y9rvm.fsf@gitster.g/\n"},{"id":"486805","messageId":"ZaUYyEAdr4oTImL-@tanuki","threadId":"60730","inReplyTo":"xmqqzfxa9usx.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-15T11:36:40Z","receivedAt":"2024-01-15T11:36:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 12, 2024 at 12:10:54PM -0800, Junio C Hamano wrote:\n> Michael Lohmann writes:\n> \n> >> but we may want to tighten the original's use of repo_get\n> >> > -\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n> >> > -\t\tdie(\"--merge without MERGE_HEAD?\");\n> >> > -\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n> >> \n> >> ... so that we won't be confused in a repository that has a tag\n> >> whose name happens to be MERGE_HEAD. We probably should be using\n> >> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ...\n> >\n> > Here I am really at the limit of my understanding of the core functions.\n> > Is this roughly what you had in mind? From my testing it seems to do the\n> > job, but I don't understand the details \"refs_resolve_ref_unsafe\"...\n> \n> Perhaps there are others who are more familiar with the ref API than\n> I am, but I was just looking at refs.h today and wonder if something\n> along the lines of this ...\n> \n> if (read_ref_full(\"MERGE_HEAD\",\n> \t\t RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n> \t\t &oid, NULL))\n> \tdie(\"no MERGE_HEAD\");\n> if (is_null_oid(&oid))\n> \tdie(\"MERGE_HEAD is a symbolic ref???\");\n> \n> ... would be simpler.\n> \n> With the above, we are discarding the refname read_ref_full()\n> obtains from its refs_resolve_ref_unsafe(), but I think that is\n> totally fine. We expect it to be \"MERGE_HEAD\" in the good case.\n> The returned value can be different from \"MERGE_HEAD\" if it is\n> a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be\n> trusted, we should catch that case with the NULL-ness check on oid.\n\nYeah, this should be fine.\n\nEven though I have stared at our refs API for extended periods of time\nduring the last months I still have to look up this part of the API\nalmost every time. I find it to be hard to use because you not only have\nto pay attention to the return value, but also to the flags in some edge\ncases. I wouldn't be surprised if there were many callsites that get\nthis wrong.\n\n> Which would mean that we do not need a separate \"other_head\"\n> variable, and instead can use \"MERGE_HEAD\".\n\nThere is no need for this, is there? We have already resolved the ref to\nan object ID, so why not use that via `add_pending_oid()`?\n\nPatrick\n"},{"id":"486813","messageId":"xmqq4jfe5xb1.fsf@gitster.g","threadId":"60730","inReplyTo":"ZaUYyEAdr4oTImL-@tanuki","subject":"Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-15T17:19:30Z","receivedAt":"2024-01-15T17:23:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> Which would mean that we do not need a separate \"other_head\"\n>> variable, and instead can use \"MERGE_HEAD\".\n>\n> There is no need for this, is there? We have already resolved the ref to\n> an object ID, so why not use that via `add_pending_oid()`?\n\nadd_pending_oid() and its callees use the name only for human\nconsumption (e.g., in error messages), as they all already have the\nobject name.\n"},{"id":"486911","messageId":"20240117081405.14012-1-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":"xmqqzfxa9usx.fsf@gitster.g","subject":"[PATCH v3 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-17T08:14:04Z","receivedAt":"2024-01-17T08:14:18Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"This is done to\n(1) ensure MERGE_HEAD is a ref,\n(2) obtain the oid without any prefixing by refs.c:repo_dwim_ref()\n(3) error out when MERGE_HEAD is a symref.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Michael Lohmann \n---\n\nChanges with respect to v2:\n- use read_ref_full instead of refs_resolve_ref_unsafe\n- don't use \"other_head\" variable to store MERGE_HEAD\n- check for symbolic ref\n\n revision.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex 2424c9bd67..aa4c4dc778 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1973,8 +1973,12 @@ static void prepare_show_merge(struct rev_info *revs)\n \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n \t\tdie(\"--merge without HEAD?\");\n \thead = lookup_commit_or_die(&oid, \"HEAD\");\n-\tif (repo_get_oid(the_repository, \"MERGE_HEAD\", &oid))\n+\tif (read_ref_full(\"MERGE_HEAD\",\n+\t\t\tRESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t&oid, NULL))\n \t\tdie(\"--merge without MERGE_HEAD?\");\n+\tif (is_null_oid(&oid))\n+\t\tdie(\"MERGE_HEAD is a symbolic ref???\");\n \tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n \tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n-- \n2.39.3 (Apple Git-145)\n\n"},{"id":"486912","messageId":"20240117081405.14012-2-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":"20240117081405.14012-1-mi.al.lohmann@gmail.com","subject":"[PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-17T08:14:05Z","receivedAt":"2024-01-17T08:14:18Z","isPatch":true,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Co-authored-by: Johannes Sixt \nSigned-off-by: Michael Lohmann \n---\n\nOn 12. Jan 2024, at 21:18, Junio C Hamano wrote:\n> I like the way your other_merge_candidate() loops over an array of\n> possible candidates, which makes it a lot easier to extend, though,\n> admittedly the \"die()\" message needs auto-generating if we really\n> wanted to make it maintenance free ;-)\n\nI would certainly like that but I did not find an easy way of achieving\nthis in C. Help wanted.\n\nChanges with respect to v2:\n- use read_ref_full instead of refs_resolve_ref_unsafe\n- check for symbolic ref\n- extract \"other_name\" in this patch, instead of patch 1\n\n revision.c | 31 +++++++++++++++++++++++--------\n 1 file changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex aa4c4dc778..c778413c7d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs,\n \t}\n }\n \n+static const char *lookup_other_head(struct object_id *oid)\n+{\n+\tint i;\n+\tstatic const char *const other_head[] = {\n+\t\t\"MERGE_HEAD\", \"REBASE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\"\n+\t};\n+\n+\tfor (i = 0; i < ARRAY_SIZE(other_head); i++)\n+\t\tif (!read_ref_full(other_head[i],\n+\t\t\t\tRESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t\toid, NULL)) {\n+\t\t\tif (is_null_oid(oid))\n+\t\t\t\tdie(\"%s is a symbolic ref???\", other_head[i]);\n+\t\t\treturn other_head[i];\n+\t\t}\n+\n+\tdie(\"--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?\");\n+}\n+\n static void prepare_show_merge(struct rev_info *revs)\n {\n \tstruct commit_list *bases;\n \tstruct commit *head, *other;\n \tstruct object_id oid;\n+\tconst char *other_name;\n \tconst char **prune = NULL;\n \tint i, prune_num = 1; /* counting terminating NULL */\n \tstruct index_state *istate = revs->repo->index;\n@@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs)\n \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n \t\tdie(\"--merge without HEAD?\");\n \thead = lookup_commit_or_die(&oid, \"HEAD\");\n-\tif (read_ref_full(\"MERGE_HEAD\",\n-\t\t\tRESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n-\t\t\t&oid, NULL))\n-\t\tdie(\"--merge without MERGE_HEAD?\");\n-\tif (is_null_oid(&oid))\n-\t\tdie(\"MERGE_HEAD is a symbolic ref???\");\n-\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n+\tother_name = lookup_other_head(&oid);\n+\tother = lookup_commit_or_die(&oid, other_name);\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n-\tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n+\tadd_pending_object(revs, &other->object, other_name);\n \tbases = repo_get_merge_bases(the_repository, head, other);\n \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n-- \n2.39.3 (Apple Git-145)\n\n"},{"id":"486914","messageId":"20240117091905.14354-1-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":"20240117081405.14012-2-mi.al.lohmann@gmail.com","subject":"Full disclosure","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-17T09:19:05Z","receivedAt":"2024-01-17T09:19:11Z","isPatch":false,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Just as a disclosure: I was told that my contributions are not welcome [1]\n(even though I have to say that I don't fully agree with the reasoning), but I\ndid not want to leave these patches alone.\n\n@Junio C Hamano: Please take this into account when deciding if you want to\naccept the patches. This is just for transparancy and I will not do any more\ncontributions than potentially finishing this one. If you do not want these\npatches from me, but it was still deemed to be an interesting feature: could\nsomeone else take over?\n\nMichael\n\n[1]: https://lore.kernel.org/git/xmqqil3ybets.fsf@gitster.g/\n"},{"id":"486915","messageId":"CAP8UFD3g_N+JJ_cdas2ZdsBPxuQFBXW8yY+1nC6AKvQppOYaDw@mail.gmail.com","threadId":"60730","inReplyTo":"20240117091905.14354-1-mi.al.lohmann@gmail.com","subject":"Re: Full disclosure","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-01-17T09:58:31Z","receivedAt":"2024-01-17T09:58:44Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jan 17, 2024 at 10:20 AM Michael Lohmann\n wrote:\n>\n> Just as a disclosure: I was told that my contributions are not welcome [1]\n> (even though I have to say that I don't fully agree with the reasoning), but I\n> did not want to leave these patches alone.\n\nI might have missed something, but as far as I see in the email you\nmention, you weren't told that your contributions are not welcome.\nJunio reviewed your patch and agreed with some of Peff's comments\nabout your patch. It just means that they both think your patch could\nbe further improved. They didn't reject your patch, nor your\ncontributions in general.\n\n> @Junio C Hamano: Please take this into account when deciding if you want to\n> accept the patches. This is just for transparancy and I will not do any more\n> contributions than potentially finishing this one. If you do not want these\n> patches from me, but it was still deemed to be an interesting feature: could\n> someone else take over?\n>\n> Michael\n>\n> [1]: https://lore.kernel.org/git/xmqqil3ybets.fsf@gitster.g/\n>\n"},{"id":"486920","messageId":"20240117174125.22915-1-mi.al.lohmann@gmail.com","threadId":"60730","inReplyTo":"CAP8UFD3g_N+JJ_cdas2ZdsBPxuQFBXW8yY+1nC6AKvQppOYaDw@mail.gmail.com","subject":"Full disclosure","fromName":"Michael Lohmann","fromEmail":"mi.al.lohmann@gmail.com","sentAt":"2024-01-17T17:41:24Z","receivedAt":"2024-01-17T17:41:45Z","isPatch":false,"sender":{"key":"mi.al.lohmann@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6929993?v=4"},"body":"Hi Christian!\n\nYes: I agree that the current state of the last submitted patch in that\ndiscussion is far from optimal.\nAfter Jeff Kings explanation I had a much better understanding for the\nsituation and the reasoning (and his suggestion was definitely better than\nmine).\n\nI had already prepared a new version which tackled (I think) pretty much all of\nthe criticisms. But then the above mentioned message came in and when I read\nthis:\n\n> [...] they are trying to be different for the sake of being different, which\n> is not a good sign. I'd want our contributors to be original where being\n> original matters more.\n\nI am reading:\n\n1) I am \"trying to be different for the sake of being different\"\n2) Contributors like this are not wanted\n\nSo yes, I do understand this as a general statement on me as a contributor\nwithout any proposal for me at least to explain the situation from my side.\n\nI teach my colleges not to name variables with how they are initialized, but\nwith what information they actually convey and I found the \"_NONE\" one at least\nmisleading in its name.\n\nSo in the initial discussion I was a bit stubborn, because Philip wrote \"I\ndon't have a strong opinion\" and from my perspective the only argument was\n\"over there we also do it that way\" (which _can_ 100% be a valid reason), but\nJunio C Hamano did not even acknowledge my criticisms of the other the variable\nname. I am totally fine with a decision like this if done consciously, but if I\ndon't even get an acknowledgement, let alone an explanation, my demands I place\non my code quality are that I write the best code I can. And with all the info\nI had (prior to Jeffs message) I did still favour my first suggestion.\n\nIn my eyes it would be helpful to at least tell me what your (in my eyes not\nobvious) preferences are on naming things, because otherwise I will rather\nstick to my standards than blindly follow a single instance of other code where\nI don't even know if that was a concious decision or it just happened by\naccident.\n\nSo no, I don't agree with the assessment of point 1), but I still read the\nmessage like that. I will accept it and instead use my skills in different\nprojects where they are indeed valued.\n\nCheers\nMichael\n"},{"id":"486921","messageId":"xmqq7ck7x10y.fsf@gitster.g","threadId":"60730","inReplyTo":"20240117091905.14354-1-mi.al.lohmann@gmail.com","subject":"Re: Full disclosure","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-17T18:33:49Z","receivedAt":"2024-01-17T18:33:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Lohmann writes:\n\n> Just as a disclosure: I was told that my contributions are not welcome [1]\n> (even though I have to say that I don't fully agree with the reasoning), but I\n> did not want to leave these patches alone.\n\nRereading the message I agree that the message came out in a wrong\nway that I did not intend. It is sure that I do not want to see\ncertain attitude and behaviour in the patches in our contributors,\nbut that is totally different from \"contribution from a specific\ncontributor is not welcome\". The phrasing was making of my\nincompetence in communication skills, not from malice.\n\nSo I am sorry that I wrote something that it is fair to read as\nsaying \"you are unwelcome\". I didn't mean it that way.\n\n"},{"id":"487161","messageId":"de816c41-6889-4178-add1-7c2bf636640d@mrbrklyn.com","threadId":"60730","inReplyTo":"20240117174125.22915-1-mi.al.lohmann@gmail.com","subject":"Re: Full disclosure","fromName":"Ruben Safir","fromEmail":"ruben@mrbrklyn.com","sentAt":"2024-01-21T00:41:01Z","receivedAt":"2024-01-21T00:47:41Z","isPatch":false,"sender":{"key":"ruben@mrbrklyn.com","avatar":null},"body":"\nhttps://www.zazzle.com/idit_aharons_tzfat_original_backpack_design-256246398431855710\n\nOn 1/17/24 12:41, Michael Lohmann wrote:\n> Hi Christian!\n> \n> Yes: I agree that the current state of the last submitted patch in that\n> discussion is far from optimal.\n> After Jeff Kings explanation I had a much better understanding for the\n> situation and the reasoning (and his suggestion was definitely better than\n> mine).\n> \n> I had already prepared a new version which tackled (I think) pretty much all of\n> the criticisms. But then the above mentioned message came in and when I read\n> this:\n> \n>> [...] they are trying to be different for the sake of being different, which\n>> is not a good sign. I'd want our contributors to be original where being\n>> original matters more.\n> \n> I am reading:\n> \n> 1) I am \"trying to be different for the sake of being different\"\n> 2) Contributors like this are not wanted\n> \n> So yes, I do understand this as a general statement on me as a contributor\n> without any proposal for me at least to explain the situation from my side.\n> \n> I teach my colleges not to name variables with how they are initialized, but\n> with what information they actually convey and I found the \"_NONE\" one at least\n> misleading in its name.\n> \n> So in the initial discussion I was a bit stubborn, because Philip wrote \"I\n> don't have a strong opinion\" and from my perspective the only argument was\n> \"over there we also do it that way\" (which _can_ 100% be a valid reason), but\n> Junio C Hamano did not even acknowledge my criticisms of the other the variable\n> name. I am totally fine with a decision like this if done consciously, but if I\n> don't even get an acknowledgement, let alone an explanation, my demands I place\n> on my code quality are that I write the best code I can. And with all the info\n> I had (prior to Jeffs message) I did still favour my first suggestion.\n> \n> In my eyes it would be helpful to at least tell me what your (in my eyes not\n> obvious) preferences are on naming things, because otherwise I will rather\n> stick to my standards than blindly follow a single instance of other code where\n> I don't even know if that was a concious decision or it just happened by\n> accident.\n> \n> So no, I don't agree with the assessment of point 1), but I still read the\n> message like that. I will accept it and instead use my skills in different\n> projects where they are indeed valued.\n> \n> Cheers\n> Michael\n\n-- \nSo many immigrant groups have swept through our town\nthat Brooklyn, like Atlantis, reaches mythological\nproportions in the mind of the world - RI Safir 1998\nhttp://www.mrbrklyn.com\nDRM is THEFT - We are the STAKEHOLDERS - RI Safir 2002\n\nhttp://www.nylxs.com - Leadership Development in Free Software\nhttp://www.brooklyn-living.com\n\nBeing so tracked is for FARM ANIMALS and extermination camps,\nbut incompatible with living as a free human being. -RI Safir 2013\n\n"},{"id":"487307","messageId":"CABPp-BE4zqRX=wd5EBj96hzCS8V73QpdN-2pCpv7qMdkpkX93w@mail.gmail.com","threadId":"60730","inReplyTo":"20240117081405.14012-2-mi.al.lohmann@gmail.com","subject":"Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-01-24T07:06:08Z","receivedAt":"2024-01-24T07:06:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 17, 2024 at 12:14 AM Michael Lohmann\n wrote:\n>\n> Co-authored-by: Johannes Sixt \n> Signed-off-by: Michael Lohmann \n> ---\n>\n> On 12. Jan 2024, at 21:18, Junio C Hamano wrote:\n> > I like the way your other_merge_candidate() loops over an array of\n> > possible candidates, which makes it a lot easier to extend, though,\n> > admittedly the \"die()\" message needs auto-generating if we really\n> > wanted to make it maintenance free ;-)\n>\n> I would certainly like that but I did not find an easy way of achieving\n> this in C. Help wanted.\n>\n> Changes with respect to v2:\n> - use read_ref_full instead of refs_resolve_ref_unsafe\n> - check for symbolic ref\n> - extract \"other_name\" in this patch, instead of patch 1\n>\n> revision.c | 31 +++++++++++++++++++++++--------\n> 1 file changed, 23 insertions(+), 8 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index aa4c4dc778..c778413c7d 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs,\n> }\n> }\n>\n> +static const char *lookup_other_head(struct object_id *oid)\n> +{\n> + int i;\n> + static const char *const other_head[] = {\n> + \"MERGE_HEAD\", \"REBASE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\"\n> + };\n> +\n> + for (i = 0; i < ARRAY_SIZE(other_head); i++)\n> + if (!read_ref_full(other_head[i],\n> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n> + oid, NULL)) {\n> + if (is_null_oid(oid))\n> + die(\"%s is a symbolic ref???\", other_head[i]);\n> + return other_head[i];\n> + }\n> +\n> + die(\"--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?\");\n> +}\n> +\n> static void prepare_show_merge(struct rev_info *revs)\n> {\n> struct commit_list *bases;\n> struct commit *head, *other;\n> struct object_id oid;\n> + const char *other_name;\n> const char **prune = NULL;\n> int i, prune_num = 1; /* counting terminating NULL */\n> struct index_state *istate = revs->repo->index;\n> @@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs)\n> if (repo_get_oid(the_repository, \"HEAD\", &oid))\n> die(\"--merge without HEAD?\");\n> head = lookup_commit_or_die(&oid, \"HEAD\");\n> - if (read_ref_full(\"MERGE_HEAD\",\n> - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n> - &oid, NULL))\n> - die(\"--merge without MERGE_HEAD?\");\n> - if (is_null_oid(&oid))\n> - die(\"MERGE_HEAD is a symbolic ref???\");\n> - other = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n> + other_name = lookup_other_head(&oid);\n> + other = lookup_commit_or_die(&oid, other_name);\n> add_pending_object(revs, &head->object, \"HEAD\");\n> - add_pending_object(revs, &other->object, \"MERGE_HEAD\");\n> + add_pending_object(revs, &other->object, other_name);\n> bases = repo_get_merge_bases(the_repository, head, other);\n> add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n> add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n> --\n> 2.39.3 (Apple Git-145)\n\nI had to go look up previous versions to see the discussion of why\nthis was useful for things other than merge. I agree with Phillip\nfrom https://lore.kernel.org/git/648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com/,\nthat the commit message _needs_ to explain this, likely using some of\nJunio's explanation.\n\nAlso, what about cases where users do a cherry-pick in the middle of a\nrebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What\nthen?\n"},{"id":"487340","messageId":"xmqqr0i6fxe2.fsf@gitster.g","threadId":"60730","inReplyTo":"CABPp-BE4zqRX=wd5EBj96hzCS8V73QpdN-2pCpv7qMdkpkX93w@mail.gmail.com","subject":"Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T17:34:45Z","receivedAt":"2024-01-24T17:34:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I had to go look up previous versions to see the discussion of why\n> this was useful for things other than merge. I agree with Phillip\n> from https://lore.kernel.org/git/648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com/,\n> that the commit message _needs_ to explain this, likely using some of\n> Junio's explanation.\n\nPlease note that I am not very happy with that \"explanation\" myself.\nThe only thing I can still agree to in that message myself is that\nit is sensible for \"log --merge\" to go down all the way to the root\nof the histories leading to MERGE_HEAD and HEAD in the \"merging two\nunrelated histories\" scenario. Treating CHERRY_PICK_HEAD and others\nthe same way, to me, almost sounds as if we are saying \"all the\ncommits behind the commits involved in the conflicted operation are\nworth looking at\", which is not a very useful or productive thing.\n\n> Also, what about cases where users do a cherry-pick in the middle of a\n> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What\n> then?\n\n;-)\n"},{"id":"487342","messageId":"dfb582cf-b1e4-414d-bfe1-0f93d910ec54@kdbg.org","threadId":"60730","inReplyTo":"CABPp-BE4zqRX=wd5EBj96hzCS8V73QpdN-2pCpv7qMdkpkX93w@mail.gmail.com","subject":"Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-01-24T17:19:39Z","receivedAt":"2024-01-24T18:03:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.01.24 um 08:06 schrieb Elijah Newren:\n> On Wed, Jan 17, 2024 at 12:14 AM Michael Lohmann\n>> +static const char *lookup_other_head(struct object_id *oid)\n>> +{\n>> + int i;\n>> + static const char *const other_head[] = {\n>> + \"MERGE_HEAD\", \"REBASE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\"\n>> + };\n>> +\n>> + for (i = 0; i < ARRAY_SIZE(other_head); i++)\n>> + if (!read_ref_full(other_head[i],\n>> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n>> + oid, NULL)) {\n>> + if (is_null_oid(oid))\n>> + die(\"%s is a symbolic ref???\", other_head[i]);\n>> + return other_head[i];\n>> + }\n>> +\n>> + die(\"--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?\");\n>> +}\n...\n> Also, what about cases where users do a cherry-pick in the middle of a\n> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What\n> then?\n\nGood point! IMO, REBASE_HEAD should have lower precedence than all the\nother *_HEADs. It would mean to reorder the entries:\n\n\tstatic const char *const other_head[] = {\n\t\t\"MERGE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\", \"REBASE_HEAD\"\n\t};\n\n(and perhaps adjust the error message, too).\n\n-- Hannes\n\n"},{"id":"487346","messageId":"xmqq1qa6ecqr.fsf@gitster.g","threadId":"60730","inReplyTo":"dfb582cf-b1e4-414d-bfe1-0f93d910ec54@kdbg.org","subject":"Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T19:46:04Z","receivedAt":"2024-01-24T19:46:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Good point! IMO, REBASE_HEAD should have lower precedence than all the\n> other *_HEADs. It would mean to reorder the entries:\n>\n> \tstatic const char *const other_head[] = {\n> \t\t\"MERGE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\", \"REBASE_HEAD\"\n> \t};\n>\n> (and perhaps adjust the error message, too).\n\nAnd probably give a warning saying that we noticed you are rebasing\nand cherry-picking and we chose to show the --merge based on the\nrelationship between cherry-pick-head and head, ignoring your rebase\nstatus, or something.\n\n\n"},{"id":"487356","messageId":"0a061bee-9b78-419f-9f1a-ced69ca96002@kdbg.org","threadId":"60730","inReplyTo":"xmqq1qa6ecqr.fsf@gitster.g","subject":"Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2024-01-24T22:06:49Z","receivedAt":"2024-01-24T22:07:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.01.24 um 20:46 schrieb Junio C Hamano:\n> Johannes Sixt writes:\n> \n>> Good point! IMO, REBASE_HEAD should have lower precedence than all the\n>> other *_HEADs. It would mean to reorder the entries:\n>>\n>> \tstatic const char *const other_head[] = {\n>> \t\t\"MERGE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\", \"REBASE_HEAD\"\n>> \t};\n>>\n>> (and perhaps adjust the error message, too).\n> \n> And probably give a warning saying that we noticed you are rebasing\n> and cherry-picking and we chose to show the --merge based on the\n> relationship between cherry-pick-head and head, ignoring your rebase\n> status, or something.\n\nI don't think that's necessary. When rebase stopped with a merge\nconflict, neither of the other commands can begin their work until the\nconflicted state is removed. That should be a concious act, just like\nwhen thereafter one of these other commands is used and leads to a\nconflict. At least I would certainly not need a reminder.\n\n-- Hannes\n\n"},{"id":"487357","messageId":"xmqqbk9abcru.fsf@gitster.g","threadId":"60730","inReplyTo":"0a061bee-9b78-419f-9f1a-ced69ca96002@kdbg.org","subject":"Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-24T22:13:41Z","receivedAt":"2024-01-24T22:13:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 24.01.24 um 20:46 schrieb Junio C Hamano:\n>> Johannes Sixt writes:\n>> \n>>> Good point! IMO, REBASE_HEAD should have lower precedence than all the\n>>> other *_HEADs. It would mean to reorder the entries:\n>>>\n>>> \tstatic const char *const other_head[] = {\n>>> \t\t\"MERGE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\", \"REBASE_HEAD\"\n>>> \t};\n>>>\n>>> (and perhaps adjust the error message, too).\n>> \n>> And probably give a warning saying that we noticed you are rebasing\n>> and cherry-picking and we chose to show the --merge based on the\n>> relationship between cherry-pick-head and head, ignoring your rebase\n>> status, or something.\n>\n> I don't think that's necessary. When rebase stopped with a merge\n> conflict, neither of the other commands can begin their work until the\n> conflicted state is removed. That should be a concious act, just like\n> when thereafter one of these other commands is used and leads to a\n> conflict. At least I would certainly not need a reminder.\n\nOK.\n"},{"id":"488328","messageId":"xmqq7cjdchx8.fsf@gitster.g","threadId":"60730","inReplyTo":"dfb582cf-b1e4-414d-bfe1-0f93d910ec54@kdbg.org","subject":"Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-09T23:54:43Z","receivedAt":"2024-02-09T23:54:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n>> Also, what about cases where users do a cherry-pick in the middle of a\n>> rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What\n>> then?\n>\n> Good point! IMO, REBASE_HEAD should have lower precedence than all the\n> other *_HEADs. It would mean to reorder the entries:\n>\n> \tstatic const char *const other_head[] = {\n> \t\t\"MERGE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\", \"REBASE_HEAD\"\n> \t};\n>\n> (and perhaps adjust the error message, too).\n\nI've tweaked this change into the commit.\n\nThanks, all.\n\n------- >8 ------------- >8 ------------- >8 ------------- >8 -------\nFrom: Michael Lohmann \nDate: Wed, 17 Jan 2024 09:14:05 +0100\nSubject: [PATCH] revision: implement `git log --merge` also for\n rebase/cherry_pick/revert\n\nCo-authored-by: Johannes Sixt \nSigned-off-by: Michael Lohmann \n[jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last]\nSigned-off-by: Junio C Hamano \n---\n revision.c | 31 +++++++++++++++++++++++--------\n 1 file changed, 23 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex aa4c4dc778..36dc2f94f7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs,\n \t}\n }\n \n+static const char *lookup_other_head(struct object_id *oid)\n+{\n+\tint i;\n+\tstatic const char *const other_head[] = {\n+\t\t\"MERGE_HEAD\", \"CHERRY_PICK_HEAD\", \"REVERT_HEAD\", \"REBASE_HEAD\"\n+\t};\n+\n+\tfor (i = 0; i < ARRAY_SIZE(other_head); i++)\n+\t\tif (!read_ref_full(other_head[i],\n+\t\t\t\tRESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n+\t\t\t\toid, NULL)) {\n+\t\t\tif (is_null_oid(oid))\n+\t\t\t\tdie(\"%s is a symbolic ref???\", other_head[i]);\n+\t\t\treturn other_head[i];\n+\t\t}\n+\n+\tdie(\"--merge without MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD?\");\n+}\n+\n static void prepare_show_merge(struct rev_info *revs)\n {\n \tstruct commit_list *bases;\n \tstruct commit *head, *other;\n \tstruct object_id oid;\n+\tconst char *other_name;\n \tconst char **prune = NULL;\n \tint i, prune_num = 1; /* counting terminating NULL */\n \tstruct index_state *istate = revs->repo->index;\n@@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs)\n \tif (repo_get_oid(the_repository, \"HEAD\", &oid))\n \t\tdie(\"--merge without HEAD?\");\n \thead = lookup_commit_or_die(&oid, \"HEAD\");\n-\tif (read_ref_full(\"MERGE_HEAD\",\n-\t\t\tRESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n-\t\t\t&oid, NULL))\n-\t\tdie(\"--merge without MERGE_HEAD?\");\n-\tif (is_null_oid(&oid))\n-\t\tdie(\"MERGE_HEAD is a symbolic ref???\");\n-\tother = lookup_commit_or_die(&oid, \"MERGE_HEAD\");\n+\tother_name = lookup_other_head(&oid);\n+\tother = lookup_commit_or_die(&oid, other_name);\n \tadd_pending_object(revs, &head->object, \"HEAD\");\n-\tadd_pending_object(revs, &other->object, \"MERGE_HEAD\");\n+\tadd_pending_object(revs, &other->object, other_name);\n \tbases = repo_get_merge_bases(the_repository, head, other);\n \tadd_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM);\n \tadd_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);\n-- \n2.44.0-rc0\n\n\n\n"}]}