"ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
