[PATCH v12 0/2] [GSOC] trailer: add new .cmd config option

["ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com>]

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.

Change from last version: Change docs example "Count-count" to "Helped-by".

ZheNing Hu (2):
  [GSOC] docs: correct descript of trailer.<token>.command
  [GSOC] trailer: add new .cmd config option

 Documentation/git-interpret-trailers.txt | 94 ++++++++++++++++++++----
 t/t7513-interpret-trailers.sh            | 84 +++++++++++++++++++++
 trailer.c                                | 35 ++++++---
 3 files changed, 187 insertions(+), 26 deletions(-)


base-commit: 142430338477d9d1bb25be66267225fb58498d92
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-913%2Fadlternative%2Ftrailer-pass-ARG-env-v12
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-913/adlternative/trailer-pass-ARG-env-v12
Pull-Request: https://github.com/gitgitgadget/git/pull/913

Range-diff vs v11:

 1:  34210e5bd3da ! 1:  8129ef6c476b [GSOC] docs: correct description of .command
     @@ Metadata
      Author: ZheNing Hu <adlternative@gmail.com>
      
       ## Commit message ##
     -    [GSOC] docs: correct description of .command
     +    [GSOC] docs: correct descript of trailer.<token>.command
      
          In the original documentation of `trailer.<token>.command`,
          some descriptions are easily misunderstood. So let's modify
 2:  9c0fc91aba24 ! 2:  edb7f1961ddb [GSOC] trailer: add new .cmd config option
     @@ Commit message
          replaced with the value given to the `interpret-trailer`
          command for the token in a '--trailer <token>=<value>' argument.
      
     -    This has two downsides:
     +    This has three downsides:
      
          * The use of $ARG in the mechanism misleads the users that
          the value is passed in the shell variable, and tempt them
     @@ Commit message
          a broken command that is not syntactically correct (or
          worse).
      
     +    * The first occurrence of substring `$ARG` will be replaced
     +    with the empty string, in the command when the command is
     +    first called to add a trailer with the specified <token>.
     +    This is a bad design, the nature of automatic execution
     +    causes it to add a trailer that we don't expect.
     +
          Introduce a new `trailer.<token>.cmd` configuration that
          takes higher precedence to deprecate and eventually remove
          `trailer.<token>.command`, which passes the value as an
     @@ Commit message
          refer to the value as positional argument, $1, in their
          scripts. At the same time, in order to allow
          `git interpret-trailers` to better simulate the behavior
     -    of `git command -s`, the first implicitly executed command
     -    will not pass positional parameters, users can use this
     -    feature to suppress its output.
     +    of `git command -s`, 'trailer.<token>.cmd' will not
     +    automatically execute.
      
          Helped-by: Junio C Hamano <gitster@pobox.com>
          Helped-by: Christian Couder <christian.couder@gmail.com>
     @@ Documentation/git-interpret-trailers.txt: leading and trailing whitespace trimme
      -occurrence of substring `$ARG` in the command. This way the
      -command can produce a <value> computed from the <value> passed
      -in the '--trailer <token>=<value>' argument.
     -+of these arguments, if any, will be passed to the command as its
     -+first argument. This way the command can produce a <value> computed
     -+from the <value> passed in the '--trailer <token>=<value>' argument.
     - +
     +-+
      -For consistency, the first occurrence of substring `$ARG` is
      -also replaced, this time with the empty string, in the command
      -when the command is first called to add a trailer with the
      -specified <token>.
     -+It is worth mentioning that the command is first called to add a
     -+trailer with the specified <token> and without positional argument.
     -+Users can make use of this output when they need automatically add
     -+some trailers. On the other hand, users can use a trick to suppress
     -+this output by judging whether the number of positional parameters
     -+is equal to one, if it is true, execute the commands, otherwise exit
     -+with non-zero to suppress the output.
     ++of these arguments, if any, will be passed to the command as its
     ++first argument. This way the command can produce a <value> computed
     ++from the <value> passed in the '--trailer <token>=<value>' argument.
       
       EXAMPLES
       --------
     @@ Documentation/git-interpret-trailers.txt: subject
       Fix #42
       ------------
       
     -+* Configure a 'cnt' trailer with a cmd use a global script `gcount`
     -+to record commit counts of a specified author and show how it works:
     ++* Configure a 'help' trailer with a cmd use a script `glog-find-author`
     ++  which search specified author identity from git log in git repository
     ++  and show how it works:
      ++
      +------------
     -+$ cat ~/bin/gcount
     ++$ cat ~/bin/glog-find-author
      +#!/bin/sh
     -+if test "$#" != 1
     -+then
     -+	exit 1
     -+else
     -+	test -n "$1" && git shortlog -s --author="$1" HEAD || true
     -+fi
     -+$ 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
     ++test -n "$1" && git log --author="$1" --pretty="%an <%ae>" -1 || true
     ++$ git config trailer.help.key "Helped-by: "
     ++$ git config trailer.help.ifExists "addIfDifferentNeighbor"
     ++$ git config trailer.help.cmd "~/bin/glog-find-author"
     ++$ git interpret-trailers --trailer="help:Junio" --trailer="help:Couder" <<EOF
      +> subject
     -+>
     ++> 
      +> message
     -+>
     ++> 
      +> EOF
      +subject
      +
      +message
      +
     -+Commit-count: 22484     Junio C Hamano
     -+Commit-count: 1117      Linus Torvalds
     ++Helped-by: Junio C Hamano <gitster@pobox.com>
     ++Helped-by: Christian Couder <christian.couder@gmail.com>
      +------------
      +
     -+* Configure a 'ref' trailer with a cmd use a global script `glog-grep`
     ++* Configure a 'ref' trailer with a cmd use a script `glog-grep`
      +  to grep last relevant commit from git log in the git repository
      +  and show how it works:
      ++
      +------------
      +$ cat ~/bin/glog-grep
      +#!/bin/sh
     -+if test "$#" != 1
     -+then
     -+	exit 1
     -+else
     -+	test -n "$1" && git log --grep "$1" --pretty=reference -1 || true
     -+fi
     ++test -n "$1" && git log --grep "$1" --pretty=reference -1 || true
      +$ git config trailer.ref.key "Reference-to: "
      +$ git config trailer.ref.ifExists "replace"
      +$ git config trailer.ref.cmd "~/bin/glog-grep"
      +$ git interpret-trailers --trailer="ref:Add copyright notices." <<EOF
      +> subject
     -+>
     ++> 
      +> message
     -+>
     ++> 
      +> EOF
      +subject
      +
     @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' '
      +	git config trailer.bug.cmd "echo \"maybe is\"" &&
      +	cat >expected2 <<-EOF &&
      +
     -+	Bug-maker: maybe is
      +	Bug-maker: maybe is him
      +	Bug-maker: maybe is me
      +	EOF
     @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' '
      +	git config trailer.bug.cmd "echo \"\$1\" is" &&
      +	cat >expected2 <<-EOF &&
      +
     -+	Bug-maker: is
      +	Bug-maker: him is him
      +	Bug-maker: me is me
      +	EOF
     @@ t/t7513-interpret-trailers.sh: test_expect_success 'setup' '
      +	EOF
      +	cat >echoscript <<-EOF &&
      +	#!/bin/sh
     -+	if test "\$#" != 1
     -+	then
     -+		exit 1
     -+	else
     -+		echo who is "\$1"
     -+	fi
     ++	echo who is "\$1"
      +	EOF
      +	chmod +x echoscript &&
      +	git interpret-trailers --trailer "bug: him" --trailer "bug:me" \
      +		>actual2 &&
      +	test_cmp expected2 actual2
      +'
     -+
     -+test_expect_success 'with cmd, $1 and without --trailer' '
     -+	test_when_finished "git config --remove-section trailer.bug" &&
     -+	test_when_finished "git config --remove-section trailer.gub" &&
     -+	git config trailer.bug.key "Bug-maker: " &&
     -+	git config trailer.bug.ifExists "replace" &&
     -+	git config trailer.bug.cmd "./echoscript" &&
     -+	git config trailer.gub.key "Gub-maker: " &&
     -+	git config trailer.gub.ifExists "replace" &&
     -+	git config trailer.gub.cmd "./echoscript2" &&
     -+	cat >expected2 <<-EOF &&
     -+
     -+	Gub-maker: si ohw
     -+	EOF
     -+	cat >echoscript <<-EOF &&
     -+	#!/bin/sh
     -+	if test "\$#" != 1
     -+	then
     -+		exit 1
     -+	else
     -+		echo who is "\$1"
     -+	fi
     -+	EOF
     -+	cat >echoscript2 <<-EOF &&
     -+		echo si ohw "\$1"
     -+	EOF
     -+	chmod +x echoscript &&
     -+	chmod +x echoscript2 &&
     -+	git interpret-trailers >actual2 &&
     -+	test_cmp expected2 actual2
     -+'
      +
       test_expect_success 'without config' '
       	sed -e "s/ Z\$/ /" >expected <<-\EOF &&
     @@ trailer.c: static void free_arg_item(struct arg_item *item)
       	free(item->conf.command);
      +	free(item->conf.cmd);
       	free(item->token);
     --	free(item->value);
     -+	if (item->value)
     -+		FREE_AND_NULL(item->value);
     + 	free(item->value);
       	free(item);
     - }
     - 
      @@ trailer.c: static int check_if_different(struct trailer_item *in_tok,
       	return 1;
       }
     @@ trailer.c: static int check_if_different(struct trailer_item *in_tok,
       	cp.env = local_repo_env;
       	cp.no_stdin = 1;
       	cp.use_shell = 1;
     - 
     - 	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);
     -+			result = xstrdup("");
     -+		} else
     -+			result = NULL;
     - 	} else {
     - 		strbuf_trim(&buf);
     - 		result = strbuf_detach(&buf, NULL);
      @@ trailer.c: 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) {
     --		const char *arg;
     --		if (arg_tok->value && arg_tok->value[0]) {
      +	if (arg_tok->conf.command || arg_tok->conf.cmd) {
     -+		const char *arg = NULL;
     -+
     -+		if ((arg_tok->value && arg_tok->value[0]) ||
     -+		   (arg_tok->conf.cmd && !arg_tok->value)) {
     + 		const char *arg;
     + 		if (arg_tok->value && arg_tok->value[0]) {
       			arg = arg_tok->value;
     - 		} else {
     - 			if (in_tok && in_tok->value)
      @@ trailer.c: 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);
     --		free((char *)arg);
      +		arg_tok->value = apply_command(&arg_tok->conf, arg);
     -+		if (arg)
     -+			free((char *)arg);
     + 		free((char *)arg);
       	}
       }
     - 
     -@@ trailer.c: static void apply_arg_if_exists(struct trailer_item *in_tok,
     - 		break;
     - 	case EXISTS_REPLACE:
     - 		apply_item_command(in_tok, arg_tok);
     -+		if (!arg_tok->value) {
     -+			free_arg_item(arg_tok);
     -+			return;
     -+		}
     - 		add_arg_to_input_list(on_tok, arg_tok);
     - 		list_del(&in_tok->list);
     - 		free_trailer_item(in_tok);
     - 		break;
     - 	case EXISTS_ADD:
     - 		apply_item_command(in_tok, arg_tok);
     -+		if (!arg_tok->value) {
     -+			free_arg_item(arg_tok);
     -+			return;
     -+		}
     - 		add_arg_to_input_list(on_tok, arg_tok);
     - 		break;
     - 	case EXISTS_ADD_IF_DIFFERENT:
     - 		apply_item_command(in_tok, arg_tok);
     -+		if (!arg_tok->value) {
     -+			free_arg_item(arg_tok);
     -+			return;
     -+		}
     - 		if (check_if_different(in_tok, arg_tok, 1, head))
     - 			add_arg_to_input_list(on_tok, arg_tok);
     - 		else
     -@@ trailer.c: static void apply_arg_if_exists(struct trailer_item *in_tok,
     - 		break;
     - 	case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
     - 		apply_item_command(in_tok, arg_tok);
     -+		if (!arg_tok->value) {
     -+			free_arg_item(arg_tok);
     -+			return;
     -+		}
     - 		if (check_if_different(on_tok, arg_tok, 0, head))
     - 			add_arg_to_input_list(on_tok, arg_tok);
     - 		else
     -@@ trailer.c: static void apply_arg_if_missing(struct list_head *head,
     - 	case MISSING_ADD:
     - 		where = arg_tok->conf.where;
     - 		apply_item_command(NULL, arg_tok);
     -+		if (!arg_tok->value) {
     -+			free_arg_item(arg_tok);
     -+			return;
     -+		}
     - 		to_add = trailer_from_arg(arg_tok);
     - 		if (after_or_end(where))
     - 			list_add_tail(&to_add->list, head);
      @@ trailer.c: static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
       	dst->name = xstrdup_or_null(src->name);
       	dst->key = xstrdup_or_null(src->key);
     @@ trailer.c: static int git_trailer_config(const char *conf_key, const char *value
       	case TRAILER_WHERE:
       		if (trailer_set_where(&conf->where, value))
       			warning(_("unknown value '%s' for key '%s'"), value, conf_key);
     -@@ trailer.c: static void process_command_line_args(struct list_head *arg_head,
     - 				     xstrdup(token_from_item(item, NULL)),
     - 				     xstrdup(""),
     - 				     &item->conf, NULL);
     -+		else if (item->conf.cmd)
     -+			add_arg_item(arg_head,
     -+				     xstrdup(token_from_item(item, NULL)),
     -+				     NULL,
     -+				     &item->conf, NULL);
     - 	}
     - 
     - 	/* Add an arg item for each trailer on the command line */

-- 
gitgitgadget