From: ZheNing Hu <adlternative@gmail.com> To: Junio C Hamano <gitster@pobox.com> Cc: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>, Git List <git@vger.kernel.org>, Christian Couder <christian.couder@gmail.com> Subject: Re: [PATCH v3] [GSOC]trailer: pass arg as positional parameter Date: Fri, 26 Mar 2021 21:29:46 +0800 [thread overview] Message-ID: <CAOLTT8TMFjvH1TzZ9qRtW1v7aDYypdajJxRKpmZ0TaG_xx5H0w@mail.gmail.com> (raw) In-Reply-To: <xmqqwntuuav7.fsf@gitster.g> Junio C Hamano <gitster@pobox.com> 于2021年3月26日周五 上午6:28写道: > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: ZheNing Hu <adlternative@gmail.com> > > > > Original implementation of `trailer.<token>.command` use > > uses > > > `strbuf_replace` to replace $ARG in command with the <value> > > of the trailer, but it have a problem: `strbuf_replace` > > has > > > replace the $ARG only once, If the user's trailer command > > replaces the $ARG only once. > Okay... singular and plural problems. > > have used more than one $ARG, the remaining replacement will > > fail. > > "will fail" is quite vague. It is just left unreplaced, and if the > user expects all of them to be replaced, then the expectation and > reality would not match, but all of that you have already said by > "replaces the $ARG only once.", so I think this sentence should be > removed. > Indeed so. > > If directly modify the implementation of the original > > `trailer.<token>.command`, The user’s previous `'$ARG'` in > > trailer command will not be replaced. > > That statement does not make much sense. Depending on the way how > the implementation is "directly" modified, you can fix the "replaces > only once" problem without introducing such a problem. Just look > for '$ARG' in the string and replace all of them, not just the first > one. It's not too difficult. > > This confusion primarily comes from the fact that you forgot to > explain the other problem you are fixing, I think. Even though the > trailer.<token>.command documentation implies that the user is > expected to give a shell script or some sort as the command and the > use of $ARG makes it look like a shell variable, the original > implementation does not treat it as a shell variable at all. And > the textual replacement is done without making sure the value being > replaced has characters with special meaning in the shell language. > Yes! The accidental injection problem caused should have been the focus of my explanation. > So existing .command may incorrectly use $ARG inside a single-quote > pair and expect it to be replaced to a string inside a single-quote > pair. A malformed, or worse, malicious, value would escape out of > the single-quote pair (remember, the '; rm -fr .' example?) and > execute arbitrary code in an unexpected way. The (ungrammatical) > "if directly modify the implementation" refers to a potential way to > fix these two problems at the same time by doing the $ARG thing > without using textual replacement, namely, exporting the value as an > environment variable ARG. If that approach was taken, then, $ARG > enclosed in a single-quote pair will no longer be replaced, which > makes it a backward incompatible change. > Oh, I remeber it: terrible shell injection. Specifically, it looks like this: $ git config trailer.sign.command "git log --author='\$ARG'" $ git interpret-trailers --trailer "sign = adl' && rm -rf ./repo/'" now I know that should be "backward incompatible change" as you said. > But without describing what solution you are talking about, your > three-line description does not make much sense. > > > So now add new config > > "trailer.<token>.cmd", pass trailer's value as positional > > parameter 1 to the user's command, the user can use $1 as > > trailer's value, to implement original variable replacement. > > > > If the user has these two configuration: "trailer.<token>.cmd" > > and "trailer.<token>.command", "cmd" will execute and "command" > > will not executed. > > > > Original `trailer.<token>.command` can still be used until git > > completely abandoned it. > > > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > Let's rewrite it completely. > > The `trailer.<token>.command` configuration variable > specifies a command (run via the shall, so it does not have > to be a single name of or path to the command, but can be a > shell script), and the first occurrence of substring $ARG is > replaced with the value given to the `interpret-trailer` > command for the token. 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 > to use $ARG more than once, but that would not work, as > the second and subsequent $ARG are not replaced. > > * Because $ARG is textually replaced without regard to the > shell language syntax, even '$ARG' (inside a single-quote > pair), which a user would expect to stay intact, would be > replaced, and worse, if the value had an unmatching single > quote (imagine a name like "O'Connor", substituted into > NAME='$ARG' to make it NAME='O'Connor), it would result in > a broken command that is not syntactically correct (or > worse). > > Introduce a new `trailer.<token>.cmd` configuration that > takes higher precedence to deprecate and eventually remove > `trailer.<token>.command`, which passes the value as a > parameter to the command. Instead of "$ARG", the users will > refer to the value as positional argument, $1, in their > scripts. > > I tried to cover everything we need to tell the reviewers about this > change with the above. Did I miss anything? Nothing to blame, feature of the old command, 2 problems, 1 solution, this is what the log should look like. > > > +trailer.<token>.cmd:: > > + Similar to 'trailer.<token>.command'. But the difference is that > > + `$1` is used in the command to replace the value of the trailer > > + instead of the original `$ARG`, which means that we can pass the > > + trailer value multiple times in the command. > > We eventually want to deprecate the .command, so we'd prefer not to > rely on its description too much (e.g. try to find a way to say what > you want to say without "instead of the original `$ARG`"). > Oh! here I can’t rely on the documentation of the old `.command`, otherwise it’s not easy to delete the old documentation. > The command specified by this configuration variable is run > with a single parameter, which is the <value> part of an > existing trailer with the same <token>. The output from the > command is then used as the value for the <token> in the > resulting trailer. > > would be the replacement for the part that talks about $ARG in the > description of trailer.<token>.command. > > The original description for `trailer.<token>.command` is so jumbled > (not your failure at all) that I had a hard time to understand what > it is trying to say (e.g. what does "as if a special <token>=<value> > argument were added at the beginning of the command line" mean? Is > it making a one-shot export of environment variable to run the > command???), so the above may need further adjustment. Christian? > Care to help out? > > > + E.g. `git config trailer.sign.cmd "test -n \"$1\" && echo \"$1\" || true "`. > > An example is good. There is a whole EXAMPLES section in this > manual page, and the one that uses $ARG may be a good candidate to > look at and change to use .cmd (instead of .command). > Okay, I will modify the paragraphs containing `.command` in these examples. > > + If the user has these two configuration: "trailer.<token>.cmd" > > + and "trailer.<token>.command", "cmd" will be executed and "command" > > + will not be executed. > > When both .cmd and .command are given for the same <token>, > .cmd is used and .command is ignored. > > > diff --git a/trailer.c b/trailer.c > > index be4e9726421c..634d3f1ff04a 100644 > > --- a/trailer.c > > +++ b/trailer.c > > @@ -14,6 +14,7 @@ struct conf_info { > > char *name; > > char *key; > > char *command; > > + char *cmd; > > enum trailer_where where; > > enum trailer_if_exists if_exists; > > enum trailer_if_missing if_missing; > > @@ -127,6 +128,7 @@ static void free_arg_item(struct arg_item *item) > > free(item->conf.name); > > free(item->conf.key); > > free(item->conf.command); > > + free(item->conf.cmd); > > free(item->token); > > free(item->value); > > free(item); > > @@ -216,18 +218,24 @@ static int check_if_different(struct trailer_item *in_tok, > > return 1; > > } > > > > -static char *apply_command(const char *command, const char *arg) > > +static char *apply_command(const char *command, const char *cmd_, const char *arg) > > { > > struct strbuf cmd = STRBUF_INIT; > > struct strbuf buf = STRBUF_INIT; > > struct child_process cp = CHILD_PROCESS_INIT; > > char *result; > > > > - strbuf_addstr(&cmd, command); > > - if (arg) > > - strbuf_replace(&cmd, TRAILER_ARG_STRING, arg); > > - > > - strvec_push(&cp.args, cmd.buf); > > + if (cmd_) { > > + strbuf_addstr(&cmd, cmd_); > > + strvec_push(&cp.args, cmd.buf); > > + if (arg) > > + strvec_push(&cp.args, arg); > > + } else if (command) { > > + strbuf_addstr(&cmd, command); > > + strvec_push(&cp.args, cmd.buf); > > + if (arg) > > + strbuf_replace(&cmd, TRAILER_ARG_STRING, arg); > > + } > > OK. it is clear cmd_ takes precedence this way. > > Later (not as part of this patch, but a few releases down the road), > we may want to add a warning() about using a deprecated feature when > "else if (command)" block is taken. > Fine, I will keep this version of "cmd priority execution" for now. > > @@ -247,7 +255,7 @@ 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) { > > + if (arg_tok->conf.command || arg_tok->conf.cmd) { > > const char *arg; > > if (arg_tok->value && arg_tok->value[0]) { > > arg = arg_tok->value; > > @@ -257,7 +265,7 @@ 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); > > + arg_tok->value = apply_command(arg_tok->conf.command, arg_tok->conf.cmd, arg); > > It might be cleaner to just pass arg_tok->conf to apply_command() > and hide "cmd takes precedence over command" as an implementation > detail of that helper function. > > The implementation looks as good as the original "command" with that > change at this point. Documentation may need a bit more polishing. > you're right. > Thanks. Thanks, Junio. You and the people in the git community are very enthusiastic, You have patiently explained these small mistakes that I made, and taught me a lot of problems that I didn't notice. Grateful. -- ZheNing Hu
next prev parent reply other threads:[~2021-03-26 13:30 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 [this message] 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 ` [PATCH v11 0/2] " Junio C Hamano 2021-04-18 7:47 ` 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=CAOLTT8TMFjvH1TzZ9qRtW1v7aDYypdajJxRKpmZ0TaG_xx5H0w@mail.gmail.com \ --to=adlternative@gmail.com \ --cc=christian.couder@gmail.com \ --cc=git@vger.kernel.org \ --cc=gitgitgadget@gmail.com \ --cc=gitster@pobox.com \ --subject='Re: [PATCH v3] [GSOC]trailer: pass arg as positional parameter' \ /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).