From: Junio C Hamano <gitster@pobox•com>
To: Jacob Keller <jacob.e.keller@intel•com>
Cc: git@vger•kernel.org, Johan Herland <johan@herland•net>,
Michael Haggerty <mhagger@alum•mit.edu>,
Eric Sunshine <sunshine@sunshineco•com>,
Jacob Keller <jacob.keller@gmail•com>
Subject: Re: [PATCH v7 4/4] notes: teach git-notes about notes.<ref>.mergestrategy option
Date: Fri, 14 Aug 2015 15:01:53 -0700 [thread overview]
Message-ID: <xmqq8u9dh6lq.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1439586835-15712-5-git-send-email-jacob.e.keller@intel.com> (Jacob Keller's message of "Fri, 14 Aug 2015 14:13:55 -0700")
Jacob Keller <jacob.e.keller@intel•com> 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.
next prev parent reply other threads:[~2015-08-14 22:01 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-14 21:13 [PATCH v7 0/4] notes.mergestrategy option(s) Jacob Keller
2015-08-14 21:13 ` [PATCH v7 1/4] notes: document cat_sort_uniq rewriteMode Jacob Keller
2015-08-14 22:11 ` Junio C Hamano
2015-08-14 22:53 ` Jacob Keller
2015-08-15 10:06 ` Johan Herland
2015-08-14 21:13 ` [PATCH v7 2/4] notes: add tests for --commit/--abort/--strategy exclusivity Jacob Keller
2015-08-14 21:13 ` [PATCH v7 3/4] notes: add notes.mergestrategy option to select default strategy Jacob Keller
2015-08-15 9:09 ` Johan Herland
2015-08-14 21:13 ` [PATCH v7 4/4] notes: teach git-notes about notes.<ref>.mergestrategy option Jacob Keller
2015-08-14 22:01 ` Junio C Hamano [this message]
2015-08-14 22:10 ` Eric Sunshine
2015-08-14 22:50 ` Jacob Keller
2015-08-17 17:33 ` Junio C Hamano
2015-08-14 22:48 ` Jacob Keller
2015-08-17 17:22 ` Junio C Hamano
2015-08-15 9:25 ` Johan Herland
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=xmqq8u9dh6lq.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=jacob.e.keller@intel$(echo .)com \
--cc=jacob.keller@gmail$(echo .)com \
--cc=johan@herland$(echo .)net \
--cc=mhagger@alum$(echo .)mit.edu \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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