Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option

[Johan Herland <johan@herland.net>]

On Wed, Aug 12, 2015 at 4:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johan Herland <johan@herland.net> writes:
>> I know that we don't yet have a "proper" place to put remote notes refs,
>> but the <ref> in notes.<ref>.merge _must_ be a "local" notes ref (you even
>> use the <localref> notation in the documentation below). Thus, I believe
>> we can presume that the local notes ref must live under refs/notes/*,
>> and hence drop the "refs/notes/" prefix from the config key (just like
>> branch.<name>.* can presume that <name> lives under refs/heads/*).
>
> I am OK going in that direction, as long as we promise that "notes
> merge" will forever refuse to work on --notes=$ref where $ref does
> not begin with refs/notes/.

If we don't already refuse to merge into a ref outside refs/notes, then
I would consider that a bug to be fixed, and not some corner use case that
we must preserve for all future.

After all, we do already have a test in t3308 named 'fail to merge into
various non-notes refs', where one of the non-notes ref being tested are:

  test_must_fail git -c "core.notesRef=refs/heads/master" notes merge x

>> Except that this patch in its current form will occupy the .merge config
>> key...
>>
>> Can you rename to notes.<name>.mergestrategy instead?
>
> This is an excellent suggestion.
>
>> Or even better, take inspiration from branch.<name>.mergeoptions,
>
> Please don't.
>
> That is one of the design mistakes that was copied from another
> design mistake (remotes.*.tagopt).  I'd want to see us not to repeat
> these design mistakes.
>
> These configuration variables were made to take free-form text value
> that is split according to shell rules, primarily because it was
> expedient to implement.  Read its value into a $variable and put it
> at the end of the command line to let the shell split it.  "tagopt"
> was done a bit more carefully in that it made to react only with a
> fixed string "--no-tags", so it was hard to abuse, but "mergeoptions"
> allowed you to override something that you wouldn't want to (e.g. it
> even allowed you to feed '--message=foo').
>
> Once you start from such a broken design, it would be hard to later
> make it saner, even if you wanted to.  You have to retroactively
> forbid something that "worked" (with some definition of "working"),
> or you have to split, parse and then reject something that does not
> make sense yourself, reimplementing dequote/split rule used in the
> shell---which is especially problematic when you no longer write in
> shell scripts.
>
> So a single string value that names one of the supported strategy
> stored in notes.<name>.mergestrategy is an excellent choice.  An
> arbitrary string in notes.<name>.mergeoptions is to be avoided.

Understood. And agreed.

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net