Re: [RFC/PATCH] Add interpret-trailers builtin

From: Johan Herland <johan@herland.net>
Date: Nov 5, 2013, 02:45 UTC
Message-ID: <CALKQrgcweo+B8JKH85-4-SOHsAi6SAxstnXhRWCst_qEyXYmhA@mail.gmail.com>
In-Reply-To: <xmqqfvrcyoaj.fsf@gitster.dls.corp.google.com>

On Mon, Nov 4, 2013 at 8:12 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

> Johan Herland <johan@herland.net> writes:
>>> +{
>>> +       char *end = strchr(arg, '=');
>>> +       if (!end)
>>> +               end = strchr(arg, ':');
>>
>> So both '=' (preferred) and ':' are accepted as field/value
>> separators. That's ok for the command-line, I believe.
>
> Why?
>
> Sometimes you have to be loose from the beginning _if_ some existing
> uses and established conventions make it easier for the users, but
> if you do not have to start from being loose, it is almost always a
> mistake to do so.  The above code just closed the door to use ":"
> for some other useful purposes we may later discover, and will make
> us regret for doing so.

Although I agree with the principle, I think there are (at least) two established conventions that will be commonly used from the start, and that we should support:

 - Using short forms with '=', e.g. "ack=Peff". There is already a
convention on how we specify <name> + <value> pairs on the command
line, e.g. "git -c foo=bar ..."

 - Copy-pasting footers from existing commit messages. These will have
the same format as the expected output of this command, and not
accepting the same format in its input seems silly, IMHO.

That said, I think this applies only to the formatting on the _command line_. When it comes to how the resulting footers are formatted in the commit message, I would argue it only makes sense to use ':', and I think the testcase named 'with config setup and = sign' in the above patch is ugly and unnecessary.

Show 14 quoted lines

>> From the enum values, I assume that these declare the desired behavior
>> when there are two (or more?) or the same footer (i.e. same "field
>> name"). However, it's not (yet) obvious to me in which order they are
>> processed. There are several opportunities for multiple instances of
>> the same "field name":
>>
>>  - Two (or more) occurences in the infile
>>  - Two (or more) occurences on the command line
>>  - One occurence in the infile, and one of the command line
>
> Also, there is a distinction between fields with the same field name
> appearing twice and fields with the same field name and the same
> field value appearing twice. Some headers do want to have them, some
> do not.

True. This complexity is partly why I initially wanted to leave this whole thing up to hooks, but I guess now we have to deal with it... That said, I believe we don't need to cater to every possibility under the sun, just the most common ones. If a minority of users are not satisfied, they can simply configure this to leave all duplicates in place, and then write their own commit-msg hook to do whatever weird consolidation/cleanup they prefer.

...Johan

PS: Since this program will be run by "git commit", and might also be
invoked by hooks, we need to keep the following in mind:

 - Design for idempotence. A given set of headers might be filtered
through this program multiple times, and we should make sure that
multiple runs over the same data does not itself cause problems.

 - Optional/flexible configuration. When a hook runs this program, it
may want to impose its own set of rules that does not entirely (or at
all) coincide with what is set in the config. We should therefore
consider providing a way for the caller to specify a separate source
of trailer/footer config to apply on a given run.

-- 
Johan Herland, <johan@herland.net>
www.herland.net