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

[Christian Couder <christian.couder@gmail.com>]

On Fri, Apr 9, 2021 at 3:37 PM ZheNing Hu via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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.

Maybe: s/is incorrected/can be puzzling/

>  2. $ARG is textually replaced without shell syntax, which may result a

Not sure what "without shell syntax" means here.

Also: s/may result a/may result in a/

>     broken command when $ARG include some unmatching single quote, very

s/when/if/
s/include/includes/
s/some unmatching/an unmatched/

>     unsafe.
>
> Now pass trailer value as $1 to the trailer command with another
> trailer.<token>.cmd config, to solve these above two problems,

I think the important thing here is to explain that we want to
introduce a new 'trailer.<token>.cmd' config option, so that we can
start deprecating 'trailer.<token>.command' when people have started
using the new 'trailer.<token>.cmd' config option. Maybe something
like:

"To address these issues, let's introduce a new 'trailer.<token>.cmd'
config option that behaves in the same way as
'trailer.<token>.command' except that it passes the trailer value as
$1 to the configured command instead of textually replacing the first
occurence of '$ARG' in it. This will let us slowly deprecate
'trailer.<token>.command' in favor of 'trailer.<token>.cmd' in the
future."

> We are now writing documents that are more readable and correct than before.

I would suggest removing this sentence as I don't think it adds much
to the above.

> ZheNing Hu (2):
>   [GSOC] docs: correct descript of trailer.<token>.command

By the way the following title might be a bit better and shorter:

"[GSOC] docs: improve 'trailer.<token>.command' doc"

>   [GSOC] trailer: add new .cmd config option

Maybe we can afford: s/.cmd/'trailer.<token>.cmd'/

>      -    [GSOC] trailer: add new trailer.<token>.cmd config option
>      +    [GSOC] trailer: add new .cmd config option

Was the previous title too long? Or is there an issue with
GitGitGadget because it contains <token>?