Re: Suggestion: better error reporting when setting HEAD
[Junio cc'd, as I think he may have comments on the patch at the end]
On Thu, Dec 18, 2008 at 06:58:25PM +1300, Francois Marier wrote:
Show 8 quoted lines
> If you take any valid git repo and then do the following:
>
> git symbolic-ref HEAD master
> (instead of "git symbolic-ref HEAD refs/heads/master")
>
> then other git commands (e.g. "git log") will return this error message:
>
> fatal: Not a git repository
I've done this exact same thing, and it is very annoying.
Show 6 quoted lines
> I'd like to suggest two "usability" improvements:
>
> 1- changing the error message to explain why the directory is no longer
> considered a git repo. Maybe something along the lines of:
>
> fatal: Invalid reference in .git/HEAD
This can be done, but is more complex than you might hope; the test for "is this is a git directory" may get run multiple times while trying to find out if we are in a repo. So giving a reason why we aren't in a git repo is going to end up something like:
I tried X, but its HEAD ref had cruft in it;
then I tried Y, but it had no HEAD file;
then I gave up
Obviously with a working tree, the first directory with the '.git' repo is a likely candidate for being the right thing, but with an error. But if you are looking for a bare repository, then it could be any parent directory. Maybe it is worth just spewing "there is a HEAD file here, but it doesn't look right" when it fails to validate. It would give a false positive if you had a file named HEAD (and you weren't actually in a git repo), but that is unlikely to happen. And if you get multiple ones, so be it.
> 2- To prevent this error from happening in the first place, git-symbolic-ref
> could refuse to change HEAD to a non-existent ref.
Actually, we sometimes need to point to non-existent refs; these are "branches yet to be born". The reason that the new HEAD doesn't validate is that it is supposed to start with "refs/".
We could fix it like this:
diff --git a/builtin-symbolic-ref.c b/builtin-symbolic-ref.c
index bfc78bb..08fa3ce 100644
--- a/builtin-symbolic-ref.c
+++ b/builtin-symbolic-ref.c
@@ -44,6 +44,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
check_symref(argv[0], quiet);
break;
case 2:
+ if (prefixcmp(argv[1], "refs/"))
+ die("Refusing to point %s outside of refs/", argv[0]);
create_symref(argv[0], argv[1], msg);
break;
default:
which tries to match the behavior used by validate_headref, but there
are a few open questions:
- Should it require this of _all_ symbolic refs, or just HEAD?
- Should there be a "-f" force option?
- Should it be moved to the create_symref library function. It gets
called in several places, and I didn't audit all of them.
- I looked at trying to share validate_headref's logic (so they don't
get out of sync), but really the only thing that applies is the
"refs" check. The rest is about checking parts of the symref that are
outside of the actual content.
- This doesn't allow a sha1 for detached HEAD. However, that currently
doesn't work _anyway_, as it creates a HEAD with "ref: $SHA1", which
is totally bogus.
-Peff
