Git development
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>, Marko Kungla <marko.kungla@gmail.com>,
	git@vger.kernel.org
Subject: [PATCH 1/3] check-ref-format --branch: do not expand @{...} outside repository
Date: Tue, 17 Oct 2017 00:08:08 -0700	[thread overview]
Message-ID: <20171017070808.plddffhzdobyekmo@aiede.mtv.corp.google.com> (raw)
In-Reply-To: <20171017070619.bdgvujmbw4uxvwle@aiede.mtv.corp.google.com>

From: Junio C Hamano <gitster@pobox.com>

Running "git check-ref-format --branch @{-1}" from outside any
repository produces

	$ git check-ref-format --branch @{-1}
	BUG: environment.c:182: git environment hasn't been setup

This is because the expansion of @{-1} must come from the HEAD reflog,
which involves opening the repository.  @{u} and @{push} (which are
more unusual because they typically would not expand to a local
branch) trigger the same assertion.

This has been broken since day one.  Before v2.13.0-rc0~48^2
(setup_git_env: avoid blind fall-back to ".git", 2016-10-02), the
breakage was more subtle: Git would read reflogs from ".git" within
the current directory even if it was not a valid repository.  Usually
that is harmless because Git is not being run from the root directory
of an invalid repository, but in edge cases such accesses can be
confusing or harmful.  Since v2.13.0, the problem is easier to
diagnose because Git aborts with a BUG message.

Erroring out is the right behavior: when asked to interpret a branch
name like "@{-1}", there is no reasonable answer in this context.
But we should print a message saying so instead of an assertion failure.

We do not forbid "check-ref-format --branch" from outside a repository
altogether because it is ok for a script to pre-process branch
arguments without @{...} in such a context.  For example, with
pre-2.13 Git, a script that does

	branch='master'; # default value
	parse_options
	branch=$(git check-ref-format --branch "$branch")

to normalize an optional branch name provided by the user would work
both inside a repository (where the user could provide '@{-1}') and
outside (where '@{-1}' should not be accepted).

So disable the "expand @{...}" half of the feature when run outside a
repository, but keep the check of the syntax of a proposed branch
name. This way, when run from outside a repository, "git
check-ref-format --branch @{-1}" will gracefully fail:

	$ git check-ref-format --branch @{-1}
	fatal: '@{-1}' is not a valid branch name

and "git check-ref-format --branch master" will succeed as before:

	$ git check-ref-format --branch master
	master

restoring the usual pre-2.13 behavior.

[jn: split out from a larger patch; moved conditional to
 strbuf_check_branch_ref instead of its caller; fleshed out commit
 message; some style tweaks in tests]

Reported-by: Marko Kungla <marko.kungla@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 sha1_name.c                 |  5 ++++-
 t/t1402-check-ref-format.sh | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/sha1_name.c b/sha1_name.c
index c7c5ab376c..603e667faa 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1331,7 +1331,10 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
-	strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
+	if (startup_info->have_repository)
+		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
+	else
+		strbuf_addstr(sb, name);
 	if (name[0] == '-')
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
diff --git a/t/t1402-check-ref-format.sh b/t/t1402-check-ref-format.sh
index 0790edf60d..98e4a8613b 100755
--- a/t/t1402-check-ref-format.sh
+++ b/t/t1402-check-ref-format.sh
@@ -144,6 +144,11 @@ test_expect_success "check-ref-format --branch @{-1}" '
 	refname2=$(git check-ref-format --branch @{-2}) &&
 	test "$refname2" = master'
 
+test_expect_success 'check-ref-format --branch -naster' '
+	test_must_fail git check-ref-format --branch -naster >actual &&
+	test_must_be_empty actual
+'
+
 test_expect_success 'check-ref-format --branch from subdir' '
 	mkdir subdir &&
 
@@ -161,6 +166,17 @@ test_expect_success 'check-ref-format --branch from subdir' '
 	test "$refname" = "$sha1"
 '
 
+test_expect_success 'check-ref-format --branch @{-1} from non-repo' '
+	nongit test_must_fail git check-ref-format --branch @{-1} >actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'check-ref-format --branch master from non-repo' '
+	echo master >expect &&
+	nongit git check-ref-format --branch master >actual &&
+	test_cmp expect actual
+'
+
 valid_ref_normalized() {
 	prereq=
 	case $1 in
-- 
2.15.0.rc1.287.g2b38de12cc


  reply	other threads:[~2017-10-17  7:08 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-08  1:17 bug with check-ref-format outside of repository Marko Kungla
2017-07-14 18:03 ` Jeff King
2017-07-14 18:18   ` [PATCH] check-ref-format: require a repository for --branch Jeff King
2017-07-17 17:27     ` Jonathan Nieder
2017-07-17 21:18       ` Marko Kungla
2017-08-17 10:23         ` Jeff King
2017-08-17 10:22       ` Jeff King
2017-08-17 21:30         ` Junio C Hamano
2017-08-18  6:20           ` Jeff King
2017-08-18  7:45             ` Junio C Hamano
2017-10-16  6:44         ` Junio C Hamano
2017-10-16 10:45           ` Junio C Hamano
2017-10-16 22:45             ` Jeff King
2017-10-16 23:00               ` Jonathan Nieder
2017-10-17  1:22               ` Junio C Hamano
2017-10-17  2:42                 ` Jeff King
2017-10-17  3:33                   ` Junio C Hamano
2017-10-17  4:44                     ` Junio C Hamano
2017-10-17  7:06                       ` [PATCH 0/3] " Jonathan Nieder
2017-10-17  7:08                         ` Jonathan Nieder [this message]
2017-10-17  7:10                         ` [PATCH 2/3] check-ref-format --branch: strip refs/heads/ using skip_prefix Jonathan Nieder
2017-10-17  7:12                         ` [PATCH 0/3] Re: [PATCH] check-ref-format: require a repository for --branch Junio C Hamano
2017-10-17  7:17                           ` Jonathan Nieder
2017-10-17  8:21                             ` Junio C Hamano
2017-10-17  7:12                         ` [PATCH 3/3] check-ref-format doc: --branch validates and expands <branch> Jonathan Nieder
2017-10-17 20:55                           ` Junio C Hamano
2017-10-17 21:06                             ` Jonathan Nieder
2017-10-18  5:10                             ` Jeff King
2017-10-17  1:27             ` [PATCH] check-ref-format: require a repository for --branch Kevin Daudt
2017-10-17  2:40               ` Junio C Hamano
2017-10-17  4:30                 ` Kevin Daudt
2017-10-16 22:42           ` Jeff King
2017-10-17  4:41           ` Jonathan Nieder
2017-10-17  7:05             ` Junio C Hamano
2017-10-17  7:25               ` Jonathan Nieder
2017-10-17  7:34               ` Jonathan Nieder

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=20171017070808.plddffhzdobyekmo@aiede.mtv.corp.google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=marko.kungla@gmail.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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