# [RFC PATCH] `log --merge` also for rebase/cherry pick/revert 30 messages from 2024-01-11 to 2024-02-09. Participants: Michael Lohmann, Junio C Hamano, Johannes Sixt, phillip.wood123@gmail.com, Phillip Wood, Patrick Steinhardt, Christian Couder, Ruben Safir, Elijah Newren. Thread: https://gitlist.dev/t/60730 ## Michael Lohmann, 2024-01-11 23:33 Subject: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: <20240111233311.64734-1-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240111233311.64734-1-mi.al.lohmann%40gmail.com ``` This extends the functionality of `git log --merge` to also work with conflicts for rebase, cherry pick and revert. Signed-off-by: Michael Lohmann --- Hi everybody! When Phillip Wood gave me some nice hints on his workflow concerning conflicts [1] (we discussed if `--show-current-patch` would make sense for cherry pick/revert). When I learned about `git log --merge` I was a bit sad to discover that those do not exist for rebase/revert/cherry pick since they look really valuable for getting an understanding on what was changed. It is basically the counterpart to `git show ${ACTION}_HEAD` for understanding the source of the conflict. I am curious if also other people would be interested in having an easy way to get a log of only the relevant commits touching conflicting files for said actions. With this patch I think the functionality is there, just hijacking the `--merge` code - maybe an alias like `git log --conflict` or similar might be more descriptive now? What do you think about this idea? (Or am I maybe missing an easy way to achieve it with the existing code as well?) Michael [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/ revision.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/revision.c b/revision.c index 2424c9bd67..2e5c00dabd 100644 --- a/revision.c +++ b/revision.c @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs, } } +static char* get_action_head_name(struct object_id* oid) +{ + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) { + return "MERGE_HEAD"; + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) { + return "REBASE_HEAD"; + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) { + return "CHERRY_PICK_HEAD"; + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) { + return "REVERT_HEAD"; + } else + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); +} + static void prepare_show_merge(struct rev_info *revs) { struct commit_list *bases; struct commit *head, *other; struct object_id oid; const char **prune = NULL; + const char *action_head_name; int i, prune_num = 1; /* counting terminating NULL */ struct index_state *istate = revs->repo->index; if (repo_get_oid(the_repository, "HEAD", &oid)) die("--merge without HEAD?"); head = lookup_commit_or_die(&oid, "HEAD"); - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) - die("--merge without MERGE_HEAD?"); - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); + action_head_name = get_action_head_name(&oid); + other = lookup_commit_or_die(&oid, action_head_name); add_pending_object(revs, &head->object, "HEAD"); - add_pending_object(revs, &other->object, "MERGE_HEAD"); + add_pending_object(revs, &other->object, action_head_name); bases = repo_get_merge_bases(the_repository, head, other); add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM); -- 2.39.3 (Apple Git-145) ``` ## Junio C Hamano, 2024-01-12 00:15 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: URL: https://gitlist.dev/e/xmqqy1cvcsp3.fsf%40gitster.g In-Reply-To: <20240111233311.64734-1-mi.al.lohmann@gmail.com> ``` Michael Lohmann writes: > This extends the functionality of `git log --merge` to also work with > conflicts for rebase, cherry pick and revert. > > Signed-off-by: Michael Lohmann > --- > ... It is basically the counterpart to > `git show ${ACTION}_HEAD` for understanding the source of the conflict. I do not know about the validity of that approach to use *_HEAD, but we may want to tighten the original's use of repo_get_oid() here ... > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > - die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); ... so that we won't be confused in a repository that has a tag whose name happens to be MERGE_HEAD. We probably should be using refs.c:refs_resolve_ref_unsafe() instead to (1) ensure MERGE_HEAD is a ref, (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref(), and optionally (3) error out when MERGE_HEAD is a symref. As your patch makes the problem even worse, if we were to do such a tightening (and I do not see a reason not to), it may want to be done before, not after, this patch. I won't comment on the coding style violations in the patch below in this message. Thanks. > diff --git a/revision.c b/revision.c > index 2424c9bd67..2e5c00dabd 100644 > --- a/revision.c > +++ b/revision.c > @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs, > } > } > > +static char* get_action_head_name(struct object_id* oid) > +{ > + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) { > + return "MERGE_HEAD"; > + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) { > + return "REBASE_HEAD"; > + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) { > + return "CHERRY_PICK_HEAD"; > + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) { > + return "REVERT_HEAD"; > + } else > + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); > +} > + > static void prepare_show_merge(struct rev_info *revs) > { > struct commit_list *bases; > struct commit *head, *other; > struct object_id oid; > const char **prune = NULL; > + const char *action_head_name; > int i, prune_num = 1; /* counting terminating NULL */ > struct index_state *istate = revs->repo->index; > > if (repo_get_oid(the_repository, "HEAD", &oid)) > die("--merge without HEAD?"); > head = lookup_commit_or_die(&oid, "HEAD"); > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > - die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > + action_head_name = get_action_head_name(&oid); > + other = lookup_commit_or_die(&oid, action_head_name); > add_pending_object(revs, &head->object, "HEAD"); > - add_pending_object(revs, &other->object, "MERGE_HEAD"); > + add_pending_object(revs, &other->object, action_head_name); > bases = repo_get_merge_bases(the_repository, head, other); > add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); > add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM); ``` ## Johannes Sixt, 2024-01-12 07:35 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: URL: https://gitlist.dev/e/ce46229d-8964-4445-9a17-cff40aca1cb4%40kdbg.org In-Reply-To: <20240111233311.64734-1-mi.al.lohmann@gmail.com> ``` Am 12.01.24 um 00:33 schrieb Michael Lohmann: > This extends the functionality of `git log --merge` to also work with > conflicts for rebase, cherry pick and revert. > > Signed-off-by: Michael Lohmann > --- > Hi everybody! > > When Phillip Wood gave me some nice hints on his workflow concerning > conflicts [1] (we discussed if `--show-current-patch` would make sense > for cherry pick/revert). When I learned about `git log --merge` I was a > bit sad to discover that those do not exist for rebase/revert/cherry > pick since they look really valuable for getting an understanding on > what was changed. It is basically the counterpart to > `git show ${ACTION}_HEAD` for understanding the source of the conflict. > > I am curious if also other people would be interested in having an easy > way to get a log of only the relevant commits touching conflicting files > for said actions. > > With this patch I think the functionality is there, just hijacking the > `--merge` code - maybe an alias like `git log --conflict` or similar > might be more descriptive now? > > What do you think about this idea? (Or am I maybe missing an easy way to > achieve it with the existing code as well?) Ha! Very nice patch. For comparison, have a look at my patch to achieve the same that I never submitted (in particular, the author date): https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0 This implementation is more complete than mine. I like it. > > Michael > > > [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/ > > revision.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/revision.c b/revision.c > index 2424c9bd67..2e5c00dabd 100644 > --- a/revision.c > +++ b/revision.c > @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs, > } > } > > +static char* get_action_head_name(struct object_id* oid) > +{ > + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) { > + return "MERGE_HEAD"; > + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) { > + return "REBASE_HEAD"; > + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) { > + return "CHERRY_PICK_HEAD"; > + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) { > + return "REVERT_HEAD"; > + } else > + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); > +} > + > static void prepare_show_merge(struct rev_info *revs) > { > struct commit_list *bases; > struct commit *head, *other; > struct object_id oid; > const char **prune = NULL; > + const char *action_head_name; > int i, prune_num = 1; /* counting terminating NULL */ > struct index_state *istate = revs->repo->index; > > if (repo_get_oid(the_repository, "HEAD", &oid)) > die("--merge without HEAD?"); > head = lookup_commit_or_die(&oid, "HEAD"); > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > - die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > + action_head_name = get_action_head_name(&oid); > + other = lookup_commit_or_die(&oid, action_head_name); > add_pending_object(revs, &head->object, "HEAD"); > - add_pending_object(revs, &other->object, "MERGE_HEAD"); > + add_pending_object(revs, &other->object, action_head_name); > bases = repo_get_merge_bases(the_repository, head, other); > add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); > add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM); ``` ## Johannes Sixt, 2024-01-12 07:59 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: URL: https://gitlist.dev/e/ff7a568d-dcf7-4230-ba08-5234b505ab8f%40kdbg.org In-Reply-To: ``` Am 12.01.24 um 08:35 schrieb Johannes Sixt: > ... (in particular, the author date): > https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0 The Github UI is a bit unhelpful here. The author date is Nov 11, 2014. -- Hannes ``` ## phillip.wood123@gmail.com, 2024-01-12 11:01 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: <47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com> URL: https://gitlist.dev/e/47a4418b-68bf-4850-ba8b-1a5264f923e4%40gmail.com In-Reply-To: <20240111233311.64734-1-mi.al.lohmann@gmail.com> ``` Hi Michael On 11/01/2024 23:33, Michael Lohmann wrote: > This extends the functionality of `git log --merge` to also work with > conflicts for rebase, cherry pick and revert. > > Signed-off-by: Michael Lohmann > --- > Hi everybody! > > When Phillip Wood gave me some nice hints on his workflow concerning > conflicts [1] (we discussed if `--show-current-patch` would make sense > for cherry pick/revert). When I learned about `git log --merge` I was a > bit sad to discover that those do not exist for rebase/revert/cherry > pick since they look really valuable for getting an understanding on > what was changed. It is basically the counterpart to > `git show ${ACTION}_HEAD` for understanding the source of the conflict. > > I am curious if also other people would be interested in having an easy > way to get a log of only the relevant commits touching conflicting files > for said actions. > > With this patch I think the functionality is there, just hijacking the > `--merge` code - maybe an alias like `git log --conflict` or similar > might be more descriptive now? > > What do you think about this idea? (Or am I maybe missing an easy way to > achieve it with the existing code as well?) I should start by saying that I didn't know "git log --merge" existed before I saw this message so please correct me if I've misunderstood what this patch is doing. If I understand correctly it shows the commits from each side of the merge and is equivalent to git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD) When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*] so I'm not sure what git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor. As I say I've not used "git log --merge" so maybe I'm missing the reason why it would be useful when cherry-picking or rebasing. Best Wishes Phillip [*] assuming we're not picking a merge commit > Michael > > > [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/ > > revision.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/revision.c b/revision.c > index 2424c9bd67..2e5c00dabd 100644 > --- a/revision.c > +++ b/revision.c > @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs, > } > } > > +static char* get_action_head_name(struct object_id* oid) > +{ > + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) { > + return "MERGE_HEAD"; > + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) { > + return "REBASE_HEAD"; > + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) { > + return "CHERRY_PICK_HEAD"; > + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) { > + return "REVERT_HEAD"; > + } else > + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); > +} > + > static void prepare_show_merge(struct rev_info *revs) > { > struct commit_list *bases; > struct commit *head, *other; > struct object_id oid; > const char **prune = NULL; > + const char *action_head_name; > int i, prune_num = 1; /* counting terminating NULL */ > struct index_state *istate = revs->repo->index; > > if (repo_get_oid(the_repository, "HEAD", &oid)) > die("--merge without HEAD?"); > head = lookup_commit_or_die(&oid, "HEAD"); > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > - die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > + action_head_name = get_action_head_name(&oid); > + other = lookup_commit_or_die(&oid, action_head_name); > add_pending_object(revs, &head->object, "HEAD"); > - add_pending_object(revs, &other->object, "MERGE_HEAD"); > + add_pending_object(revs, &other->object, action_head_name); > bases = repo_get_merge_bases(the_repository, head, other); > add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); > add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM); ``` ## Michael Lohmann, 2024-01-12 15:03 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: <20240112150346.73735-1-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240112150346.73735-1-mi.al.lohmann%40gmail.com In-Reply-To: <47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com> ``` Hi Phillip, On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote: > I should start by saying that I didn't know "git log --merge" existed before > I saw this message I also just found it and it looked very useful... > so please correct me if I've misunderstood what this patch is doing. If I > understand correctly it shows the commits from each side of the merge and is > equivalent to > > git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD) > > When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*] > so I'm not sure what > > git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) Almost, but not quite: "git log —merge" only shows the commits touching the conflict, so it would be equivalent to (I think): git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative) (or replace CHERRY_PICK with one of the other actions) > Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor. True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I have to confess I did not check how it would behave under those circumstances. It could either error, or (more helpful) show the log touching the file until the root commit. Best wishes Michael ``` ## Michael Lohmann, 2024-01-12 15:50 Subject: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge Message-ID: <20240112155033.77204-1-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240112155033.77204-1-mi.al.lohmann%40gmail.com In-Reply-To: ``` This is done to (1) ensure MERGE_HEAD is a ref, (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref() Helped-by: Junio C Hamano Signed-off-by: Michael Lohmann --- Hi! On 12. Jan 2024, at 01:15, Junio C Hamano wrote: > > This extends the functionality of `git log --merge` to also work with > > conflicts for rebase, cherry pick and revert. > > > > Signed-off-by: Michael Lohmann > > --- > > ... It is basically the counterpart to > > `git show ${ACTION}_HEAD` for understanding the source of the conflict. > > I do not know about the validity of that approach to use *_HEAD, What I wanted to convey is that e.g. "git show CHERRY_PICK_HEAD" will tell you about the conflict from the perspective of the commit that is currently to be applied while "git log --merge" tells the story from the perspective of HEAD. So they are by no means the same, but can complement each other in getting an understanding about the conflict. > but we may want to tighten the original's use of repo_get > > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > > - die("--merge without MERGE_HEAD?"); > > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > > ... so that we won't be confused in a repository that has a tag > whose name happens to be MERGE_HEAD. We probably should be using > refs.c:refs_resolve_ref_unsafe() instead to _oid() here ... Here I am really at the limit of my understanding of the core functions. Is this roughly what you had in mind? From my testing it seems to do the job, but I don't understand the details "refs_resolve_ref_unsafe"... revision.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/revision.c b/revision.c index 2424c9bd67..786e1a3e89 100644 --- a/revision.c +++ b/revision.c @@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs) struct commit *head, *other; struct object_id oid; const char **prune = NULL; + const char *other_head; int i, prune_num = 1; /* counting terminating NULL */ struct index_state *istate = revs->repo->index; if (repo_get_oid(the_repository, "HEAD", &oid)) die("--merge without HEAD?"); head = lookup_commit_or_die(&oid, "HEAD"); - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) + other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository), + "MERGE_HEAD", + RESOLVE_REF_READING, + &oid, + NULL); + if (!other_head) die("--merge without MERGE_HEAD?"); - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); + other = lookup_commit_or_die(&oid, other_head); add_pending_object(revs, &head->object, "HEAD"); - add_pending_object(revs, &other->object, "MERGE_HEAD"); + add_pending_object(revs, &other->object, other_head); bases = repo_get_merge_bases(the_repository, head, other); add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM); -- 2.43.0.284.g6c31128a96 ``` ## Michael Lohmann, 2024-01-12 15:50 Subject: [PATCH v2 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: <20240112155033.77204-2-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240112155033.77204-2-mi.al.lohmann%40gmail.com In-Reply-To: <20240112155033.77204-1-mi.al.lohmann@gmail.com> ``` Co-authored-by: Johannes Sixt Signed-off-by: Michael Lohmann --- > I won't comment on the coding style violations in the patch below in > this message. I hope this one is better. The other one was just a proof of the general concept and meant as a starting point for a discussion if this is wanted at all. But I should still have taken more care. On 12. Jan 2024, at 08:35, Johannes Sixt wrote: > Ha! Very nice patch. For comparison, have a look at my patch to achieve > the same that I never submitted (in particular, the author date): > https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0 > > This implementation is more complete than mine. I like it. Ha! Nice one! I took a few of your changes as an inspiration and put you as a co-author. Cheers Michael Difference compared to v1: Basically complete rewrite using "refs_resolve_ref_unsafe". Has to be applied after [PATCH v2 1/2] to avoid conflict. revision.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/revision.c b/revision.c index 786e1a3e89..b0b47dd241 100644 --- a/revision.c +++ b/revision.c @@ -1961,6 +1961,25 @@ static void add_pending_commit_list(struct rev_info *revs, } } +static const char *lookup_other_head(struct object_id *oid) +{ + struct ref_store *refs = get_main_ref_store(the_repository); + const char *name; + int i; + static const char *const other_head[] = { + "MERGE_HEAD", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD" + }; + + for (i = 0; i < ARRAY_SIZE(other_head); i++) { + name = refs_resolve_ref_unsafe(refs, other_head[i], + RESOLVE_REF_READING, oid, NULL); + if (name) + return name; + } + + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); +} + static void prepare_show_merge(struct rev_info *revs) { struct commit_list *bases; @@ -1974,13 +1993,7 @@ static void prepare_show_merge(struct rev_info *revs) if (repo_get_oid(the_repository, "HEAD", &oid)) die("--merge without HEAD?"); head = lookup_commit_or_die(&oid, "HEAD"); - other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository), - "MERGE_HEAD", - RESOLVE_REF_READING, - &oid, - NULL); - if (!other_head) - die("--merge without MERGE_HEAD?"); + other_head = lookup_other_head(&oid); other = lookup_commit_or_die(&oid, other_head); add_pending_object(revs, &head->object, "HEAD"); add_pending_object(revs, &other->object, other_head); -- 2.43.0.284.g6c31128a96 ``` ## Junio C Hamano, 2024-01-12 20:10 Subject: Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge Message-ID: URL: https://gitlist.dev/e/xmqqzfxa9usx.fsf%40gitster.g In-Reply-To: <20240112155033.77204-1-mi.al.lohmann@gmail.com> ``` Michael Lohmann writes: >> but we may want to tighten the original's use of repo_get >> > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) >> > - die("--merge without MERGE_HEAD?"); >> > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); >> >> ... so that we won't be confused in a repository that has a tag >> whose name happens to be MERGE_HEAD. We probably should be using >> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ... > > Here I am really at the limit of my understanding of the core functions. > Is this roughly what you had in mind? From my testing it seems to do the > job, but I don't understand the details "refs_resolve_ref_unsafe"... Perhaps there are others who are more familiar with the ref API than I am, but I was just looking at refs.h today and wonder if something along the lines of this ... if (read_ref_full("MERGE_HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, &oid, NULL)) die("no MERGE_HEAD"); if (is_null_oid(&oid)) die("MERGE_HEAD is a symbolic ref???"); ... would be simpler. With the above, we are discarding the refname read_ref_full() obtains from its refs_resolve_ref_unsafe(), but I think that is totally fine. We expect it to be "MERGE_HEAD" in the good case. The returned value can be different from "MERGE_HEAD" if it is a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be trusted, we should catch that case with the NULL-ness check on oid. Which would mean that we do not need a separate "other_head" variable, and instead can use "MERGE_HEAD". > revision.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/revision.c b/revision.c > index 2424c9bd67..786e1a3e89 100644 > --- a/revision.c > +++ b/revision.c > @@ -1967,17 +1967,23 @@ static void prepare_show_merge(struct rev_info *revs) > struct commit *head, *other; > struct object_id oid; > const char **prune = NULL; > + const char *other_head; > int i, prune_num = 1; /* counting terminating NULL */ > struct index_state *istate = revs->repo->index; > > if (repo_get_oid(the_repository, "HEAD", &oid)) > die("--merge without HEAD?"); > head = lookup_commit_or_die(&oid, "HEAD"); > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > + other_head = refs_resolve_ref_unsafe(get_main_ref_store(the_repository), > + "MERGE_HEAD", > + RESOLVE_REF_READING, > + &oid, > + NULL); > + if (!other_head) > die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > + other = lookup_commit_or_die(&oid, other_head); > add_pending_object(revs, &head->object, "HEAD"); > - add_pending_object(revs, &other->object, "MERGE_HEAD"); > + add_pending_object(revs, &other->object, other_head); > bases = repo_get_merge_bases(the_repository, head, other); > add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); > add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM); ``` ## Junio C Hamano, 2024-01-12 20:18 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: URL: https://gitlist.dev/e/xmqqv87y9ufq.fsf%40gitster.g In-Reply-To: ``` Johannes Sixt writes: >> What do you think about this idea? (Or am I maybe missing an easy way to >> achieve it with the existing code as well?) > > Ha! Very nice patch. For comparison, have a look at my patch to achieve > the same that I never submitted (in particular, the author date): > https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0 > > This implementation is more complete than mine. I like it. I like the way your other_merge_candidate() loops over an array of possible candidates, which makes it a lot easier to extend, though, admittedly the "die()" message needs auto-generating if we really wanted to make it maintenance free ;-) Thanks. ``` ## Junio C Hamano, 2024-01-12 20:21 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: URL: https://gitlist.dev/e/xmqqr0im9ub9.fsf%40gitster.g In-Reply-To: <47a4418b-68bf-4850-ba8b-1a5264f923e4@gmail.com> ``` phillip.wood123@gmail.com writes: > I should start by saying that I didn't know "git log --merge" existed > before I saw this message so please correct me if I've misunderstood > what this patch is doing. If I understand correctly it shows the > commits from each side of the merge and is equivalent to > > git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD) > > When a commit is cherry-picked the merge base used is > CHERRY_PICK_HEAD^ [*] so I'm not sure what > > git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) > > tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a > common ancestor. As I say I've not used "git log --merge" so maybe I'm > missing the reason why it would be useful when cherry-picking or > rebasing. Good question. It is the whole point of cherry-pick that CHERRY_PICK_HEAD and the current HEAD may not have any meaningful ancestry relationship, so yes, it is questionable to use the same logic for "log --merge" between HEAD...CHERRY_PICK_HEAD, while using HEAD...MERGE_HEAD does make perfect sense. ``` ## Michael Lohmann, 2024-01-12 21:06 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: <20240112210650.5455-1-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240112210650.5455-1-mi.al.lohmann%40gmail.com In-Reply-To: ``` > > tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a > > common ancestor. As I say I've not used "git log --merge" so maybe I'm > > missing the reason why it would be useful when cherry-picking or > > rebasing. > > Good question. It is the whole point of cherry-pick that > CHERRY_PICK_HEAD and the current HEAD may not have any meaningful > ancestry relationship, so yes, it is questionable to use the same > logic for "log --merge" between HEAD...CHERRY_PICK_HEAD, while using > HEAD...MERGE_HEAD does make perfect sense. I tried to say it in [1]: a merge also does not need a common ancestor! Try it: git init test cd test echo something > README.md git add --all git commit -m "initial commit" git remote add origin_git git://git.kernel.org/pub/scm/git/git.git git fetch origin_git master # obviously no common ancestor git merge --allow-unrelated-histories origin_git/master # merge conflict git log --merge # Still loggs all the conflicting commits So indeed the command that Phillip wrote is not equivalent and the existing logic already can deal with unrelated histories. Or am I misunderstanding? Michael [1] https://lore.kernel.org/git/20240112150346.73735-1-mi.al.lohmann@gmail.com/ ``` ## Junio C Hamano, 2024-01-12 21:14 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: URL: https://gitlist.dev/e/xmqqil3y9rvm.fsf%40gitster.g In-Reply-To: <20240112150346.73735-1-mi.al.lohmann@gmail.com> ``` Michael Lohmann writes: > Almost, but not quite: "git log —merge" only shows the commits touching the > conflict, so it would be equivalent to (I think): > > git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative) > > (or replace CHERRY_PICK with one of the other actions) It can certainly _reduce_ the noise, but I am not sure if it works over the right history segment. Let me think aloud a bit. Let's imagine that in a history forked long time ago, O----O----O----O----X HEAD \ Z---Z---Z---Z---A---B CHERRY_PICK_HEAD where all commits modified the same path in question that you saw conflicts in when your "git cherry-pick B" stopped. I do not know what to think about the changes to that path by the commits denoted by 'O', but the changes done to the path by commits denoted by 'Z' should have absolutely no relevance, as the whole point of cherry-picking B relative to A is because we do not care about what Zs did, no? For that matter, given that how we structure the 3-way merge for such a cherry-pick to "move from X the same way as you would move from A to B", how you got to X (i.e. Os) should not matter, either. On the other hand, such a conflict may arise from the fact that Os and Zs made changes differently to cause the contents of the path at X and A differ significantly. So, OK, I can buy your argument that what Os and Zs to the conflicted path did can be interesting when understanding the conflict during such a cherry-pick. >> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor. > > True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I But that is very different, isn't it? Merging two unrelated histories is like merging two histories where the common ancestor had an empty tree, i.e. o---o---o---X HEAD / (v) an imaginary ancestor with an empty tree \ o---o---o---O MERGE_HEAD so it is a reasonable degenerated behaviour to consider what all commits on both sides did to the paths in question. ``` ## Phillip Wood, 2024-01-15 10:48 Subject: Re: [RFC PATCH] `log --merge` also for rebase/cherry pick/revert Message-ID: <648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com> URL: https://gitlist.dev/e/648774b5-5208-42d3-95c7-e0cba4d6a159%40gmail.com In-Reply-To: <20240112150346.73735-1-mi.al.lohmann@gmail.com> ``` Hi Michael On 12/01/2024 15:03, Michael Lohmann wrote: > Hi Phillip, > > On 12. Jan 2024, at 12:01, phillip.wood123@gmail.com wrote: >> I should start by saying that I didn't know "git log --merge" existed before >> I saw this message > I also just found it and it looked very useful... > >> so please correct me if I've misunderstood what this patch is doing. If I >> understand correctly it shows the commits from each side of the merge and is >> equivalent to >> >> git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD) >> >> When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*] >> so I'm not sure what >> >> git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) > > Almost, but not quite: "git log —merge" only shows the commits touching the > conflict, so it would be equivalent to (I think): > > git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative) > > (or replace CHERRY_PICK with one of the other actions) Thanks for clarifying that. >> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor. > > True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I > have to confess I did not check how it would behave under those circumstances. > It could either error, or (more helpful) show the log touching the file until > the root commit. What I was trying to get at was that with "git merge" "git log --merge" will show commits that are part of the merge. With "git cherry-pick" that's not the case because we're selecting the commits to show using the merge base of HEAD and CHERRY_PICK_HEAD while cherry-pick uses CHERRY_PICK_HEAD^ as the base of the merge. I think Junio explains why it is still useful to show those commits in [1] i.e. they help the user understand the conflicts even though they are not part of the merge. It might be worth expanding the commit message to explain that. Best Wishes Phillip [1] https://lore.kernel.org/git/xmqqil3y9rvm.fsf@gitster.g/ ``` ## Patrick Steinhardt, 2024-01-15 11:36 Subject: Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge Message-ID: URL: https://gitlist.dev/e/ZaUYyEAdr4oTImL-%40tanuki In-Reply-To: ``` On Fri, Jan 12, 2024 at 12:10:54PM -0800, Junio C Hamano wrote: > Michael Lohmann writes: > > >> but we may want to tighten the original's use of repo_get > >> > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > >> > - die("--merge without MERGE_HEAD?"); > >> > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > >> > >> ... so that we won't be confused in a repository that has a tag > >> whose name happens to be MERGE_HEAD. We probably should be using > >> refs.c:refs_resolve_ref_unsafe() instead to _oid() here ... > > > > Here I am really at the limit of my understanding of the core functions. > > Is this roughly what you had in mind? From my testing it seems to do the > > job, but I don't understand the details "refs_resolve_ref_unsafe"... > > Perhaps there are others who are more familiar with the ref API than > I am, but I was just looking at refs.h today and wonder if something > along the lines of this ... > > if (read_ref_full("MERGE_HEAD", > RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > &oid, NULL)) > die("no MERGE_HEAD"); > if (is_null_oid(&oid)) > die("MERGE_HEAD is a symbolic ref???"); > > ... would be simpler. > > With the above, we are discarding the refname read_ref_full() > obtains from its refs_resolve_ref_unsafe(), but I think that is > totally fine. We expect it to be "MERGE_HEAD" in the good case. > The returned value can be different from "MERGE_HEAD" if it is > a symbolic ref, but if the comment in refs.h on NO_RECURSE is to be > trusted, we should catch that case with the NULL-ness check on oid. Yeah, this should be fine. Even though I have stared at our refs API for extended periods of time during the last months I still have to look up this part of the API almost every time. I find it to be hard to use because you not only have to pay attention to the return value, but also to the flags in some edge cases. I wouldn't be surprised if there were many callsites that get this wrong. > Which would mean that we do not need a separate "other_head" > variable, and instead can use "MERGE_HEAD". There is no need for this, is there? We have already resolved the ref to an object ID, so why not use that via `add_pending_oid()`? Patrick ``` ## Junio C Hamano, 2024-01-15 17:19 Subject: Re: [PATCH v2 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge Message-ID: URL: https://gitlist.dev/e/xmqq4jfe5xb1.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: >> Which would mean that we do not need a separate "other_head" >> variable, and instead can use "MERGE_HEAD". > > There is no need for this, is there? We have already resolved the ref to > an object ID, so why not use that via `add_pending_oid()`? add_pending_oid() and its callees use the name only for human consumption (e.g., in error messages), as they all already have the object name. ``` ## Michael Lohmann, 2024-01-17 08:14 Subject: [PATCH v3 1/2] revision: ensure MERGE_HEAD is a ref in prepare_show_merge Message-ID: <20240117081405.14012-1-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240117081405.14012-1-mi.al.lohmann%40gmail.com In-Reply-To: ``` This is done to (1) ensure MERGE_HEAD is a ref, (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref() (3) error out when MERGE_HEAD is a symref. Helped-by: Junio C Hamano Signed-off-by: Michael Lohmann --- Changes with respect to v2: - use read_ref_full instead of refs_resolve_ref_unsafe - don't use "other_head" variable to store MERGE_HEAD - check for symbolic ref revision.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/revision.c b/revision.c index 2424c9bd67..aa4c4dc778 100644 --- a/revision.c +++ b/revision.c @@ -1973,8 +1973,12 @@ static void prepare_show_merge(struct rev_info *revs) if (repo_get_oid(the_repository, "HEAD", &oid)) die("--merge without HEAD?"); head = lookup_commit_or_die(&oid, "HEAD"); - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) + if (read_ref_full("MERGE_HEAD", + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, + &oid, NULL)) die("--merge without MERGE_HEAD?"); + if (is_null_oid(&oid)) + die("MERGE_HEAD is a symbolic ref???"); other = lookup_commit_or_die(&oid, "MERGE_HEAD"); add_pending_object(revs, &head->object, "HEAD"); add_pending_object(revs, &other->object, "MERGE_HEAD"); -- 2.39.3 (Apple Git-145) ``` ## Michael Lohmann, 2024-01-17 08:14 Subject: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: <20240117081405.14012-2-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240117081405.14012-2-mi.al.lohmann%40gmail.com In-Reply-To: <20240117081405.14012-1-mi.al.lohmann@gmail.com> ``` Co-authored-by: Johannes Sixt Signed-off-by: Michael Lohmann --- On 12. Jan 2024, at 21:18, Junio C Hamano 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) ``` ## Michael Lohmann, 2024-01-17 09:19 Subject: Full disclosure Message-ID: <20240117091905.14354-1-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240117091905.14354-1-mi.al.lohmann%40gmail.com In-Reply-To: <20240117081405.14012-2-mi.al.lohmann@gmail.com> ``` 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, 2024-01-17 09:58 Subject: Re: Full disclosure Message-ID: URL: https://gitlist.dev/e/CAP8UFD3g_N%2BJJ_cdas2ZdsBPxuQFBXW8yY%2B1nC6AKvQppOYaDw%40mail.gmail.com In-Reply-To: <20240117091905.14354-1-mi.al.lohmann@gmail.com> ``` On Wed, Jan 17, 2024 at 10:20 AM Michael Lohmann 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, 2024-01-17 17:41 Subject: Full disclosure Message-ID: <20240117174125.22915-1-mi.al.lohmann@gmail.com> URL: https://gitlist.dev/e/20240117174125.22915-1-mi.al.lohmann%40gmail.com In-Reply-To: ``` 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, 2024-01-17 18:33 Subject: Re: Full disclosure Message-ID: URL: https://gitlist.dev/e/xmqq7ck7x10y.fsf%40gitster.g In-Reply-To: <20240117091905.14354-1-mi.al.lohmann@gmail.com> ``` Michael Lohmann 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, 2024-01-21 00:41 Subject: Re: Full disclosure Message-ID: URL: https://gitlist.dev/e/de816c41-6889-4178-add1-7c2bf636640d%40mrbrklyn.com In-Reply-To: <20240117174125.22915-1-mi.al.lohmann@gmail.com> ``` https://www.zazzle.com/idit_aharons_tzfat_original_backpack_design-256246398431855710 On 1/17/24 12:41, Michael Lohmann wrote: > Hi Christian! > > Yes: I agree that the current state of the last submitted patch in that > discussion is far from optimal. > After Jeff Kings explanation I had a much better understanding for the > situation and the reasoning (and his suggestion was definitely better than > mine). > > I had already prepared a new version which tackled (I think) pretty much all of > the criticisms. But then the above mentioned message came in and when I read > this: > >> [...] they are trying to be different for the sake of being different, which >> is not a good sign. I'd want our contributors to be original where being >> original matters more. > > I am reading: > > 1) I am "trying to be different for the sake of being different" > 2) Contributors like this are not wanted > > So yes, I do understand this as a general statement on me as a contributor > without any proposal for me at least to explain the situation from my side. > > I teach my colleges not to name variables with how they are initialized, but > with what information they actually convey and I found the "_NONE" one at least > misleading in its name. > > So in the initial discussion I was a bit stubborn, because Philip wrote "I > don't have a strong opinion" and from my perspective the only argument was > "over there we also do it that way" (which _can_ 100% be a valid reason), but > Junio C Hamano did not even acknowledge my criticisms of the other the variable > name. I am totally fine with a decision like this if done consciously, but if I > don't even get an acknowledgement, let alone an explanation, my demands I place > on my code quality are that I write the best code I can. And with all the info > I had (prior to Jeffs message) I did still favour my first suggestion. > > In my eyes it would be helpful to at least tell me what your (in my eyes not > obvious) preferences are on naming things, because otherwise I will rather > stick to my standards than blindly follow a single instance of other code where > I don't even know if that was a concious decision or it just happened by > accident. > > So no, I don't agree with the assessment of point 1), but I still read the > message like that. I will accept it and instead use my skills in different > projects where they are indeed valued. > > Cheers > Michael -- So many immigrant groups have swept through our town that Brooklyn, like Atlantis, reaches mythological proportions in the mind of the world - RI Safir 1998 http://www.mrbrklyn.com DRM is THEFT - We are the STAKEHOLDERS - RI Safir 2002 http://www.nylxs.com - Leadership Development in Free Software http://www.brooklyn-living.com Being so tracked is for FARM ANIMALS and extermination camps, but incompatible with living as a free human being. -RI Safir 2013 ``` ## Elijah Newren, 2024-01-24 07:06 Subject: Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: URL: https://gitlist.dev/e/CABPp-BE4zqRX%3Dwd5EBj96hzCS8V73QpdN-2pCpv7qMdkpkX93w%40mail.gmail.com In-Reply-To: <20240117081405.14012-2-mi.al.lohmann@gmail.com> ``` On Wed, Jan 17, 2024 at 12:14 AM Michael Lohmann wrote: > > Co-authored-by: Johannes Sixt > Signed-off-by: Michael Lohmann > --- > > On 12. Jan 2024, at 21:18, Junio C Hamano wrote: > > I like the way your other_merge_candidate() loops over an array of > > possible candidates, which makes it a lot easier to extend, though, > > admittedly the "die()" message needs auto-generating if we really > > wanted to make it maintenance free ;-) > > I would certainly like that but I did not find an easy way of achieving > this in C. Help wanted. > > Changes with respect to v2: > - use read_ref_full instead of refs_resolve_ref_unsafe > - check for symbolic ref > - extract "other_name" in this patch, instead of patch 1 > > revision.c | 31 +++++++++++++++++++++++-------- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/revision.c b/revision.c > index aa4c4dc778..c778413c7d 100644 > --- a/revision.c > +++ b/revision.c > @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs, > } > } > > +static const char *lookup_other_head(struct object_id *oid) > +{ > + int i; > + static const char *const other_head[] = { > + "MERGE_HEAD", "REBASE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD" > + }; > + > + for (i = 0; i < ARRAY_SIZE(other_head); i++) > + if (!read_ref_full(other_head[i], > + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > + oid, NULL)) { > + if (is_null_oid(oid)) > + die("%s is a symbolic ref???", other_head[i]); > + return other_head[i]; > + } > + > + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); > +} > + > static void prepare_show_merge(struct rev_info *revs) > { > struct commit_list *bases; > struct commit *head, *other; > struct object_id oid; > + const char *other_name; > const char **prune = NULL; > int i, prune_num = 1; /* counting terminating NULL */ > struct index_state *istate = revs->repo->index; > @@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs) > if (repo_get_oid(the_repository, "HEAD", &oid)) > die("--merge without HEAD?"); > head = lookup_commit_or_die(&oid, "HEAD"); > - if (read_ref_full("MERGE_HEAD", > - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > - &oid, NULL)) > - die("--merge without MERGE_HEAD?"); > - if (is_null_oid(&oid)) > - die("MERGE_HEAD is a symbolic ref???"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > + other_name = lookup_other_head(&oid); > + other = lookup_commit_or_die(&oid, other_name); > add_pending_object(revs, &head->object, "HEAD"); > - add_pending_object(revs, &other->object, "MERGE_HEAD"); > + add_pending_object(revs, &other->object, other_name); > bases = repo_get_merge_bases(the_repository, head, other); > add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); > add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM); > -- > 2.39.3 (Apple Git-145) I had to go look up previous versions to see the discussion of why this was useful for things other than merge. I agree with Phillip from https://lore.kernel.org/git/648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com/, that the commit message _needs_ to explain this, likely using some of Junio's explanation. Also, what about cases where users do a cherry-pick in the middle of a rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What then? ``` ## Junio C Hamano, 2024-01-24 17:34 Subject: Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: URL: https://gitlist.dev/e/xmqqr0i6fxe2.fsf%40gitster.g In-Reply-To: ``` Elijah Newren writes: > I had to go look up previous versions to see the discussion of why > this was useful for things other than merge. I agree with Phillip > from https://lore.kernel.org/git/648774b5-5208-42d3-95c7-e0cba4d6a159@gmail.com/, > that the commit message _needs_ to explain this, likely using some of > Junio's explanation. Please note that I am not very happy with that "explanation" myself. The only thing I can still agree to in that message myself is that it is sensible for "log --merge" to go down all the way to the root of the histories leading to MERGE_HEAD and HEAD in the "merging two unrelated histories" scenario. Treating CHERRY_PICK_HEAD and others the same way, to me, almost sounds as if we are saying "all the commits behind the commits involved in the conflicted operation are worth looking at", which is not a very useful or productive thing. > Also, what about cases where users do a cherry-pick in the middle of a > rebase, so that both REBASE_HEAD and CHERRY_PICK_HEAD exist? What > then? ;-) ``` ## Johannes Sixt, 2024-01-24 17:19 Subject: Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: URL: https://gitlist.dev/e/dfb582cf-b1e4-414d-bfe1-0f93d910ec54%40kdbg.org In-Reply-To: ``` 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, 2024-01-24 19:46 Subject: Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: URL: https://gitlist.dev/e/xmqq1qa6ecqr.fsf%40gitster.g In-Reply-To: ``` Johannes Sixt 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, 2024-01-24 22:06 Subject: Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: <0a061bee-9b78-419f-9f1a-ced69ca96002@kdbg.org> URL: https://gitlist.dev/e/0a061bee-9b78-419f-9f1a-ced69ca96002%40kdbg.org In-Reply-To: ``` Am 24.01.24 um 20:46 schrieb Junio C Hamano: > Johannes Sixt 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, 2024-01-24 22:13 Subject: Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: URL: https://gitlist.dev/e/xmqqbk9abcru.fsf%40gitster.g In-Reply-To: <0a061bee-9b78-419f-9f1a-ced69ca96002@kdbg.org> ``` Johannes Sixt writes: > Am 24.01.24 um 20:46 schrieb Junio C Hamano: >> Johannes Sixt 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, 2024-02-09 23:54 Subject: Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert Message-ID: URL: https://gitlist.dev/e/xmqq7cjdchx8.fsf%40gitster.g In-Reply-To: ``` Johannes Sixt 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 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 Signed-off-by: Michael Lohmann [jc: tweaked in j6t's precedence fix that tries REBASE_HEAD last] Signed-off-by: Junio C Hamano --- revision.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/revision.c b/revision.c index aa4c4dc778..36dc2f94f7 100644 --- a/revision.c +++ b/revision.c @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs, } } +static const char *lookup_other_head(struct object_id *oid) +{ + int i; + static const char *const other_head[] = { + "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD" + }; + + for (i = 0; i < ARRAY_SIZE(other_head); i++) + if (!read_ref_full(other_head[i], + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, + oid, NULL)) { + if (is_null_oid(oid)) + die("%s is a symbolic ref???", other_head[i]); + return other_head[i]; + } + + die("--merge without MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD?"); +} + static void prepare_show_merge(struct rev_info *revs) { struct commit_list *bases; struct commit *head, *other; struct object_id oid; + const char *other_name; const char **prune = NULL; int i, prune_num = 1; /* counting terminating NULL */ struct index_state *istate = revs->repo->index; @@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs) if (repo_get_oid(the_repository, "HEAD", &oid)) die("--merge without HEAD?"); head = lookup_commit_or_die(&oid, "HEAD"); - if (read_ref_full("MERGE_HEAD", - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - &oid, NULL)) - die("--merge without MERGE_HEAD?"); - if (is_null_oid(&oid)) - die("MERGE_HEAD is a symbolic ref???"); - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); + other_name = lookup_other_head(&oid); + other = lookup_commit_or_die(&oid, other_name); add_pending_object(revs, &head->object, "HEAD"); - add_pending_object(revs, &other->object, "MERGE_HEAD"); + add_pending_object(revs, &other->object, other_name); bases = repo_get_merge_bases(the_repository, head, other); add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM); -- 2.44.0-rc0 ```