From: Johan Herland Date: Wed, 12 Aug 2015 00:34:32 GMT Subject: Re: [PATCH v4 4/4] notes: teach git-notes about notes..merge option Message-ID: In-Reply-To: <1439326641-9447-5-git-send-email-jacob.e.keller@intel.com> On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller wrote: > From: Jacob Keller > > Add new option "notes..merge" option which specifies the merge > strategy for merging into a given notes ref. This option enables > selection of merge strategy for particular notes refs, rather than all > notes ref merges, as user may not want cat_sort_uniq for all refs, but > only some. Note that the is the local reference we are merging > into, not the remote ref we merged from. The assumption is that users > will mostly want to configure separate local ref merge strategies rather > than strategies depending on which remote ref they merge from. Also, > notes..merge overrides the general behavior as it is more specific. Thanks for working on this, and I apologize for not properly reviewing this part of the series before now. More comments below. > > Signed-off-by: Jacob Keller > --- > Documentation/config.txt | 6 ++++++ > Documentation/git-notes.txt | 6 ++++++ > builtin/notes.c | 7 +++++-- > t/t3309-notes-merge-auto-resolve.sh | 39 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 56 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 488c2e8eec1b..2c283ebc309e 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -1912,6 +1912,12 @@ notes.merge:: > STRATEGIES" section of linkgit:git-notes[1] for more information > on each strategy. > > +notes..merge:: > + Which merge strategy to choose if the local ref for a notes merge > + matches , overriding "notes.merge". just be a s/just/must/ > + fully qualified refname. See "NOTES MERGE STRATEGIES" section in > + linkgit:git-notes[1] for more information on the available strategies. I sort of get the reason for "fully qualified refname", but I think notes.refs/notes/commits.merge looks much uglier than notes.commits.merge Especially since we have the opposite precedence for branch..*, e.g. we already have branch.master.merge and not branch.refs/heads/master.merge 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/*). ... Which brings me to another small gripe about the naming here: branch..merge names the remote ref (at branch..remote) that we will pull from, but notes..merge has a very different meaning. If we - in the future - were to provide a similar config mechanism for a hypothetical "git notes pull" command, then it would be natural to model its config similarly: notes..remote and notes..merge specifies whence we fetch, and what we (notes-)merge, respectively. Except that this patch in its current form will occupy the .merge config key... Can you rename to notes..mergestrategy instead? Or even better, take inspiration from branch..mergeoptions, and provide notes..mergeoptions instead, which you can then set like: git config notes.foo.mergeoptions "--strategy=cat_sort_uniq" Even though 'git notes merge' don't yet accept any other options that should be configurable, I think it's worth emulating the mechanisms that alread exist for branches. It gives is some amount of future- proofing as well. ...Johan -- Johan Herland, www.herland.net