| Message ID | 20181106171637.15562-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH/RFC] Support --append-trailer in cherry-pick and revert | expand |
On Tue, Nov 06 2018, Nguyễn Thái Ngọc Duy wrote: > 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. > > Both will show either > > Reverts: <commit>[~<num>] > > or > > Cherry-picked-from: <commit>[~<num>] > > --append-trailer could be added to more commands (e.g. merge) that > generate commit messages if they have something for machine > consumption. > > After this, perhaps we could have a config key to turn this on by > default (for revert; for cherry-pick it will turn off "-x" too). Then > after a couple releases, the we got good reception, we'll make it > default? > > No tests, no proper commit message since I think we're still going to > discuss a bit more before settling down. > > 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". > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> The implementation looks fine to me, but as noted in https://public-inbox.org/git/8736se6qyc.fsf@evledraar.gmail.com/ I wonder what the plausible end-game is. That we'll turn this on by default in a few years, and then you can only worry about git-interpret-trailers for repos created as of 2020 or something? Otherwise it seems we'll need to *also* parse out the existing messages we've added.
On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote: > The implementation looks fine to me, but as noted in > https://public-inbox.org/git/8736se6qyc.fsf@evledraar.gmail.com/ I > wonder what the plausible end-game is. That we'll turn this on by > default in a few years, and then you can only worry about > git-interpret-trailers for repos created as of 2020 or something? > > Otherwise it seems we'll need to *also* parse out the existing messages > we've added. Could we help the reading scripts by normalizing old and new output via interpret-trailers, %(trailers), etc? I think "(cherry picked from ...)" is already considered a trailer by the trailer code. If the caller instructs us to, we could probably rewrite it to: Cherry-picked-from: ... in the output. Then the end-game is that scripts should just use interpret-trailers, etc, and old and new commits will Just Work. -Peff
On Tue, Nov 06, 2018 at 05:11:18PM -0500, Jeff King wrote: > On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote: > > > The implementation looks fine to me, but as noted in > > https://public-inbox.org/git/8736se6qyc.fsf@evledraar.gmail.com/ I > > wonder what the plausible end-game is. That we'll turn this on by > > default in a few years, and then you can only worry about > > git-interpret-trailers for repos created as of 2020 or something? > > > > Otherwise it seems we'll need to *also* parse out the existing messages > > we've added. > > Could we help the reading scripts by normalizing old and new output via > interpret-trailers, %(trailers), etc? > > I think "(cherry picked from ...)" is already considered a trailer by > the trailer code. If the caller instructs us to, we could probably > rewrite it to: > > Cherry-picked-from: ... > > in the output. Then the end-game is that scripts should just use > interpret-trailers, etc, and old and new commits will Just Work. I.e., something like this: diff --git a/trailer.c b/trailer.c index 0796f326b3..491c7c240e 100644 --- a/trailer.c +++ b/trailer.c @@ -46,9 +46,11 @@ static int configured; #define TRAILER_ARG_STRING "$ARG" +#define CHERRY_PICK_PREFIX "(cherry picked from commit " + static const char *git_generated_prefixes[] = { "Signed-off-by: ", - "(cherry picked from commit ", + CHERRY_PICK_PREFIX, NULL }; @@ -1141,6 +1143,8 @@ static void format_trailer_info(struct strbuf *out, for (i = 0; i < info->trailer_nr; i++) { char *trailer = info->trailers[i]; ssize_t separator_pos = find_separator(trailer, separators); + const char *hex; + struct object_id oid; if (separator_pos >= 1) { struct strbuf tok = STRBUF_INIT; @@ -1154,6 +1158,12 @@ static void format_trailer_info(struct strbuf *out, strbuf_release(&tok); strbuf_release(&val); + } else if (/* should check opts->normalize_cherry_pick */1 && + skip_prefix(trailer, CHERRY_PICK_PREFIX, &hex) && + !get_oid_hex(hex, &oid)) { + strbuf_addf(out, "Cherry-picked-from: %s\n", + oid_to_hex(&oid)); + } else if (!opts->only_trailers) { strbuf_addstr(out, trailer); } That would need to add the normalize_cherry_pick flag to the options struct, and then plumb it through. But it's enough to play around with, and: git log --format='%h%n%(trailers:only)' works. -Peff
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)); > + }
Jeff King <peff@peff.net> writes: > Could we help the reading scripts by normalizing old and new output via > interpret-trailers, %(trailers), etc? > > I think "(cherry picked from ...)" is already considered a trailer by > the trailer code. ;-) Great minds think alike, I guess. I think it is a great idea to ask interpret-trailers to help us here.
On Tue, Nov 6, 2018 at 11:11 PM Jeff King <peff@peff.net> wrote: > > On Tue, Nov 06, 2018 at 06:48:22PM +0100, Ævar Arnfjörð Bjarmason wrote: > > > The implementation looks fine to me, but as noted in > > https://public-inbox.org/git/8736se6qyc.fsf@evledraar.gmail.com/ I > > wonder what the plausible end-game is. That we'll turn this on by > > default in a few years, and then you can only worry about > > git-interpret-trailers for repos created as of 2020 or something? > > > > Otherwise it seems we'll need to *also* parse out the existing messages > > we've added. > > Could we help the reading scripts by normalizing old and new output via > interpret-trailers, %(trailers), etc? > > I think "(cherry picked from ...)" is already considered a trailer by > the trailer code. If the caller instructs us to, we could probably > rewrite it to: > > Cherry-picked-from: ... > > in the output. Then the end-game is that scripts should just use > interpret-trailers, etc, and old and new commits will Just Work. There is still one thing to settle. "revert -m1" could produce something like this This reverts commit <SHA1>, reversing changes made to <SHA2>. My proposal produces this Reverts: <SHA1>^2 And I can't really convert the former to latter without accessing object database (probably not a good idea?) to check if SHA2 is the second parent of SHA1. So either - I access object database anyway - Generate just "Reverts: <SHA1>" (i.e. losing info) with interpret-trailers - Change Reverts: tag to a different output format, or maybe use two tags instead.
On Wed, Nov 07, 2018 at 04:30:38PM +0100, Duy Nguyen wrote: > > Could we help the reading scripts by normalizing old and new output via > > interpret-trailers, %(trailers), etc? > > > > I think "(cherry picked from ...)" is already considered a trailer by > > the trailer code. If the caller instructs us to, we could probably > > rewrite it to: > > > > Cherry-picked-from: ... > > > > in the output. Then the end-game is that scripts should just use > > interpret-trailers, etc, and old and new commits will Just Work. > > There is still one thing to settle. "revert -m1" could produce > something like this > > This reverts commit <SHA1>, reversing > changes made to <SHA2>. > > My proposal produces this > > Reverts: <SHA1>^2 > > And I can't really convert the former to latter without accessing > object database (probably not a good idea?) to check if SHA2 is the > second parent of SHA1. So either > > - I access object database anyway > - Generate just "Reverts: <SHA1>" (i.e. losing info) with interpret-trailers > - Change Reverts: tag to a different output format, or maybe use two > tags instead. IMHO the revert case is way less interesting for automated parsing. In a workflow like Git's, cherry-picks aren't very common, but there _are_ workflows where there's a lot of cherry-picking between dev/release branches, and automated analysis is useful there. Whereas for revert, it's almost always a human-scale thing. A commit was bad, so you revert it. The annotation is useful if you're digging, but it's not generally going to be a fundamental part of a workflow. And it's not really any different than fixing a bug later. And I think that's reflected in the way we just casually stick the reverted oid in the human-readable part of the commit message (and the lack of any tools to parse it). So IMHO it would be OK to treat this less carefully than the cherry-pick case. -Peff
Duy Nguyen <pclouds@gmail.com> writes: > There is still one thing to settle. "revert -m1" could produce > something like this > > This reverts commit <SHA1>, reversing > changes made to <SHA2>. I do not think it is relevant, with or without multiple parents, to even attempt to read this message. The description is not meant to be machine readable/parseable, but is meant to be updated to describe the reason why the reversion was made for human readers. Spending any cycle to attempt interpreting it by machines will give a wrong signal to encourage people not to touch it. Instead we should actively encourage people to take that as the beginning of their description. I even suspect that an update to that message to read something like these "This reverts commit <SHA-1> because FILL IN THE REASONS HERE" "This reverts commit <SHA-1>, reversing changes made to <SHA-1>, because FILL IN THE REASONS HERE" would be a good idea. It of course is orthogonal to the topic of introducing a new footer to record the "what happened" (without the "why") in a machine-readable way.
On Thu, Nov 08, 2018 at 09:36:56AM +0900, Junio C Hamano wrote: > Duy Nguyen <pclouds@gmail.com> writes: > > > There is still one thing to settle. "revert -m1" could produce > > something like this > > > > This reverts commit <SHA1>, reversing > > changes made to <SHA2>. > > I do not think it is relevant, with or without multiple parents, to > even attempt to read this message. > > The description is not meant to be machine readable/parseable, but > is meant to be updated to describe the reason why the reversion was > made for human readers. Spending any cycle to attempt interpreting > it by machines will give a wrong signal to encourage people not to > touch it. Instead we should actively encourage people to take that > as the beginning of their description. > > I even suspect that an update to that message to read something like > these > > "This reverts commit <SHA-1> because FILL IN THE REASONS HERE" > > "This reverts commit <SHA-1>, reversing changes made to > <SHA-1>, because FILL IN THE REASONS HERE" Yeah, that was the point I was trying to make earlier today in this thread. I really think of this as a human-readable message. But as far as how this impacts the addition of a trailer: once we have a machine-readable "Reverts:", people naturally may want to know about "Reverts" for older commits. Our options are: 1. say "sorry, there was no machine-readable version back then" 2. try to parse our "This reverts" message and normalize it into "Reverts" for their benefit. Before introducing the new footer, it's worth thinking about the end-game here. If we do (1), then people may want to parse themselves. They are stuck parsing both the old and new (to handle old and new commits). We could make life a little easier on them if we included _both_ the English text and the machine-readable version. And then if they just chose to parse the English, it would work for old and new. But I guess that is really just a losing battle, if we consider the English to be changeable. And doing it ourselves in (2) is really just pushing that losing battle onto ourselves. So if we are comfortable with saying that this is a new feature to have the machine-readable trailer version, and there isn't a robust way to get historical revert information (because there really isn't[1]), then I think we can just punt on any kind of trailer-normalization magic. -Peff [1] Thinking back on reverts I have done, they are often _not_ straight-up reverts. For example, I may end up dropping half of a commit, but leaving some traces of it in place in order to build up the correct solution on top (i.e., fixing whatever problem caused me to revert in the first place). I list those as "this is morally a revert of 1234abcd...", which is definitely not machine readable. ;)
Jeff King <peff@peff.net> writes: > So if we are comfortable with saying that this is a new feature to have > the machine-readable trailer version, and there isn't a robust way to > get historical revert information (because there really isn't[1]), then > I think we can just punt on any kind of trailer-normalization magic. Yes, I do consider that the original suggestion was two-part - cherry-pick did have machine readable info, but by historical accident, it is shaped differently from "trailers", so we'd transition into the new format. - revert did not have machine readble info at all, so we are adding one, even though it is not that interesting as cherry-pick (for the reasons you stated in an earlier message in this thread). So my "honest answer" is your #1, "sorry, there was no machine-readable version back then", for reverts. We do not have such a problem with cherry-pick luckily ;-) > [1] Thinking back on reverts I have done, they are often _not_ > straight-up reverts. For example, I may end up dropping half of a > commit, but leaving some traces of it in place in order to build up > the correct solution on top (i.e., fixing whatever problem caused me > to revert in the first place). I list those as "this is morally a > revert of 1234abcd...", which is definitely not machine readable. ;) Yup, and it is debatable if it even makes sense to add the machine readable trailer for such a commit. A human-made claim that it is a "moral equivalent of reverting X" may not look any different from a "textual revert of X" to a machine, but the actual patch text would be quite different---unless the machine reading it understands "moral equivalence", letting it blindly take on faith whatever humans say may not be a good idea.
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index d35d771fc8..b5dff29ead 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -102,6 +102,12 @@ effect to your index in a row. Add Signed-off-by line at the end of the commit message. See the signoff option in linkgit:git-commit[1] for more information. +--append-trailer:: + When recording a commit, append a "Cherry-picked-from:" line + with object name of the cherry-picked commit. If a merge is + cherry-picked with `-m`, the extended SHA-1 syntax is used + to indicate the side of the merge to be cherry-picked. + -S[<keyid>]:: --gpg-sign[=<keyid>]:: GPG-sign commits. The `keyid` argument is optional and diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt index 837707a8fd..e08010b200 100644 --- a/Documentation/git-revert.txt +++ b/Documentation/git-revert.txt @@ -91,6 +91,12 @@ effect to your index in a row. Add Signed-off-by line at the end of the commit message. See the signoff option in linkgit:git-commit[1] for more information. +--append-trailer:: + When recording a commit, append a "Cherry-picked-from:" line + with object name of the cherry-picked commit. If a merge is + cherry-picked with `-m`, the extended SHA-1 syntax is used + to indicate the side of the merge to be cherry-picked. + --strategy=<strategy>:: Use the given merge strategy. Should only be used once. See the MERGE STRATEGIES section in linkgit:git-merge[1] diff --git a/builtin/revert.c b/builtin/revert.c index c93393c89b..3bd8f57b90 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -119,6 +119,7 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) if (opts->action == REPLAY_PICK) { struct option cp_extra[] = { OPT_BOOL('x', NULL, &opts->record_origin, N_("append commit name")), + OPT_BOOL(0, "append-trailer", &opts->append_trailer, N_("record cherry picked commit as trailer")), OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")), OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")), OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")), @@ -126,6 +127,12 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) OPT_END(), }; options = parse_options_concat(options, cp_extra); + } else if (opts->action == REPLAY_REVERT) { + struct option cp_extra[] = { + OPT_BOOL(0, "append-trailer", &opts->append_trailer, N_("record reverted commit as trailer")), + OPT_END(), + }; + options = parse_options_concat(options, cp_extra); } argc = parse_options(argc, argv, NULL, options, usage_str, diff --git a/sequencer.c b/sequencer.c index 9e1ab3a2a7..e8fa307109 100644 --- a/sequencer.c +++ b/sequencer.c @@ -911,7 +911,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts, if ((flags & EDIT_MSG)) argv_array_push(&cmd.args, "-e"); else if (!(flags & CLEANUP_MSG) && - !opts->signoff && !opts->record_origin && + !opts->signoff && !opts->record_origin && !opts->append_trailer && git_config_get_value("commit.cleanup", &value)) argv_array_push(&cmd.args, "--cleanup=verbatim"); @@ -1669,7 +1669,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, char *author = NULL; struct commit_message msg = { NULL, NULL, NULL, NULL }; struct strbuf msgbuf = STRBUF_INIT; - int res, unborn = 0, allow; + int res, unborn = 0, allow, parent_id = -1; if (opts->no_commit) { /* @@ -1716,6 +1716,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, return error(_("commit %s does not have parent %d"), oid_to_hex(&commit->object.oid), opts->mainline); parent = p->item; + parent_id = cnt; } else if (0 < opts->mainline) return error(_("mainline was specified but commit %s is not a merge."), oid_to_hex(&commit->object.oid)); @@ -1768,6 +1769,17 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, strbuf_addstr(&msgbuf, oid_to_hex(&parent->object.oid)); } strbuf_addstr(&msgbuf, ".\n"); + + 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)); + } } else { const char *p; @@ -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) { + 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)); + } if (!is_fixup(command)) author = get_author(msg.message); } @@ -2227,6 +2253,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data) opts->signoff = git_config_bool_or_int(key, value, &error_flag); else if (!strcmp(key, "options.record-origin")) opts->record_origin = git_config_bool_or_int(key, value, &error_flag); + else if (!strcmp(key, "options.append-trailer")) + opts->append_trailer = git_config_bool_or_int(key, value, &error_flag); else if (!strcmp(key, "options.allow-ff")) opts->allow_ff = git_config_bool_or_int(key, value, &error_flag); else if (!strcmp(key, "options.mainline")) @@ -2618,6 +2646,8 @@ static int save_opts(struct replay_opts *opts) res |= git_config_set_in_file_gently(opts_file, "options.signoff", "true"); if (opts->record_origin) res |= git_config_set_in_file_gently(opts_file, "options.record-origin", "true"); + if (opts->append_trailer) + res |= git_config_set_in_file_gently(opts_file, "options.append-trailer", "true"); if (opts->allow_ff) res |= git_config_set_in_file_gently(opts_file, "options.allow-ff", "true"); if (opts->mainline) { @@ -3432,7 +3462,8 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) setenv(GIT_REFLOG_ACTION, action_name(opts), 0); if (opts->allow_ff) assert(!(opts->signoff || opts->no_commit || - opts->record_origin || opts->edit)); + opts->record_origin || opts->append_trailer || + opts->edit)); if (read_and_refresh_cache(opts)) return -1; diff --git a/sequencer.h b/sequencer.h index 660cff5050..7d7c1fe6b4 100644 --- a/sequencer.h +++ b/sequencer.h @@ -31,6 +31,7 @@ struct replay_opts { /* Boolean options */ int edit; int record_origin; + int append_trailer; int no_commit; int signoff; int allow_ff;
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. Both will show either Reverts: <commit>[~<num>] or Cherry-picked-from: <commit>[~<num>] --append-trailer could be added to more commands (e.g. merge) that generate commit messages if they have something for machine consumption. After this, perhaps we could have a config key to turn this on by default (for revert; for cherry-pick it will turn off "-x" too). Then after a couple releases, the we got good reception, we'll make it default? No tests, no proper commit message since I think we're still going to discuss a bit more before settling down. 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". Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- Documentation/git-cherry-pick.txt | 6 +++++ Documentation/git-revert.txt | 6 +++++ builtin/revert.c | 7 ++++++ sequencer.c | 39 +++++++++++++++++++++++++++---- sequencer.h | 1 + 5 files changed, 55 insertions(+), 4 deletions(-)