Re: [PATCH/RFC] Support --append-trailer in cherry-pick and revert
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 7, 2018, 00:31 UTC
- Message-ID
- <xmqq4lctenjo.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20181106171637.15562-1-pclouds@gmail.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".
Show 10 quoted lines
> + 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.
Show 17 quoted lines
> @@ -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.
Show 10 quoted lines
> + 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)); > + }