Make 'git fsck' complain about non-commit branches

Make 'git fsck' complain about non-commit branches

[Linus Torvalds]

Since having non-commits in branches is a no-no, and just means you cannot 
commit on them, let's make fsck tell you when a branch is bad.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

 builtin-fsck.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 8876d34..6fc9525 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -555,20 +555,23 @@ static int fsck_handle_reflog(const char *logname, const unsigned char *sha1, in
 	return 0;
 }
 
+static int is_branch(const char *refname)
+{
+	return !strcmp(refname, "HEAD") || !prefixcmp(refname, "refs/heads/");
+}
+
 static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	struct object *obj;
 
-	obj = lookup_object(sha1);
+	obj = parse_object(sha1);
 	if (!obj) {
-		if (has_sha1_file(sha1)) {
-			default_refs++;
-			return 0; /* it is in a pack */
-		}
 		error("%s: invalid sha1 pointer %s", refname, sha1_to_hex(sha1));
 		/* We'll continue with the rest despite the error.. */
 		return 0;
 	}
+	if (obj->type != OBJ_COMMIT && is_branch(refname))
+		error("%s: not a commit", refname);
 	default_refs++;
 	obj->used = 1;
 	mark_reachable(obj, REACHABLE);

Re: Make 'git fsck' complain about non-commit branches

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Since having non-commits in branches is a no-no, and just means you cannot 
> commit on them, let's make fsck tell you when a branch is bad.
>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

I think all of the three patches (so far -- do you have more?)
look sane from the Porcelain-plumbing combination point of view,
but I wonder if we should make it more explicit that we are now
moving in the direction that makes plumbing much more aware of
the Porcelain convention.

So far, the plumbing level did not care much about the Porcelain
convention, such as refs/heads and refs/remotes (you seem to
have forgot) are about "branches" and must point at commit
objects.

> ---
>
>  builtin-fsck.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/builtin-fsck.c b/builtin-fsck.c
> index 8876d34..6fc9525 100644
> --- a/builtin-fsck.c
> +++ b/builtin-fsck.c
> @@ -555,20 +555,23 @@ static int fsck_handle_reflog(const char *logname, const unsigned char *sha1, in
>  	return 0;
>  }
>  
> +static int is_branch(const char *refname)
> +{
> +	return !strcmp(refname, "HEAD") || !prefixcmp(refname, "refs/heads/");
> +}
> +
>  static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
>  {
>  	struct object *obj;
>  
> -	obj = lookup_object(sha1);
> +	obj = parse_object(sha1);
>  	if (!obj) {
> -		if (has_sha1_file(sha1)) {
> -			default_refs++;
> -			return 0; /* it is in a pack */
> -		}
>  		error("%s: invalid sha1 pointer %s", refname, sha1_to_hex(sha1));
>  		/* We'll continue with the rest despite the error.. */
>  		return 0;
>  	}
> +	if (obj->type != OBJ_COMMIT && is_branch(refname))
> +		error("%s: not a commit", refname);
>  	default_refs++;
>  	obj->used = 1;
>  	mark_reachable(obj, REACHABLE);

Re: Make 'git fsck' complain about non-commit branches

[Linus Torvalds]

On Tue, 15 Jan 2008, Junio C Hamano wrote:
> 
> So far, the plumbing level did not care much about the Porcelain
> convention, such as refs/heads and refs/remotes (you seem to
> have forgot) are about "branches" and must point at commit
> objects.

Yeah. I'm not sure this is all a great idea, but I think they are correct 
(and no, "refs/remotes/" would *not* have been correct). 

The "be more careful about parents" patch (builtin-commit.c) is 
unquestionably a good idea - those things really *have* to be commits from 
a plumbing standpoint.

The other ones follow the same rules: "HEAD" really does need to be a 
commit, since that will otherwise cause breakage (not just for "git 
commit", but for "git clone" too). The same is true of "git checkout" and 
"refs/heads/" - if a "refs/heads/" ref isn't a commit, switching branches 
will get confused!

So now git-fsck verifies things that would confuse git itself. But that's 
also why refs/remotes/* aren't checked for being commits, and really a 
remote branch is very *different* from a local branch - because those 
things would never be used for a commit chain by the native git commands, 
so git itself shouldn't care.

We've clearly moved a lot of the porcelain layer into git internals, and 
maybe this went too far, but I suspect not. You can still do whatever the 
heck you want from a porcelain angle, it just has to follow the (fairly 
lax) rules that core git itself does expect.

		Linus

Re: Make 'git fsck' complain about non-commit branches

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, 15 Jan 2008, Junio C Hamano wrote:
>> 
>> So far, the plumbing level did not care much about the Porcelain
>> convention, such as refs/heads and refs/remotes (you seem to
>> have forgot) are about "branches" and must point at commit
>> objects.
>
> Yeah. I'm not sure this is all a great idea, but I think they are correct 
> (and no, "refs/remotes/" would *not* have been correct). 

If we take that "plumbing knows much more about Porcelain
convention" shift-of-paradigm all the way, refs/remotes/ would
contain what are copied from refs/heads/ elsewhere, so checking
would have been correct.  If you are saying that we are not
prepared to take the change that far (which I tend to agree
with, as I like to keep the door open for people to do things
that at the first sight seem insane but later turns out to be
useful in workflows we haven't imagined so far), I'd agree that
not insisting on commitness under refs/remotes/ is correct.

Is that where your "refs/remotes would *not* have been correct"
comes from, or did I miss something more fundamental?

Re: Make 'git fsck' complain about non-commit branches

[Linus Torvalds]

On Wed, 16 Jan 2008, Junio C Hamano wrote:
> 
> If we take that "plumbing knows much more about Porcelain
> convention" shift-of-paradigm all the way, refs/remotes/ would
> contain what are copied from refs/heads/ elsewhere, so checking
> would have been correct.

Yes. I'm taking a slightly weaker approach, which is to say that git 
should check not "conventions", but "requirements".

So the reason I made it check HEAD and refs/heads/* is that yes, it's a 
convention that they be branches (and thus must point to commit objects), 
but it's also something deeper than that: git cit commands actually break 
when it's not true.

So I think there is a difference between "convention" and "requirement". 
One is how we do things, the other is how things must be done to work.

And yes, conventions have a tendency to become requirements, as people 
start depending on them, so it's not a black-and-white or even a static 
thing, and it changes over time.

I just suspect that at least right now, if refs/remotes/xyzzy isn't a 
commit, nothing actually technically *breaks*, even if it is against the 
convention.

For example, would it be wrong to have "remote tags" listed under 
refs/remotes/<remotename>/tags? I don't think it necessarily is wrong. 
It's not something we support right now, of course, and maybe we never 
will, but I don't think it's something that is necessarily conceptually 
broken - and maybe some external porcelain would want to do it that way?

So if core git doesn't really care, it shouldn't set the rules. But yes, 
the rules clearly have expanded over time, and probably will continue to 
do so..

		Linus