From: Johan Herland Date: Wed, 12 Aug 2015 21:46:43 GMT Subject: Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option Message-ID: In-Reply-To: On Wed, Aug 12, 2015 at 4:26 AM, Junio C Hamano wrote: > Johan Herland writes: >> I know that we don't yet have a "proper" place to put remote notes refs, >> but the in notes..merge _must_ be a "local" notes ref (you even >> use the 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..* can presume that 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..mergestrategy instead? > > This is an excellent suggestion. > >> Or even better, take inspiration from branch..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..mergestrategy is an excellent choice. An > arbitrary string in notes..mergeoptions is to be avoided. Understood. And agreed. ...Johan -- Johan Herland, www.herland.net