Jacob Keller <jacob.e.keller@xxxxxxxxx> writes:
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 12a42b583f98..bdfd9c7d29b4 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> ...
> @@ -833,7 +833,14 @@ static int merge(int argc, const char **argv, const char *prefix)
> usage_with_options(git_notes_merge_usage, options);
> }
> } else {
> - git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
> + if (!skip_prefix(o.local_ref, "refs/notes/", &short_ref))
> + die("Refusing to merge notes into %s (outside of refs/notes/)",
> + o.local_ref);
> +
Sorry, but I lost track.
Do I understand correctly the consensus on the previous discussion?
My understanding is:
(1) We do not currently refuse to merge notes into anywhere outside
of refs/notes/;
(2) But that is not a designed behaviour---we simply forgot to
check it---we should start checking and refusing.
If that is the concensus, having this check somewhere in the merge()
function is indeed necessary, but this looks very out of place.
Think what happens if the user passes "--stratagy manual" from the
command line. This check is not even performed, is it?
I'd prefer to see:
* "Let's start making sure that we do not allow touching outside
refs/notes/" as a separate patch, perhaps as a preparatory step.
* Have the check apply consistently, regardless of where the
strategy comes from.
* That separate patch to add this restriction should test that
the refusal triggers correctly when it should, and it does not
trigger when it shouldn't.
> + strbuf_addf(&merge_key, "notes.%s.mergestrategy", short_ref);
> +
> + if (git_config_get_notes_strategy(merge_key.buf, &o.strategy))
> + git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
> }
I think you are leaking merge_key after you are done using it.
It is tempting to suggest writing the above like so:
git_config_get_notes_strategy(merge_key.buf, &o.strategy)) ||
git_config_get_notes_strategy("notes.mergestrategy", &o.strategy);
which might make it more obvious what is going on, but I do not care
too deeply about it. To be honest, I am not sure which one is
easier to read in the longer term myself ;-).
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html