| Message ID | pull.901.v7.git.1615799304883.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7,GSOC] commit: add --trailer option | expand |
On Mon, Mar 15, 2021 at 10:08 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > @@ -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" trailer s/"Signed-off"/"Signed-off-by"/ > + and the "Helped-by" trailer in the commit message.) It might be useful to point users to git interpret-trailers' documentation, for example by adding: "See linkgit:git-interpret-trailers[1]." Otherwise, this looks good to me!
On Mon, Mar 15, 2021 at 10:08 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> wrote: > + if (trailer_args.nr) { > -+ static struct child_process run_trailer = CHILD_PROCESS_INIT; > ++ struct child_process run_trailer = CHILD_PROCESS_INIT; > + > + strvec_pushl(&run_trailer.args, "interpret-trailers", > + "--in-place", "--where=end", git_path_commit_editmsg(), NULL); Actually I don't think "--where=end" should be used here. "end" is the default for the "trailer.where" config variable, so by default if nothing has been configured, it will work as if "--where=end" was passed above. If a user has configured "trailer.where" or trailer.<token>.where, then this should be respected. And users should be able to override such config variable using for example: git -c trailer.where=start commit --trailer "Signed-off-by:C O Mitter <committer@example.com>"
Christian Couder <christian.couder@gmail.com> 于2021年3月15日周一 下午6:14写道： > > On Mon, Mar 15, 2021 at 10:08 AM ZheNing Hu via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > + if (trailer_args.nr) { > > -+ static struct child_process run_trailer = CHILD_PROCESS_INIT; > > ++ struct child_process run_trailer = CHILD_PROCESS_INIT; > > + > > + strvec_pushl(&run_trailer.args, "interpret-trailers", > > + "--in-place", "--where=end", git_path_commit_editmsg(), NULL); > > Actually I don't think "--where=end" should be used here. "end" is the > default for the "trailer.where" config variable, so by default if > nothing has been configured, it will work as if "--where=end" was > passed above. > > If a user has configured "trailer.where" or trailer.<token>.where, > then this should be respected. And users should be able to override > such config variable using for example: > > git -c trailer.where=start commit --trailer "Signed-off-by:C O Mitter > <committer@example.com>" Thanks for reminding, generally speaking, we will put the trailer at the end of the commit messages.Take trailers in start, this should be something I haven't considered. I notice another question: if we commit this again with same trailer (even same email or same commiter) `--trailer` will not work again, because in `interpret_trailers`, "if-exists" default set to "addIfDifferentNeighbor", I addvice enforce use "if-exists="add". Thanks.
On Mon, Mar 15, 2021 at 12:32 PM ZheNing Hu <adlternative@gmail.com> wrote: > > Christian Couder <christian.couder@gmail.com> 于2021年3月15日周一 下午6:14写道： > > > > On Mon, Mar 15, 2021 at 10:08 AM ZheNing Hu via GitGitGadget > > <gitgitgadget@gmail.com> wrote: > > > > > + if (trailer_args.nr) { > > > -+ static struct child_process run_trailer = CHILD_PROCESS_INIT; > > > ++ struct child_process run_trailer = CHILD_PROCESS_INIT; > > > + > > > + strvec_pushl(&run_trailer.args, "interpret-trailers", > > > + "--in-place", "--where=end", git_path_commit_editmsg(), NULL); > > > > Actually I don't think "--where=end" should be used here. "end" is the > > default for the "trailer.where" config variable, so by default if > > nothing has been configured, it will work as if "--where=end" was > > passed above. > > > > If a user has configured "trailer.where" or trailer.<token>.where, > > then this should be respected. And users should be able to override > > such config variable using for example: > > > > git -c trailer.where=start commit --trailer "Signed-off-by:C O Mitter > > <committer@example.com>" > > Thanks for reminding, generally speaking, we will put the trailer at the > end of the commit messages.Take trailers in start, this should be > something I haven't considered. In general what I want to say is that `git interpret-trailers` should be considered to have sensible defaults, that can possibly be overridden using a number of config variables (or the git -c ... mechanism) which is a good thing. If something in it doesn't work well, it's possible to improve it of course. Otherwise it's better to just fully take advantage of it. > I notice another question: > if we commit this again with same trailer (even same email or same commiter) > `--trailer` will not work again, because in `interpret_trailers`, > "if-exists" default > set to "addIfDifferentNeighbor", I addvice enforce use "if-exists="add". I don't agree with using "--if-exists=add". I think the default to not add a trailer line if it would be just above or below the same line is better, as doing that wouldn't add much information. It's better to encourage people to use trailers in a meaningful way. And again if we use "--if-exists=add", then people who would want to take advantage of `git interpret-trailers` to customize what happens when the trailer already exists would not be able to do it. For example if we don't use "--if-exists=add", then: - people who want to customize what happens when the trailer already exists can do it with for example: git -c trailer.ifexists=addIfDifferent commit --trailer "Signed-off-by:C O Mitter <committer@example.com>" - which means that people who want the "--if-exists=add" behavior can still have it with: git -c trailer.ifexists=add commit --trailer "Signed-off-by:C O Mitter <committer@example.com>" While if we use "--if-exists=add", then using `git -c trailer.ifexists=... commit ...` will not customize anything as the "--if-exists=add" command line option will override any config customization.
Christian Couder <christian.couder@gmail.com> 于2021年3月16日周二 下午1:37写道： > > Thanks for reminding, generally speaking, we will put the trailer at the > > end of the commit messages.Take trailers in start, this should be > > something I haven't considered. > > In general what I want to say is that `git interpret-trailers` should > be considered to have sensible defaults, that can possibly be > overridden using a number of config variables (or the git -c ... > mechanism) which is a good thing. If something in it doesn't work > well, it's possible to improve it of course. Otherwise it's better to > just fully take advantage of it. > > > I notice another question: > > if we commit this again with same trailer (even same email or same commiter) > > `--trailer` will not work again, because in `interpret_trailers`, > > "if-exists" default > > set to "addIfDifferentNeighbor", I addvice enforce use "if-exists="add". > > I don't agree with using "--if-exists=add". I think the default to not > add a trailer line if it would be just above or below the same line is > better, as doing that wouldn't add much information. It's better to > encourage people to use trailers in a meaningful way. > > And again if we use "--if-exists=add", then people who would want to > take advantage of `git interpret-trailers` to customize what happens > when the trailer already exists would not be able to do it. > > For example if we don't use "--if-exists=add", then: > > - people who want to customize what happens when the trailer already > exists can do it with for example: > > git -c trailer.ifexists=addIfDifferent commit --trailer > "Signed-off-by:C O Mitter <committer@example.com>" > > - which means that people who want the "--if-exists=add" behavior can > still have it with: > > git -c trailer.ifexists=add commit --trailer "Signed-off-by:C O Mitter > <committer@example.com>" > > While if we use "--if-exists=add", then using `git -c > trailer.ifexists=... commit ...` will not customize anything as the > "--if-exists=add" command line option will override any config > customization. Well, I see what you mean, this will keep git better flexibility, give users more personalized configuration options. And this should be more in line with git design philosophy, I will follow your suggestion.
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 17150fa7eabe..73a7507db47f 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" trailer + and the "Helped-by" trailer in the commit message.) + -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..0ddd1891f71c 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,18 @@ 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", "--where=end", 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 +1528,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 &&