From: Junio C Hamano <gitster@pobox.com>
To: Jacob Keller <jacob.e.keller@intel.com>
Cc: git@vger.kernel.org, Jacob Keller <jacob.keller@gmail.com>,
Johan Herland <johan@herland.net>,
Michael Haggerty <mhagger@alum.mit.edu>,
Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy
Date: Wed, 05 Aug 2015 14:10:11 -0700 [thread overview]
Message-ID: <xmqqa8u530i4.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1438510226-1163-5-git-send-email-jacob.e.keller@intel.com> (Jacob Keller's message of "Sun, 2 Aug 2015 03:10:26 -0700")
Jacob Keller <jacob.e.keller@intel.com> writes:
> +notes.<localref>.merge::
> + Which merge strategy to choose if the local ref for a notes merge
> + matches <localref>. Is overridden by notes.merge and takes the same
> + values. <localref> may be fully qualified or just under refs/notes/.
> + See "NOTES MERGE STRATEGIES" section in linkgit:git-notes[1] for more
> + information on each strategy.
If I have notes.refs/notes/commit.merge and notes.merge specified,
I'd expect the former overrides the latter. The second sentence may
need correcting.
I think it is a mistake to allow both notes.refs/notes/commit.merge
and notes.commit.merge. You'd end up needing to implement quite a
complicated "the last one wins" rule if you did so.
> +notes.<localref>.merge::
> + Which strategy to choose when merging into <localref>. Uses the same
> + values as notes.merge. <localref> may be either a fully qualified ref
> + or the shortname under "refs/notes/". See "NOTES MERGE STRATEGIES"
> + section above for more information about each strategy.
As a reviewer, I can tell that "Uses the same values" wants to say
that the set of allowed values is the same, but a casual reader is
bound to read it as "notes.commit.merge must be set to the same
value as the value set to notes.merge".
> diff --git a/builtin/notes.c b/builtin/notes.c
> index de0caa00df1b..b0174d1024dc 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -92,6 +92,10 @@ static const char * const git_notes_get_ref_usage[] = {
> static const char note_template[] =
> "\nWrite/edit the notes for the following object:\n";
>
> +static struct note_ref **note_refs;
> +static int note_refs_alloc;
> +static int note_refs_nr;
> +static struct hashmap note_refs_hash;
> static enum notes_merge_strategy merge_strategy;
>
> struct note_data {
> @@ -757,12 +761,87 @@ static int parse_notes_strategy(const char *arg, enum notes_merge_strategy *stra
> return 0;
> }
> ...
> +struct note_refs_hash_key {
> + const char *str;
> + int len;
> +};
> + ...
> +static void set_strategy_for_ref(const char *ref)
> +{
> + ...
> +}
Hmmm, I do not see why you need all the complexity above.
When you come to merge(), after calling default_notes_ref(), you
know exactly which notes ref you are merging into, no? Shouldn't
then the change required for this feature just the matter of asking
the configuration system values for notes.$remote_ref.merge and
notes.merge?
IOW,
struct strbuf key = STRBUF_INIT;
char *value = NULL;
strbuf_addf(&key, "notes.%s.merge", remote_ref.buf);
git_config_get_string(key.buf, &value) ||
git_config_get_string_const("notes.merge", &value));
if (value)
parse_notes_strategy(value, &configured_merge_strategy);
...
parse_options();
if (strategy)
parse_notes_strategy(value, &configured_merge_strategy);
or something?
prev parent reply other threads:[~2015-08-05 21:10 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-02 10:10 [PATCH v3 0/4] notes: add support for notes.merge option Jacob Keller
2015-08-02 10:10 ` [PATCH v3 1/4] notes: document cat_sort_uniq rewriteMode Jacob Keller
2015-08-02 10:10 ` [PATCH v3 2/4] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
2015-08-02 10:10 ` [PATCH v3 3/4] notes: add notes.merge option to select default strategy Jacob Keller
2015-08-05 20:47 ` Junio C Hamano
2015-08-06 22:37 ` Eric Sunshine
2015-08-02 10:10 ` [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy Jacob Keller
2015-08-05 21:10 ` Junio C Hamano [this message]
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=xmqqa8u530i4.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=jacob.e.keller@intel.com \
--cc=jacob.keller@gmail.com \
--cc=johan@herland.net \
--cc=mhagger@alum.mit.edu \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).