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

[Junio C Hamano <gitster@pobox.com>]

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