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

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Sorry for reappearing in this thread after such a long absence.  I
> wanted to see what is coming up (I think this interpret-trailers command
> will be handy!) so I read this documentation patch carefully, and added
> some questions and suggestions below.

Thanks for reading the patch carefully.  It helps to have fresh set
of eyes that are not contaminated by the preconception formed by
previous discussions, especially when reviewing the documentation
whose primary target audiences are those who do not care about these
previous back-and-forth.

>> +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:
> ...
>> +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?

It is interesting and somewhat sad that it all has to come back
together inter-twined.  From the very beginning, I was opposed to
having logical complexity that requires multi-words in both variable
names (e.g. "if-exist") and values (e.g. "add-if-different"), and
after $gmane/241929 where I let the devil's advocate "how about
making the variable simpler without logical operation and put all
the conditional on the value side?" suggestion shot down, I somehow
was hoping that the value part got a lot simpler not to require
multi-words, which would have meant that we would not have to worry
about "Is it addIfDifferent? add-if-different? or Add_If_Different?"
at all.  Sadly that is not what we have ended up with.

So, with that realization...

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

... very true.  Having to have these enum values as so complex to
require multi-words is probably the root cause of the confusion, and
we might probably be better off if we did not have to, but it would
be helpful to allow various different spellings (i.e. make them case
insensitive to allow random camel spellings, and also accept things
like "add-if-different" as well) if we absolutely have to have these
complex values.

But you had a lot of good questions and suggestions for possible
future enhancements that we would need to take into account while
designing the overall scheme to later allow them to fit into.  Maybe
a value that is a single-token that consists of just a few words
(e.g. "addIfDifferent") may not be the best way to go after all.

I dunno.

> What if there are multiple existing trailers with the same token?  Are
> they all overwritten?
> ...
> What if the key appears multiple times in existing trailers?

All good questions, I would think.