Re: "git symbolic-ref" doesn't do a very good job
- From
Linus Torvalds <torvalds@linux-foundation.org>
- Date
- Jul 30, 2022, 20:21 UTC
- Message-ID
- <CAHk-=wg9LaHeg0UmZ90gLOaBpO-5fhoaH22iNNm=1eror95pFg@mail.gmail.com>
- In-Reply-To
- <CAHk-=wh9f0EmsNFgoxUa8BzVej06+7MbLr-MLBjDjtj_=Pf90A@mail.gmail.com>
On Sat, Jul 30, 2022 at 12:53 PM Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > And "git symbolic-ref" is perfectly happy to take complete garbage > input. There seems to be no advantage over using that silly "echo" > model.
Side note: it looks like that patch may break a test.
And that test most definitely *should* be broken.
t3200-branch.sh does
git symbolic-ref refs/heads/dangling-symref nowhere
which really depends on that whole "git symbolic-ref does no sanity checking at all".
In fact, it seems to depend particularly on the fact that for non-HEAD refs, it does even *less* sanity checking, and doesn't even check that the ref starts with a "refs/"
There is also t4202-log, which wants to test that "git log" reacts well to a bad ref. But now that "git symbolic-ref" refuses to create such a bad ref, that test fails.
Anyway, here's a slightly updated patch that just fixes that test that depended on not just a dangling symref, but an *invalid* dangling symref. And it changes t4202-log to use "echo" to create the bad ref instead. Which is what the previous test did too, to create the bogus hash.
Again - this is such a low-level plumbing thing that maybe nobody cares, but it just struck me as a bad idea to have these kinds of maintenance commands that can be used to just mess up your repository. And if you have a bare repo, this really does look like the command that *should* be used to change HEAD, since it's not about "git checkout"
Linus
builtin/symbolic-ref.c | 2 ++ t/t3200-branch.sh | 4 ++-- t/t4202-log.sh | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c index e547a08d6c..5354cfb4f1 100644 --- a/builtin/symbolic-ref.c +++ b/builtin/symbolic-ref.c @@ -71,6 +71,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix) if (!strcmp(argv[0], "HEAD") && !starts_with(argv[1], "refs/")) die("Refusing to point HEAD outside of refs/"); + if (check_refname_format(argv[1], 0) < 0) + die("Refusing to set '%s' to invalid ref '%s'", argv[0], argv[1]); ret = !!create_symref(argv[0], argv[1], msg); break; default: diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 9723c2827c..b194c1b09b 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -723,9 +723,9 @@ test_expect_success 'deleting a symref' ' ' test_expect_success 'deleting a dangling symref' ' - git symbolic-ref refs/heads/dangling-symref nowhere && + git symbolic-ref refs/heads/dangling-symref refs/heads/nowhere && test_path_is_file .git/refs/heads/dangling-symref && - echo "Deleted branch dangling-symref (was nowhere)." >expect && + echo "Deleted branch dangling-symref (was refs/heads/nowhere)." >expect && git branch -d dangling-symref >actual && test_path_is_missing .git/refs/heads/dangling-symref && test_cmp expect actual diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 6e66352558..427b06442d 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -2114,7 +2114,7 @@ test_expect_success REFFILES 'log diagnoses bogus HEAD hash' ' test_expect_success 'log diagnoses bogus HEAD symref' ' git init empty && - git --git-dir empty/.git symbolic-ref HEAD refs/heads/invalid.lock && + echo "ref: refs/heads/invalid.lock" > empty/.git/HEAD && test_must_fail git -C empty log 2>stderr && test_i18ngrep broken stderr && test_must_fail git -C empty log --default totally-bogus 2>stderr &&