Re: [PATCH v6] [GSOC] trailer: add new trailer.<token>.cmd config option

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 4, 2021, 05:34 UTC
Message-ID: <xmqqa6qezk7c.fsf@gitster.g>
In-Reply-To: <CAOLTT8Tq+-tRzgG1AFD2cE2Vp9z3E94eW1eyN2BxKKg17FzTng@mail.gmail.com>

ZheNing Hu <adlternative@gmail.com> writes:

Show 14 quoted lines

>> >  If some '<token>=<value>' arguments are also passed on the command
>> >  line, when a 'trailer.<token>.command' is configured, the command will
>> >  also be executed for each of these arguments. And the <value> part of
>>
>> This talks about 'trailer.<token>.command'.  Should this be changed
>> to '.cmd'?
>>
>> Or does everything after "When this option is specified, if there is
>> no trailer with ..." apply to both the old .command and new .cmd?
>> If so, that was not clear at all---we'd need to clarify this part.
>>
>
> Because ".command" will be eliminated, may be only leave those
> description Information to ".cmd" is better.

Not really.

Until .command goes away, people who want to migrate a config file written in the .command days to .cmd would need to know what rules govern .command variant. Otherwise they would not know what the original they inherited, using .command, wanted to do, and have no way to emulate it with the new .cmd approach. If something is shared between the two, at least you need to mention that it applies to both.

Until .command actually gets removed, that is.

Show 7 quoted lines

>> Unusual indentation here.  But more importantly, I am not sure if
>> having both 'see' and 'help' examples is worth it---they are similar
>> enough that the second one does not teach anything new to those who
>> studied the first one already, aren't they?
>>
>
> Ok, I will think about other examples.

Or just use one, and let somebody else in the future encounter real world example that is different enough to come up with a follow up patch to describe that example.

Show 19 quoted lines

>> > diff --git a/trailer.c b/trailer.c
>> > index be4e9726421c..6aeff6a1bd33 100644
>> > --- a/trailer.c
>> > +++ b/trailer.c
>> > ...
>> > -     strvec_push(&cp.args, cmd.buf);
>> > +     if (conf->cmd) {
>> > +             // cp.shell_no_implicit_args = 1;
>>
>> Do not add new code that is commented out.  Besides we do not use // comment.
>>
>> > +             strbuf_addstr(&cmd, conf->cmd);
>> > +             strvec_push(&cp.args, cmd.buf);
>> > +             if (arg)
>> > +                     strvec_push(&cp.args, arg);
>>
>> Thanks.
>
> Thanks.