Message ID | f81b6e66a6ba6b2fc8e6307dbc5fba9cddcff181.1615813658.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | commit: add --trailer option | expand |
On Mon, Mar 15 2021, ZheNing Hu via GitGitGadget wrote: > From: ZheNing Hu <adlternative@gmail.com> > > Historically, Git has supported the 'Signed-off-by' commit trailer > using the '--signoff' and the '-s' option from the command line. > But users may need to provide other trailer information from the > command line such as "Helped-by", "Reported-by", "Mentored-by", > > Now implement a new `--trailer <token>[(=|:)<value>]` option to pass > other trailers to `interpret-trailers` and insert them into commit > messages. I paged through v1-v8, looking mostly good so far. Some comments: > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > Documentation/git-commit.txt | 9 ++++++++- > builtin/commit.c | 23 +++++++++++++++++++++++ > t/t7502-commit-porcelain.sh | 20 ++++++++++++++++++++ > 3 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt > index 17150fa7eabe..f74d320f0c96 100644 > --- a/Documentation/git-commit.txt > +++ b/Documentation/git-commit.txt > @@ -14,7 +14,7 @@ SYNOPSIS > [--allow-empty-message] [--no-verify] [-e] [--author=<author>] > [--date=<date>] [--cleanup=<mode>] [--[no-]status] > [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]] > - [-S[<keyid>]] [--] [<pathspec>...] > + [-S[<keyid>]] [--] [<pathspec>...] [(--trailer <token>[(=|:)<value>])...] > > DESCRIPTION > ----------- > @@ -166,6 +166,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. > > include::signoff-option.txt[] > > +--trailer <token>[(=|:)<value>]:: > + Specify a (<token>, <value>) pair that should be applied as a > + trailer. (e.g. `git commit --trailer "Signed-off-by:C O Mitter \ > + <committer@example.com>" --trailer "Helped-by:C O Mitter \ > + <committer@example.com>"` will add the "Signed-off-by" trailer > + and the "Helped-by" trailer in the commit message.) > + See linkgit:git-interpret-trailers[1] for details. > -n:: > --no-verify:: > This option bypasses the pre-commit and commit-msg hooks. > diff --git a/builtin/commit.c b/builtin/commit.c > index 739110c5a7f6..250e93d977df 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -113,6 +113,7 @@ static int config_commit_verbose = -1; /* unspecified */ > static int no_post_rewrite, allow_empty_message, pathspec_file_nul; > static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg; > static char *sign_commit, *pathspec_from_file; > +static struct strvec trailer_args = STRVEC_INIT; > > /* > * The default commit message cleanup mode will remove the lines > @@ -131,6 +132,14 @@ static struct strbuf message = STRBUF_INIT; > > static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; > > +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset) > +{ > + BUG_ON_OPT_NEG(unset); > + > + strvec_pushl(&trailer_args, "--trailer", arg, NULL); > + return 0; > +} > + > static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset) > { > enum wt_status_format *value = (enum wt_status_format *)opt->value; > @@ -958,6 +967,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > > fclose(s->fp); > > + if (trailer_args.nr) { > + struct child_process run_trailer = CHILD_PROCESS_INIT; > + > + strvec_pushl(&run_trailer.args, "interpret-trailers", > + "--in-place", "--if-exists=add", > + git_path_commit_editmsg(), NULL); > + strvec_pushv(&run_trailer.args, trailer_args.v); > + run_trailer.git_cmd = 1; > + if (run_command(&run_trailer)) > + strvec_clear(&run_trailer.args); This is git-commit, shouldn't we die() here instead of ignoring errors in sub-processes? > + strvec_clear(&trailer_args); > + } > + > /* > * Reject an attempt to record a non-merge empty commit without > * explicit --allow-empty. In the cherry-pick case, it may be > @@ -1507,6 +1529,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")), > OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")), > OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")), > + OPT_CALLBACK_F(0, "trailer", NULL, N_("trailer"), N_("trailer(s) to add"), PARSE_OPT_NONEG, opt_pass_trailer), > OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")), Not required for this change, but perhaps a change here to N_() (if we can get it to fit) + doc update saying that we prefer --trailer="Signed-Off-By: to --signoff"? More on that later. > OPT_FILENAME('t', "template", &template_file, N_("use specified template file")), > OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")), > diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh > index 6396897cc818..0acf23799931 100755 > --- a/t/t7502-commit-porcelain.sh > +++ b/t/t7502-commit-porcelain.sh > @@ -154,6 +154,26 @@ test_expect_success 'sign off' ' > > ' > > +test_expect_success 'trailer' ' > + >file1 && > + git add file1 && > + git commit -s --trailer "Signed-off-by:C O Mitter1 <committer1@example.com>" \ > + --trailer "Helped-by:C O Mitter2 <committer2@example.com>" \ > + --trailer "Reported-by:C O Mitter3 <committer3@example.com>" \ > + --trailer "Mentored-by:C O Mitter4 <committer4@example.com>" \ > + -m "hello" && > + git cat-file commit HEAD >commit.msg && > + sed -e "1,7d" commit.msg >actual && > + cat >expected <<-\EOF && > + Signed-off-by: C O Mitter <committer@example.com> > + Signed-off-by: C O Mitter1 <committer1@example.com> > + Helped-by: C O Mitter2 <committer2@example.com> > + Reported-by: C O Mitter3 <committer3@example.com> > + Mentored-by: C O Mitter4 <committer4@example.com> > + EOF > + test_cmp expected actual > +' > + How does this interact with cases where the user has configured "trailer.separators" to have a value that doesn't contain ":"? I haven't tested, but my reading of git-interpret-trailers(1) is that if you supplied "=" instead that case would just work: By default only : is recognized as a trailer separator, except that = is always accepted on the command line for compatibility with other git commands. I don't know if that does the right thing in the presence of --if-exists=add. So it would be good to update these tests so you test: * For the --if-exists=add case at all, there's no tests for it now. I.e. add some trailers manually to the commit (via -F or whatever) and then see if they get added to, replacet etc. * Ditto but for the user having configured trailer.separators (see the test_config helper for how to set config in a test). I.e. if it's "=" does adding trailers work, how about if it's "=" on the CLI but the config/commit message has ";" instead of ":" or something? * Hrm, actually I think tweaking "-c trailer.ifexists" won't work at all, since the CLI switch would override it. I honestly don't know, but why not not supply it and keep the addIfDifferentNeighbor default? If it's essential that seems like a good test / documentation addition... * For the above -c ... case I can't think of a good way to deal with it that doesn't involve pulling in git_trailer_config() into git_commit_config(), but perhaps the least nasty way is to just set a flag in git_commit_config() if we see a "trailer.ifexists" flag, and if so don't provide "--if-exists=add", if there's no config (this will include "git -c ... commit" we set provide "--if-exists=add" ) or as noted above, maybe we can skip the whole thing and use the addIfDifferentNeighbor default. And, not needed for this patch but worth thinking about: * We pass through --trailer to git-interpret-trailers, what should we do about the other options? Should git-commit eventually support --trailer-where and pass it along as --where to git-interpret-trailers, or is "git -c trailer.where=... commit" good enough? * It would be good to test for and document if that "-c trailer.*" trick works (no reason it shouldn't). I.e. to add something like this after what you have (along with tests, and check if it's even true): Only the `--trailer` argument to linkgit:git-interpret-trailers[1] is supported. Other pass-through switches may be added in the future, but currently you'll need to pass arguments to linkgit:git-interpret-trailers[1] along as config, e.g. `git -c trailer.where=start commit [...] --trailer=[...]`. * We have a longer-term goal of having the .mailmap apply to trailers, it would be nice if git-interpret-trailers had some fuzzy-matching to check if the RHS of a trailer is a name/E-Mail pair, and if so did stricter validation on it with the ident functions we use for fsck etc. (that's copied & subtly different in several different places in the codebase, unfortunately[1]). More thoughts: * Having written all the above I checked how --signoff is implemented. It seems to me to be a good idea to (at least for testing) convert the --signoff trailer to your implementation. We have plenty of tests for it, does migrating it over pass or fail those? * I also agree with Junio that we shouldn't have a --fixed-by or whatever and wouldn't add --signoff today, but it seems very useful to me to have a shortcut like: --trailer "Signed-off-by" I.e. omitting the value, or: --trailer "Signed-off-by=" Or some other thing we deem sufficiently useful/sane syntax/unambiguous.n Then the value would be provided by fmt_name(WANT_COMMITTER_IDENT) just as we do in append_signoff() now. I think a *very common* case for this would be something like: git commit --amend -v --trailer "Reviewed-by" And it would be useful to help that along and not have to do: git commit --amend -v --trailer "Reviewed-by=$(git config user.name) <$(git config user.email)>" Or worse yet, manually typo your name/e-mail address, as I'm sure I and many others will inevitably do when using this option... 1. https://lore.kernel.org/git/87bld8ov9q.fsf@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年3月16日周二 下午8:52写道: > > + if (run_command(&run_trailer)) > > + strvec_clear(&run_trailer.args); > > This is git-commit, shouldn't we die() here instead of ignoring errors > in sub-processes? After thinking about it carefully, your opinion is more reasonable, because if the user uses the wrong `--trailer` and does not get the information he needs, I think he will have to use `--amend` to modify, and `die()` can exit this commit directly. > > > + strvec_clear(&trailer_args); > > + } > > + > > /* > > * Reject an attempt to record a non-merge empty commit without > > * explicit --allow-empty. In the cherry-pick case, it may be > > @@ -1507,6 +1529,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > > OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")), > > OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")), > > OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")), > > + OPT_CALLBACK_F(0, "trailer", NULL, N_("trailer"), N_("trailer(s) to add"), PARSE_OPT_NONEG, opt_pass_trailer), > > OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")), > > Not required for this change, but perhaps a change here to N_() (if we > can get it to fit) + doc update saying that we prefer > --trailer="Signed-Off-By: to --signoff"? More on that later. > > > OPT_FILENAME('t', "template", &template_file, N_("use specified template file")), > > OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")), > > diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh > > index 6396897cc818..0acf23799931 100755 > > --- a/t/t7502-commit-porcelain.sh > > +++ b/t/t7502-commit-porcelain.sh > > @@ -154,6 +154,26 @@ test_expect_success 'sign off' ' > > > > ' > > > > +test_expect_success 'trailer' ' > > + >file1 && > > + git add file1 && > > + git commit -s --trailer "Signed-off-by:C O Mitter1 <committer1@example.com>" \ > > + --trailer "Helped-by:C O Mitter2 <committer2@example.com>" \ > > + --trailer "Reported-by:C O Mitter3 <committer3@example.com>" \ > > + --trailer "Mentored-by:C O Mitter4 <committer4@example.com>" \ > > + -m "hello" && > > + git cat-file commit HEAD >commit.msg && > > + sed -e "1,7d" commit.msg >actual && > > + cat >expected <<-\EOF && > > + Signed-off-by: C O Mitter <committer@example.com> > > + Signed-off-by: C O Mitter1 <committer1@example.com> > > + Helped-by: C O Mitter2 <committer2@example.com> > > + Reported-by: C O Mitter3 <committer3@example.com> > > + Mentored-by: C O Mitter4 <committer4@example.com> > > + EOF > > + test_cmp expected actual > > +' > > + > > How does this interact with cases where the user has configured > "trailer.separators" to have a value that doesn't contain ":"? I > haven't tested, but my reading of git-interpret-trailers(1) is that if > you supplied "=" instead that case would just work: > > By default only : is recognized as a trailer separator, except that > = is always accepted on the command line for compatibility with > other git commands. > But interpret_trailers interface allow us use "=" instead of other separators. I did a simple test and modified the configuration "trailer.separators" and it still works. Now things are good here: $ git -c trailer.separators="@" commit --trailer="Signed-off-by=C O <email>" or $ git -c trailer.separators="@" commit --trailer="Signed-off-by@C O <email>" Both can work normally, --trailer="Signed-off-by@ C O <email>" will output in the commit message. > I don't know if that does the right thing in the presence of > --if-exists=add. > Yesterday, Christian Couder and I had already discussed this issue: Your idea is correct, I should not add "--if-exists = add", this will destroy the user's rights to configure by using `git -c trailer.if-exist`. > So it would be good to update these tests so you test: > > * For the --if-exists=add case at all, there's no tests for it > now. I.e. add some trailers manually to the commit (via -F or > whatever) and then see if they get added to, replacet etc. > > * Ditto but for the user having configured trailer.separators (see the > test_config helper for how to set config in a test). I.e. if it's "=" > does adding trailers work, how about if it's "=" on the CLI but the > config/commit message has ";" instead of ":" or something? > As mentioned above, it works normally. > * Hrm, actually I think tweaking "-c trailer.ifexists" won't work at > all, since the CLI switch would override it. I honestly don't know, > but why not not supply it and keep the addIfDifferentNeighbor > default? > > If it's essential that seems like a good test / documentation > addition... > > * For the above -c ... case I can't think of a good way to deal with it > that doesn't involve pulling in git_trailer_config() into > git_commit_config(), but perhaps the least nasty way is to just set a > flag in git_commit_config() if we see a "trailer.ifexists" flag, and > if so don't provide "--if-exists=add", if there's no config (this > will include "git -c ... commit" we set provide "--if-exists=add" ) > or as noted above, maybe we can skip the whole thing and use the > addIfDifferentNeighbor default. > Has been restored to the default settings. > And, not needed for this patch but worth thinking about: > > * We pass through --trailer to git-interpret-trailers, what should we > do about the other options? Should git-commit eventually support > --trailer-where and pass it along as --where to > git-interpret-trailers, or is "git -c trailer.where=... commit" good > enough? > Logically speaking, `interpret_trailers` should be dedicated to `commit` or other sub-commands that require trailers. But I think that in the later stage, the parse_options of the `cmd_commit` can keep the unrecognized options, and then these choices can be directly passed to the `interpret_trailers` backend. > * It would be good to test for and document if that "-c trailer.*" > trick works (no reason it shouldn't). I.e. to add something like this > after what you have (along with tests, and check if it's even true): > I haven't tested them for the time being, but I will do it. > Only the `--trailer` argument to > linkgit:git-interpret-trailers[1] is supported. Other > pass-through switches may be added in the future, but currently > you'll need to pass arguments to > linkgit:git-interpret-trailers[1] along as config, e.g. `git -c > trailer.where=start commit [...] --trailer=[...]`. > I think this is worth writing in the documentation. > * We have a longer-term goal of having the .mailmap apply to trailers, > it would be nice if git-interpret-trailers had some fuzzy-matching to > check if the RHS of a trailer is a name/E-Mail pair, and if so did > stricter validation on it with the ident functions we use for fsck > etc. (that's copied & subtly different in several different places in > the codebase, unfortunately[1]). > I may not know much about fuzzy-matching, which may be worth studying later. > More thoughts: > > * Having written all the above I checked how --signoff is implemented. > > It seems to me to be a good idea to (at least for testing) convert > the --signoff trailer to your implementation. We have plenty of tests > for it, does migrating it over pass or fail those? > I don’t know how to migrating yet, it may take a long time. Even I think I can leave it as #leftoverbit later. > * I also agree with Junio that we shouldn't have a --fixed-by or > whatever and wouldn't add --signoff today, but it seems very useful > to me to have a shortcut like: > > --trailer "Signed-off-by" > > I.e. omitting the value, or: > > --trailer "Signed-off-by=" > > Or some other thing we deem sufficiently useful/sane > syntax/unambiguous.n > > Then the value would be provided by fmt_name(WANT_COMMITTER_IDENT) > just as we do in append_signoff() now. I think a *very common* case > for this would be something like: > > git commit --amend -v --trailer "Reviewed-by" > > And it would be useful to help that along and not have to do: > > git commit --amend -v --trailer "Reviewed-by=$(git config user.name) <$(git config user.email)>" > > Or worse yet, manually typo your name/e-mail address, as I'm sure I > and many others will inevitably do when using this option... > I think this idea is very good and easy to implement. We only need to do a simple string match when we get the "trailer" string, If it can be completed, it can indeed bring great convenience to users. > 1. https://lore.kernel.org/git/87bld8ov9q.fsf@evledraar.gmail.com/ Thanks, Ævar Arnfjörð Bjarmason! -- ZheNing Hu
On Wed, Mar 17 2021, ZheNing Hu wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年3月16日周二 下午8:52写道: >> > + if (run_command(&run_trailer)) >> > + strvec_clear(&run_trailer.args); >> >> This is git-commit, shouldn't we die() here instead of ignoring errors >> in sub-processes? > > After thinking about it carefully, your opinion is more > reasonable, because if the user uses the wrong `--trailer` > and does not get the information he needs, I think he will > have to use `--amend` to modify, and `die()` can exit > this commit directly. Yeah, we don't want to silently lose data. >> >> > + strvec_clear(&trailer_args); >> > + } >> > + >> > /* >> > * Reject an attempt to record a non-merge empty commit without >> > * explicit --allow-empty. In the cherry-pick case, it may be >> > @@ -1507,6 +1529,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >> > OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")), >> > OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")), >> > OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")), >> > + OPT_CALLBACK_F(0, "trailer", NULL, N_("trailer"), N_("trailer(s) to add"), PARSE_OPT_NONEG, opt_pass_trailer), >> > OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")), >> >> Not required for this change, but perhaps a change here to N_() (if we >> can get it to fit) + doc update saying that we prefer >> --trailer="Signed-Off-By: to --signoff"? More on that later. >> >> > OPT_FILENAME('t', "template", &template_file, N_("use specified template file")), >> > OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")), >> > diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh >> > index 6396897cc818..0acf23799931 100755 >> > --- a/t/t7502-commit-porcelain.sh >> > +++ b/t/t7502-commit-porcelain.sh >> > @@ -154,6 +154,26 @@ test_expect_success 'sign off' ' >> > >> > ' >> > >> > +test_expect_success 'trailer' ' >> > + >file1 && >> > + git add file1 && >> > + git commit -s --trailer "Signed-off-by:C O Mitter1 <committer1@example.com>" \ >> > + --trailer "Helped-by:C O Mitter2 <committer2@example.com>" \ >> > + --trailer "Reported-by:C O Mitter3 <committer3@example.com>" \ >> > + --trailer "Mentored-by:C O Mitter4 <committer4@example.com>" \ >> > + -m "hello" && >> > + git cat-file commit HEAD >commit.msg && >> > + sed -e "1,7d" commit.msg >actual && >> > + cat >expected <<-\EOF && >> > + Signed-off-by: C O Mitter <committer@example.com> >> > + Signed-off-by: C O Mitter1 <committer1@example.com> >> > + Helped-by: C O Mitter2 <committer2@example.com> >> > + Reported-by: C O Mitter3 <committer3@example.com> >> > + Mentored-by: C O Mitter4 <committer4@example.com> >> > + EOF >> > + test_cmp expected actual >> > +' >> > + >> >> How does this interact with cases where the user has configured >> "trailer.separators" to have a value that doesn't contain ":"? I >> haven't tested, but my reading of git-interpret-trailers(1) is that if >> you supplied "=" instead that case would just work: >> >> By default only : is recognized as a trailer separator, except that >> = is always accepted on the command line for compatibility with >> other git commands. >> > But interpret_trailers interface allow us use "=" instead of other separators. > > I did a simple test and modified the configuration "trailer.separators" > and it still works. Now things are good here: > > $ git -c trailer.separators="@" commit --trailer="Signed-off-by=C O <email>" > > or > > $ git -c trailer.separators="@" commit --trailer="Signed-off-by@C O <email>" > > Both can work normally, > > --trailer="Signed-off-by@ C O <email>" > > will output in the commit message. > >> I don't know if that does the right thing in the presence of >> --if-exists=add. >> > > Yesterday, Christian Couder and I had already discussed this issue: > Your idea is correct, I should not add "--if-exists = add", this will destroy > the user's rights to configure by using `git -c trailer.if-exist`. > >> So it would be good to update these tests so you test: >> >> * For the --if-exists=add case at all, there's no tests for it >> now. I.e. add some trailers manually to the commit (via -F or >> whatever) and then see if they get added to, replacet etc. >> >> * Ditto but for the user having configured trailer.separators (see the >> test_config helper for how to set config in a test). I.e. if it's "=" >> does adding trailers work, how about if it's "=" on the CLI but the >> config/commit message has ";" instead of ":" or something? >> > > As mentioned above, it works normally. > >> * Hrm, actually I think tweaking "-c trailer.ifexists" won't work at >> all, since the CLI switch would override it. I honestly don't know, >> but why not not supply it and keep the addIfDifferentNeighbor >> default? >> >> If it's essential that seems like a good test / documentation >> addition... >> >> * For the above -c ... case I can't think of a good way to deal with it >> that doesn't involve pulling in git_trailer_config() into >> git_commit_config(), but perhaps the least nasty way is to just set a >> flag in git_commit_config() if we see a "trailer.ifexists" flag, and >> if so don't provide "--if-exists=add", if there's no config (this >> will include "git -c ... commit" we set provide "--if-exists=add" ) >> or as noted above, maybe we can skip the whole thing and use the >> addIfDifferentNeighbor default. >> > > Has been restored to the default settings. To clarify: What I really mean is for all these things you've tested: let's add those to the tests as part of the patch. >> And, not needed for this patch but worth thinking about: >> >> * We pass through --trailer to git-interpret-trailers, what should we >> do about the other options? Should git-commit eventually support >> --trailer-where and pass it along as --where to >> git-interpret-trailers, or is "git -c trailer.where=... commit" good >> enough? >> > Logically speaking, `interpret_trailers` should be dedicated to `commit` > or other sub-commands that require trailers. > > But I think that in the later stage, the parse_options of the `cmd_commit` > can keep the unrecognized options, and then these choices can be directly > passed to the `interpret_trailers` backend. We have this interaction with e.g. range-diff and "log", it's often surprising. You add an option to one command and it appears in the other. >> * It would be good to test for and document if that "-c trailer.*" >> trick works (no reason it shouldn't). I.e. to add something like this >> after what you have (along with tests, and check if it's even true): >> > > I haven't tested them for the time being, but I will do it. > >> Only the `--trailer` argument to >> linkgit:git-interpret-trailers[1] is supported. Other >> pass-through switches may be added in the future, but currently >> you'll need to pass arguments to >> linkgit:git-interpret-trailers[1] along as config, e.g. `git -c >> trailer.where=start commit [...] --trailer=[...]`. >> > > I think this is worth writing in the documentation. > >> * We have a longer-term goal of having the .mailmap apply to trailers, >> it would be nice if git-interpret-trailers had some fuzzy-matching to >> check if the RHS of a trailer is a name/E-Mail pair, and if so did >> stricter validation on it with the ident functions we use for fsck >> etc. (that's copied & subtly different in several different places in >> the codebase, unfortunately[1]). >> > > I may not know much about fuzzy-matching, which may be worth studying later. > >> More thoughts: >> >> * Having written all the above I checked how --signoff is implemented. >> >> It seems to me to be a good idea to (at least for testing) convert >> the --signoff trailer to your implementation. We have plenty of tests >> for it, does migrating it over pass or fail those? >> > I don’t know how to migrating yet, it may take a long time. > Even I think I can leave it as #leftoverbit later. Sure, I mean (having looked at it) that at least for your own local testing it would make sense to change it (even if just search-replacing the --signoff in the test suite) to see if it behaves as you expect. I.e. does the --trailer behavior mirror --signoff? >> * I also agree with Junio that we shouldn't have a --fixed-by or >> whatever and wouldn't add --signoff today, but it seems very useful >> to me to have a shortcut like: >> >> --trailer "Signed-off-by" >> >> I.e. omitting the value, or: >> >> --trailer "Signed-off-by=" >> >> Or some other thing we deem sufficiently useful/sane >> syntax/unambiguous.n >> >> Then the value would be provided by fmt_name(WANT_COMMITTER_IDENT) >> just as we do in append_signoff() now. I think a *very common* case >> for this would be something like: >> >> git commit --amend -v --trailer "Reviewed-by" >> >> And it would be useful to help that along and not have to do: >> >> git commit --amend -v --trailer "Reviewed-by=$(git config user.name) <$(git config user.email)>" >> >> Or worse yet, manually typo your name/e-mail address, as I'm sure I >> and many others will inevitably do when using this option... >> > I think this idea is very good and easy to implement. > We only need to do a simple string match when we get the "trailer" string, > If it can be completed, it can indeed bring great convenience to users. > >> 1. https://lore.kernel.org/git/87bld8ov9q.fsf@evledraar.gmail.com/ > > Thanks, Ævar Arnfjörð Bjarmason! And thanks for working on this.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年3月17日周三 下午4:08写道: > > Logically speaking, `interpret_trailers` should be dedicated to `commit` > > or other sub-commands that require trailers. > > > > But I think that in the later stage, the parse_options of the `cmd_commit` > > can keep the unrecognized options, and then these choices can be directly > > passed to the `interpret_trailers` backend. > > We have this interaction with e.g. range-diff and "log", it's often > surprising. You add an option to one command and it appears in the > other. > All right, I'm wrong, I may have reference to an wrong experience of `difftool`-->`diff`. > >> It seems to me to be a good idea to (at least for testing) convert > >> the --signoff trailer to your implementation. We have plenty of tests > >> for it, does migrating it over pass or fail those? > >> > > I don’t know how to migrating yet, it may take a long time. > > Even I think I can leave it as #leftoverbit later. > > Sure, I mean (having looked at it) that at least for your own local > testing it would make sense to change it (even if just search-replacing > the --signoff in the test suite) to see if it behaves as you > expect. I.e. does the --trailer behavior mirror --signoff? > > >> * I also agree with Junio that we shouldn't have a --fixed-by or > >> whatever and wouldn't add --signoff today, but it seems very useful > >> to me to have a shortcut like: > >> > >> --trailer "Signed-off-by" > >> > >> I.e. omitting the value, or: > >> > >> --trailer "Signed-off-by=" > >> > >> Or some other thing we deem sufficiently useful/sane > >> syntax/unambiguous.n > >> > >> Then the value would be provided by fmt_name(WANT_COMMITTER_IDENT) > >> just as we do in append_signoff() now. I think a *very common* case > >> for this would be something like: > >> > >> git commit --amend -v --trailer "Reviewed-by" > >> > >> And it would be useful to help that along and not have to do: > >> > >> git commit --amend -v --trailer "Reviewed-by=$(git config user.name) <$(git config user.email)>" > >> > >> Or worse yet, manually typo your name/e-mail address, as I'm sure I > >> and many others will inevitably do when using this option... > >> Well, that's what I think here: Now we can go through: $ git -c trailer.signoff.key = "Signed-off-by" commit --trailer "signoff = commiter <email>" to get a trailer: "Signed-off-by: commiter <email>", this means we can't just do simple string matching in `cmd_commit` to replace `--trailer="Signed-off-by"` or `--trailer="Reviewed-by"` to user's own identity, to replace the trailers which have omitting value we passed in, but I think we can provide a new option to `commit` which can mandatory that trailers with no value can be replaced with the identity of the user. e.g. $ git -c trailer.signoff.key = "Signed-off-by" commit --trailer "signoff" --trailer "Helped-by" \ --trailer "Helped-by = C <E>" --own_ident will output like this: Signed-off-by: $(git config user.name) <$(git config user.email)> Signed-off-by: $(git config user.name) <$(git config user.email)> Helped-by: $(git config user.name) <$(git config user.email)> Helped-by: C <E> I don't know if this idea is good, I will try to do it first. Thanks.
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 17150fa7eabe..f74d320f0c96 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -14,7 +14,7 @@ SYNOPSIS [--allow-empty-message] [--no-verify] [-e] [--author=<author>] [--date=<date>] [--cleanup=<mode>] [--[no-]status] [-i | -o] [--pathspec-from-file=<file> [--pathspec-file-nul]] - [-S[<keyid>]] [--] [<pathspec>...] + [-S[<keyid>]] [--] [<pathspec>...] [(--trailer <token>[(=|:)<value>])...] DESCRIPTION ----------- @@ -166,6 +166,13 @@ The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. include::signoff-option.txt[] +--trailer <token>[(=|:)<value>]:: + Specify a (<token>, <value>) pair that should be applied as a + trailer. (e.g. `git commit --trailer "Signed-off-by:C O Mitter \ + <committer@example.com>" --trailer "Helped-by:C O Mitter \ + <committer@example.com>"` will add the "Signed-off-by" trailer + and the "Helped-by" trailer in the commit message.) + See linkgit:git-interpret-trailers[1] for details. -n:: --no-verify:: This option bypasses the pre-commit and commit-msg hooks. diff --git a/builtin/commit.c b/builtin/commit.c index 739110c5a7f6..250e93d977df 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -113,6 +113,7 @@ static int config_commit_verbose = -1; /* unspecified */ static int no_post_rewrite, allow_empty_message, pathspec_file_nul; static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg; static char *sign_commit, *pathspec_from_file; +static struct strvec trailer_args = STRVEC_INIT; /* * The default commit message cleanup mode will remove the lines @@ -131,6 +132,14 @@ static struct strbuf message = STRBUF_INIT; static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED; +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset) +{ + BUG_ON_OPT_NEG(unset); + + strvec_pushl(&trailer_args, "--trailer", arg, NULL); + return 0; +} + static int opt_parse_porcelain(const struct option *opt, const char *arg, int unset) { enum wt_status_format *value = (enum wt_status_format *)opt->value; @@ -958,6 +967,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix, fclose(s->fp); + if (trailer_args.nr) { + struct child_process run_trailer = CHILD_PROCESS_INIT; + + strvec_pushl(&run_trailer.args, "interpret-trailers", + "--in-place", "--if-exists=add", + git_path_commit_editmsg(), NULL); + strvec_pushv(&run_trailer.args, trailer_args.v); + run_trailer.git_cmd = 1; + if (run_command(&run_trailer)) + strvec_clear(&run_trailer.args); + strvec_clear(&trailer_args); + } + /* * Reject an attempt to record a non-merge empty commit without * explicit --allow-empty. In the cherry-pick case, it may be @@ -1507,6 +1529,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) OPT_STRING(0, "fixup", &fixup_message, N_("commit"), N_("use autosquash formatted message to fixup specified commit")), OPT_STRING(0, "squash", &squash_message, N_("commit"), N_("use autosquash formatted message to squash specified commit")), OPT_BOOL(0, "reset-author", &renew_authorship, N_("the commit is authored by me now (used with -C/-c/--amend)")), + OPT_CALLBACK_F(0, "trailer", NULL, N_("trailer"), N_("trailer(s) to add"), PARSE_OPT_NONEG, opt_pass_trailer), OPT_BOOL('s', "signoff", &signoff, N_("add a Signed-off-by trailer")), OPT_FILENAME('t', "template", &template_file, N_("use specified template file")), OPT_BOOL('e', "edit", &edit_flag, N_("force edit of commit")), diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh index 6396897cc818..0acf23799931 100755 --- a/t/t7502-commit-porcelain.sh +++ b/t/t7502-commit-porcelain.sh @@ -154,6 +154,26 @@ test_expect_success 'sign off' ' ' +test_expect_success 'trailer' ' + >file1 && + git add file1 && + git commit -s --trailer "Signed-off-by:C O Mitter1 <committer1@example.com>" \ + --trailer "Helped-by:C O Mitter2 <committer2@example.com>" \ + --trailer "Reported-by:C O Mitter3 <committer3@example.com>" \ + --trailer "Mentored-by:C O Mitter4 <committer4@example.com>" \ + -m "hello" && + git cat-file commit HEAD >commit.msg && + sed -e "1,7d" commit.msg >actual && + cat >expected <<-\EOF && + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C O Mitter1 <committer1@example.com> + Helped-by: C O Mitter2 <committer2@example.com> + Reported-by: C O Mitter3 <committer3@example.com> + Mentored-by: C O Mitter4 <committer4@example.com> + EOF + test_cmp expected actual +' + test_expect_success 'multiple -m' ' >negative &&