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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 12, 2015, 02:26 UTC
Message-ID: <xmqqy4hhmedb.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CALKQrgeDuRkXm2LzDOuZDZLOBRXjLmmRvhtXfXScWfLKX+9t=g@mail.gmail.com>

Johan Herland <johan@herland.net> writes:

Show 6 quoted lines

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

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

Thanks.