Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> OK here is a less constroversal attempt to add new trailers. Instead
> of changing the default behavior (which could be done incrementally
> later), this patch simply adds a new option --append-trailer to revert
> and cherry-pick.

I almost agree, except that the the textual description in a revert
and the funny-looking trailer-wannabe in a cherry-pick are two
fundamentally different things, and adding duplicate to the latter
does not make much sense, while keeping both does make sense for the
former.

> PS. maybe --append-trailer is too generic? We have --signoff which is
> also a trailer. But that one carries more weights than just "machine
> generated tags".

> +		if (opts->append_trailer) {
> +			strbuf_addstr(&msgbuf, "\n");
> +			if (parent_id != -1)
> +				strbuf_addf(&msgbuf, "Reverts: %s~%d\n",
> +					    oid_to_hex(&commit->object.oid),
> +					    parent_id);
> +			else
> +				strbuf_addf(&msgbuf, "Reverts: %s\n",
> +					    oid_to_hex(&commit->object.oid));
> +		}

Makes sense, I guess.

> @@ -1780,14 +1792,28 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
>  		if (find_commit_subject(msg.message, &p))
>  			strbuf_addstr(&msgbuf, p);
>  
> -		if (opts->record_origin) {
> +		if (opts->record_origin || opts->append_trailer) {
>  			strbuf_complete_line(&msgbuf);
>  			if (!has_conforming_footer(&msgbuf, NULL, 0))
>  				strbuf_addch(&msgbuf, '\n');
> +		}
> +
> +		if (opts->record_origin) {
>  			strbuf_addstr(&msgbuf, cherry_picked_prefix);
>  			strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
>  			strbuf_addstr(&msgbuf, ")\n");
>  		}
> +		if (opts->append_trailer) {

These should be either-or, I would think, as adding exactly the same
piece of information in two different machine-readable form does not
make much sense.  The command line parser may even want to make sure
that both are not given at the same time.

Alternatively, we can keep using -x as "we are going to record where
the change was cherry-picked from" and use .record_origin to store
that bit, and use the new .append_trailer bit as "if we are recording
the origin, in which format between the old and the new do we do
so?" bit.

I think the latter makes more sense, at least to me.


> +			if (opts->record_origin)
> +				strbuf_addstr(&msgbuf, "\n");
> +			if (parent_id != -1)
> +				strbuf_addf(&msgbuf, "Cherry-picked-from: %s~%d\n",
> +					    oid_to_hex(&commit->object.oid),
> +					    parent_id);
> +			else
> +				strbuf_addf(&msgbuf, "Cherry-picked-from: %s\n",
> +					    oid_to_hex(&commit->object.oid));
> +		}