Re: [PATCH RFC 4/4] notes: add per-ref configuration of merge strategy
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:06:06
Jacob Keller [off-list ref] 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".
quoted hunk
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?