| Message ID | 20240111233311.64734-1-mi.al.lohmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] `log --merge` also for rebase/cherry pick/revert | expand |
Michael Lohmann <mi.al.lohmann@gmail.com> writes: > This extends the functionality of `git log --merge` to also work with > conflicts for rebase, cherry pick and revert. > > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> > --- > ... It is basically the counterpart to > `git show ${ACTION}_HEAD` for understanding the source of the conflict. I do not know about the validity of that approach to use *_HEAD, but we may want to tighten the original's use of repo_get_oid() here ... > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > - die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); ... so that we won't be confused in a repository that has a tag whose name happens to be MERGE_HEAD. We probably should be using refs.c:refs_resolve_ref_unsafe() instead to (1) ensure MERGE_HEAD is a ref, (2) obtain the oid without any prefixing by refs.c:repo_dwim_ref(), and optionally (3) error out when MERGE_HEAD is a symref. As your patch makes the problem even worse, if we were to do such a tightening (and I do not see a reason not to), it may want to be done before, not after, this patch. I won't comment on the coding style violations in the patch below in this message. Thanks. > diff --git a/revision.c b/revision.c > index 2424c9bd67..2e5c00dabd 100644 > --- a/revision.c > +++ b/revision.c > @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs, > } > } > > +static char* get_action_head_name(struct object_id* oid) > +{ > + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) { > + return "MERGE_HEAD"; > + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) { > + return "REBASE_HEAD"; > + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) { > + return "CHERRY_PICK_HEAD"; > + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) { > + return "REVERT_HEAD"; > + } else > + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); > +} > + > static void prepare_show_merge(struct rev_info *revs) > { > struct commit_list *bases; > struct commit *head, *other; > struct object_id oid; > const char **prune = NULL; > + const char *action_head_name; > int i, prune_num = 1; /* counting terminating NULL */ > struct index_state *istate = revs->repo->index; > > if (repo_get_oid(the_repository, "HEAD", &oid)) > die("--merge without HEAD?"); > head = lookup_commit_or_die(&oid, "HEAD"); > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > - die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > + action_head_name = get_action_head_name(&oid); > + other = lookup_commit_or_die(&oid, action_head_name); > add_pending_object(revs, &head->object, "HEAD"); > - add_pending_object(revs, &other->object, "MERGE_HEAD"); > + add_pending_object(revs, &other->object, action_head_name); > bases = repo_get_merge_bases(the_repository, head, other); > add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); > add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
Am 12.01.24 um 00:33 schrieb Michael Lohmann: > This extends the functionality of `git log --merge` to also work with > conflicts for rebase, cherry pick and revert. > > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> > --- > Hi everybody! > > When Phillip Wood gave me some nice hints on his workflow concerning > conflicts [1] (we discussed if `--show-current-patch` would make sense > for cherry pick/revert). When I learned about `git log --merge` I was a > bit sad to discover that those do not exist for rebase/revert/cherry > pick since they look really valuable for getting an understanding on > what was changed. It is basically the counterpart to > `git show ${ACTION}_HEAD` for understanding the source of the conflict. > > I am curious if also other people would be interested in having an easy > way to get a log of only the relevant commits touching conflicting files > for said actions. > > With this patch I think the functionality is there, just hijacking the > `--merge` code - maybe an alias like `git log --conflict` or similar > might be more descriptive now? > > What do you think about this idea? (Or am I maybe missing an easy way to > achieve it with the existing code as well?) Ha! Very nice patch. For comparison, have a look at my patch to achieve the same that I never submitted (in particular, the author date): https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0 This implementation is more complete than mine. I like it. > > Michael > > > [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/ > > revision.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/revision.c b/revision.c > index 2424c9bd67..2e5c00dabd 100644 > --- a/revision.c > +++ b/revision.c > @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs, > } > } > > +static char* get_action_head_name(struct object_id* oid) > +{ > + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) { > + return "MERGE_HEAD"; > + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) { > + return "REBASE_HEAD"; > + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) { > + return "CHERRY_PICK_HEAD"; > + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) { > + return "REVERT_HEAD"; > + } else > + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); > +} > + > static void prepare_show_merge(struct rev_info *revs) > { > struct commit_list *bases; > struct commit *head, *other; > struct object_id oid; > const char **prune = NULL; > + const char *action_head_name; > int i, prune_num = 1; /* counting terminating NULL */ > struct index_state *istate = revs->repo->index; > > if (repo_get_oid(the_repository, "HEAD", &oid)) > die("--merge without HEAD?"); > head = lookup_commit_or_die(&oid, "HEAD"); > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > - die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > + action_head_name = get_action_head_name(&oid); > + other = lookup_commit_or_die(&oid, action_head_name); > add_pending_object(revs, &head->object, "HEAD"); > - add_pending_object(revs, &other->object, "MERGE_HEAD"); > + add_pending_object(revs, &other->object, action_head_name); > bases = repo_get_merge_bases(the_repository, head, other); > add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); > add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
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
Hi Michael On 11/01/2024 23:33, Michael Lohmann wrote: > This extends the functionality of `git log --merge` to also work with > conflicts for rebase, cherry pick and revert. > > Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> > --- > Hi everybody! > > When Phillip Wood gave me some nice hints on his workflow concerning > conflicts [1] (we discussed if `--show-current-patch` would make sense > for cherry pick/revert). When I learned about `git log --merge` I was a > bit sad to discover that those do not exist for rebase/revert/cherry > pick since they look really valuable for getting an understanding on > what was changed. It is basically the counterpart to > `git show ${ACTION}_HEAD` for understanding the source of the conflict. > > I am curious if also other people would be interested in having an easy > way to get a log of only the relevant commits touching conflicting files > for said actions. > > With this patch I think the functionality is there, just hijacking the > `--merge` code - maybe an alias like `git log --conflict` or similar > might be more descriptive now? > > What do you think about this idea? (Or am I maybe missing an easy way to > achieve it with the existing code as well?) I should start by saying that I didn't know "git log --merge" existed before I saw this message so please correct me if I've misunderstood what this patch is doing. If I understand correctly it shows the commits from each side of the merge and is equivalent to git log HEAD MERGE_HEAD ^$(git merge-base HEAD MERGE_HEAD) When a commit is cherry-picked the merge base used is CHERRY_PICK_HEAD^ [*] so I'm not sure what git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) tells us. Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor. As I say I've not used "git log --merge" so maybe I'm missing the reason why it would be useful when cherry-picking or rebasing. Best Wishes Phillip [*] assuming we're not picking a merge commit > Michael > > > [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/ > > revision.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/revision.c b/revision.c > index 2424c9bd67..2e5c00dabd 100644 > --- a/revision.c > +++ b/revision.c > @@ -1961,23 +1961,37 @@ static void add_pending_commit_list(struct rev_info *revs, > } > } > > +static char* get_action_head_name(struct object_id* oid) > +{ > + if (!repo_get_oid(the_repository, "MERGE_HEAD", oid)) { > + return "MERGE_HEAD"; > + } else if (!repo_get_oid(the_repository, "REBASE_HEAD", oid)) { > + return "REBASE_HEAD"; > + } else if (!repo_get_oid(the_repository, "CHERRY_PICK_HEAD", oid)) { > + return "CHERRY_PICK_HEAD"; > + } else if (!repo_get_oid(the_repository, "REVERT_HEAD", oid)) { > + return "REVERT_HEAD"; > + } else > + die("--merge without MERGE_HEAD, REBASE_HEAD, CHERRY_PICK_HEAD or REVERT_HEAD?"); > +} > + > static void prepare_show_merge(struct rev_info *revs) > { > struct commit_list *bases; > struct commit *head, *other; > struct object_id oid; > const char **prune = NULL; > + const char *action_head_name; > int i, prune_num = 1; /* counting terminating NULL */ > struct index_state *istate = revs->repo->index; > > if (repo_get_oid(the_repository, "HEAD", &oid)) > die("--merge without HEAD?"); > head = lookup_commit_or_die(&oid, "HEAD"); > - if (repo_get_oid(the_repository, "MERGE_HEAD", &oid)) > - die("--merge without MERGE_HEAD?"); > - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); > + action_head_name = get_action_head_name(&oid); > + other = lookup_commit_or_die(&oid, action_head_name); > add_pending_object(revs, &head->object, "HEAD"); > - add_pending_object(revs, &other->object, "MERGE_HEAD"); > + add_pending_object(revs, &other->object, action_head_name); > bases = repo_get_merge_bases(the_repository, head, other); > add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); > add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);
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
Johannes Sixt <j6t@kdbg.org> writes: >> What do you think about this idea? (Or am I maybe missing an easy way to >> achieve it with the existing code as well?) > > Ha! Very nice patch. For comparison, have a look at my patch to achieve > the same that I never submitted (in particular, the author date): > https://github.com/j6t/git/commit/2327526213bc2fc3c109c1d8b4b0d95032346ff0 > > This implementation is more complete than mine. I like it. I like the way your other_merge_candidate() loops over an array of possible candidates, which makes it a lot easier to extend, though, admittedly the "die()" message needs auto-generating if we really wanted to make it maintenance free ;-) Thanks.
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.
> > 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/
Michael Lohmann <mi.al.lohmann@gmail.com> writes: > Almost, but not quite: "git log —merge" only shows the commits touching the > conflict, so it would be equivalent to (I think): > > git log HEAD CHERRY_PICK_HEAD ^$(git merge-base HEAD CHERRY_PICK_HEAD) -- $(git diff --name-only --diff-filter=U --relative) > > (or replace CHERRY_PICK with one of the other actions) It can certainly _reduce_ the noise, but I am not sure if it works over the right history segment. Let me think aloud a bit. Let's imagine that in a history forked long time ago, O----O----O----O----X HEAD \ Z---Z---Z---Z---A---B CHERRY_PICK_HEAD where all commits modified the same path in question that you saw conflicts in when your "git cherry-pick B" stopped. I do not know what to think about the changes to that path by the commits denoted by 'O', but the changes done to the path by commits denoted by 'Z' should have absolutely no relevance, as the whole point of cherry-picking B relative to A is because we do not care about what Zs did, no? For that matter, given that how we structure the 3-way merge for such a cherry-pick to "move from X the same way as you would move from A to B", how you got to X (i.e. Os) should not matter, either. On the other hand, such a conflict may arise from the fact that Os and Zs made changes differently to cause the contents of the path at X and A differ significantly. So, OK, I can buy your argument that what Os and Zs to the conflicted path did can be interesting when understanding the conflict during such a cherry-pick. >> Indeed there HEAD and CHERRY_PICK_HEAD may not share a common ancestor. > > True - but same for MERGE_HEAD ("git merge --allow-unrelated-histories"). I But that is very different, isn't it? Merging two unrelated histories is like merging two histories where the common ancestor had an empty tree, i.e. o---o---o---X HEAD / (v) an imaginary ancestor with an empty tree \ o---o---o---O MERGE_HEAD so it is a reasonable degenerated behaviour to consider what all commits on both sides did to the paths in question.
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/
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);
This extends the functionality of `git log --merge` to also work with conflicts for rebase, cherry pick and revert. Signed-off-by: Michael Lohmann <mi.al.lohmann@gmail.com> --- Hi everybody! When Phillip Wood gave me some nice hints on his workflow concerning conflicts [1] (we discussed if `--show-current-patch` would make sense for cherry pick/revert). When I learned about `git log --merge` I was a bit sad to discover that those do not exist for rebase/revert/cherry pick since they look really valuable for getting an understanding on what was changed. It is basically the counterpart to `git show ${ACTION}_HEAD` for understanding the source of the conflict. I am curious if also other people would be interested in having an easy way to get a log of only the relevant commits touching conflicting files for said actions. With this patch I think the functionality is there, just hijacking the `--merge` code - maybe an alias like `git log --conflict` or similar might be more descriptive now? What do you think about this idea? (Or am I maybe missing an easy way to achieve it with the existing code as well?) Michael [1] https://lore.kernel.org/git/cfba7098-3c23-4a81-933c-b7fefdedec8a@gmail.com/ revision.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)