git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
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 v3] [GSOC]trailer: pass arg as positional parameter
Date: Thu, 25 Mar 2021 15:28:12 -0700	[thread overview]
Message-ID: <xmqqwntuuav7.fsf@gitster.g> (raw)
In-Reply-To: <pull.913.v3.git.1616673200809.gitgitgadget@gmail.com> (ZheNing Hu via GitGitGadget's message of "Thu, 25 Mar 2021 11:53:20 +0000")

"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.

> 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.

> 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.

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.

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?

> +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`").

	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).

> +	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.

> @@ -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.

Thanks.

  reply	other threads:[~2021-03-25 22:29 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 [this message]
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                     ` [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=xmqqwntuuav7.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 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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git