Re: [PATCH v10 11/12] Documentation: add documentation for 'git interpret-trailers'

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

On Tue, Apr 8, 2014 at 9:30 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
>> +This command is a filter. It reads the standard input for a commit
>> +message and applies the `token` arguments, if any, to this
>> +message. The resulting message is emited on the standard output.
>
> s/emited/emitted/

Ok.

>> +Some configuration variables control the way the `token` arguments are
>> +applied to the message and the way any existing trailer in the message
>> +is changed. They also make it possible to automatically add some
>> +trailers.
>> +
>> +By default, a 'token=value' or 'token:value' argument will be added
>> +only if no trailer with the same (token, value) pair is already in the
>> +message. The 'token' and 'value' parts will be trimmed to remove
>> +starting and trailing whitespace, and the resulting trimmed 'token'
>> +and 'value' will appear in the message like this:
>> +
>> +------------------------------------------------
>> +token: value
>> +------------------------------------------------
>> +
>> +By default, if there are already trailers with the same 'token', the
>> +new trailer will appear just after the last trailer with the same
>> +'token'. Otherwise it will appear at the end of the message.
>
> How are existing trailers recognized in the input commit message?  Do
> trailers have to be configured to be recognized?  Or are all lines
> matching a specific pattern considered trailers?  If so, it might be
> helpful to include a regexp here that describes the trailer "syntax".

The trailers are recognized in the input commit message using the
following rules:
 - only lines that contains a ':' are considered trailers,
 - the trailer lines must all be next to each other,
 - after them it's only possible to have some lines that contain only spaces,
 - before them there must be at least one line with only spaces

> What about blank lines?  I see that you try to add a blank line before
> new trailers.  But what about on input?

One line with only spaces has to be before the trailers. Some can be
after the trailers.

> Do the trailer lines have to be
> separated from the free-form comment by a blank line to be recognized?

Yes.

> What if there are blank lines between trailer lines, or after them?

After them is ok. Between is not ok (only the trailers after the blank
lines will be recognized).

> Is it allowed to have non-trailer lines between or after trailer lines?

No except lines with spaces after the trailers lines.

>> +Note that 'trailers' do not follow and are not intended to follow many
>> +rules that are in RFC 822. For example they do not follow the line
>> +breaking rules, the encoding rules and probably many other rules.
>> +
>> +OPTIONS
>> +-------
>> +--trim-empty::
>> +     If the 'value' part of any trailer contains only whitespace,
>> +     the whole trailer will be removed from the resulting message.
>
> Does this apply to existing trailers, new trailers, or both?

Both.

> If it applies to existing trailers, then it seems a bit dangerous, in the
> sense that the command might end up changing trailers that are unrelated
> to the one that the command is trying to add.

The command is not just for adding trailers.
But there could be an option to just trim trailers that are added.

>> +CONFIGURATION VARIABLES
>> +-----------------------
>> +
>> +trailer.<token>.key::
>> +     This 'key' will be used instead of 'token' in the
>> +     trailer. After some alphanumeric characters, it can contain
>
> Trailer keys can also contain '-', right?

Yes.
I should have written "after the last alphanumeric character".
I will fix that.

>> +     some non alphanumeric characters like ':', '=' or '#' that will
>> +     be used instead of ':' to separate the token from the value in
>> +     the trailer, though the default ':' is more standard.
>
> Above it looks like the default separator is not ':' but rather ': '
> (with a space).  Is the space always added regardless of the value of
> this configuration variable, or should the configuration value include
> the trailing space if it is desired?  Is there any way to get a trailer
> that doesn't include a space, like
>
>     foo=bar
>
> ?  (Changing this to "foo= bar" would look pretty ugly.)

I will have a look, but I think that:

- a space is always added after ':' or '=',
- a space is never added after '#',
- it doesn't matter if there is a space or not in the configured key.

> If a commit message containing trailer lines with separators other than
> ':' is input to the program, will it recognize them as trailer lines?

No, '=' and '#' are not supported in the input message, only in the output.

> Do such trailer lines have to have the same separator as the one listed
> in this configuration setting to be recognized?

No they need to have ':' as a separator.

The reason why only ':' is supported is because it is the cannonical
trailer separator and it could create problems with many input
messages if other separators where supported.

Maybe we could detect a special line like the following:

# TRAILERS START

in the input message and consider everyhting after that line as trailers.
In this case it would be ok to accept other separators.

> I suppose that there is some compelling reason to allow non-colon
> separators here.  If not, it seems like it adds a lot of complexity and
> should maybe be omitted, or limited to only a few specific separators.

Yeah, but in the early threads concerning this subject, someone said
that GitHub for example uses "bug #XXX".
I will have a look again.

>> +trailer.<token>.where::
>> +     This can be either `after`, which is the default, or
>> +     `before`. If it is `before`, then a trailer with the specified
>> +     token, will appear before, instead of after, other trailers
>> +     with the same token, or otherwise at the beginning, instead of
>> +     at the end, of all the trailers.
>
> Brainstorming: some other options that might make sense here someday:
>
> `end`: add new trailer after all existing trailers (even those with
> different keys).  This would allow trailers to be kept in chronological
> order.
>
> `beginning`: add new trailer before the first existing trailer (allows
> reverse chronological order).
>
> `sorted`: add new trailer among the existing trailers with the same key
> so as to keep their values in lexicographic order.

Yeah, I thought about these, but I don't think there is a need for
them right now.

>> +trailer.<token>.ifexist::
>> +     This option makes it possible to choose what action will be
>> +     performed when there is already at least one trailer with the
>> +     same token in the message.
>> ++
>> +The valid values for this option are: `addIfDifferent` (this is the
>> +default), `addIfDifferentNeighbor`, `add`, `overwrite` or `doNothing`.
>
> Are these option values case sensitive?  If so, it might be a little bit
> confusing because the same camel-case is often used in documentation for
> configuration *keys*, which are not case sensitive [1], and users might
> have gotten used to thinking of strings that look like this to be
> non-case-sensitive.

There were some discussions a few versions of this series ago with
Peff, Junio and perhaps others about this.
I thought that being case insensitive was better and Peff kind of
agreed with that, but as Junio disagreed it is now case sensitive.

>> +With `addIfDifferent`, a new trailer will be added only if no trailer
>> +with the same (token, value) pair is already in the message.
>> ++
>> +With `addIfDifferentNeighbor`, a new trailer will be added only if no
>> +trailer with the same (token, value) pair is above or below the line
>> +where the new trailer will be added.
>> ++
>> +With `add`, a new trailer will be added, even if some trailers with
>> +the same (token, value) pair are already in the message.
>> ++
>> +With `overwrite`, the new trailer will overwrite an existing trailer
>> +with the same token.
>
> What if there are multiple existing trailers with the same token?  Are
> they all overwritten?

No, if where == after, only the last one is overwritten, and if where
== before, only the first one is overwritten.

I could add an "overwriteAll" option. It could be interesting to use
when a command using "$ARG" is configured, as this way the command
would apply to all the trailers with the given token instead of just
the last or first one.

>> +With `doNothing`, nothing will be done, that is no new trailer will be
>> +added if there is already one with the same token in the message.
>> +
>> +trailer.<token>.ifmissing::
>> +     This option makes it possible to choose what action will be
>> +     performed when there is not yet any trailer with the same
>> +     token in the message.
>> ++
>> +The valid values for this option are: `add` (this is the default) and
>> +`doNothing`.
>> ++
>> +With `add`, a new trailer will be added.
>> ++
>> +With `doNothing`, nothing will be done.
>> +
>> +trailer.<token>.command::
>> +     This option can be used to specify a shell command that will
>> +     be used to automatically add or modify a trailer with the
>> +     specified 'token'.
>> ++
>> +When this option is specified, it is like if a special 'token=value'
>> +argument is added at the end of the command line, where 'value' will
>> +be given by the standard output of the specified command.
>
> Maybe reword to
>
>     When this option is specified, the behavior is as if a special
>     'token=value' argument were added at the end of the command line,
>     where 'value' is taken to be the standard output of the specified
>     command.
>
> And if it is the case, maybe add "with leading and trailing whitespace
> trimmed off" at the end of the sentence.

Ok.

>> +If the command contains the `$ARG` string, this string will be
>> +replaced with the 'value' part of an existing trailer with the same
>> +token, if any, before the command is launched.
>
> What if the key appears multiple times in existing trailers?

It will be done only once for the last or first trailer with the key
depending on "where".

>> +
>> +SEE ALSO
>> +--------
>> +linkgit:git-commit[1]
>> +
>> +GIT
>> +---
>> +Part of the linkgit:git[1] suite
>>
>
> Doesn't this command have to be added to command-list.txt?

Maybe, I will have a look.

Thanks,
Christian.