| Message ID | pull.913.v11.git.1618672417.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | trailer: add new .cmd config option | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > In https://lore.kernel.org/git/xmqqv99i4ck2.fsf@gitster.g/ Junio and > Christian talked about the problem of using strbuf_replace() to replace > $ARG: > > 1. if the user's script has more than one $ARG, only the first one will be > replaced, which is incorrected. > 2. $ARG is textually replaced without shell syntax, which may result a > broken command when $ARG include some unmatching single quote, very > unsafe. > > Now pass trailer value as $1 to the trailer command with another > trailer.<token>.cmd config, to solve these above problems. > > We are now writing documents that are more readable and correct than before. Here is a good spot to summarize what changed since the previous round. It seems that this now has "exit non-zero to tell the caller not to add the trailer for this execuation". Is that the only change you made? I was hoping that we'd declare victory with what was in v10 (with possibly typos and minor stylistic fixes if needed---I no longer remember details), let it go through the usual course of cooking in 'next' and merged down to 'master', and then after the dust settles, we'd be adding this "by exiting with non-zero status, scripts can signal a trailer not to be added for a particular invocation" as a new feature, if it turns out to be necessary. But let's see what's new in this iteration. > +#!/bin/sh > -+test -n "$1" && git shortlog -s --author="$1" HEAD || true > ++if test "$#" != 1 > ++then > ++ exit 1 > ++else > ++ test -n "$1" && git shortlog -s --author="$1" HEAD || true > ++fi I find this dubious. Why not if test "$#" != 1 || test -z "$1" then exit 1 else git shortlog -s --author="$1" HEAD fi That is, if you happened to give an empty string, your version gives "" to <value> and returns success, letting a trailer "cnt:" with empty value. Is that what we really want? > +$ git config trailer.cnt.key "Commit-count: " > +$ git config trailer.cnt.ifExists "addIfDifferentNeighbor" > +$ git config trailer.cnt.cmd "~/bin/gcount" > +$ git interpret-trailers --trailer="cnt:Junio" --trailer="cnt:Linus Torvalds"<<EOF > +> subject > -+> > ++> > +> message > -+> > ++> > +> EOF > +subject > + > @@ Documentation/git-interpret-trailers.txt: subject > +------------ > +$ cat ~/bin/glog-grep > +#!/bin/sh > -+test -n "$1" && git log --grep "$1" --pretty=reference -1 || true > ++if test "$#" != 1 > ++then > ++ exit 1 > ++else > ++ test -n "$1" && git log --grep "$1" --pretty=reference -1 || true > ++fi Ditto. > + if (capture_command(&cp, &buf, 1024)) { > +- error(_("running trailer command '%s' failed"), cmd.buf); > + strbuf_release(&buf); > +- result = xstrdup(""); > ++ if (!conf->cmd || arg) { > ++ error(_("running trailer command '%s' failed"), cmd.buf); I am not sure about this part. If .cmd (the new style) exits with a non-zero status for user-supplied --trailer=<token>:<value> (because it did not like the <value>), is that "running failed"? The script is expected to express yes/no with its exit status, so I would say it is not failing, but successfully expressed its displeasure and vetoed the particular trailer from getting added. IOW, "|| arg" part in the condition feels iffy to me. > ++ result = xstrdup(""); > ++ } else > ++ result = NULL; > + } else { > + strbuf_trim(&buf); > + result = strbuf_detach(&buf, NULL); OK.
Junio C Hamano <gitster@pobox.com> 于2021年4月18日周日 上午6:26写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > In https://lore.kernel.org/git/xmqqv99i4ck2.fsf@gitster.g/ Junio and > > Christian talked about the problem of using strbuf_replace() to replace > > $ARG: > > > > 1. if the user's script has more than one $ARG, only the first one will be > > replaced, which is incorrected. > > 2. $ARG is textually replaced without shell syntax, which may result a > > broken command when $ARG include some unmatching single quote, very > > unsafe. > > > > Now pass trailer value as $1 to the trailer command with another > > trailer.<token>.cmd config, to solve these above problems. > > > > We are now writing documents that are more readable and correct than before. > > Here is a good spot to summarize what changed since the previous > round. > > It seems that this now has "exit non-zero to tell the caller not to > add the trailer for this execuation". Is that the only change you > made? > Yes, I think the more accurate one should be "exit non-zero to tell the caller not to add the trailer for this implicit execuation", You also said below, it may not be so good. > I was hoping that we'd declare victory with what was in v10 (with > possibly typos and minor stylistic fixes if needed---I no longer > remember details), let it go through the usual course of cooking in > 'next' and merged down to 'master', and then after the dust settles, > we'd be adding this "by exiting with non-zero status, scripts can > signal a trailer not to be added for a particular invocation" as a > new feature, if it turns out to be necessary. > Thanks for your and Christian's help this month! OK, I understand, then I can wait for a while until `trailer_cmd` merge to master. > But let's see what's new in this iteration. > > > > +#!/bin/sh > > -+test -n "$1" && git shortlog -s --author="$1" HEAD || true > > ++if test "$#" != 1 > > ++then > > ++ exit 1 > > ++else > > ++ test -n "$1" && git shortlog -s --author="$1" HEAD || true > > ++fi > > I find this dubious. Why not > > if test "$#" != 1 || test -z "$1" > then > exit 1 > else > git shortlog -s --author="$1" HEAD > fi > > That is, if you happened to give an empty string, your version gives > "" to <value> and returns success, letting a trailer "cnt:" with > empty value. Is that what we really want? If it's the user use `--trailer="cnt:"` instread of command implict running, I think keep it is right. In fact, it should be said that it is equivalent to exit(0) if the user use `--trailer="cnt:"`. > > > +$ git config trailer.cnt.key "Commit-count: " > > +$ git config trailer.cnt.ifExists "addIfDifferentNeighbor" > > +$ git config trailer.cnt.cmd "~/bin/gcount" > > +$ git interpret-trailers --trailer="cnt:Junio" --trailer="cnt:Linus Torvalds"<<EOF > > +> subject > > -+> > > ++> > > +> message > > -+> > > ++> > > +> EOF > > +subject > > + > > @@ Documentation/git-interpret-trailers.txt: subject > > +------------ > > +$ cat ~/bin/glog-grep > > +#!/bin/sh > > -+test -n "$1" && git log --grep "$1" --pretty=reference -1 || true > > ++if test "$#" != 1 > > ++then > > ++ exit 1 > > ++else > > ++ test -n "$1" && git log --grep "$1" --pretty=reference -1 || true > > ++fi > > Ditto. > > > + if (capture_command(&cp, &buf, 1024)) { > > +- error(_("running trailer command '%s' failed"), cmd.buf); > > + strbuf_release(&buf); > > +- result = xstrdup(""); > > ++ if (!conf->cmd || arg) { > > ++ error(_("running trailer command '%s' failed"), cmd.buf); > > I am not sure about this part. If .cmd (the new style) exits with a > non-zero status for user-supplied --trailer=<token>:<value> (because > it did not like the <value>), is that "running failed"? The script > is expected to express yes/no with its exit status, so I would say it > is not failing, but successfully expressed its displeasure and vetoed > the particular trailer from getting added. IOW, "|| arg" part in > the condition feels iffy to me. > Well, you mean we can take advantage of non-zero exits instead of just removing implicitly executed content. I argee with you, this place is worth change. > > ++ result = xstrdup(""); > > ++ } else > > ++ result = NULL; > > + } else { > > + strbuf_trim(&buf); > > + result = strbuf_detach(&buf, NULL); > > OK. Thanks. -- ZheNing Hu
ZheNing Hu <adlternative@gmail.com> writes: > OK, I understand, then I can wait for a while until `trailer_cmd` merge > to master. > >> But let's see what's new in this iteration. >> >> >> > +#!/bin/sh >> > -+test -n "$1" && git shortlog -s --author="$1" HEAD || true >> > ++if test "$#" != 1 >> > ++then >> > ++ exit 1 >> > ++else >> > ++ test -n "$1" && git shortlog -s --author="$1" HEAD || true >> > ++fi >> >> I find this dubious. Why not >> >> if test "$#" != 1 || test -z "$1" >> then >> exit 1 >> else >> git shortlog -s --author="$1" HEAD >> fi >> >> That is, if you happened to give an empty string, your version gives >> "" to <value> and returns success, letting a trailer "cnt:" with >> empty value. Is that what we really want? > > If it's the user use `--trailer="cnt:"` instread of command implict running, > I think keep it is right. No, if you give an empty string, you'd end up running "shortlog" with --author="" and give whatever random number it comes up with, which I do not think is what you would want. That is why --trailer=cnt: without name to match --author can be rejected with "exit 1" to demonstrate the feature. The .cmd can squelch not just the "unasked for extra invocation", but invocation from the command line whose <value> was bogus, unlike the .runmode feature we've seen proposed earlier. >> > + if (capture_command(&cp, &buf, 1024)) { >> > +- error(_("running trailer command '%s' failed"), cmd.buf); >> > + strbuf_release(&buf); >> > +- result = xstrdup(""); >> > ++ if (!conf->cmd || arg) { >> > ++ error(_("running trailer command '%s' failed"), cmd.buf); >> >> I am not sure about this part. If .cmd (the new style) exits with a >> non-zero status for user-supplied --trailer=<token>:<value> (because >> it did not like the <value>), is that "running failed"? The script >> is expected to express yes/no with its exit status, so I would say it >> is not failing, but successfully expressed its displeasure and vetoed >> the particular trailer from getting added. IOW, "|| arg" part in >> the condition feels iffy to me. > > Well, you mean we can take advantage of non-zero exits instead of > just removing implicitly executed content. I argee with you, this > place is worth change. Yup, that is what I meant. In any case, let's see how well the base topic fares. Thanks.
Junio C Hamano <gitster@pobox.com> 于2021年4月21日周三 上午8:09写道： > > ZheNing Hu <adlternative@gmail.com> writes: > > > OK, I understand, then I can wait for a while until `trailer_cmd` merge > > to master. > > > >> But let's see what's new in this iteration. > >> > >> > >> > +#!/bin/sh > >> > -+test -n "$1" && git shortlog -s --author="$1" HEAD || true > >> > ++if test "$#" != 1 > >> > ++then > >> > ++ exit 1 > >> > ++else > >> > ++ test -n "$1" && git shortlog -s --author="$1" HEAD || true > >> > ++fi > >> > >> I find this dubious. Why not > >> > >> if test "$#" != 1 || test -z "$1" > >> then > >> exit 1 > >> else > >> git shortlog -s --author="$1" HEAD > >> fi > >> > >> That is, if you happened to give an empty string, your version gives > >> "" to <value> and returns success, letting a trailer "cnt:" with > >> empty value. Is that what we really want? > > > > If it's the user use `--trailer="cnt:"` instread of command implict running, > > I think keep it is right. > > No, if you give an empty string, you'd end up running "shortlog" > with --author="" and give whatever random number it comes up with, > which I do not think is what you would want. > > That is why --trailer=cnt: without name to match --author can be > rejected with "exit 1" to demonstrate the feature. The .cmd can > squelch not just the "unasked for extra invocation", but invocation > from the command line whose <value> was bogus, unlike the .runmode > feature we've seen proposed earlier. > I admit that your idea makes sense, but we actually have another requirement: Construct a trailer with an empty value. The Commit-Count example above may not be good, Let’s take a look at the Signed-off-by. e.g. `--trailer="sign:"`, we expect to output a "Signed-off-by: ", then we can fill it with the "name <email>" pair we want, this is when we shouldn't return non-zero and suppress its output. > >> > + if (capture_command(&cp, &buf, 1024)) { > >> > +- error(_("running trailer command '%s' failed"), cmd.buf); > >> > + strbuf_release(&buf); > >> > +- result = xstrdup(""); > >> > ++ if (!conf->cmd || arg) { > >> > ++ error(_("running trailer command '%s' failed"), cmd.buf); > >> > >> I am not sure about this part. If .cmd (the new style) exits with a > >> non-zero status for user-supplied --trailer=<token>:<value> (because > >> it did not like the <value>), is that "running failed"? The script > >> is expected to express yes/no with its exit status, so I would say it > >> is not failing, but successfully expressed its displeasure and vetoed > >> the particular trailer from getting added. IOW, "|| arg" part in > >> the condition feels iffy to me. > > > > Well, you mean we can take advantage of non-zero exits instead of > > just removing implicitly executed content. I argee with you, this > > place is worth change. > > Yup, that is what I meant. > > In any case, let's see how well the base topic fares. > Yes, I don't know if Christian agrees with temporary situation. > Thanks. Thanks. -- ZheNing Hu
ZheNing Hu <adlternative@gmail.com> writes: > I admit that your idea makes sense, but we actually have another requirement: > Construct a trailer with an empty value. It can be done with a different script given to .cmd, which would say "exit 0" when allowing an empty string given as its input to appear in the output. I was reacting what the "count" example does, and found that counting commits by all authors (that is what an empty string would match when given to --author="") somewhat illogical in the context of that example. After all, these examples are to pique readers' interest by demonstrating what the mechanism can do and how it can be used, and for this feature, I think showing that (1) we can squelch the output from unasked-for execution; (2) we can reject --trailer=<key>:<value> when we do not like the given <value>; (3) we can insert the trailer with the value we compute (and it is OK for the computed result happens to be an empty string). should be plenty sufficient.
Junio C Hamano <gitster@pobox.com> 于2021年4月22日周四 上午7:40写道： > > ZheNing Hu <adlternative@gmail.com> writes: > > > I admit that your idea makes sense, but we actually have another requirement: > > Construct a trailer with an empty value. > > It can be done with a different script given to .cmd, which would > say "exit 0" when allowing an empty string given as its input to > appear in the output. > Now I think that we should keep those trailers which ask for a "name <email>" pair, like "Helped-by", "Signed-off-by", when we provide a "help:","sign:" in command line, This allows the user to dynamically fill in the "name <email>" pair of other people in the commit message later. It is worthwhile for users to exit with exit(0). But those dispensable things like "Commit-Count", It must depend on a person's statistics in the git repository. So "cnt:" is meaningless, users' script can let it exit(1). > I was reacting what the "count" example does, and found that > counting commits by all authors (that is what an empty string would > match when given to --author="") somewhat illogical in the context > of that example. > The "Commit-Count" in the example here can only target a specific person, which has great limitations. I have a bold idea: Our current --trailer can only fill in one data item, and we don't expect it to fill in multiple rows. something like "Commit-Count", we hope to count the number of commits from everyone. But as we can see: Commit-count: 7 Linus Arver 1117 Linus Torvalds So If we have the opportunity to capture every line or every "block" of content, and feed it to git interpret-trailer, maybe we can get something like: Commit-count: 7 Linus Arver Commit-count: 1117 Linus Torvalds This will definitely make it easy for us to generate a lot of trailer at once. For example, a newbie like me, after making a patch, want to --cc all authors of one file, maybe I only need a command to get it. I don't know if it's a bit whimsical. > After all, these examples are to pique readers' interest by > demonstrating what the mechanism can do and how it can be used, and > for this feature, I think showing that > > (1) we can squelch the output from unasked-for execution; > > (2) we can reject --trailer=<key>:<value> when we do not like the > given <value>; > > (3) we can insert the trailer with the value we compute (and it is > OK for the computed result happens to be an empty string). > > should be plenty sufficient. OK. I will add these three examples in the new patch (when .cmd merge to master). Thanks. -- ZheNing Hu
ZheNing Hu <adlternative@gmail.com> writes: > Now I think that we should keep those trailers which ask for a > "name <email>" pair, like "Helped-by", "Signed-off-by", when we > provide a "help:","sign:" in command line, This allows the user to > dynamically fill in the "name <email>" pair of other people in the > commit message later. It is worthwhile for users to exit with exit(0). > > But those dispensable things like "Commit-Count", It must depend > on a person's statistics in the git repository. So "cnt:" is meaningless, > users' script can let it exit(1). Perhaps, but at this point what you think (or what I think) does not matter. That was the whole point of letting .cmd script signal Git if the result from the invocation should be kept or discarded with its exit status. What would be sufficient here for us to do is to agree that it would be good to have a minimal set (perhaps a pair) of examples to demonstrate that the script can choose to keep or discard a meaningless trailer entry with its exit status.
Junio C Hamano <gitster@pobox.com> 于2021年4月27日周二 下午2:49写道： > > ZheNing Hu <adlternative@gmail.com> writes: > > > Now I think that we should keep those trailers which ask for a > > "name <email>" pair, like "Helped-by", "Signed-off-by", when we > > provide a "help:","sign:" in command line, This allows the user to > > dynamically fill in the "name <email>" pair of other people in the > > commit message later. It is worthwhile for users to exit with exit(0). > > > > But those dispensable things like "Commit-Count", It must depend > > on a person's statistics in the git repository. So "cnt:" is meaningless, > > users' script can let it exit(1). > > Perhaps, but at this point what you think (or what I think) does not > matter. That was the whole point of letting .cmd script signal Git > if the result from the invocation should be kept or discarded with > its exit status. What would be sufficient here for us to do is to > agree that it would be good to have a minimal set (perhaps a pair) > of examples to demonstrate that the script can choose to keep or > discard a meaningless trailer entry with its exit status. Yes, I argee. Due to previous attempts, it seems that such an example is well given: "Commit-Count" is the trailer that should be discarded. "Signed-off-by" is the trailer worth be kept. Thanks. -- ZheNing Hu
In https://lore.kernel.org/git/xmqqv99i4ck2.fsf@gitster.g/ Junio and Christian talked about the problem of using strbuf_replace() to replace $ARG: 1. if the user's script has more than one $ARG, only the first one will be replaced, which is incorrected. 2. $ARG is textually replaced without shell syntax, which may result a broken command when $ARG include some unmatching single quote, very unsafe. Now pass trailer value as $1 to the trailer command with another trailer.<token>.cmd config, to solve these above problems. We are now writing documents that are more readable and correct than before. ZheNing Hu (2): [GSOC] docs: correct description of .command [GSOC] trailer: add new .cmd config option Documentation/git-interpret-trailers.txt | 111 ++++++++++++++++++--- t/t7513-interpret-trailers.sh | 122 +++++++++++++++++++++++ trailer.c | 79 ++++++++++++--- 3 files changed, 280 insertions(+), 32 deletions(-) base-commit: 142430338477d9d1bb25be66267225fb58498d92 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-913%2Fadlternative%2Ftrailer-pass-ARG-env-v11 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v11 Pull-Request: https://github.com/gitgitgadget/git/pull/913 Range-diff vs v10: 1: 8129ef6c476b ! 1: 34210e5bd3da [GSOC] docs: correct descript of trailer.<token>.command @@ Metadata Author: ZheNing Hu <adlternative@gmail.com> ## Commit message ## - [GSOC] docs: correct descript of trailer.<token>.command + [GSOC] docs: correct description of .command In the original documentation of `trailer.<token>.command`, some descriptions are easily misunderstood. So let's modify 2: daa889bd0ade ! 2: 9c0fc91aba24 [GSOC] trailer: add new .cmd config option @@ Commit message replaced with the value given to the `interpret-trailer` command for the token in a '--trailer <token>=<value>' argument. - This has three downsides: + This has two downsides: * The use of $ARG in the mechanism misleads the users that the value is passed in the shell variable, and tempt them @@ Commit message a broken command that is not syntactically correct (or worse). - * The first occurrence of substring `$ARG` will be replaced - with the empty string, in the command when the command is - first called to add a trailer with the specified <token>. - This is a bad design, the nature of automatic execution - causes it to add a trailer that we don't expect. - Introduce a new `trailer.<token>.cmd` configuration that takes higher precedence to deprecate and eventually remove `trailer.<token>.command`, which passes the value as an @@ Commit message refer to the value as positional argument, $1, in their scripts. At the same time, in order to allow `git interpret-trailers` to better simulate the behavior - of `git command -s`, 'trailer.<token>.cmd' will not - automatically execute. + of `git command -s`, the first implicitly executed command + will not pass positional parameters, users can use this + feature to suppress its output. Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Christian Couder <christian.couder@gmail.com> @@ Documentation/git-interpret-trailers.txt: leading and trailing whitespace trimme -occurrence of substring `$ARG` in the command. This way the -command can produce a <value> computed from the <value> passed -in the '--trailer <token>=<value>' argument. --+ ++of these arguments, if any, will be passed to the command as its ++first argument. This way the command can produce a <value> computed ++from the <value> passed in the '--trailer <token>=<value>' argument. + + -For consistency, the first occurrence of substring `$ARG` is -also replaced, this time with the empty string, in the command -when the command is first called to add a trailer with the -specified <token>. -+of these arguments, if any, will be passed to the command as its -+first argument. This way the command can produce a <value> computed -+from the <value> passed in the '--trailer <token>=<value>' argument. ++It is worth mentioning that the command is first called to add a ++trailer with the specified <token> and without positional argument. ++Users can make use of this output when they need automatically add ++some trailers. On the other hand, users can use a trick to suppress ++this output by judging whether the number of positional parameters ++is equal to one, if it is true, execute the commands, otherwise exit ++with non-zero to suppress the output. EXAMPLES -------- @@ Documentation/git-interpret-trailers.txt: subject +------------ +$ cat ~/bin/gcount +#!/bin/sh -+test -n "$1" && git shortlog -s --author="$1" HEAD || true ++if test "$#" != 1 ++then ++ exit 1 ++else ++ test -n "$1" && git shortlog -s --author="$1" HEAD || true ++fi +$ git config trailer.cnt.key "Commit-count: " +$ git config trailer.cnt.ifExists "addIfDifferentNeighbor" +$ git config trailer.cnt.cmd "~/bin/gcount" +$ git interpret-trailers --trailer="cnt:Junio" --trailer="cnt:Linus Torvalds"<<EOF +> subject -+> ++> +> message -+> ++> +> EOF +subject + @@ Documentation/git-interpret-trailers.txt: subject +------------ +$ cat ~/bin/glog-grep +#!/bin/sh -+test -n "$1" && git log --grep "$1" --pretty=reference -1 || true ++if test "$#" != 1 ++then ++ exit 1 ++else ++ test -n "$1" && git log --grep "$1" --pretty=reference -1 || true ++fi +$ git config trailer.ref.key "Reference-to: " +$ git config trailer.ref.ifExists "replace" +$ git config trailer.ref.cmd "~/bin/glog-grep" +$ git interpret-trailers --trailer="ref:Add copyright notices." <<EOF +> subject -+> ++> +> message -+> ++> +> EOF +subject + @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' ' + git config trailer.bug.cmd "echo \"maybe is\"" && + cat >expected2 <<-EOF && + ++ Bug-maker: maybe is + Bug-maker: maybe is him + Bug-maker: maybe is me + EOF @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' ' + git config trailer.bug.cmd "echo \"\$1\" is" && + cat >expected2 <<-EOF && + ++ Bug-maker: is + Bug-maker: him is him + Bug-maker: me is me + EOF @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' ' + EOF + cat >echoscript <<-EOF && + #!/bin/sh -+ echo who is "\$1" ++ if test "\$#" != 1 ++ then ++ exit 1 ++ else ++ echo who is "\$1" ++ fi + EOF + chmod +x echoscript && + git interpret-trailers --trailer "bug: him" --trailer "bug:me" \ + >actual2 && + test_cmp expected2 actual2 +' ++ ++test_expect_success 'with cmd, $1 and without --trailer' ' ++ test_when_finished "git config --remove-section trailer.bug" && ++ test_when_finished "git config --remove-section trailer.gub" && ++ git config trailer.bug.key "Bug-maker: " && ++ git config trailer.bug.ifExists "replace" && ++ git config trailer.bug.cmd "./echoscript" && ++ git config trailer.gub.key "Gub-maker: " && ++ git config trailer.gub.ifExists "replace" && ++ git config trailer.gub.cmd "./echoscript2" && ++ cat >expected2 <<-EOF && ++ ++ Gub-maker: si ohw ++ EOF ++ cat >echoscript <<-EOF && ++ #!/bin/sh ++ if test "\$#" != 1 ++ then ++ exit 1 ++ else ++ echo who is "\$1" ++ fi ++ EOF ++ cat >echoscript2 <<-EOF && ++ echo si ohw "\$1" ++ EOF ++ chmod +x echoscript && ++ chmod +x echoscript2 && ++ git interpret-trailers >actual2 && ++ test_cmp expected2 actual2 ++' + test_expect_success 'without config' ' sed -e "s/ Z\$/ /" >expected <<-\EOF && @@ trailer.c: static void free_arg_item(struct arg_item *item) free(item->conf.command); + free(item->conf.cmd); free(item->token); - free(item->value); +- free(item->value); ++ if (item->value) ++ FREE_AND_NULL(item->value); free(item); + } + @@ trailer.c: static int check_if_different(struct trailer_item *in_tok, return 1; } @@ trailer.c: static int check_if_different(struct trailer_item *in_tok, cp.env = local_repo_env; cp.no_stdin = 1; cp.use_shell = 1; + + if (capture_command(&cp, &buf, 1024)) { +- error(_("running trailer command '%s' failed"), cmd.buf); + strbuf_release(&buf); +- result = xstrdup(""); ++ if (!conf->cmd || arg) { ++ error(_("running trailer command '%s' failed"), cmd.buf); ++ result = xstrdup(""); ++ } else ++ result = NULL; + } else { + strbuf_trim(&buf); + result = strbuf_detach(&buf, NULL); @@ trailer.c: static char *apply_command(const char *command, const char *arg) static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok) { - if (arg_tok->conf.command) { +- const char *arg; +- if (arg_tok->value && arg_tok->value[0]) { + if (arg_tok->conf.command || arg_tok->conf.cmd) { - const char *arg; - if (arg_tok->value && arg_tok->value[0]) { ++ const char *arg = NULL; ++ ++ if ((arg_tok->value && arg_tok->value[0]) || ++ (arg_tok->conf.cmd && !arg_tok->value)) { arg = arg_tok->value; + } else { + if (in_tok && in_tok->value) @@ trailer.c: static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg else arg = xstrdup(""); } - arg_tok->value = apply_command(arg_tok->conf.command, arg); +- free((char *)arg); + arg_tok->value = apply_command(&arg_tok->conf, arg); - free((char *)arg); ++ if (arg) ++ free((char *)arg); } } + +@@ trailer.c: static void apply_arg_if_exists(struct trailer_item *in_tok, + break; + case EXISTS_REPLACE: + apply_item_command(in_tok, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + add_arg_to_input_list(on_tok, arg_tok); + list_del(&in_tok->list); + free_trailer_item(in_tok); + break; + case EXISTS_ADD: + apply_item_command(in_tok, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + add_arg_to_input_list(on_tok, arg_tok); + break; + case EXISTS_ADD_IF_DIFFERENT: + apply_item_command(in_tok, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + if (check_if_different(in_tok, arg_tok, 1, head)) + add_arg_to_input_list(on_tok, arg_tok); + else +@@ trailer.c: static void apply_arg_if_exists(struct trailer_item *in_tok, + break; + case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR: + apply_item_command(in_tok, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + if (check_if_different(on_tok, arg_tok, 0, head)) + add_arg_to_input_list(on_tok, arg_tok); + else +@@ trailer.c: static void apply_arg_if_missing(struct list_head *head, + case MISSING_ADD: + where = arg_tok->conf.where; + apply_item_command(NULL, arg_tok); ++ if (!arg_tok->value) { ++ free_arg_item(arg_tok); ++ return; ++ } + to_add = trailer_from_arg(arg_tok); + if (after_or_end(where)) + list_add_tail(&to_add->list, head); @@ trailer.c: static void duplicate_conf(struct conf_info *dst, const struct conf_info *src) dst->name = xstrdup_or_null(src->name); dst->key = xstrdup_or_null(src->key); @@ trailer.c: static int git_trailer_config(const char *conf_key, const char *value case TRAILER_WHERE: if (trailer_set_where(&conf->where, value)) warning(_("unknown value '%s' for key '%s'"), value, conf_key); +@@ trailer.c: static void process_command_line_args(struct list_head *arg_head, + xstrdup(token_from_item(item, NULL)), + xstrdup(""), + &item->conf, NULL); ++ else if (item->conf.cmd) ++ add_arg_item(arg_head, ++ xstrdup(token_from_item(item, NULL)), ++ NULL, ++ &item->conf, NULL); + } + + /* Add an arg item for each trailer on the command line */