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