Re: [PATCH] rebase -i: introduce `pick -x` to add "cherry picked from commit ..."

[Junio C Hamano <gitster@pobox.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".

> +
> +--------------
> +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.

> 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?

> @@ -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.

> @@ -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?

> @@ -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";

?