From 7c3f847aad79f847e3b786e1bb56361af3f61a69 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 17 Oct 2017 00:08:08 -0700 Subject: [PATCH 1/3] check-ref-format --branch: do not expand @{...} outside repository 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 Helped-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- 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 5e2ec37b65..a1f13cfed1 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -1321,7 +1321,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 From 7ccc94ff4590cd035b7f78dfd9debbd7e692f3e6 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 17 Oct 2017 00:10:00 -0700 Subject: [PATCH 2/3] check-ref-format --branch: strip refs/heads/ using skip_prefix The expansion returned from strbuf_check_branch_ref always starts with "refs/heads/" by construction, but there is nothing about its name or advertised API making that obvious. This command is used to process human-supplied input from the command line and is usually not the inner loop, so we can spare some cycles to be more defensive. Instead of hard-coding the offset strlen("refs/heads/") to skip, verify that the expansion actually starts with refs/heads/. [jn: split out from a larger patch, added explanation] Signed-off-by: Junio C Hamano Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/check-ref-format.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c index eac499450f..737e5ecba3 100644 --- a/builtin/check-ref-format.c +++ b/builtin/check-ref-format.c @@ -39,12 +39,14 @@ static char *collapse_slashes(const char *refname) static int check_ref_format_branch(const char *arg) { struct strbuf sb = STRBUF_INIT; + const char *name; int nongit; setup_git_directory_gently(&nongit); - if (strbuf_check_branch_ref(&sb, arg)) + if (strbuf_check_branch_ref(&sb, arg) || + !skip_prefix(sb.buf, "refs/heads/", &name)) die("'%s' is not a valid branch name", arg); - printf("%s\n", sb.buf + 11); + printf("%s\n", name); return 0; } From 89dd32aedcb52a77240f3923a687edc8a61662e7 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 17 Oct 2017 00:12:34 -0700 Subject: [PATCH 3/3] check-ref-format doc: --branch validates and expands "git check-ref-format --branch $name" feature was originally introduced (and was advertised) as a way for scripts to take any end-user supplied string (like "master", "@{-1}" etc.) and see if it is usable when Git expects to see a branch name, and also obtain the concrete branch name that the at-mark magic expands to. Emphasize that "see if it is usable" role in the description and clarify that the @{...} expansion only occurs when run from within a repository. [jn: split out from a larger patch] Helped-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- Documentation/git-check-ref-format.txt | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt index 92777cef25..cf0a0b7df2 100644 --- a/Documentation/git-check-ref-format.txt +++ b/Documentation/git-check-ref-format.txt @@ -77,7 +77,14 @@ reference name expressions (see linkgit:gitrevisions[7]): . at-open-brace `@{` is used as a notation to access a reflog entry. -With the `--branch` option, it expands the ``previous branch syntax'' +With the `--branch` option, the command takes a name and checks if +it can be used as a valid branch name (e.g. when creating a new +branch). The rule `git check-ref-format --branch $name` implements +may be stricter than what `git check-ref-format refs/heads/$name` +says (e.g. a dash may appear at the beginning of a ref component, +but it is explicitly forbidden at the beginning of a branch name). +When run with `--branch` option in a repository, the input is first +expanded for the ``previous branch syntax'' `@{-n}`. For example, `@{-1}` is a way to refer the last branch you were on. This option should be used by porcelains to accept this syntax anywhere a branch name is expected, so they can act as if you