On Mon, Nov 4, 2013 at 8:12 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Johan Herland <johan@xxxxxxxxxxx> 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.
>> 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@xxxxxxxxxxx>
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html