Junio C Hamano <gitster@xxxxxxxxx> 于2021年4月18日周日 上午6:26写道： > > "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? > 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
