| Message ID | 42590e95deeece6ba65e0432c3a59746e717fee3.1616066156.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | commit: add --trailer option | expand |
On 2021-03-18 11:15:55+0000, ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > From: ZheNing Hu <adlternative@gmail.com> > > Beacuse `git commit --trailer="Signed-off-by: \ s/Beacuse/Because/ And I think, it's easier to read if we write the command in its own (indented) line. > $(git config user.name) <$(git config user.email)>"` > is difficult for users to add their own identities, > so teach interpret-trailers a new option `--own-identity` > which allow those trailers with no value add the user’s own > identity. This will help the use of `commit --trailer` as > easy as `--signoff`. Perhap, saying that we're optionalise <value> in --trailer, by substitute user's identity if missing instead? > @@ -131,6 +144,7 @@ OPTIONS > when you know your input contains just the commit message itself > (and not an email or the output of `git format-patch`). > > + I think it's better to not add this line change > CONFIGURATION VARIABLES > ----------------------- > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c > index 84748eafc01b..be7f502a58d7 100644 > --- a/builtin/interpret-trailers.c > +++ b/builtin/interpret-trailers.c
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: ZheNing Hu <adlternative@gmail.com> > > Beacuse `git commit --trailer="Signed-off-by: \ > $(git config user.name) <$(git config user.email)>"` > is difficult for users to add their own identities, > so teach interpret-trailers a new option `--own-identity` > which allow those trailers with no value add the user’s own > identity. This will help the use of `commit --trailer` as > easy as `--signoff`. I have a suspicion that this is too narrowly focused to be useful in practice, and I find that the proposed "--own-identity" is quite a mouthful. > +--own-identity:: > + Used with `--trailer`. Those trailers without value with the > + `--own-identity` option all will add the user's own identity. So, the assumption here is that the name of the trailer tag alone, without the ':' separator, can identify which trailer the user is talking about, and it can be distinguished from the tag name plus ':' and nothing else which is calling for a trailer entry with an empty string as its value? OK. The reason why this looks too narrowly focused on oneself alone to be useful to me is because I often need to add various -by trailers to incoming patches, and have a script to do exactly that (which does not use interpret-trailers, as I do not think interpret-trailers can accept a patch email as its input, and the script predates interpret-trailers) but it will be useless if that script were limited to add -by for myself. Wouldn't it be a lot more useful if git commit --trailer="Helped-by:@Ch.*Couder" is expanded (note: I am not married to the syntax, but only for illustration purposes, I am using "a value prefixed by @ triggers the 'name expansion'" convention in this example. People can come up with better convention) by finding an author or a committer whose name matches the given pattern from "git log"? Then, instead of git commit --own-identity --trailer=Signed-off-by I can say git commit --trailer=Signed-off-by:@gitster and I can even add more than one, e.g. git commit --trailer=Helped-by:@peff --trailer=Signed-off-by:@gitster
Đoàn Trần Công Danh <congdanhqx@gmail.com> 于2021年3月19日周五 上午12:45写道： > > On 2021-03-18 11:15:55+0000, ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: ZheNing Hu <adlternative@gmail.com> > > > > Beacuse `git commit --trailer="Signed-off-by: \ > > s/Beacuse/Because/ > > And I think, it's easier to read if we write the command in its own > (indented) line. > > > $(git config user.name) <$(git config user.email)>"` > > is difficult for users to add their own identities, > > so teach interpret-trailers a new option `--own-identity` > > which allow those trailers with no value add the user’s own > > identity. This will help the use of `commit --trailer` as > > easy as `--signoff`. > > Perhap, saying that we're optionalise <value> in --trailer, by > substitute user's identity if missing instead? Indeed so. > > > @@ -131,6 +144,7 @@ OPTIONS > > when you know your input contains just the commit message itself > > (and not an email or the output of `git format-patch`). > > > > + > > I think it's better to not add this line change > > > CONFIGURATION VARIABLES > > ----------------------- > > > > diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c > > index 84748eafc01b..be7f502a58d7 100644 > > --- a/builtin/interpret-trailers.c > > +++ b/builtin/interpret-trailers.c > Thanks for these kindful advices. > -- > Danh -- ZheNing Hu
Junio C Hamano <gitster@pobox.com> 于2021年3月19日周五 上午3:20写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: ZheNing Hu <adlternative@gmail.com> > > > > Beacuse `git commit --trailer="Signed-off-by: \ > > $(git config user.name) <$(git config user.email)>"` > > is difficult for users to add their own identities, > > so teach interpret-trailers a new option `--own-identity` > > which allow those trailers with no value add the user’s own > > identity. This will help the use of `commit --trailer` as > > easy as `--signoff`. > > I have a suspicion that this is too narrowly focused to be useful in > practice, and I find that the proposed "--own-identity" is quite a > mouthful. > Well, the original meaning of this `--trailer`and`--own-identity` option is to imitate the behavior of `--signoff` to create other trailers, For the time being, it can only "imitation", it does not yet support the ability to provide multiple identities instead of "own-identity". If `--own-identity` is mouthful, is there a better name? > > +--own-identity:: > > + Used with `--trailer`. Those trailers without value with the > > + `--own-identity` option all will add the user's own identity. > > So, the assumption here is that the name of the trailer tag alone, > without the ':' separator, can identify which trailer the user is > talking about, and it can be distinguished from the tag name plus ':' > and nothing else which is calling for a trailer entry with an empty > string as its value? > Yes. `--own-ident` is for trailers without a separator. > OK. > > The reason why this looks too narrowly focused on oneself alone to > be useful to me is because I often need to add various -by trailers > to incoming patches, and have a script to do exactly that (which > does not use interpret-trailers, as I do not think > interpret-trailers can accept a patch email as its input, and the > script predates interpret-trailers) but it will be useless if that > script were limited to add -by for myself. > I understand it. But I think `--own-ident` is more inclined to add identities for users. If users want to add other people’s identities, they need to use another method. > Wouldn't it be a lot more useful if > > git commit --trailer="Helped-by:@Ch.*Couder" > > is expanded (note: I am not married to the syntax, but only for > illustration purposes, I am using "a value prefixed by @ triggers > the 'name expansion'" convention in this example. People can come > up with better convention) by finding an author or a committer whose > name matches the given pattern from "git log"? Then, instead of > > git commit --own-identity --trailer=Signed-off-by > > I can say > > git commit --trailer=Signed-off-by:@gitster > > and I can even add more than one, e.g. > > git commit --trailer=Helped-by:@peff --trailer=Signed-off-by:@gitster > I agree that this idea can be extended again from "own-identity " mode to "multiple-identity mode",‘@’ seems to have a lot of important meanings in git, but I don’t know how to design a solution that is more suitable than yours for the time being. Another doubt is what happens if one person corresponds to multiple mailboxes or one mailbox corresponds to multiple people? I think `--own-ident` may be a somewhat "narrow" option that can be kept. Also add a new option for interpret-trailers to resolve the identity of other people? Thanks for suggestions.
ZheNing Hu <adlternative@gmail.com> writes:
> If `--own-identity` is mouthful, is there a better name?
I originally had "perhaps XXXX?" in the message you are responding
to, but I hoped that the message, especially the examples at the
end, would be sufficient to make you realize that the option itself
is not such a great idea (an additional ":@gitster" in whatever
syntax used would be even shorter than "--own-identity", and
obviously more flexible in that it can name other people).
If you really want to have this option, perhaps call it "--self"? I
still do not think it is a good idea, though.
Junio C Hamano <gitster@pobox.com> 于2021年3月19日周五 下午11:36写道： > > ZheNing Hu <adlternative@gmail.com> writes: > > > If `--own-identity` is mouthful, is there a better name? > > I originally had "perhaps XXXX?" in the message you are responding > to, but I hoped that the message, especially the examples at the > end, would be sufficient to make you realize that the option itself > is not such a great idea (an additional ":@gitster" in whatever > syntax used would be even shorter than "--own-identity", and > obviously more flexible in that it can name other people). > > If you really want to have this option, perhaps call it "--self"? I > still do not think it is a good idea, though. Fine, If this option is really not particularly useful, I am willing to give it up. I am looking for how to extract each author or committer from the log. I just know I can use: $ git log --pretty="%an %ae" | sort | uniq get all unique author <email> from a git repo. Is there any function in the source code of git to get them? I notice Ævar Arnfjörð Bjarmason mentioned `.mailmap`, I have seen the `.mailmap` under git repo. Would it be better to extract the author or committer from the .mailmap than to extract it from git log? If so, the operation of the trailer may depend on the contents of `.mailmap`. If there is no `.mailmap` in the user git repo, then the identity in the trailer cannot be parsed. Thanks.
On Sat, Mar 20, 2021 at 10:54:38AM +0800, ZheNing Hu wrote: > I am looking for how to extract each author or committer from the log. > I just know I can use: > > $ git log --pretty="%an %ae" | sort | uniq > > get all unique author <email> from a git repo. > Is there any function in the source code of git to get them? If I understand Junio's suggestion correctly, it is very similar to how "commit --author" works. See how it calls find_author_by_nickname(), which finds the first commit matching the name, and then pulls out the full name from format_commit_message(). -Peff
Jeff King <peff@peff.net> writes: > If I understand Junio's suggestion correctly, it is very similar to how > "commit --author" works. See how it calls find_author_by_nickname(), > which finds the first commit matching the name, and then pulls out the > full name from format_commit_message(). Yup. But I have to warn readers that it would not be a sane approach to simply expose find_author_by_nickname() from builtin/commit.c as if it is a generally reusable helper function. In a very limited context of "git commit", what the helper does is OK to run a single revision traversal without cleaning the parsed commit objects and object flag bits after finding a single commit. But generally, it would not be an approach that would scale (e.g. I do not know if we can expect to be able to call the helper function twice and get sensible results out of it).
Junio C Hamano <gitster@pobox.com> 于2021年3月20日周六 下午1:50写道： > > Jeff King <peff@peff.net> writes: > > > If I understand Junio's suggestion correctly, it is very similar to how > > "commit --author" works. See how it calls find_author_by_nickname(), > > which finds the first commit matching the name, and then pulls out the > > full name from format_commit_message(). > > Yup. > > But I have to warn readers that it would not be a sane approach to > simply expose find_author_by_nickname() from builtin/commit.c as if > it is a generally reusable helper function. In a very limited > context of "git commit", what the helper does is OK to run a single > revision traversal without cleaning the parsed commit objects and > object flag bits after finding a single commit. > > But generally, it would not be an approach that would scale (e.g. I > do not know if we can expect to be able to call the helper function > twice and get sensible results out of it). Hi, Jeff and Junio, `find_author_by_nickname` is indeed a good way to find the `author <email>` in the "last" commit that matches the specified name, but after testing, `find_author_by_nickname` is as Junio say, it is not reusable(use it twice, git will call die), but why?
In actually ,
@@ -1071,6 +1071,7 @@ static const char *find_author_by_nickname(const
char *name)
strbuf_release(&buf);
format_commit_message(commit, "%aN <%aE>", &buf, &ctx);
clear_mailmap(&mailmap);
+ reset_revision_walk();
return strbuf_detach(&buf, NULL);
}
then we can reuse this function.
But I think I can give find_author_by_nickname another arg for choice if we want
`reset_revision_walk()`.
Thanks, Junio and Jeff.
ZheNing Hu <adlternative@gmail.com> writes: > In actually , > > @@ -1071,6 +1071,7 @@ static const char *find_author_by_nickname(const > char *name) > strbuf_release(&buf); > format_commit_message(commit, "%aN <%aE>", &buf, &ctx); > clear_mailmap(&mailmap); > + reset_revision_walk(); > return strbuf_detach(&buf, NULL); > } > > then we can reuse this function. > > But I think I can give find_author_by_nickname another arg for choice if we want > `reset_revision_walk()`. That is half fixing and half breaking. It would allow us to call the helper number of times as long as there is no other revision traversal is in progress; calling reset_revision_walk() would mean any and all revision traversal in progress will be broken. So we cannot use the helper to tweak each and every commit we encounter while running "git log", for example. Imagine adding an option to "git format-patch" to allow each commit it formats to be tweaked by adding "--trailers=foo:@ZheNing" and the like. There is one primary traversal that is used to list which commit to format in what order, and every time that primary traversal yields a commit, if we run find_author_by_nickname(), we end up initiating another traversal, and then by calling reset, we clear all object flags that are used for revision traversal, thereby breaking the primary traversal. The only safe way to introduce a generally usable helper (without rewriting the revision traversal machinery) is to spawn a subprocess and do an equivalent of find_author_by_nickname() in it. A standalone "interpret-trailers" command, as long as it won't do any other revision traversal, would not have such a problem, and calling reset every time find_author_by_nickname() is called may be sufficient. The only thing I care about is *not* to pretend that find_author_by_nickname() plus reset() is the generally reusable helper function and advertise it as such, which will mislead future developers into misusing the function in a context they shouldn't (i.e. while they are performing their own revision traversal). Thanks.
Junio C Hamano <gitster@pobox.com> 于2021年3月20日周六 下午2:53写道： > > ZheNing Hu <adlternative@gmail.com> writes: > > > In actually , > > > > @@ -1071,6 +1071,7 @@ static const char *find_author_by_nickname(const > > char *name) > > strbuf_release(&buf); > > format_commit_message(commit, "%aN <%aE>", &buf, &ctx); > > clear_mailmap(&mailmap); > > + reset_revision_walk(); > > return strbuf_detach(&buf, NULL); > > } > > > > then we can reuse this function. > > > > But I think I can give find_author_by_nickname another arg for choice if we want > > `reset_revision_walk()`. > > That is half fixing and half breaking. It would allow us to call > the helper number of times as long as there is no other revision > traversal is in progress; calling reset_revision_walk() would mean > any and all revision traversal in progress will be broken. So we > cannot use the helper to tweak each and every commit we encounter > while running "git log", for example. Imagine adding an option to > "git format-patch" to allow each commit it formats to be tweaked by > adding "--trailers=foo:@ZheNing" and the like. There is one primary > traversal that is used to list which commit to format in what order, > and every time that primary traversal yields a commit, if we run > find_author_by_nickname(), we end up initiating another traversal, > and then by calling reset, we clear all object flags that are used > for revision traversal, thereby breaking the primary traversal. > I understand what you meaning. I may not be very thorough in considering the potential problems. This is as dangerous as calling `list_del` in the middle of `list_for_each`. > The only safe way to introduce a generally usable helper (without > rewriting the revision traversal machinery) is to spawn a subprocess > and do an equivalent of find_author_by_nickname() in it. > If I didn't notice what you mentioned later, I might think about it for a long time. Execute a child process specifically to execute `find_author_by_nickname()`, which feels a bit wasteful. > A standalone "interpret-trailers" command, as long as it won't do > any other revision traversal, would not have such a problem, and > calling reset every time find_author_by_nickname() is called may be > sufficient. The only thing I care about is *not* to pretend that > find_author_by_nickname() plus reset() is the generally reusable > helper function and advertise it as such, which will mislead future > developers into misusing the function in a context they shouldn't > (i.e. while they are performing their own revision traversal). > > Thanks. After listening to your previous explanation, I agree with this view. `interpret-trailers` alone as a process, as long as it does not traverse the revision externally, It is safe for us to call `reset_revision_walk()` after `find_author_by_nickname()` inside interpret-trailers. Thank you for your thoughts and answers.
diff --git a/Documentation/git-interpret-trailers.txt b/Documentation/git-interpret-trailers.txt index 96ec6499f001..ace44446c3e3 100644 --- a/Documentation/git-interpret-trailers.txt +++ b/Documentation/git-interpret-trailers.txt @@ -84,6 +84,19 @@ OPTIONS trailer to the input messages. See the description of this command. +--own-identity:: + Used with `--trailer`. Those trailers without value with the + `--own-identity` option all will add the user's own identity. + For example,` git interpret-trailers --trailer "A:B" --trailer \ + "Signed-off-by" --trailer "Helped-by" --own-identity --inplace a.txt` + will output: + " + A:B + Signed-off-by: C O Mitter <committer@example.com> + Helped-by: C O Mitter <committer@example.com> + " + in `a.txt`. + --where <placement>:: --no-where:: Specify where all new trailers will be added. A setting @@ -131,6 +144,7 @@ OPTIONS when you know your input contains just the commit message itself (and not an email or the output of `git format-patch`). + CONFIGURATION VARIABLES ----------------------- diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c index 84748eafc01b..be7f502a58d7 100644 --- a/builtin/interpret-trailers.c +++ b/builtin/interpret-trailers.c @@ -47,6 +47,7 @@ static void new_trailers_clear(struct list_head *trailers) list_for_each_safe(pos, tmp, trailers) { item = list_entry(pos, struct new_trailer_item, list); list_del(pos); + free(item->text); free(item); } } @@ -66,7 +67,7 @@ static int option_parse_trailer(const struct option *opt, return -1; item = xmalloc(sizeof(*item)); - item->text = arg; + item->text = xstrdup(arg); item->where = where; item->if_exists = if_exists; item->if_missing = if_missing; @@ -94,7 +95,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix) struct option options[] = { OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")), OPT_BOOL(0, "trim-empty", &opts.trim_empty, N_("trim empty trailers")), - + OPT_BOOL(0, "own-identity", &opts.own_identity, + N_("specify the user's own identity for omitted trailers value")), OPT_CALLBACK(0, "where", NULL, N_("action"), N_("where to place the new trailer"), option_parse_where), OPT_CALLBACK(0, "if-exists", NULL, N_("action"), diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh index 6602790b5f4c..f82cee93bfb2 100755 --- a/t/t7513-interpret-trailers.sh +++ b/t/t7513-interpret-trailers.sh @@ -63,6 +63,18 @@ test_expect_success 'without config' ' test_cmp expected actual ' +test_expect_success 'without config with --own-identity' ' + cat >expected <<-\EOF && + + Acked-by: A B <C> + Helped-by: C O Mitter <committer@example.com> + Signed-off-by: C O Mitter <committer@example.com> + EOF + git interpret-trailers --trailer "Acked-by: A B <C>" --trailer "Helped-by" \ + --trailer "Signed-off-by" --own-identity empty >actual && + test_cmp expected actual +' + test_expect_success 'without config in another order' ' sed -e "s/ Z\$/ /" >expected <<-\EOF && diff --git a/trailer.c b/trailer.c index 249ed618ed8e..cd4f85e71c9a 100644 --- a/trailer.c +++ b/trailer.c @@ -690,8 +690,18 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val, list_add_tail(&new_item->list, arg_head); } +static void add_user_own_identity(struct new_trailer_item *item) +{ + struct strbuf buf = STRBUF_INIT; + strbuf_addstr(&buf, item->text); + strbuf_add(&buf, "=", 1); + strbuf_addstr(&buf, fmt_name(WANT_COMMITTER_IDENT)); + free(item->text); + item->text = buf.buf; +} + static void process_command_line_args(struct list_head *arg_head, - struct list_head *new_trailer_head) + struct list_head *new_trailer_head, int own_identity) { struct arg_item *item; struct strbuf tok = STRBUF_INIT; @@ -728,6 +738,10 @@ static void process_command_line_args(struct list_head *arg_head, error(_("empty trailer token in trailer '%.*s'"), (int) sb.len, sb.buf); strbuf_release(&sb); + } else if (separator_pos == -1 && own_identity) { + add_user_own_identity(tr); + pos = pos->prev; + continue; } else { parse_trailer(&tok, &val, &conf, tr->text, separator_pos); @@ -1048,7 +1062,7 @@ void process_trailers(const char *file, if (!opts->only_input) { LIST_HEAD(arg_head); - process_command_line_args(&arg_head, new_trailer_head); + process_command_line_args(&arg_head, new_trailer_head ,opts->own_identity); process_trailers_lists(&head, &arg_head); } diff --git a/trailer.h b/trailer.h index 795d2fccfd95..235dfdfa1978 100644 --- a/trailer.h +++ b/trailer.h @@ -57,7 +57,7 @@ struct trailer_info { struct new_trailer_item { struct list_head list; - const char *text; + char *text; enum trailer_where where; enum trailer_if_exists if_exists; @@ -73,6 +73,7 @@ struct process_trailer_options { int no_divider; int key_only; int value_only; + int own_identity; const struct strbuf *separator; const struct strbuf *key_value_separator; int (*filter)(const struct strbuf *, void *);