Re: [PATCH v7 4/4] notes: teach git-notes about notes.<ref>.mergestrategy option

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 14, 2015, 22:01 UTC
Message-ID: <xmqq8u9dh6lq.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1439586835-15712-5-git-send-email-jacob.e.keller@intel.com>

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

Show 14 quoted lines

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

Show 5 quoted lines

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