{"thread":{"id":"11635","subject":"Make 'git fsck' complain about non-commit branches","startedAt":"2008-01-16T00:34:17Z","lastAt":"2008-01-16T20:28:23Z","messageCount":5,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"65444","messageId":"alpine.LFD.1.00.0801151618300.2806@woody.linux-foundation.org","threadId":"11635","inReplyTo":null,"subject":"Make 'git fsck' complain about non-commit branches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-16T00:34:17Z","receivedAt":"2008-01-16T00:34:17Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nSince having non-commits in branches is a no-no, and just means you cannot \ncommit on them, let's make fsck tell you when a branch is bad.\n\nSigned-off-by: Linus Torvalds \n---\n\n builtin-fsck.c | 13 ++++++++-----\n 1 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-fsck.c b/builtin-fsck.c\nindex 8876d34..6fc9525 100644\n--- a/builtin-fsck.c\n+++ b/builtin-fsck.c\n@@ -555,20 +555,23 @@ static int fsck_handle_reflog(const char *logname, const unsigned char *sha1, in\n \treturn 0;\n }\n \n+static int is_branch(const char *refname)\n+{\n+\treturn !strcmp(refname, \"HEAD\") || !prefixcmp(refname, \"refs/heads/\");\n+}\n+\n static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n {\n \tstruct object *obj;\n \n-\tobj = lookup_object(sha1);\n+\tobj = parse_object(sha1);\n \tif (!obj) {\n-\t\tif (has_sha1_file(sha1)) {\n-\t\t\tdefault_refs++;\n-\t\t\treturn 0; /* it is in a pack */\n-\t\t}\n \t\terror(\"%s: invalid sha1 pointer %s\", refname, sha1_to_hex(sha1));\n \t\t/* We'll continue with the rest despite the error.. */\n \t\treturn 0;\n \t}\n+\tif (obj->type != OBJ_COMMIT && is_branch(refname))\n+\t\terror(\"%s: not a commit\", refname);\n \tdefault_refs++;\n \tobj->used = 1;\n \tmark_reachable(obj, REACHABLE);\n"},{"id":"65446","messageId":"7v8x2qd2hu.fsf@gitster.siamese.dyndns.org","threadId":"11635","inReplyTo":"alpine.LFD.1.00.0801151618300.2806@woody.linux-foundation.org","subject":"Re: Make 'git fsck' complain about non-commit branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T00:43:09Z","receivedAt":"2008-01-16T00:43:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> Since having non-commits in branches is a no-no, and just means you cannot \n> commit on them, let's make fsck tell you when a branch is bad.\n>\n> Signed-off-by: Linus Torvalds \n\nI think all of the three patches (so far -- do you have more?)\nlook sane from the Porcelain-plumbing combination point of view,\nbut I wonder if we should make it more explicit that we are now\nmoving in the direction that makes plumbing much more aware of\nthe Porcelain convention.\n\nSo far, the plumbing level did not care much about the Porcelain\nconvention, such as refs/heads and refs/remotes (you seem to\nhave forgot) are about \"branches\" and must point at commit\nobjects.\n\n> ---\n>\n> builtin-fsck.c | 13 ++++++++-----\n> 1 files changed, 8 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin-fsck.c b/builtin-fsck.c\n> index 8876d34..6fc9525 100644\n> --- a/builtin-fsck.c\n> +++ b/builtin-fsck.c\n> @@ -555,20 +555,23 @@ static int fsck_handle_reflog(const char *logname, const unsigned char *sha1, in\n> \treturn 0;\n> }\n> \n> +static int is_branch(const char *refname)\n> +{\n> +\treturn !strcmp(refname, \"HEAD\") || !prefixcmp(refname, \"refs/heads/\");\n> +}\n> +\n> static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n> {\n> \tstruct object *obj;\n> \n> -\tobj = lookup_object(sha1);\n> +\tobj = parse_object(sha1);\n> \tif (!obj) {\n> -\t\tif (has_sha1_file(sha1)) {\n> -\t\t\tdefault_refs++;\n> -\t\t\treturn 0; /* it is in a pack */\n> -\t\t}\n> \t\terror(\"%s: invalid sha1 pointer %s\", refname, sha1_to_hex(sha1));\n> \t\t/* We'll continue with the rest despite the error.. */\n> \t\treturn 0;\n> \t}\n> +\tif (obj->type != OBJ_COMMIT && is_branch(refname))\n> +\t\terror(\"%s: not a commit\", refname);\n> \tdefault_refs++;\n> \tobj->used = 1;\n> \tmark_reachable(obj, REACHABLE);\n"},{"id":"65448","messageId":"alpine.LFD.1.00.0801151654050.2806@woody.linux-foundation.org","threadId":"11635","inReplyTo":"7v8x2qd2hu.fsf@gitster.siamese.dyndns.org","subject":"Re: Make 'git fsck' complain about non-commit branches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-16T01:01:57Z","receivedAt":"2008-01-16T01:01:57Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 15 Jan 2008, Junio C Hamano wrote:\n> \n> So far, the plumbing level did not care much about the Porcelain\n> convention, such as refs/heads and refs/remotes (you seem to\n> have forgot) are about \"branches\" and must point at commit\n> objects.\n\nYeah. I'm not sure this is all a great idea, but I think they are correct \n(and no, \"refs/remotes/\" would *not* have been correct). \n\nThe \"be more careful about parents\" patch (builtin-commit.c) is \nunquestionably a good idea - those things really *have* to be commits from \na plumbing standpoint.\n\nThe other ones follow the same rules: \"HEAD\" really does need to be a \ncommit, since that will otherwise cause breakage (not just for \"git \ncommit\", but for \"git clone\" too). The same is true of \"git checkout\" and \n\"refs/heads/\" - if a \"refs/heads/\" ref isn't a commit, switching branches \nwill get confused!\n\nSo now git-fsck verifies things that would confuse git itself. But that's \nalso why refs/remotes/* aren't checked for being commits, and really a \nremote branch is very *different* from a local branch - because those \nthings would never be used for a commit chain by the native git commands, \nso git itself shouldn't care.\n\nWe've clearly moved a lot of the porcelain layer into git internals, and \nmaybe this went too far, but I suspect not. You can still do whatever the \nheck you want from a porcelain angle, it just has to follow the (fairly \nlax) rules that core git itself does expect.\n\n\t\tLinus\n"},{"id":"65592","messageId":"7vk5m9pmmq.fsf@gitster.siamese.dyndns.org","threadId":"11635","inReplyTo":"alpine.LFD.1.00.0801151654050.2806@woody.linux-foundation.org","subject":"Re: Make 'git fsck' complain about non-commit branches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-16T19:59:41Z","receivedAt":"2008-01-16T19:59:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Tue, 15 Jan 2008, Junio C Hamano wrote:\n>> \n>> So far, the plumbing level did not care much about the Porcelain\n>> convention, such as refs/heads and refs/remotes (you seem to\n>> have forgot) are about \"branches\" and must point at commit\n>> objects.\n>\n> Yeah. I'm not sure this is all a great idea, but I think they are correct \n> (and no, \"refs/remotes/\" would *not* have been correct). \n\nIf we take that \"plumbing knows much more about Porcelain\nconvention\" shift-of-paradigm all the way, refs/remotes/ would\ncontain what are copied from refs/heads/ elsewhere, so checking\nwould have been correct. If you are saying that we are not\nprepared to take the change that far (which I tend to agree\nwith, as I like to keep the door open for people to do things\nthat at the first sight seem insane but later turns out to be\nuseful in workflows we haven't imagined so far), I'd agree that\nnot insisting on commitness under refs/remotes/ is correct.\n\nIs that where your \"refs/remotes would *not* have been correct\"\ncomes from, or did I miss something more fundamental?\n"},{"id":"65597","messageId":"alpine.LFD.1.00.0801161217050.2806@woody.linux-foundation.org","threadId":"11635","inReplyTo":"7vk5m9pmmq.fsf@gitster.siamese.dyndns.org","subject":"Re: Make 'git fsck' complain about non-commit branches","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-01-16T20:28:23Z","receivedAt":"2008-01-16T20:28:23Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 16 Jan 2008, Junio C Hamano wrote:\n> \n> If we take that \"plumbing knows much more about Porcelain\n> convention\" shift-of-paradigm all the way, refs/remotes/ would\n> contain what are copied from refs/heads/ elsewhere, so checking\n> would have been correct.\n\nYes. I'm taking a slightly weaker approach, which is to say that git \nshould check not \"conventions\", but \"requirements\".\n\nSo the reason I made it check HEAD and refs/heads/* is that yes, it's a \nconvention that they be branches (and thus must point to commit objects), \nbut it's also something deeper than that: git cit commands actually break \nwhen it's not true.\n\nSo I think there is a difference between \"convention\" and \"requirement\". \nOne is how we do things, the other is how things must be done to work.\n\nAnd yes, conventions have a tendency to become requirements, as people \nstart depending on them, so it's not a black-and-white or even a static \nthing, and it changes over time.\n\nI just suspect that at least right now, if refs/remotes/xyzzy isn't a \ncommit, nothing actually technically *breaks*, even if it is against the \nconvention.\n\nFor example, would it be wrong to have \"remote tags\" listed under \nrefs/remotes//tags? I don't think it necessarily is wrong. \nIt's not something we support right now, of course, and maybe we never \nwill, but I don't think it's something that is necessarily conceptually \nbroken - and maybe some external porcelain would want to do it that way?\n\nSo if core git doesn't really care, it shouldn't set the rules. But yes, \nthe rules clearly have expanded over time, and probably will continue to \ndo so..\n\n\t\tLinus\n"}]}