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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2014, 16:52 UTC
Message-ID: <xmqq7g6zlq5a.fsf@gitster.dls.corp.google.com>
In-Reply-To: <5343A589.10503@alum.mit.edu>

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.

Show 18 quoted lines

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

Show 5 quoted lines

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