From: Junio C Hamano <gitster@pobox.com> To: "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> Cc: git@vger.kernel.org, Christian Couder <christian.couder@gmail.com>, ZheNing Hu <adlternative@gmail.com> Subject: Re: [PATCH v11 0/2] [GSOC] trailer: add new .cmd config option Date: Sat, 17 Apr 2021 15:26:54 -0700 [thread overview] Message-ID: <xmqq5z0kbl8x.fsf@gitster.g> (raw) In-Reply-To: <pull.913.v11.git.1618672417.gitgitgadget@gmail.com> (ZheNing Hu via GitGitGadget's message of "Sat, 17 Apr 2021 15:13:35 +0000") "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.
next prev parent reply other threads:[~2021-04-17 22:27 UTC|newest] Thread overview: 101+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-23 14:53 [PATCH] [GSOC]trailer: change $ARG to environment variable ZheNing Hu via GitGitGadget 2021-03-24 15:42 ` [PATCH v2] [GSOC]trailer: pass arg as positional parameter ZheNing Hu via GitGitGadget 2021-03-24 20:18 ` Junio C Hamano 2021-03-25 1:43 ` ZheNing Hu 2021-03-25 11:53 ` [PATCH v3] " ZheNing Hu via GitGitGadget 2021-03-25 22:28 ` Junio C Hamano 2021-03-26 13:29 ` ZheNing Hu 2021-03-26 16:13 ` [PATCH v4] " ZheNing Hu via GitGitGadget 2021-03-27 18:04 ` Junio C Hamano 2021-03-27 19:53 ` Christian Couder 2021-03-28 10:46 ` ZheNing Hu 2021-03-29 9:04 ` Christian Couder 2021-03-29 13:43 ` ZheNing Hu 2021-03-30 8:45 ` Christian Couder 2021-03-30 11:22 ` ZheNing Hu 2021-03-30 15:07 ` ZheNing Hu 2021-03-30 17:14 ` Junio C Hamano 2021-03-31 5:14 ` ZheNing Hu 2021-03-31 18:19 ` Junio C Hamano 2021-03-31 18:29 ` Junio C Hamano 2021-04-01 3:56 ` ZheNing Hu 2021-04-01 19:49 ` Junio C Hamano 2021-04-02 2:08 ` ZheNing Hu 2021-04-01 3:39 ` ZheNing Hu 2021-03-31 10:05 ` [PATCH v5 0/2] " ZheNing Hu via GitGitGadget 2021-03-31 10:05 ` [PATCH v5 1/2] [GSOC] run-command: add shell_no_implicit_args option ZheNing Hu via GitGitGadget 2021-04-01 7:22 ` Christian Couder 2021-04-01 9:58 ` ZheNing Hu 2021-03-31 10:05 ` [PATCH v5 2/2] [GSOC]trailer: pass arg as positional parameter ZheNing Hu via GitGitGadget 2021-04-01 7:28 ` [PATCH v5 0/2] " Christian Couder 2021-04-01 10:02 ` ZheNing Hu 2021-04-02 13:26 ` [PATCH v6] [GSOC] trailer: add new trailer.<token>.cmd config option ZheNing Hu via GitGitGadget 2021-04-02 20:48 ` Junio C Hamano 2021-04-03 5:08 ` ZheNing Hu 2021-04-04 5:34 ` Junio C Hamano 2021-04-03 5:51 ` Christian Couder 2021-04-04 23:26 ` Junio C Hamano 2021-04-06 3:47 ` Christian Couder 2021-04-06 3:52 ` Christian Couder 2021-04-06 5:16 ` ZheNing Hu 2021-04-06 5:34 ` Junio C Hamano 2021-04-06 5:37 ` Junio C Hamano 2021-04-04 5:43 ` ZheNing Hu 2021-04-04 8:52 ` Christian Couder 2021-04-04 9:53 ` ZheNing Hu 2021-04-02 23:44 ` Junio C Hamano 2021-04-03 3:22 ` ZheNing Hu 2021-04-03 4:31 ` Junio C Hamano 2021-04-03 5:15 ` ZheNing Hu 2021-04-04 13:11 ` [PATCH v7] " ZheNing Hu via GitGitGadget 2021-04-06 16:23 ` Christian Couder 2021-04-07 4:51 ` ZheNing Hu 2021-04-09 13:37 ` [PATCH v8 0/2] [GSOC] trailer: add new .cmd " ZheNing Hu via GitGitGadget 2021-04-09 13:37 ` [PATCH v8 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget 2021-04-09 19:02 ` Christian Couder 2021-04-10 13:40 ` ZheNing Hu 2021-04-09 13:37 ` [PATCH v8 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget 2021-04-09 20:18 ` Christian Couder 2021-04-10 14:09 ` ZheNing Hu 2021-04-09 19:59 ` [PATCH v8 0/2] " Christian Couder 2021-04-12 16:39 ` [PATCH v9 " ZheNing Hu via GitGitGadget 2021-04-12 16:39 ` [PATCH v9 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget 2021-04-12 20:42 ` Junio C Hamano 2021-04-16 12:03 ` Christian Couder 2021-04-17 1:54 ` Junio C Hamano 2021-04-12 16:39 ` [PATCH v9 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget 2021-04-12 20:51 ` Junio C Hamano 2021-04-13 7:33 ` Christian Couder 2021-04-13 12:02 ` ZheNing Hu 2021-04-13 19:18 ` Junio C Hamano 2021-04-14 13:27 ` ZheNing Hu 2021-04-14 20:33 ` Junio C Hamano 2021-04-15 15:32 ` ZheNing Hu 2021-04-15 17:41 ` Junio C Hamano 2021-04-16 12:54 ` Christian Couder 2021-04-13 18:14 ` Junio C Hamano 2021-04-16 8:47 ` [PATCH v10 0/2] " ZheNing Hu via GitGitGadget 2021-04-16 8:47 ` [PATCH v10 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget 2021-04-16 19:11 ` Junio C Hamano 2021-04-16 8:47 ` [PATCH v10 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget 2021-04-16 19:13 ` Junio C Hamano 2021-04-16 19:21 ` Junio C Hamano 2021-04-16 19:25 ` Junio C Hamano 2021-04-17 2:58 ` Junio C Hamano 2021-04-17 3:36 ` Junio C Hamano 2021-04-17 7:41 ` ZheNing Hu 2021-04-17 8:11 ` Junio C Hamano 2021-04-17 15:13 ` [PATCH v11 0/2] " ZheNing Hu via GitGitGadget 2021-04-17 15:13 ` [PATCH v11 1/2] [GSOC] docs: correct description of .command ZheNing Hu via GitGitGadget 2021-04-17 15:13 ` [PATCH v11 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget 2021-04-17 22:26 ` Junio C Hamano [this message] 2021-04-18 7:47 ` [PATCH v11 0/2] " ZheNing Hu 2021-04-21 0:09 ` Junio C Hamano 2021-04-21 5:47 ` ZheNing Hu 2021-04-21 23:40 ` Junio C Hamano 2021-04-22 9:20 ` ZheNing Hu 2021-04-27 6:49 ` Junio C Hamano 2021-04-27 12:24 ` ZheNing Hu 2021-05-03 15:41 ` [PATCH v12 " ZheNing Hu via GitGitGadget 2021-05-03 15:41 ` [PATCH v12 1/2] [GSOC] docs: correct descript of trailer.<token>.command ZheNing Hu via GitGitGadget 2021-05-03 15:41 ` [PATCH v12 2/2] [GSOC] trailer: add new .cmd config option ZheNing Hu via GitGitGadget
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=xmqq5z0kbl8x.fsf@gitster.g \ --to=gitster@pobox.com \ --cc=adlternative@gmail.com \ --cc=christian.couder@gmail.com \ --cc=git@vger.kernel.org \ --cc=gitgitgadget@gmail.com \ --subject='Re: [PATCH v11 0/2] [GSOC] trailer: add new .cmd config option' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).