| Message ID | pull.901.v4.git.1615737505834.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,GSOC] commit: add --trailer option | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > @@ -958,6 +970,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix, > > fclose(s->fp); > > + if (run_trailer.args.nr != 4) { > + run_trailer.git_cmd = 1; > + run_command(&run_trailer); This hardcoded magic 4 is very brittle. It probably makes sense to have another string vector, that is used only to accumulate --trailer arguments and nothing else, and check if that string vector is empty here. IOW this part would become ... if (trailer_args.nr) { strvec_pushl(&run_trailer.args, "interpret-trailers", "--in-place", ...); strvec_pushv(&run_trailer.args, trailer_args.v); run_trailer.git_cmd = 1; run_command(&run_trailer); } > + } else > + strvec_clear(&run_trailer.args); ... and there is no need to have "else" that won't need to do anything. > /* > * Reject an attempt to record a non-merge empty commit without > * explicit --allow-empty. In the cherry-pick case, it may be > @@ -1507,6 +1525,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(0, "trailer", &trailer, N_("trailer"), N_("trailer(s) to add"), 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")), > @@ -1577,6 +1596,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > die(_("could not parse HEAD commit")); > } > verbose = -1; /* unspecified */ > + strvec_pushl(&run_trailer.args, "interpret-trailers", > + "--in-place", "--where=end", git_path_commit_editmsg(), NULL); And this line will be gone. > argc = parse_and_validate_options(argc, argv, builtin_commit_options, > builtin_commit_usage, > prefix, current_head, &s);
Junio C Hamano <gitster@pobox.com> 于2021年3月15日周一 上午7:52写道： > IOW this part would become ... > > if (trailer_args.nr) { > strvec_pushl(&run_trailer.args, "interpret-trailers", > "--in-place", ...); > strvec_pushv(&run_trailer.args, trailer_args.v); > run_trailer.git_cmd = 1; > run_command(&run_trailer); > } > > > + } else > > + strvec_clear(&run_trailer.args); > > ... and there is no need to have "else" that won't need to do > anything. Yes, but we also should clear "trailer_args" in "else" here, and check the return value of the "run_command()" for clear "run_trailer.args". > > > verbose = -1; /* unspecified */ > > + strvec_pushl(&run_trailer.args, "interpret-trailers", > > + "--in-place", "--where=end", git_path_commit_editmsg(), NULL); > > And this line will be gone. > Indeed so. Thanks.
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: One thing I forgot to mention. > +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset) > +{ > + if (unset) { > + strvec_clear(&run_trailer.args); > + return -1; What input is this part of the code designed to handle, and what outcome does it want to show to the user? Does the test added in this patch cover this codepath, where unset is true? I think this codepath is reached when the command line says "--no-trailer", and I am guessing that by clearing the code wants to say "ignore the accumulated trailers and start accumulating afresh", i.e. git commit --trailer='T1: V1' --trailer='T2: V2' \ --no-trailer \ --trailer='T3: V3' would want to add only the "T3: V3" trailer; which is a sensible design, but I do not think returning -1 would be compatible with that design. If on the other hand the code wants to say "--no-trailer is a command line error", then using PARSE_OPT_NONEG to let the parse-options API issue an error message and die would be more appropriate. That is an often used pattern for an option that can appear on the command line only once without accumulating, which may be less appropriate for the "--trailer", though. > + } > + strvec_pushl(&run_trailer.args, "--trailer", arg, NULL); > + return 0; > +}
ZheNing Hu <adlternative@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> 于2021年3月15日周一 上午7:52写道： >> IOW this part would become ... >> >> if (trailer_args.nr) { >> strvec_pushl(&run_trailer.args, "interpret-trailers", >> "--in-place", ...); >> strvec_pushv(&run_trailer.args, trailer_args.v); >> run_trailer.git_cmd = 1; >> run_command(&run_trailer); >> } >> >> > + } else >> > + strvec_clear(&run_trailer.args); >> >> ... and there is no need to have "else" that won't need to do >> anything. > > Yes, but we also should clear "trailer_args" in "else" here, and check the > return value of the "run_command()" for clear "run_trailer.args". No. If you introduce the separate strvec, the "else" clause runs only when trailer_args haven't got anything, so there is nothing to clear.
Junio C Hamano <gitster@pobox.com> 于2021年3月15日周一 下午12:38写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > One thing I forgot to mention. > > > +static int opt_pass_trailer(const struct option *opt, const char *arg, int unset) > > +{ > > + if (unset) { > > + strvec_clear(&run_trailer.args); > > + return -1; > > What input is this part of the code designed to handle, and what > outcome does it want to show to the user? > > Does the test added in this patch cover this codepath, where unset > is true? > > I think this codepath is reached when the command line says > "--no-trailer", and I am guessing that by clearing the code wants to > say "ignore the accumulated trailers and start accumulating afresh", > i.e. > > git commit --trailer='T1: V1' --trailer='T2: V2' \ > --no-trailer \ > --trailer='T3: V3' > > would want to add only the "T3: V3" trailer; which is a sensible > design, but I do not think returning -1 would be compatible with > that design. > > If on the other hand the code wants to say "--no-trailer is a > command line error", then using PARSE_OPT_NONEG to let the > parse-options API issue an error message and die would be more > appropriate. That is an often used pattern for an option that can > appear on the command line only once without accumulating, which may > be less appropriate for the "--trailer", though. > As you said, what I want to express is "--no-trailer is acommand line error". `--no-trailer` may don't have any big benefits for users. I will follow you suggections. > > + } > > + strvec_pushl(&run_trailer.args, "--trailer", arg, NULL); > > + return 0; > > +} >
Junio C Hamano <gitster@pobox.com> 于2021年3月15日周一 下午12:42写道： > > ZheNing Hu <adlternative@gmail.com> writes: > > > Junio C Hamano <gitster@pobox.com> 于2021年3月15日周一 上午7:52写道： > >> IOW this part would become ... > >> > >> if (trailer_args.nr) { > >> strvec_pushl(&run_trailer.args, "interpret-trailers", > >> "--in-place", ...); > >> strvec_pushv(&run_trailer.args, trailer_args.v); > >> run_trailer.git_cmd = 1; > >> run_command(&run_trailer); > >> } > >> > >> > + } else > >> > + strvec_clear(&run_trailer.args); > >> > >> ... and there is no need to have "else" that won't need to do > >> anything. > > > > Yes, but we also should clear "trailer_args" in "else" here, and check the > > return value of the "run_command()" for clear "run_trailer.args". > > No. If you introduce the separate strvec, the "else" clause runs > only when trailer_args haven't got anything, so there is nothing to > clear. > I admit I was wrong before. But this may be the right thing to do. + if (trailer_args.nr) { + 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); + }
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..b35ae5c9790d 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -113,6 +113,8 @@ 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; +struct child_process run_trailer = CHILD_PROCESS_INIT; +static const char *trailer; /* * The default commit message cleanup mode will remove the lines @@ -131,6 +133,16 @@ 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) +{ + if (unset) { + strvec_clear(&run_trailer.args); + return -1; + } + strvec_pushl(&run_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 +970,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix, fclose(s->fp); + if (run_trailer.args.nr != 4) { + run_trailer.git_cmd = 1; + run_command(&run_trailer); + } else + strvec_clear(&run_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 +1525,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(0, "trailer", &trailer, N_("trailer"), N_("trailer(s) to add"), 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")), @@ -1577,6 +1596,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) die(_("could not parse HEAD commit")); } verbose = -1; /* unspecified */ + strvec_pushl(&run_trailer.args, "interpret-trailers", + "--in-place", "--where=end", git_path_commit_editmsg(), NULL); argc = parse_and_validate_options(argc, argv, builtin_commit_options, builtin_commit_usage, prefix, current_head, &s); 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 &&