{"thread":{"id":"8809","subject":"[PATCH] Make git-prune submodule aware (and fix a SEGFAULT in the process)","startedAt":"2007-07-02T12:56:58Z","lastAt":"2007-08-20T08:39:50Z","messageCount":8,"participants":["Andy Parkins","Junio C Hamano","Johannes Sixt","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"46233","messageId":"200707021356.58553.andyparkins@gmail.com","threadId":"8809","inReplyTo":null,"subject":"[PATCH] Make git-prune submodule aware (and fix a SEGFAULT in the process)","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-07-02T12:56:58Z","receivedAt":"2007-07-02T12:56:58Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"I ran git-prune on a repository and got this:\n\n $ git-prune\n error: Object 228f8065b930120e35fc0c154c237487ab02d64a is a blob, not a commit\n Segmentation fault (core dumped)\n\nThis repository was a strange one in that it was being used to provide\nits own submodule.  That is, the repository was cloned into a\nsubdirectory, an independent branch checked out in that subdirectory,\nand then it was marked as a submodule.  git-prune then failed in the\nabove manner.\n\nThe problem was that git-prune was not submodule aware in two areas.\n\nLinus said:\n\n > So what happens is that something traverses a tree object, looks at each\n > entry, sees that it's not a tree, and tries to look it up as a blob. But\n > subprojects are commits, not blobs, and then when you look at the object\n > more closely, you get the above kind of object type confusion.\n\nand included a patch to add an S_ISGITLINK() test to reachable.c's\nprocess_tree() function.  That fixed the first git-prune error, and\nstopped it from trying to process the gitlink entries in trees as if\nthey were pointers to other trees (and of course failing, because\ngitlinks _aren't_ trees).  That part of this patch is his.\n\nThe second area is add_cache_refs().  This is called before starting the\nreachability analysis, and was calling lookup_blob() on every object\nhash found in the index.  However, it is no longer true that every hash\nin the index is a pointer to a blob, some of them are gitlinks, and are\nnot backed by any object at all, they are commits in another repository.\nNormally this bug was not causing any problems, but in the case of the\nself-referencing repository described above, it meant that the gitlink\nhash was being marked as being of type OBJ_BLOB by add_cache_refs() call\nto lookup_blob().  Then later, because that hash was also pointed to by\na ref, add_one_ref() would treat it as a commit; lookup_commit() would\nreturn a NULL because that object was already noted as being an\nOBJ_BLOB, not an OBJ_COMMIT; and parse_commit_buffer() would SEGFAULT on\nthat NULL pointer.\n\nThe fix made by this patch is to not blindly call lookup_blob() in\nreachable.c's add_cache_refs(), and instead skip any index entries that\nare S_ISGITLINK().\n\nSigned-off-by: Andy Parkins <andyparkins@gmail.com>\n---\nThe two parts go together logically so I've put them in this single patch\nfrom me, but half of this patch should be credited to Linus. (I don't know\nhow one deals with multiple authorship on a single commit in git though)\n\nI suspect that Linus has enough credit to keep him happy and won't mind\nthat I've edited him out in this case :-)\n\n\n reachable.c |   20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex ff3dd34..6383401 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -21,6 +21,14 @@ static void process_blob(struct blob *blob,\n \t/* Nothing to do, really .. The blob lookup was the important part */\n }\n \n+static void process_gitlink(const unsigned char *sha1,\n+\t\t\t    struct object_array *p,\n+\t\t\t    struct name_path *path,\n+\t\t\t    const char *name)\n+{\n+\t/* I don't think we want to recurse into this, really. */\n+}\n+\n static void process_tree(struct tree *tree,\n \t\t\t struct object_array *p,\n \t\t\t struct name_path *path,\n@@ -47,6 +55,8 @@ static void process_tree(struct tree *tree,\n \twhile (tree_entry(&desc, &entry)) {\n \t\tif (S_ISDIR(entry.mode))\n \t\t\tprocess_tree(lookup_tree(entry.sha1), p, &me, entry.path);\n+\t\telse if (S_ISGITLINK(entry.mode))\n+\t\t\tprocess_gitlink(entry.sha1, p, &me, entry.path);\n \t\telse\n \t\t\tprocess_blob(lookup_blob(entry.sha1), p, &me, entry.path);\n \t}\n@@ -159,6 +169,16 @@ static void add_cache_refs(struct rev_info *revs)\n \n \tread_cache();\n \tfor (i = 0; i < active_nr; i++) {\n+\t\t/*\n+\t\t * The index can contain blobs and GITLINKs, GITLINKs are hashes\n+\t\t * that don't actually point to objects in the repository, it's\n+\t\t * almost guaranteed that they are NOT blobs, so we don't call\n+\t\t * lookup_blob() on them, to avoid populating the hash table\n+\t\t * with invalid information\n+\t\t */\n+\t\tif (S_ISGITLINK(ntohl(active_cache[i]->ce_mode)))\n+\t\t\tcontinue;\n+\n \t\tlookup_blob(active_cache[i]->sha1);\n \t\t/*\n \t\t * We could add the blobs to the pending list, but quite\n-- \n1.5.2.2.253.g2d8b\n"},{"id":"46299","messageId":"7vwsxiv1sf.fsf@assigned-by-dhcp.cox.net","threadId":"8809","inReplyTo":"200707021356.58553.andyparkins@gmail.com","subject":"Re: [PATCH] Make git-prune submodule aware (and fix a SEGFAULT in the process)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-02T23:11:44Z","receivedAt":"2007-07-02T23:11:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins <andyparkins@gmail.com> writes:\n\n> ...\n> The fix made by this patch is to not blindly call lookup_blob() in\n> reachable.c's add_cache_refs(), and instead skip any index entries that\n> are S_ISGITLINK().\n\nThanks, both of you.  Will go to 'maint' hopefully tonight (if I\ncan shake the day job off early enough today, that is).\n"},{"id":"50923","messageId":"200708170939.47214.andyparkins@gmail.com","threadId":"8809","inReplyTo":"200707021356.58553.andyparkins@gmail.com","subject":"Re: [PATCH] Make git-prune submodule aware (and fix a SEGFAULT in the process)","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-08-17T08:39:46Z","receivedAt":"2007-08-17T08:39:46Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Monday 2007, July 02, Andy Parkins wrote:\n\n> This repository was a strange one in that it was being used to provide\n> its own submodule.  That is, the repository was cloned into a\n> subdirectory, an independent branch checked out in that subdirectory,\n> and then it was marked as a submodule.  git-prune then failed in the\n> above manner.\n\nI think I've stumbled on another place where this is happening.  \ngit-upload-pack is crashing for me with a similar \"Object is commit not \nblob\" error just before.\n\nI'm happy to try and track it down, but I'm having difficulty because I \nthink the crash is happening in the process on the remote system, so I'm \nnot getting a core dump I can use.\n\nCould any of the guru's give me a guide to upload-pack.c?  I assume the \nproblem is going to be the same as it was for git-prune, the hash for the \ngitlink object in the tree is being assumed to be an object in the ODB; \nwhich isn't the case with gitlink entries.  Where would that be happening \nin git-upload-pack?  The fix is going to be..\n\n if( S_ISGITLINK(mode))\n      continue;\n\nBut I've got no idea where to put it :-)\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"50926","messageId":"46C56704.6D917A53@eudaptics.com","threadId":"8809","inReplyTo":"200708170939.47214.andyparkins@gmail.com","subject":"Re: [PATCH] Make git-prune submodule aware (and fix a SEGFAULT in the process)","fromName":"Johannes Sixt","fromEmail":"j.sixt@eudaptics.com","sentAt":"2007-08-17T09:14:44Z","receivedAt":"2007-08-17T09:14:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Andy Parkins wrote:\n> Could any of the guru's give me a guide to upload-pack.c?  I assume the\n> problem is going to be the same as it was for git-prune, the hash for the\n> gitlink object in the tree is being assumed to be an object in the ODB;\n> which isn't the case with gitlink entries.  Where would that be happening\n> in git-upload-pack?  The fix is going to be..\n> \n>  if( S_ISGITLINK(mode))\n>       continue;\n> \n> But I've got no idea where to put it :-)\n\nMost likely in list-objects.c:traverse_commit_list(), which is called\nfrom somewhere in upload-pack.c.\n\n-- Hannes\n"},{"id":"50936","messageId":"alpine.LFD.0.999.0708170956140.30176@woody.linux-foundation.org","threadId":"8809","inReplyTo":"200708170939.47214.andyparkins@gmail.com","subject":"Re: [PATCH] Make git-prune submodule aware (and fix a SEGFAULT in the process)","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-08-17T16:56:54Z","receivedAt":"2007-08-17T16:56:54Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 17 Aug 2007, Andy Parkins wrote:\n>\n> Could any of the guru's give me a guide to upload-pack.c?  I assume the \n> problem is going to be the same as it was for git-prune, the hash for the \n> gitlink object in the tree is being assumed to be an object in the ODB; \n> which isn't the case with gitlink entries.  Where would that be happening \n> in git-upload-pack?  The fix is going to be..\n> \n>  if( S_ISGITLINK(mode))\n>       continue;\n> \n> But I've got no idea where to put it :-)\n\nMaybe this one?\n\n\t\t\tLinus\n\n---\n builtin-pack-objects.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 24926db..77481df 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -979,6 +979,8 @@ static void add_pbase_object(struct tree_desc *tree,\n \tint cmp;\n \n \twhile (tree_entry(tree,&entry)) {\n+\t\tif (S_ISGITLINK(entry.mode))\n+\t\t\tcontinue;\n \t\tcmp = tree_entry_len(entry.path, entry.sha1) != cmplen ? 1 :\n \t\t      memcmp(name, entry.path, cmplen);\n \t\tif (cmp > 0)\n"},{"id":"50970","messageId":"7vtzqxen8b.fsf@gitster.siamese.dyndns.org","threadId":"8809","inReplyTo":"alpine.LFD.0.999.0708170956140.30176@woody.linux-foundation.org","subject":"Re: [PATCH] Make git-prune submodule aware (and fix a SEGFAULT in the process)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-17T23:48:52Z","receivedAt":"2007-08-17T23:48:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Fri, 17 Aug 2007, Andy Parkins wrote:\n>>\n>> Could any of the guru's give me a guide to upload-pack.c?  I assume the \n>> problem is going to be the same as it was for git-prune, the hash for the \n>> gitlink object in the tree is being assumed to be an object in the ODB; \n>> which isn't the case with gitlink entries.  Where would that be happening \n>> in git-upload-pack?  The fix is going to be..\n>> \n>>  if( S_ISGITLINK(mode))\n>>       continue;\n>> \n>> But I've got no idea where to put it :-)\n>\n> Maybe this one?\n>\n> ---\n>  builtin-pack-objects.c |    2 ++\n>  1 files changed, 2 insertions(+), 0 deletions(-)\n>\n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index 24926db..77481df 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -979,6 +979,8 @@ static void add_pbase_object(struct tree_desc *tree,\n>  \tint cmp;\n>  \n>  \twhile (tree_entry(tree,&entry)) {\n> +\t\tif (S_ISGITLINK(entry.mode))\n> +\t\t\tcontinue;\n>  \t\tcmp = tree_entry_len(entry.path, entry.sha1) != cmplen ? 1 :\n>  \t\t      memcmp(name, entry.path, cmplen);\n>  \t\tif (cmp > 0)\n\n\nThis sounds very plausible.\n\nAndy, in the repository your fetch fails, if a fetch-pack\nwithout \"--thin\" before Linus's patch does not barf, that\nstrongly suggests that the breakage you are seeing is related to\nthis codepath.  And with Linus's patch, \"fetch-pack --thin\"\nwould also be fixed.\n"},{"id":"50976","messageId":"7vwsvtcglv.fsf_-_@gitster.siamese.dyndns.org","threadId":"8809","inReplyTo":"alpine.LFD.0.999.0708170956140.30176@woody.linux-foundation.org","subject":"[PATCH] Make thin-pack generation subproject aware.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-18T09:54:52Z","receivedAt":"2007-08-18T09:54:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When a thin pack wants to send a tree object at \"sub/dir\", and\nthe commit that is common between the sender and the receiver\nthat is used as the base object has a subproject at that path,\nwe should not try to use the data at \"sub/dir\" of the base tree\nas a tree object.  It is not a tree to begin with, and more\nimportantly, the commit object there does not have to even\nexist.\n\n---\n\n This turned out to be trickier to trigger than I thought.  One\n case to trigger is to have a subproject in the past at sub/dir\n and then turn it into a directory.\n\n builtin-pack-objects.c       |    2 +\n t/t3050-subprojects-fetch.sh |   52 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 0 deletions(-)\n create mode 100755 t/t3050-subprojects-fetch.sh\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 24926db..77481df 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -979,6 +979,8 @@ static void add_pbase_object(struct tree_desc *tree,\n \tint cmp;\n \n \twhile (tree_entry(tree,&entry)) {\n+\t\tif (S_ISGITLINK(entry.mode))\n+\t\t\tcontinue;\n \t\tcmp = tree_entry_len(entry.path, entry.sha1) != cmplen ? 1 :\n \t\t      memcmp(name, entry.path, cmplen);\n \t\tif (cmp > 0)\ndiff --git a/t/t3050-subprojects-fetch.sh b/t/t3050-subprojects-fetch.sh\nnew file mode 100755\nindex 0000000..34f26a8\n--- /dev/null\n+++ b/t/t3050-subprojects-fetch.sh\n@@ -0,0 +1,52 @@\n+#!/bin/sh\n+\n+test_description='fetching and pushing project with subproject'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_tick &&\n+\tmkdir -p sub && (\n+\t\tcd sub &&\n+\t\tgit init &&\n+\t\t>subfile &&\n+\t\tgit add subfile\n+\t\tgit commit -m \"subproject commit #1\"\n+\t) &&\n+\t>mainfile\n+\tgit add sub mainfile &&\n+\ttest_tick &&\n+\tgit commit -m \"superproject commit #1\"\n+'\n+\n+test_expect_success clone '\n+\tgit clone file://`pwd`/.git cloned &&\n+\t(git rev-parse HEAD; git ls-files -s) >expected &&\n+\t(\n+\t\tcd cloned &&\n+\t\t(git rev-parse HEAD; git ls-files -s) >../actual\n+\t) &&\n+\tdiff -u expected actual\n+'\n+\n+test_expect_success advance '\n+\techo more >mainfile &&\n+\tgit update-index --force-remove sub &&\n+\tmv sub/.git sub/.git-disabled &&\n+\tgit add sub/subfile mainfile &&\n+\tmv sub/.git-disabled sub/.git &&\n+\ttest_tick &&\n+\tgit commit -m \"superproject commit #2\"\n+'\n+\n+test_expect_success fetch '\n+\t(git rev-parse HEAD; git ls-files -s) >expected &&\n+\t(\n+\t\tcd cloned &&\n+\t\tgit pull &&\n+\t\t(git rev-parse HEAD; git ls-files -s) >../actual\n+\t) &&\n+\tdiff -u expected actual\n+'\n+\n+test_done\n"},{"id":"51069","messageId":"200708200939.50955.andyparkins@gmail.com","threadId":"8809","inReplyTo":"7vtzqxen8b.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git-prune submodule aware (and fix a SEGFAULT in the process)","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-08-20T08:39:50Z","receivedAt":"2007-08-20T08:39:50Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Saturday 2007 August 18, Junio C Hamano wrote:\n\n> Andy, in the repository your fetch fails, if a fetch-pack\n> without \"--thin\" before Linus's patch does not barf, that\n> strongly suggests that the breakage you are seeing is related to\n> this codepath.  And with Linus's patch, \"fetch-pack --thin\"\n> would also be fixed.\n\nI'm really sorry, somehow during my attempts to find the fault, the fault went \naway.  I think it's because I managed to get the fetch to work in some way, \nand from then on fetch completed perfectly.\n\nThe upshot of this is that I have no way to test this patch, until I manage to \nget myself in a similar state.  I'll wait until it happens again though and \nthen try this patch.\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"}]}