| Message ID | 20240225-ml-log-merge-with-cherry-pick-and-other-pseudo-heads-v5-2-af1ef2d9e44d@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Implement `git log --merge` also for rebase/cherry-pick/revert | expand |
Philippe Blain <levraiphilippeblain@gmail.com> writes: > + for (i = 0; i < ARRAY_SIZE(other_head); i++) > + if (!read_ref_full(other_head[i], > + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, > + oid, NULL)) { > + if (is_null_oid(oid)) > + die(_("%s is a symbolic ref?"), other_head[i]); > + return other_head[i]; > + } > + > + die(_("--merge requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD")); > +} Just a minor nit, but reacting to recent "passive-aggressive" message change in another thread, perhaps we should stop asking a rhetorical question like the new message and instead state what we detected and what we consider is an error condition as a fact in them. The last die() in the above helper function used to be such a rhetorical question "--merge without HEAD?" but now it reads much better. The one about symbolic ref is new in this series, and we can avoid making it rhetorical from the get go. Perhaps "%s exists but it is a symbolic ref" or something?
Hi Junio, Le 2024-02-25 à 23:35, Junio C Hamano a écrit : > Philippe Blain <levraiphilippeblain@gmail.com> writes: > >> + for (i = 0; i < ARRAY_SIZE(other_head); i++) >> + if (!read_ref_full(other_head[i], >> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, >> + oid, NULL)) { >> + if (is_null_oid(oid)) >> + die(_("%s is a symbolic ref?"), other_head[i]); >> + return other_head[i]; >> + } >> + >> + die(_("--merge requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD")); >> +} > > Just a minor nit, but reacting to recent "passive-aggressive" > message change in another thread, perhaps we should stop asking a > rhetorical question like the new message and instead state what we > detected and what we consider is an error condition as a fact in > them. > > The last die() in the above helper function used to be such a > rhetorical question "--merge without HEAD?" but now it reads much > better. The one about symbolic ref is new in this series, and we > can avoid making it rhetorical from the get go. Perhaps "%s exists > but it is a symbolic ref" or something? Ok, I can make that change. I agree we should maybe keep these rethorical questions to 'BUG' calls... Thanks, Philippe.
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 2bf239ff03..9ce7a5eedc 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -341,8 +341,11 @@ See also linkgit:git-reflog[1]. Under `--pretty=reference`, this information will not be shown at all. --merge:: - After a failed merge, show refs that touch files having a - conflict and don't exist on all heads to merge. + Show commits touching conflicted paths in the range `HEAD...<other>`, + where `<other>` is the first existing pseudoref in `MERGE_HEAD`, + `CHERRY_PICK_HEAD`, `REVERT_HEAD` or `REBASE_HEAD`. Only works + when the index has unmerged entries. This option can be used to show + relevant commits when resolving conflicts from a 3-way merge. --boundary:: Output excluded boundary commits. Boundary commits are diff --git a/revision.c b/revision.c index ee26988cc6..a90a6f861b 100644 --- a/revision.c +++ b/revision.c @@ -1961,11 +1961,31 @@ static void add_pending_commit_list(struct rev_info *revs, } } +static const char *lookup_other_head(struct object_id *oid) +{ + int i; + static const char *const other_head[] = { + "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD" + }; + + for (i = 0; i < ARRAY_SIZE(other_head); i++) + if (!read_ref_full(other_head[i], + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, + oid, NULL)) { + if (is_null_oid(oid)) + die(_("%s is a symbolic ref?"), other_head[i]); + return other_head[i]; + } + + die(_("--merge requires one of the pseudorefs MERGE_HEAD, CHERRY_PICK_HEAD, REVERT_HEAD or REBASE_HEAD")); +} + static void prepare_show_merge(struct rev_info *revs) { struct commit_list *bases; struct commit *head, *other; struct object_id oid; + const char *other_name; const char **prune = NULL; int i, prune_num = 1; /* counting terminating NULL */ struct index_state *istate = revs->repo->index; @@ -1973,15 +1993,10 @@ static void prepare_show_merge(struct rev_info *revs) if (repo_get_oid(the_repository, "HEAD", &oid)) die("--merge without HEAD?"); head = lookup_commit_or_die(&oid, "HEAD"); - if (read_ref_full("MERGE_HEAD", - RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - &oid, NULL)) - die("--merge without MERGE_HEAD?"); - if (is_null_oid(&oid)) - die(_("MERGE_HEAD is a symbolic ref?")); - other = lookup_commit_or_die(&oid, "MERGE_HEAD"); + other_name = lookup_other_head(&oid); + other = lookup_commit_or_die(&oid, other_name); add_pending_object(revs, &head->object, "HEAD"); - add_pending_object(revs, &other->object, "MERGE_HEAD"); + add_pending_object(revs, &other->object, other_name); bases = repo_get_merge_bases(the_repository, head, other); add_rev_cmdline_list(revs, bases, REV_CMD_MERGE_BASE, UNINTERESTING | BOTTOM); add_pending_commit_list(revs, bases, UNINTERESTING | BOTTOM);