| Message ID | pull.901.v13.git.1616387093662.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v13,GSOC] commit: add --trailer option | expand |
On Mon, Mar 22, 2021 at 5:24 AM ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com> 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. > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > Reported-by: Why is there this "Reported-by:" trailer with an empty value? If you are looking to add trailers to this commit message, you might want to add them before your "Signed-off-by". > --- > [GSOC] commit: add --trailer option > > Now maintainers or developers can also use commit > --trailer="Signed-off-by:commiter<email>" from the command line to > provide trailers to commit messages. This solution may be more > generalized than v1. It's not a big deal as this is not going into the commit message, but at this point (v13) you might want to tell explicitly what solution v1 implemented, instead of referring to it. > @@ t/t7502-commit-porcelain.sh: test_expect_success 'sign off' '> + sed -e "1,/^\$/d" commit.msg >actual && > + test_cmp expected actual > +' > ++ > ++test_expect_success 'commit --trailer with -c and command' ' > ++ trailer_commit_base && > ++ cat >expected <<-\EOF && > ++ hello > ++ > ++ Signed-off-by: C O Mitter <committer@example.com> > ++ Signed-off-by: C1 E1 > ++ Helped-by: C2 E2 > ++ Mentored-by: C4 E4 > ++ Reported-by: A U Thor <author@example.com> > ++ EOF > ++ git -c trailer.report.key="Reported-by: " \ > ++ -c trailer.report.ifexists="replace" \ > ++ -c trailer.report.command="git log --author=\"\$ARG\" -1 \ > ++ --format=\"format:%aN <%aE>\"" \ > ++ commit --trailer "report = author" --amend && > ++ git cat-file commit HEAD >commit.msg && > ++ sed -e "1,/^\$/d" commit.msg >actual && > ++ test_cmp expected actual > ++' Nice that you have added such a test!
Christian Couder <christian.couder@gmail.com> 于2021年3月22日周一 下午3:43写道： > > On Mon, Mar 22, 2021 at 5:24 AM ZheNing Hu via GitGitGadget > <gitgitgadget@gmail.com> 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. > > > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > Reported-by: > > Why is there this "Reported-by:" trailer with an empty value? If you > are looking to add trailers to this commit message, you might want to > add them before your "Signed-off-by". > Sorry, It was purely a small accident during testing and I didn't notice it. > > --- > > [GSOC] commit: add --trailer option > > > > Now maintainers or developers can also use commit > > --trailer="Signed-off-by:commiter<email>" from the command line to > > provide trailers to commit messages. This solution may be more > > generalized than v1. > > It's not a big deal as this is not going into the commit message, but > at this point (v13) you might want to tell explicitly what solution v1 > implemented, instead of referring to it. Ok. > > > @@ t/t7502-commit-porcelain.sh: test_expect_success 'sign off' '> + sed -e "1,/^\$/d" commit.msg >actual && > > + test_cmp expected actual > > +' > > ++ > > ++test_expect_success 'commit --trailer with -c and command' ' > > ++ trailer_commit_base && > > ++ cat >expected <<-\EOF && > > ++ hello > > ++ > > ++ Signed-off-by: C O Mitter <committer@example.com> > > ++ Signed-off-by: C1 E1 > > ++ Helped-by: C2 E2 > > ++ Mentored-by: C4 E4 > > ++ Reported-by: A U Thor <author@example.com> > > ++ EOF > > ++ git -c trailer.report.key="Reported-by: " \ > > ++ -c trailer.report.ifexists="replace" \ > > ++ -c trailer.report.command="git log --author=\"\$ARG\" -1 \ > > ++ --format=\"format:%aN <%aE>\"" \ > > ++ commit --trailer "report = author" --amend && > > ++ git cat-file commit HEAD >commit.msg && > > ++ sed -e "1,/^\$/d" commit.msg >actual && > > ++ test_cmp expected actual > > ++' > > Nice that you have added such a test! Thanks. But at the same time I have two little doubt. 1. If we have your config: $ git config trailer.sign.key "Signed-off-by: " $ git config trailer.sign.ifexists replace $ git config trailer.sign.command "git log --author='\$ARG' -1 --format='format:%aN <%aE>'" Then I touch a test.c and use: $ git interpret-trailers --in-place test.c without `--trailer`, See what is happen: It seem like your local repo last commit "name <email>" pair have been record in `test.c`. Could this be considered a bug? 2. `git interpret-trailers --in-place` seem like work on git top-dir, If I am in a sub-dir `b` and I want to change a file such as `d.c`, then I must use `git interpret-trailers --in-place b/d.c` to add some trailers. I think the original intention of `--in-place` is to modify a file similar to "$COMMIT_MSG_FILE", so make it run at top-dir, but this is not reflected in the git documentation. This at least confuses people who use this option for the first time. Is it worth modifying? Or is there something wrong with the design of `--in-place`? -- ZheNing Hu
On Mon, Mar 22, 2021 at 11:23 AM ZheNing Hu <adlternative@gmail.com> wrote: > > Christian Couder <christian.couder@gmail.com> 于2021年3月22日周一 下午3:43写道： > > Nice that you have added such a test! > > Thanks. > > But at the same time I have two little doubt. > > 1. > If we have your config: > > $ git config trailer.sign.key "Signed-off-by: " > $ git config trailer.sign.ifexists replace > $ git config trailer.sign.command "git log --author='\$ARG' -1 > --format='format:%aN <%aE>'" > > Then I touch a test.c and use: > > $ git interpret-trailers --in-place test.c > > without `--trailer`, See what is happen: > > It seem like your local repo last commit "name <email>" pair > have been record in `test.c`. > > Could this be considered a bug? First it seems strange to use `git interpret-trailers` on a "test.c" file. It's supposed to be used on commit messages. Then, as the doc says, every command specified by any "trailer.<token>.command" config option is run at least once when `git interpret-trailers` is run. This is because users might want to automatically add some trailers all the time. If you want nothing to happen when $ARG isn't set, you can change the config option to something like: $ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" && git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true" (This is because it looks like $ARG is replaced only once with the actual value, which is perhaps a bug. Otherwise something like the following might work: git config trailer.sign.command "test -n '\$ARG' && git log --author='\$ARG' -1 --format='format:%aN <%aE>' || true") Then you can run `git interpret-trailers` with the --trim-empty option like this: ------ $ git interpret-trailers --trim-empty --trailer sign=Linus<<EOF EOF Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> ------ or like: ------ $ git interpret-trailers --trim-empty<<EOF > EOF ------
On Mon, Mar 22, 2021 at 11:23 AM ZheNing Hu <adlternative@gmail.com> wrote: > 2. > `git interpret-trailers --in-place` seem like work on git top-dir, > If I am in a sub-dir `b` and I want to change a file such as `d.c`, > then I must use `git interpret-trailers --in-place b/d.c` to add some > trailers. What happens without --in-place? Are the input files read correctly? > I think the original intention of `--in-place` is to modify a file similar to > "$COMMIT_MSG_FILE", so make it run at top-dir, but this is not reflected > in the git documentation. This at least confuses people who use this > option for the first time. Is it worth modifying? Or is there something > wrong with the design of `--in-place`? I haven't checked but there is perhaps a bug in create_in_place_tempfile() in "trailer.c".
Christian Couder <christian.couder@gmail.com> 于2021年3月23日周二 上午5:34写道： > > On Mon, Mar 22, 2021 at 11:23 AM ZheNing Hu <adlternative@gmail.com> wrote: > > > > Christian Couder <christian.couder@gmail.com> 于2021年3月22日周一 下午3:43写道： > > > > Nice that you have added such a test! > > > > Thanks. > > > > But at the same time I have two little doubt. > > > > 1. > > If we have your config: > > > > $ git config trailer.sign.key "Signed-off-by: " > > $ git config trailer.sign.ifexists replace > > $ git config trailer.sign.command "git log --author='\$ARG' -1 > > --format='format:%aN <%aE>'" > > > > Then I touch a test.c and use: > > > > $ git interpret-trailers --in-place test.c > > > > without `--trailer`, See what is happen: > > > > It seem like your local repo last commit "name <email>" pair > > have been record in `test.c`. > > > > Could this be considered a bug? > > First it seems strange to use `git interpret-trailers` on a "test.c" > file. It's supposed to be used on commit messages. > > Then, as the doc says, every command specified by any > "trailer.<token>.command" config option is run at least once when `git > interpret-trailers` is run. This is because users might want to > automatically add some trailers all the time. > Well, I understand it now. > If you want nothing to happen when $ARG isn't set, you can change the > config option to something like: > > $ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" && > git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true" > > (This is because it looks like $ARG is replaced only once with the > actual value, which is perhaps a bug. Otherwise something like the > following might work: this is because `$ARG` is replaced in "trailer.c" by "strbuf_replace" which only replcae the specified string for only one time, I think `strbuf_replace()` can be changed like: @@ -110,8 +110,9 @@ static inline int is_blank_line(const char *str) static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b) { - const char *ptr = strstr(sb->buf, a); - if (ptr) + char *ptr = sb->buf; + + while ((ptr = strstr(ptr, a))) strbuf_splice(sb, ptr - sb->buf, strlen(a), b, strlen(b)); } > > git config trailer.sign.command "test -n '\$ARG' && git log > --author='\$ARG' -1 --format='format:%aN <%aE>' || true") > > Then you can run `git interpret-trailers` with the --trim-empty option > like this: > > ------ > $ git interpret-trailers --trim-empty --trailer sign=Linus<<EOF > EOF > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > ------ > > or like: > > ------ > $ git interpret-trailers --trim-empty<<EOF > > EOF > > ------ Thanks for effective solution. Maybe I should also correct this part of the test.
Christian Couder <christian.couder@gmail.com> writes: > If you want nothing to happen when $ARG isn't set, you can change the > config option to something like: > > $ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" && > git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true" > > (This is because it looks like $ARG is replaced only once with the > actual value, which is perhaps a bug. Otherwise something like the > following might work: I do not know the origin of that code in trailers.c but it feels quite confused and error prone to use textual replacement with strbuf_replace(). Why doesn't the code, which knows it will use shell to execute the command line given by the end user in the configuration, to just export ARG as an environment variable and be done with it? It would also avoid quoting problem etc.
Christian Couder <christian.couder@gmail.com> 于2021年3月23日周二 上午5:55写道： > > On Mon, Mar 22, 2021 at 11:23 AM ZheNing Hu <adlternative@gmail.com> wrote: > > > 2. > > `git interpret-trailers --in-place` seem like work on git top-dir, > > If I am in a sub-dir `b` and I want to change a file such as `d.c`, > > then I must use `git interpret-trailers --in-place b/d.c` to add some > > trailers. > > What happens without --in-place? Are the input files read correctly? It's still wrong. the git die() in `read_input_file` of "trailer.c". > > > I think the original intention of `--in-place` is to modify a file similar to > > "$COMMIT_MSG_FILE", so make it run at top-dir, but this is not reflected > > in the git documentation. This at least confuses people who use this > > option for the first time. Is it worth modifying? Or is there something > > wrong with the design of `--in-place`? > > I haven't checked but there is perhaps a bug in > create_in_place_tempfile() in "trailer.c". I haven't check finished. But I think it may do something like `chdir()`. Thanks.
On Tue, Mar 23, 2021 at 7:19 AM Junio C Hamano <gitster@pobox.com> wrote: > > Christian Couder <christian.couder@gmail.com> writes: > > > If you want nothing to happen when $ARG isn't set, you can change the > > config option to something like: > > > > $ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" && > > git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true" > > > > (This is because it looks like $ARG is replaced only once with the > > actual value, which is perhaps a bug. Otherwise something like the > > following might work: > > I do not know the origin of that code in trailers.c but it feels > quite confused and error prone to use textual replacement with > strbuf_replace(). Why doesn't the code, which knows it will use > shell to execute the command line given by the end user in the > configuration, to just export ARG as an environment variable and > be done with it? It would also avoid quoting problem etc. Yeah, I agree that would be better.
Junio C Hamano <gitster@pobox.com> 于2021年3月23日周二 下午2:19写道： > > Christian Couder <christian.couder@gmail.com> writes: > > > If you want nothing to happen when $ARG isn't set, you can change the > > config option to something like: > > > > $ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" && > > git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true" > > > > (This is because it looks like $ARG is replaced only once with the > > actual value, which is perhaps a bug. Otherwise something like the > > following might work: > > I do not know the origin of that code in trailers.c but it feels > quite confused and error prone to use textual replacement with > strbuf_replace(). Why doesn't the code, which knows it will use > shell to execute the command line given by the end user in the > configuration, to just export ARG as an environment variable and > be done with it? It would also avoid quoting problem etc. > Maybe like this? -#define TRAILER_ARG_STRING "$ARG" - static const char *git_generated_prefixes[] = { "Signed-off-by: ", "(cherry picked from commit ", @@ -222,13 +220,17 @@ static char *apply_command(const char *command, const char *arg) struct strbuf buf = STRBUF_INIT; struct child_process cp = CHILD_PROCESS_INIT; char *result; + const char *const *var; strbuf_addstr(&cmd, command); - if (arg) - strbuf_replace(&cmd, TRAILER_ARG_STRING, arg); + for (var = local_repo_env; *var; var++) + strvec_push(&cp.env_array, *var); + if (arg) { + strvec_pushf(&cp.env_array, "ARG=%s", arg); + } strvec_push(&cp.args, cmd.buf); - cp.env = local_repo_env; + cp.no_stdin = 1; cp.use_shell = 1;
On Tue, Mar 23, 2021 at 11:35 AM ZheNing Hu <adlternative@gmail.com> wrote: > > Junio C Hamano <gitster@pobox.com> 于2021年3月23日周二 下午2:19写道： > Maybe like this? Yeah, it's a good idea to work on this, but please make it an independent patch unrelated to adding --trailer to `git commit`. Also the documentation might need a bit of tweeking to tell that ARG is now an exported environment variable. > -#define TRAILER_ARG_STRING "$ARG" It might still be better to use a #define or a const char [] to avoid hard coding "ARG" below. > static const char *git_generated_prefixes[] = { > "Signed-off-by: ", > "(cherry picked from commit ", > @@ -222,13 +220,17 @@ static char *apply_command(const char *command, > const char *arg) > struct strbuf buf = STRBUF_INIT; > struct child_process cp = CHILD_PROCESS_INIT; > char *result; > + const char *const *var; > > strbuf_addstr(&cmd, command); > - if (arg) > - strbuf_replace(&cmd, TRAILER_ARG_STRING, arg); > + for (var = local_repo_env; *var; var++) > + strvec_push(&cp.env_array, *var); > + if (arg) { > + strvec_pushf(&cp.env_array, "ARG=%s", arg); > + } You can drop the "{" and "}" above. > strvec_push(&cp.args, cmd.buf); > - cp.env = local_repo_env; > + > cp.no_stdin = 1; > cp.use_shell = 1; Thanks!
Christian Couder <christian.couder@gmail.com> writes: > On Tue, Mar 23, 2021 at 7:19 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> Christian Couder <christian.couder@gmail.com> writes: >> >> > If you want nothing to happen when $ARG isn't set, you can change the >> > config option to something like: >> > >> > $ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" && >> > git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true" >> > >> > (This is because it looks like $ARG is replaced only once with the >> > actual value, which is perhaps a bug. Otherwise something like the >> > following might work: >> >> I do not know the origin of that code in trailers.c but it feels >> quite confused and error prone to use textual replacement with >> strbuf_replace(). Why doesn't the code, which knows it will use >> shell to execute the command line given by the end user in the >> configuration, to just export ARG as an environment variable and >> be done with it? It would also avoid quoting problem etc. > > Yeah, I agree that would be better. It probably would have been better to do so before the feature got unleased to the public, but doing such a change retroactively would introduce regression for those who were using ARG that happens to be safe from shell quoting rules. For example, if the trailer.*.command were echo '$ARG' and the argument 'h e l l o' were to be given to it, then the current code would have textually expanded $ARG with the argument and caused echo 'h e l l o' to run, which would have been "fine" [*1*]. But exporting the environment ARG would "break" such a setting that has been "working perfectly well" for the user. Because of the single-quotes around $ARG, the command now will give literal four letter string $ARG and not 'h e l l o'. We should think such potential ramifications of changing it (and also not changing it) through before deciding what to do about it. Although I have a feeling that not many people would miss '$ARG' inside a pair of single-quotes to be replaced textually and it would be OK to make a backward incompatible bugfix, the safer and better way is not all that difficult, so I am inclined to suggest going the usual "deprecate and replace and then later remove" dance. The normal sequence of replacing a "sort of works but not recommended" feature with a "better and safer, but can break a setting that has been 'working'" feature is: - Announce deprecation of trailer.x.command and add and advertise a similar traier.x.cmd that (1) exports environment variable ARG, or (2) passes the argument as a positional parameter [*], as a replacement. Explain the reason for deprecation (i.e. unsafe substitution that works only once). When .cmd exists, .command is ignored for the corresponding trailer.x - Wait for a few releases and then remove trailer.x.command. and that is the safest way to fix this "bug". [Footnotes] *1* If the argument were ';rm -rf .;' then it wouldn't have been fine, though, and that is how the current code solicited "Huh?" reaction out of me. *2* If we passed the argument as a positional parameter, the example you gave in the quoted part of the message would become something like this: [trailer "sign"] cmd = test -n "$1" && git log --author="$1" -1 --format='%aN <%aE>'
ZheNing Hu <adlternative@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> 于2021年3月23日周二 下午2:19写道： >> >> Christian Couder <christian.couder@gmail.com> writes: >> >> > If you want nothing to happen when $ARG isn't set, you can change the >> > config option to something like: >> > >> > $ git config trailer.sign.command "NAME='\$ARG'; test -n \"\$NAME\" && >> > git log --author=\"\$NAME\" -1 --format='format:%aN <%aE>' || true" >> > >> > (This is because it looks like $ARG is replaced only once with the >> > actual value, which is perhaps a bug. Otherwise something like the >> > following might work: >> >> I do not know the origin of that code in trailers.c but it feels >> quite confused and error prone to use textual replacement with >> strbuf_replace(). Why doesn't the code, which knows it will use >> shell to execute the command line given by the end user in the >> configuration, to just export ARG as an environment variable and >> be done with it? It would also avoid quoting problem etc. >> > > Maybe like this? Code is not an important part. We should think through ramifications of making (and not making) such a change first.
> > It probably would have been better to do so before the feature got > unleased to the public, but doing such a change retroactively would > introduce regression for those who were using ARG that happens to be > safe from shell quoting rules. > > For example, if the trailer.*.command were > > echo '$ARG' > > and the argument 'h e l l o' were to be given to it, then the > current code would have textually expanded $ARG with the argument > and caused > > echo 'h e l l o' > > to run, which would have been "fine" [*1*]. > > But exporting the environment ARG would "break" such a setting that > has been "working perfectly well" for the user. Because of the > single-quotes around $ARG, the command now will give literal four > letter string $ARG and not 'h e l l o'. > > We should think such potential ramifications of changing it (and > also not changing it) through before deciding what to do about it. > > Although I have a feeling that not many people would miss '$ARG' > inside a pair of single-quotes to be replaced textually and it would > be OK to make a backward incompatible bugfix, the safer and better > way is not all that difficult, so I am inclined to suggest going the > usual "deprecate and replace and then later remove" dance. > I think what you mean is that my patch breaks the principle of "forward compatibility", which may make it impossible for users who previously worked with `'$ARG'`. > The normal sequence of replacing a "sort of works but not > recommended" feature with a "better and safer, but can break a > setting that has been 'working'" feature is: > > - Announce deprecation of trailer.x.command and add and advertise a > similar traier.x.cmd that (1) exports environment variable ARG, > or (2) passes the argument as a positional parameter [*], as a > replacement. Explain the reason for deprecation (i.e. unsafe > substitution that works only once). When .cmd exists, .command > is ignored for the corresponding trailer.x > As your example below: cmd = test -n "$1" && git log --author="$1" -1 --format='%aN <%aE>' I would like to use this way. It may be a bit cumbersome to do "deprecate and replace and then later remove", Should documents tell users that the old method "trailer.command" has been replaced by "trialer.cmd"? Or tell users that "trailer.cmd" is a new feature? Keep these doubts, I will try go to code it. > - Wait for a few releases and then remove trailer.x.command. > > and that is the safest way to fix this "bug". > > > [Footnotes] > > *1* If the argument were > > ';rm -rf .;' > > then it wouldn't have been fine, though, and that is how the > current code solicited "Huh?" reaction out of me. > A little scary. :-) > > *2* If we passed the argument as a positional parameter, the example > you gave in the quoted part of the message would become > something like this: > > [trailer "sign"] > cmd = test -n "$1" && git log --author="$1" -1 --format='%aN <%aE>'
> > Maybe like this? > > Code is not an important part. We should think through > ramifications of making (and not making) such a change first. Sorry for without careful consideration. Thank for this valuable suggestion.
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 17150fa7eabe..3fe7ef33cb07 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -14,7 +14,8 @@ 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>...] + [(--trailer <token>[(=|:)<value>])...] [-S[<keyid>]] + [--] [<pathspec>...] DESCRIPTION ----------- @@ -166,6 +167,17 @@ 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 to the commit message.) + The `trailer.*` configuration variables + (linkgit:git-interpret-trailers[1]) can be used to define if + a duplicated trailer is omitted, where in the run of trailers + each trailer would appear, and other 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..4b06672bd07d 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", git_path_commit_editmsg(), NULL); + strvec_pushv(&run_trailer.args, trailer_args.v); + run_trailer.git_cmd = 1; + if (run_command(&run_trailer)) + die(_("unable to pass trailers to --trailers")); + 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_("add custom trailer(s)"), 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..dd044fcea31c 100755 --- a/t/t7502-commit-porcelain.sh +++ b/t/t7502-commit-porcelain.sh @@ -38,6 +38,16 @@ check_summary_oneline() { test_cmp exp act } +trailer_commit_base () { + echo "fun" >>file && + git add file && + git commit -s --trailer "Signed-off-by=C1 E1 " \ + --trailer "Helped-by:C2 E2 " \ + --trailer "Reported-by=C3 E3" \ + --trailer "Mentored-by:C4 E4" \ + -m "hello" +} + test_expect_success 'output summary format' ' echo new >file1 && @@ -154,6 +164,308 @@ test_expect_success 'sign off' ' ' +test_expect_success 'commit --trailer with "="' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + EOF + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "replace" as ifexists' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Helped-by: C3 E3 + EOF + git -c trailer.ifexists="replace" \ + commit --trailer "Mentored-by: C4 E4" \ + --trailer "Helped-by: C3 E3" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "add" as ifexists' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Reported-by: C3 E3 + Mentored-by: C4 E4 + EOF + git -c trailer.ifexists="add" \ + commit --trailer "Reported-by: C3 E3" \ + --trailer "Mentored-by: C4 E4" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "donothing" as ifexists' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Reviewed-by: C6 E6 + EOF + git -c trailer.ifexists="donothing" \ + commit --trailer "Mentored-by: C5 E5" \ + --trailer "Reviewed-by: C6 E6" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "addIfDifferent" as ifexists' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Mentored-by: C5 E5 + EOF + git -c trailer.ifexists="addIfDifferent" \ + commit --trailer "Reported-by: C3 E3" \ + --trailer "Mentored-by: C5 E5" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "addIfDifferentNeighbor" as ifexists' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Reported-by: C3 E3 + EOF + git -c trailer.ifexists="addIfDifferentNeighbor" \ + commit --trailer "Mentored-by: C4 E4" \ + --trailer "Reported-by: C3 E3" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "end" as where' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Reported-by: C3 E3 + Mentored-by: C4 E4 + EOF + git -c trailer.where="end" \ + commit --trailer "Reported-by: C3 E3" \ + --trailer "Mentored-by: C4 E4" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "start" as where' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C1 E1 + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + EOF + git -c trailer.where="start" \ + commit --trailer "Signed-off-by: C O Mitter <committer@example.com>" \ + --trailer "Signed-off-by: C1 E1" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "after" as where' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Mentored-by: C5 E5 + EOF + git -c trailer.where="after" \ + commit --trailer "Mentored-by: C4 E4" \ + --trailer "Mentored-by: C5 E5" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "before" as where' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C2 E2 + Mentored-by: C3 E3 + Mentored-by: C4 E4 + EOF + git -c trailer.where="before" \ + commit --trailer "Mentored-by: C3 E3" \ + --trailer "Mentored-by: C2 E2" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "donothing" as ifmissing' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Helped-by: C5 E5 + EOF + git -c trailer.ifmissing="donothing" \ + commit --trailer "Helped-by: C5 E5" \ + --trailer "Based-by: C6 E6" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and "add" as ifmissing' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Reported-by: C3 E3 + Mentored-by: C4 E4 + Helped-by: C5 E5 + Based-by: C6 E6 + EOF + git -c trailer.ifmissing="add" \ + commit --trailer "Helped-by: C5 E5" \ + --trailer "Based-by: C6 E6" \ + --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c ack.key ' ' + echo "fun" >>file1 && + git add file1 && + cat >expected <<-\EOF && + hello + + Acked-by: Peff + EOF + git -c trailer.ack.key="Acked-by" \ + commit --trailer "ack = Peff" -m "hello" && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and ":=#" as separators' ' + echo "fun" >>file1 && + git add file1 && + cat >expected <<-\EOF && + I hate bug + + Bug #42 + EOF + git -c trailer.separators=":=#" \ + -c trailer.bug.key="Bug #" \ + commit --trailer "bug = 42" -m "I hate bug" && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + +test_expect_success 'commit --trailer with -c and command' ' + trailer_commit_base && + cat >expected <<-\EOF && + hello + + Signed-off-by: C O Mitter <committer@example.com> + Signed-off-by: C1 E1 + Helped-by: C2 E2 + Mentored-by: C4 E4 + Reported-by: A U Thor <author@example.com> + EOF + git -c trailer.report.key="Reported-by: " \ + -c trailer.report.ifexists="replace" \ + -c trailer.report.command="git log --author=\"\$ARG\" -1 \ + --format=\"format:%aN <%aE>\"" \ + commit --trailer "report = author" --amend && + git cat-file commit HEAD >commit.msg && + sed -e "1,/^\$/d" commit.msg >actual && + test_cmp expected actual +' + test_expect_success 'multiple -m' ' >negative &&