{"thread":{"id":"8756","subject":"Bug: segfault during git-prune","startedAt":"2007-06-28T10:34:58Z","lastAt":"2007-07-02T21:01:19Z","messageCount":11,"participants":["Andy Parkins","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"45964","messageId":"200706281134.58453.andyparkins@gmail.com","threadId":"8756","inReplyTo":null,"subject":"Bug: segfault during git-prune","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-06-28T10:34:58Z","receivedAt":"2007-06-28T10:34:58Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"Hello,\n\nI ran git-prune on a repository and got this:\n\n$ git-prune\nerror: Object 228f8065b930120e35fc0c154c237487ab02d64a is a blob, not a commit\nSegmentation fault (core dumped)\n$ git-cat-file -t 228f8065b930120e35fc0c154c237487ab02d64a\ncommit\n\ngit-show of the object shows it looks okay. git-fsck just shows a load\nof dangling objects - which isn't a surprise, that's why I was running\ngit-prune.\n\nHere's the backtrace:\n\nparse_commit_buffer (item=0x0, buffer=0x81124b8, size=421) at commit.c:292\n#1 0x0808ad22 in parse_object_buffer (sha1=0x8111ac5 \"\\\"\\217\\200e¹0\\022\\0165ü\\f\\025L#t\\207«\\002ÖJ\", type=OBJ_COMMIT, size=421, buffer=0x81124b8,\n eaten_p=0xafdc6c40) at object.c:152\n#2 0x0808adb7 in parse_object (sha1=0x8111ac5 \"\\\"\\217\\200e¹0\\022\\0165ü\\f\\025L#t\\207«\\002ÖJ\") at object.c:187\n#3 0x0808d1a2 in add_one_ref (path=0x8111aed \"refs/remotes/libswscale-svn\", sha1=0x8111ac5 \"\\\"\\217\\200e¹0\\022\\0165ü\\f\\025L#t\\207«\\002ÖJ\", flag=6,\n cb_data=0xafdc6d40) at reachable.c:124\n#4 0x0809133e in do_one_ref (base=, fn=0x808d190 , trim=0, cb_data=0xafdc6d40, entry=0x8111ac0) at refs.c:478\n#5 0x08092be4 in do_for_each_ref (base=0x80c8e76 \"refs/\", fn=0x808d190 , trim=0, cb_data=0xafdc6d40) at refs.c:545\n#6 0x0808cff4 in mark_reachable_objects (revs=0xafdc6d40, mark_reflog=1) at reachable.c:188\n#7 0x08071c90 in cmd_prune (argc=1, argv=0xafdc7074, prefix=0x0) at builtin-prune.c:93\n#8 0x0804aa24 in handle_internal_command (argc=1, argv=0xafdc7074, envp=) at git.c:348\n#9 0x0804b4c9 in main (argc=1, argv=Cannot access memory at address 0xe\n\nSo, I think there are two faults: git-prune is deciding that the object is\na blob, when it's actually a commit; and git-prune's error handling is broken\nin that case, because it's continuing with the NULL pointer returned by\ncheck_commit() when obj->type != OBJ_COMMIT.\n\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"45965","messageId":"200706281152.08591.andyparkins@gmail.com","threadId":"8756","inReplyTo":"200706281134.58453.andyparkins@gmail.com","subject":"Re: Bug: segfault during git-prune","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-06-28T10:52:04Z","receivedAt":"2007-06-28T10:52:04Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Thursday 2007 June 28, Andy Parkins wrote:\n\n> So, I think there are two faults: git-prune is deciding that the object is\n> a blob, when it's actually a commit; and git-prune's error handling is\n> broken in that case, because it's continuing with the NULL pointer returned\n> by check_commit() when obj->type != OBJ_COMMIT.\n\nI can't figure it out I'm afraid. I've done a bit of back tracing and found\nthe block that's generating the error; object.c:150\n\n } else if (type == OBJ_COMMIT) {\n struct commit *commit = lookup_commit(sha1);\n parse_commit_buffer(commit, buffer, size);\n if (!commit->buffer) {\n commit->buffer = buffer;\n eaten = 1;\n }\n obj = &commit->object;\n }\n\t\nNow, lookup_commit() is returning NULL because check_commit() is being called\nwith a struct object whose type field is not OBJ_COMMIT. Which it presumably\ngot from lookup_object(). gdb tells me the object returned by lookup_object() \nis\n\n {parsed = 0, used = 0, type = 3, flags = 0,\n sha1 = \"\\\"\\217\\200e¹0\\022\\0165ü\\f\\025L#t\\207«\\002ÖJ\"}\n\nWhich seems to be the right hash, 228f8065b930120e35fc0c154c237487ab02d64a.\n\nWhat should I do next? I don't understand why lookup_object() is returning\nan object with type blob instead of with type commit, when git-cat-file says \nthat same object is a commit.\n\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"45988","messageId":"alpine.LFD.0.98.0706280844460.8675@woody.linux-foundation.org","threadId":"8756","inReplyTo":"200706281134.58453.andyparkins@gmail.com","subject":"Re: Bug: segfault during git-prune","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-28T15:59:35Z","receivedAt":"2007-06-28T15:59:35Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 28 Jun 2007, Andy Parkins wrote:\n> \n> 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\nDo you have subprojects in that git repo?\n\nWhat happens is that all git object handling *refuses* (correctly) to \ntouch an object of the wrong type. If it's been marked as a commit at some \npoint, it had better be looked up as a commit the *next* time we see that \nsame object too.\n\nAnd every normal git object traversal will specify in advance what kind of \nobject it expects (except for the initial refs lookups, that don't know, \nand use a totally different interface for \"give me any kind of object, and \nfigure it out\").\n\nSo the error in this case is that we decided it was a commit once, and a \nblob once, and git is very unhappy.\n\nAnd the only case I know of that does that is using an old git binary, or \na unconverted git code-path program, on a repository with subprojects when \nthe code-path doesn't understand that a tree can contain pointers to \ncommits.\n\nSo what happens is that something traverses a tree object, looks at each \nentry, sees that it's not a tree, and tries to look it up as a blob. But \nsubprojects are commits, not blobs, and then when you look at the object \nmore closely, you get the above kind of object type confusion.\n\nAnd I did that very early on, and wanted to make sure that git objects \nwere \"strongly typed\", and you get a big fat error if you try to use the \nwrong type.\n\n> $ git-cat-file -t 228f8065b930120e35fc0c154c237487ab02d64a\n> commit\n> \n> git-show of the object shows it looks okay. git-fsck just shows a load\n> of dangling objects - which isn't a surprise, that's why I was running\n> git-prune.\n\nYeah, git-fsck knows about subprojects. I bet git-prune just doesn't. \n\nAnd indeed.. Here's a patch for it, but the fact is, you really should \n*not* prune that repository, because you'll prune away all the subproject \ndata, which you seem to have in the same repo!\n\n(General rule: never *ever* prune a shared object repository!)\n\n\t\tLinus\n\n---\n reachable.c | 11 +++++++++++\n 1 files changed, 11 insertions(+), 0 deletions(-)\n\ndiff --git a/reachable.c b/reachable.c\nindex ff3dd34..17ff929 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -21,6 +21,15 @@ 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+\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 +56,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"},{"id":"45989","messageId":"alpine.LFD.0.98.0706280901580.8675@woody.linux-foundation.org","threadId":"8756","inReplyTo":"alpine.LFD.0.98.0706280844460.8675@woody.linux-foundation.org","subject":"Re: Bug: segfault during git-prune","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-28T16:09:56Z","receivedAt":"2007-06-28T16:09:56Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 28 Jun 2007, Linus Torvalds wrote:\n> \n> Yeah, git-fsck knows about subprojects. I bet git-prune just doesn't. \n> \n> And indeed.. Here's a patch for it, but the fact is, you really should \n> *not* prune that repository, because you'll prune away all the subproject \n> data, which you seem to have in the same repo!\n> \n> (General rule: never *ever* prune a shared object repository!)\n\nBtw, we could make the \"reachable\" code say that *if* you have the commit \nof a subproject in your current object directory, it will follow that \ncommit recursively, and thus sharing an object store at the superproject \nlevel is \"ok\" as far as \"git prune\" is concerned.\n\nWe don't currently even have that kind of function, though. Calling \n\"parse_commit()\" will complain very loudly if the commit doesn't exist, \nbut it's actually perfectly fine to not exist if it's in a separate \nsubproject.\n\nSo if you actually want to use subprojects *and* prune them, *and* you \nwant to keep all the subproject objects in the same big object database, \nyou'd have to add some commit object \"parse-if-you-can-find-it\" thing, and \nuse that.\n\nIt shouldn't be fundamentally hard, but it's _not_ what my patch did. My \npatch just says: \"subprojects are separate projects, and we don't do \nreachability from superproject to subproject\".\n\n\t\tLinus\n"},{"id":"46007","messageId":"200706282321.44244.andyparkins@gmail.com","threadId":"8756","inReplyTo":"alpine.LFD.0.98.0706280844460.8675@woody.linux-foundation.org","subject":"Re: Bug: segfault during git-prune","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-06-28T22:21:42Z","receivedAt":"2007-06-28T22:21:42Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Thursday 2007, June 28, Linus Torvalds wrote:\n> On Thu, 28 Jun 2007, Andy Parkins wrote:\n> > I ran git-prune on a repository and got this:\n> >\n> > $ git-prune\n> > error: Object 228f8065b930120e35fc0c154c237487ab02d64a is a blob, not a\n> > commit Segmentation fault (core dumped)\n>\n> Do you have subprojects in that git repo?\n\nYes. I'm also doing something that is possibly very naughty; and I'm sure \nyou're going to say \"what on earth do you expect when you've done _that_\"\n\nThe subproject is the same repository... It's a git conversion of the \nffmpeg history; ffmpeg uses svn:externals for the libswscale directory; so \nI set that as an independent branch in the same repository, fetched with \ngit-svn as well. Then I cloned the repository into a subdirectory of \nitself.\n\n $ git clone -n . libswscale\n\nThen I went into libswscale/.git/ and ln -s objects, refs, info, and logs. \nIn fact the only thing that isn't shared is HEAD.\n\nThen I changed into libswscale and checked out the libswscale branch. Back \nin the ffmpeg repository I git-add the libswscale directory and everything \nseems to be working wonderfully.\n\n> And the only case I know of that does that is using an old git binary, or\n> a unconverted git code-path program, on a repository with subprojects\n> when the code-path doesn't understand that a tree can contain pointers to\n> commits.\n\nSounds like the last one to me. I tend to be only a few days behind \nupstream git.\n\n> Yeah, git-fsck knows about subprojects. I bet git-prune just doesn't.\n>\n> And indeed.. Here's a patch for it, but the fact is, you really should\n> *not* prune that repository, because you'll prune away all the subproject\n> data, which you seem to have in the same repo!\n\nCorrect; you did well figuring that out from the meagre information I gave.\n\nI had hoped that git-prune wouldn't be a risk because I have:\n\n * -- * -- * -- * -- * (ffmpeg-svn)\n\n * -- * -- * -- * (libswscale-svn)\n\nThen I forked master from ffmpeg-svn and added the libswscale-svn branch as \na submodule as described above. Now, because the submodule always refers \nto commits that are ancestors of libswscale-svn, they'll never be seen as \ndangling and pruned?\n\n> (General rule: never *ever* prune a shared object repository!)\n\nEven when I'm sure that every object of interest is behind a head ref?\n\n> ---\n> reachable.c | 11 +++++++++++\n> 1 files changed, 11 insertions(+), 0 deletions(-)\n\nThe repository in question is on my work computer, so I won't be able to try \nthis patch until Monday. I'll report back then.\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"46008","messageId":"alpine.LFD.0.98.0706281525460.8675@woody.linux-foundation.org","threadId":"8756","inReplyTo":"200706282321.44244.andyparkins@gmail.com","subject":"Re: Bug: segfault during git-prune","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-06-28T22:31:27Z","receivedAt":"2007-06-28T22:31:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 28 Jun 2007, Andy Parkins wrote:\n> \n> I had hoped that git-prune wouldn't be a risk because I have:\n> \n> * -- * -- * -- * -- * (ffmpeg-svn)\n> \n> * -- * -- * -- * (libswscale-svn)\n\nOk. If all subproject branches are also visible in the superproject as \nrefs, then \"git prune\" should work fine, and you can apply my patch and it \nshould just work very naturally: the reachability analysis will find the \nsubprojects not through the superproject link, but simply through the \ndirect refs to the subproject.\n\n> > (General rule: never *ever* prune a shared object repository!)\n> \n> Even when I'm sure that every object of interest is behind a head ref?\n\nSo yes, in that case, you're ok.\n\nThis is not unlike just having two different repositories sharing the same \nobject directory: as long as the two different repositories both have the \nappropriate refs, pruning is fine. In other words, you can see them as \njust independent branches in the same repo.\n\nAnd in fact, subprojects are obviously very much *designed* to work that \nway: a subproject is basically a \"different repository\". So the basic rule \nis that if it would work with totally independent repos, it works with \nsubprojects.\n\n[ That's all aside from the kind of bug that you found, where some code \n that parses the tree structures hadn't been updated for subprojects, of \n course ]\n\nAnyway, if that patch works for you, I'd suggest you just pass it on to \nJunio (and feel free to add my \"Signed-off-by:\" on it - but conditional on \nyou having actually tested it).\n\n\t\tLinus\n"},{"id":"46045","messageId":"200706291340.00429.andyparkins@gmail.com","threadId":"8756","inReplyTo":"alpine.LFD.0.98.0706281525460.8675@woody.linux-foundation.org","subject":"Re: Bug: segfault during git-prune","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-06-29T12:39:58Z","receivedAt":"2007-06-29T12:39:58Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Thursday 2007, June 28, Linus Torvalds wrote:\n> On Thu, 28 Jun 2007, Andy Parkins wrote:\n> > I had hoped that git-prune wouldn't be a risk because I have:\n> >\n> > * -- * -- * -- * -- * (ffmpeg-svn)\n> >\n> > * -- * -- * -- * (libswscale-svn)\n>\n> Ok. If all subproject branches are also visible in the superproject as\n> refs, then \"git prune\" should work fine, and you can apply my patch and\n> it should just work very naturally: the reachability analysis will find\n> the subprojects not through the superproject link, but simply through the\n> direct refs to the subproject.\n\nExcellent. That's what I'd hoped.\n\nI'm actually really impressed that git is robust enough that my symbolic \nlinking of all the .git subdirectories works so well. It's actually turned \nout to be a really natural way of using a repository for strongly connected \nsubmodules.\n\n> This is not unlike just having two different repositories sharing the\n> same object directory: as long as the two different repositories both\n> have the appropriate refs, pruning is fine. In other words, you can see\n> them as just independent branches in the same repo.\n\nAbsolutely. With my poor-mans submodule script that I was using before git \nhad it's own submodule support, I had the script make a refs/superrefs \ndirectory, and every time I committed to the supermodule the script would \nwrite $subhash to $submodule/.git/refs/heads/superrefs/$subhash. The it \nwas safe to git-prune the submodules as well.\n\n> And in fact, subprojects are obviously very much *designed* to work that\n> way: a subproject is basically a \"different repository\". So the basic\n> rule is that if it would work with totally independent repos, it works\n> with subprojects.\n\nI certainly agree, it's an extremely elegant way of working. I can't \nimagine that any other VCS is capable of this sort of manipulation.\n\n> Anyway, if that patch works for you, I'd suggest you just pass it on to\n> Junio (and feel free to add my \"Signed-off-by:\" on it - but conditional\n> on you having actually tested it).\n\nWill do. I'll certainly test it, and am happy to forward it on if that test \nis successful.\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"46220","messageId":"200707021100.16610.andyparkins@gmail.com","threadId":"8756","inReplyTo":"alpine.LFD.0.98.0706281525460.8675@woody.linux-foundation.org","subject":"Re: Bug: segfault during git-prune","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-07-02T10:00:15Z","receivedAt":"2007-07-02T10:00:15Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Thursday 2007 June 28, Linus Torvalds wrote:\n\n> Anyway, if that patch works for you, I'd suggest you just pass it on to\n> Junio (and feel free to add my \"Signed-off-by:\" on it - but conditional on\n> you having actually tested it).\n\nOkay; tested with this patch, but no change in behaviour.\n\n$ git-prune\nerror: Object 228f8065b930120e35fc0c154c237487ab02d64a is a blob, not a commit\nSegmentation fault (core dumped)\n\nLooking at your patch: is it possible that S_ISDIR() is true for gitlinks as \nwell as S_ISGITLINK()? S_ISDIR() is from unistd.h; and is presumably \nsomething like:\n \n S_ISDIR() { return mode & S_IFDIR; }\n\nGiven the GITLINK mode is S_IFLINK | S_IFDIR; then S_ISDIR() will be true and\n\n if (S_ISDIR(entry.mode))\n process_tree(lookup_tree(entry.sha1), p, &me, entry.path);\n+ else if (S_ISGITLINK(entry.mode))\n+ process_gitlink(entry.sha1, p, &me, entry.path);\n else\n process_blob(lookup_blob(entry.sha1), p, &me, entry.path);\n\nwill never get to the process_gitlink() call.\n\nHowever; I tried fixing this by swapping the order of the tests and the \nproblem hasn't gone away. I'm not sure that it's even getting as far as \nprocess_tree(). (incidentally I think the same fault exists in \nlist-objects.c's process_tree).\n\nGiven the hints you gave me in your previous reply, I've looked at the \nbacktrace again and understood more what's happening.\n\n - mark_reachable_objects() calls add_cache_refs()\n - which uses lookup_blob() to mark every hash in the index as an OBJ_BLOB \n type of hash; including the GITLINK entries.\n - mark_reachable_objects() calls add_one_ref() for_each_ref(), which finds\n a ref pointing to one of the GITLINK entries, and via \n parse_object_buffer(), tries to lookup_commit(), which finds the GITLINKed\n object using lookup_object() only it is not an OBJ_COMMIT, it's an OBJ_BLOB\n - all hell breaks loose\n\nI think the fault is in add_cache_refs() which assumes that every hash in the \nindex is an OBJ_BLOB. I think that add_cache_refs() shouldn't be calling \nlookup_blob() for S_ISGITLINK() index entries. Therefore I think this patch \nis the right one; what do you reckon?\n\ndiff --git a/reachable.c b/reachable.c\nindex ff3dd34..ffc8d0a 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -21,6 +21,15 @@ static void process_blob(struct blob *blob,\n /* Nothing to do, really .. The blob lookup was the important part */\n }\n \n+static void process_gitlink(const unsigned char *sha1,\n+ struct object_array *p,\n+ struct name_path *path,\n+ const char *name)\n+{\n+ /* I don't think we want to recurse into this, really. */\n+}\n+\n+\n static void process_tree(struct tree *tree,\n struct object_array *p,\n struct name_path *path,\n@@ -45,7 +54,9 @@ static void process_tree(struct tree *tree,\n init_tree_desc(&desc, tree->buffer, tree->size);\n \n while (tree_entry(&desc, &entry)) {\n- if (S_ISDIR(entry.mode))\n+ if (S_ISGITLINK(entry.mode))\n+ process_gitlink(entry.sha1, p, &me, entry.path);\n+ else if (S_ISDIR(entry.mode))\n process_tree(lookup_tree(entry.sha1), p, &me, entry.path);\n else\n process_blob(lookup_blob(entry.sha1), p, &me, entry.path);\n@@ -159,6 +170,16 @@ static void add_cache_refs(struct rev_info *revs)\n \n read_cache();\n for (i = 0; i < active_nr; i++) {\n+ /*\n+ * The index can contain blobs and GITLINKs, GITLINKs are hashes\n+ * that don't actually point to objects in the repository, it's\n+ * almost guaranteed that they are NOT blobs, so we don't call\n+ * lookup_blob() on them, to avoid populating the hash table\n+ * with invalid information\n+ */\n+ if (S_ISGITLINK(ntohl(active_cache[i]->ce_mode)))\n+ continue;\n+\n lookup_blob(active_cache[i]->sha1);\n /*\n * We could add the blobs to the pending list, but quite\n\nIf you think I'm on the right lines with this, I'll make better patches for \nJunio.\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"46230","messageId":"alpine.LFD.0.98.0707020441460.3323@woody.linux-foundation.org","threadId":"8756","inReplyTo":"200707021100.16610.andyparkins@gmail.com","subject":"Re: Bug: segfault during git-prune","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-02T11:45:27Z","receivedAt":"2007-07-02T11:45:27Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 2 Jul 2007, Andy Parkins wrote:\n> \n> Looking at your patch: is it possible that S_ISDIR() is true for gitlinks as \n> well as S_ISGITLINK()? S_ISDIR() is from unistd.h; and is presumably \n> something like:\n> \n> S_ISDIR() { return mode & S_IFDIR; }\n\nNo, it's going to be something like \"(mode & S_IFMASK) == S_IFDIR\", so \nthat part should be ok.\n\nBUT:\n\n> Given the hints you gave me in your previous reply, I've looked at the \n> backtrace again and understood more what's happening.\n> \n> - mark_reachable_objects() calls add_cache_refs()\n> - which uses lookup_blob() to mark every hash in the index as an OBJ_BLOB \n> type of hash; including the GITLINK entries.\n\nYes, this sounds like the real issue (or the remaining issue, at least).\n\n> I think the fault is in add_cache_refs() which assumes that every hash in the \n> index is an OBJ_BLOB. I think that add_cache_refs() shouldn't be calling \n> lookup_blob() for S_ISGITLINK() index entries. Therefore I think this patch \n> is the right one; what do you reckon?\n\nLooks good to me.\n\n> + /*\n> + * The index can contain blobs and GITLINKs, GITLINKs are hashes\n> + * that don't actually point to objects in the repository, it's\n> + * almost guaranteed that they are NOT blobs, so we don't call\n> + * lookup_blob() on them, to avoid populating the hash table\n> + * with invalid information\n> + */\n> + if (S_ISGITLINK(ntohl(active_cache[i]->ce_mode)))\n> + continue;\n\nJust for clarity, I'd use \"process_gitlink()\" here (and it would probably \nbe a good idea to use process_blob() on the blob we look up too), and the \nproper tab indentation, but yes, I think you're otherwise exactly right.\n\n\t\tLinus\n"},{"id":"46234","messageId":"200707021425.38367.andyparkins@gmail.com","threadId":"8756","inReplyTo":"alpine.LFD.0.98.0707020441460.3323@woody.linux-foundation.org","subject":"Re: Bug: segfault during git-prune","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-07-02T13:25:37Z","receivedAt":"2007-07-02T13:25:37Z","isPatch":false,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Monday 2007 July 02, Linus Torvalds wrote:\n\n> Just for clarity, I'd use \"process_gitlink()\" here (and it would probably\n> be a good idea to use process_blob() on the blob we look up too), and the\n> proper tab indentation, but yes, I think you're otherwise exactly right.\n\nI've pushed it out as-is as a bug fix patch now; and will happilly make the \nabove change on top. However, I've not understood what all the parameters \nare for in the process_XXXX() functions:\n\nstatic 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\nthe sha1 and name I can read out of the cache_entry structure, but I'm not \nsure about the object_array and name_path structures in the context of \nadd_cache_refs().\n\n\n\nAndy\n-- \nDr Andy Parkins, M Eng (hons), MIET\nandyparkins@gmail.com\n"},{"id":"46276","messageId":"alpine.LFD.0.98.0707021356570.9434@woody.linux-foundation.org","threadId":"8756","inReplyTo":"200707021425.38367.andyparkins@gmail.com","subject":"Re: Bug: segfault during git-prune","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-07-02T21:01:19Z","receivedAt":"2007-07-02T21:01:19Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 2 Jul 2007, Andy Parkins wrote:\n> \n> I've pushed it out as-is as a bug fix patch now; and will happilly make the \n> above change on top. However, I've not understood what all the parameters \n> are for in the process_XXXX() functions:\n\nYeah, never mind, your patch is fine. The process_xxx() parameters are \nreally only there so that process_tree() can generate the proper path and \nnames, and they are passed to process_blob/gitlink() just to make all the \nprocessing look the same.\n\nBut it's nonsensical to do for the case of the index (although you could \npass a NULL path and the name from the index), and we haven't created the \nobject array there, so just ignore my suggestion - your original patch is \nfine.\n\n\t\tLinus\n"}]}