On Wed, Aug 12, 2015 at 4:26 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Johan Herland <johan@xxxxxxxxxxx> 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@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