threads / discuss / 11635

Make 'git fsck' complain about non-commit branches

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

## tl;dr

5 messages between Jan 16, 2008 and Jan 16, 2008.

replies: 4people: 2as markdown or json

Linus Torvalds· Jan 16, 2008, 00:34 UTC · lore

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);

Junio C Hamano· Jan 16, 2008, 00:43 UTC · re: Linus Torvalds · lore

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

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.

Show 38 quoted lines

> ---
>
>  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);

Linus Torvalds· Jan 16, 2008, 01:01 UTC · re: Junio C Hamano · lore

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

On Tue, 15 Jan 2008, Junio C Hamano wrote:

Show 5 quoted lines

> 
> 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

Junio C Hamano· Jan 16, 2008, 19:59 UTC · re: Linus Torvalds · lore

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

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

Show 9 quoted lines

> 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?

Linus Torvalds· Jan 16, 2008, 20:28 UTC · re: Junio C Hamano · lore

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

On Wed, 16 Jan 2008, Junio C Hamano wrote:

Show 5 quoted lines

> 
> 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