Re: [PATCH] rebase -i: introduce `pick -x` to add "cherry picked from commit ..."
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 5, 2026, 18:58 UTC
- Message-ID
- <xmqqldbpclhh.fsf@gitster.g>
- In-Reply-To
- <20260705140931.98262-2-tg@trevorgross.com>
Trevor Gross <tg@trevorgross.com> writes:
First, I have to say that I personally am not a huge fan of these "cherry picked from..." messages.
Especially because I was the one who initially introduced them and enabled it as the default behaviour, and it turned out that people really hated to see them (and rightfully so, given that the original commit object were often not available to them) so much that they threw raw eggs at me until I made it disabled by default.
Oh, the raw egg part is an exaggeration, but it was a traumatic experience for me nevertheless ;-)
Anyway, let's see what we have here.
> Of note is that rebase will fastforward wherever possible, meaning the > check for TODO_RECORD_ORIGIN doesn't get hit and the message will not > get amended. This differs from the cherry-pick logic, which will add > "cherry picked from ..." even if a rewrite isn't otherwise necessary.
Why should it behave differently? Ease of implementation, or are there inherent design reasons behind this difference (if so that needs to be described here).
> +Similar to `git cherry-pick`, `-x` can be specified to append a "(cherry > +picked from commit …)" line to the commit body if the the commit base > +changes. That is, the following todo list:
"the the".
Show 5 quoted lines
> + > +-------------- > +pick 123456 -x > +edit 654321 -x > +--------------
You do not mean "$verb -x 123456" (where verb in (pick, edit))?
The help text seems to contradict with the above.
Show 11 quoted lines
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 809f76a87b..6a86ab5a94 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -47,9 +47,9 @@ void append_todo_help(int command_count,
> struct strbuf *buf)
> {
> const char *msg = _("\nCommands:\n"
> +"p, pick [ -x ] <commit> = use commit\n"
> +"r, reword [ -x ] <commit> = use commit, but edit the commit message\n"
> +"e, edit [ -x ] <commit> = use commit, but stop for amending\n"So presumably the documentation part needs fixing?
Show 9 quoted lines
> @@ -2758,6 +2759,14 @@ static int parse_insn_line(struct repository *r, struct replay_opts *opts,
> return error(_("missing arguments for %s"),
> command_to_string(item->command));
>
> + if (item->command == TODO_PICK || item->command == TODO_REWORD ||
> + item->command == TODO_EDIT) {
> + if (skip_prefix(bol, "-x", &bol)) {
> + bol += strspn(bol, " \t");
> + item->flags |= TODO_RECORD_ORIGIN;"pick -xabcdef 123456 commit title"
is parsed just like "pick -x" but somewhere downstream it would fail to pick up the commit object name and barf, with something like "'abcdef' is not a commit object name"? Or worse, do we mistake it as picking commit abcdef whose title is "123456 commit title"?
In any case, since a valid <commit> will never begin with '-', we should be able to design/implement a much better error checking here.
Show 6 quoted lines
> @@ -5524,7 +5533,7 @@ static int single_pick(struct repository *r,
> struct replay_opts *opts)
> {
> int check_todo;
> - struct todo_item item;
> + struct todo_item item = { 0 };This may be a good change, but I do not think the proposed commit log message touched upon it. It should. Is it a bug that we somehow were lucky that nobody made an access to uninitialized piece of memory here?
Show 9 quoted lines
> @@ -6340,6 +6349,12 @@ static void todo_list_to_strbuf(struct repository *r,
> short_commit_name(r, item->commit) :
> oid_to_hex(&item->commit->object.oid);
>
> + if (item->command == TODO_PICK || item->command == TODO_EDIT ||
> + item->command == TODO_REWORD) {
> + if (item->flags & TODO_RECORD_ORIGIN)
> + strbuf_addstr(buf, " -x");
> + }Why two nested conditional, instead of
if ((item->command == ... || item->command == ... || item->command == ...) && (item->flags & RECORD_ORIGIN)) add " -x";
?