From: Junio C Hamano <gitster@pobox•com>
To: Johan Herland <johan@herland•net>
Cc: Jacob Keller <jacob.e.keller@intel•com>,
Git mailing list <git@vger•kernel.org>,
Michael Haggerty <mhagger@alum•mit.edu>,
Eric Sunshine <sunshine@sunshineco•com>,
Jacob Keller <jacob.keller@gmail•com>
Subject: Re: [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option
Date: Tue, 11 Aug 2015 19:26:08 -0700 [thread overview]
Message-ID: <xmqqy4hhmedb.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CALKQrgeDuRkXm2LzDOuZDZLOBRXjLmmRvhtXfXScWfLKX+9t=g@mail.gmail.com> (Johan Herland's message of "Wed, 12 Aug 2015 02:34:32 +0200")
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/.
> 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.
next prev parent reply other threads:[~2015-08-12 2:26 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-11 20:57 [PATCH v4 0/4] add notes strategy configuration options Jacob Keller
2015-08-11 20:57 ` [PATCH v4 1/4] notes: document cat_sort_uniq rewriteMode Jacob Keller
2015-08-12 0:35 ` Johan Herland
2015-08-11 20:57 ` [PATCH v4 2/4] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
2015-08-12 0:36 ` Johan Herland
2015-08-11 20:57 ` [PATCH v4 3/4] notes: add notes.merge option to select default strategy Jacob Keller
2015-08-12 0:04 ` Johan Herland
2015-08-11 20:57 ` [PATCH v4 4/4] notes: teach git-notes about notes.<ref>.merge option Jacob Keller
2015-08-12 0:11 ` Eric Sunshine
2015-08-12 0:34 ` Johan Herland
2015-08-12 2:26 ` Junio C Hamano [this message]
2015-08-12 19:05 ` Jacob Keller
2015-08-12 19:09 ` Junio C Hamano
2015-08-12 19:16 ` Jacob Keller
2015-08-12 21:43 ` Jacob Keller
2015-08-12 22:04 ` Johan Herland
2015-08-12 21:46 ` Johan Herland
2015-08-12 21:57 ` Jacob Keller
2015-08-12 22:03 ` Jacob Keller
2015-08-12 22:41 ` Junio C Hamano
2015-08-12 22:51 ` Jacob Keller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqy4hhmedb.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jacob.e.keller@intel$(echo .)com \
--cc=jacob.keller@gmail$(echo .)com \
--cc=johan@herland$(echo .)net \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox