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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 4, 2013, 19:12 UTC
Message-ID: <xmqqfvrcyoaj.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CALKQrgdJ6d2SVWNQGa6d-eLYPAL-C21=tCyJczCDExLQRfq=jA@mail.gmail.com>

Johan Herland <johan@herland.net> writes:

Thanks for looking this over. I am mostly in agreement and will elide the parts I do not have much to add.

Show 10 quoted lines

>> This command should help with RFC 822 style headers, called
>> "trailers", that are found at the end of commit messages.
>
> As has been asked earlier in this discussion, we should probably
> specify explicitly what we _mean_ with "RFC822-style"
> headers/footers/trailers, and exactly how closely we follow the actual
> RFC... E.g. do we make use of the linebreaking rules? encoding
> handling? etc... We may want to take a more relaxed approach (after
> all, we're not including a complete RFC822/RFC2822 implementation),
> but we should at least state so, and possibly how/why we do so.

Indeed; especially I do not think we would want to do the 2822 contination lines, or 2047 MIME quoting, ever.

>> For a long time, these trailers have become a de facto standard
>> way to add helpful information into commit messages.

"helpful" is not the key aspect of these footers. They are added at the end to introduce some structure into a free-form part of the commit objects.

Show 7 quoted lines

>> The following features are implemented:
>>         - the result is printed on stdout
>>         - the [<token[=value]>...] arguments are interpreted
>>         - a commit message passed using the "--infile=file" option is interpreted
>
> If the output is written to stdout, then why is not the input taken
> from stdin? Or vice versa: why not --outfile?

I'd say just make it a filter without --in/outfile ;-)

Show 7 quoted lines

>> +{
>> +       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.

Show 9 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.