Re: [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2015, 21:10 UTC
Message-ID: <xmqqa8u530i4.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1438510226-1163-5-git-send-email-jacob.e.keller@intel.com>

Jacob Keller <jacob.e.keller@intel.com> writes:

Show 6 quoted lines

> +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.

Show 5 quoted lines

> +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".

Show 28 quoted lines

> 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?