On Thursday 2007 June 28, Linus Torvalds wrote:
> Anyway, if that patch works for you, I'd suggest you just pass it on to
> Junio (and feel free to add my "Signed-off-by:" on it - but conditional on
> you having actually tested it).
Okay; tested with this patch, but no change in behaviour.
$ git-prune
error: Object 228f8065b930120e35fc0c154c237487ab02d64a is a blob, not a commit
Segmentation fault (core dumped)
Looking at your patch: is it possible that S_ISDIR() is true for gitlinks as
well as S_ISGITLINK()? S_ISDIR() is from unistd.h; and is presumably
something like:
S_ISDIR() { return mode & S_IFDIR; }
Given the GITLINK mode is S_IFLINK | S_IFDIR; then S_ISDIR() will be true and
if (S_ISDIR(entry.mode))
process_tree(lookup_tree(entry.sha1), p, &me, entry.path);
+ else if (S_ISGITLINK(entry.mode))
+ process_gitlink(entry.sha1, p, &me, entry.path);
else
process_blob(lookup_blob(entry.sha1), p, &me, entry.path);
will never get to the process_gitlink() call.
However; I tried fixing this by swapping the order of the tests and the
problem hasn't gone away. I'm not sure that it's even getting as far as
process_tree(). (incidentally I think the same fault exists in
list-objects.c's process_tree).
Given the hints you gave me in your previous reply, I've looked at the
backtrace again and understood more what's happening.
- mark_reachable_objects() calls add_cache_refs()
- which uses lookup_blob() to mark every hash in the index as an OBJ_BLOB
type of hash; including the GITLINK entries.
- mark_reachable_objects() calls add_one_ref() for_each_ref(), which finds
a ref pointing to one of the GITLINK entries, and via
parse_object_buffer(), tries to lookup_commit(), which finds the GITLINKed
object using lookup_object() only it is not an OBJ_COMMIT, it's an OBJ_BLOB
- all hell breaks loose
I think the fault is in add_cache_refs() which assumes that every hash in the
index is an OBJ_BLOB. I think that add_cache_refs() shouldn't be calling
lookup_blob() for S_ISGITLINK() index entries. Therefore I think this patch
is the right one; what do you reckon?
diff --git a/reachable.c b/reachable.c
index ff3dd34..ffc8d0a 100644
--- a/reachable.c
+++ b/reachable.c
@@ -21,6 +21,15 @@ static void process_blob(struct blob *blob,
/* Nothing to do, really .. The blob lookup was the important part */
}
+static void process_gitlink(const unsigned char *sha1,
+ struct object_array *p,
+ struct name_path *path,
+ const char *name)
+{
+ /* I don't think we want to recurse into this, really. */
+}
+
+
static void process_tree(struct tree *tree,
struct object_array *p,
struct name_path *path,
@@ -45,7 +54,9 @@ static void process_tree(struct tree *tree,
init_tree_desc(&desc, tree->buffer, tree->size);
while (tree_entry(&desc, &entry)) {
- if (S_ISDIR(entry.mode))
+ if (S_ISGITLINK(entry.mode))
+ process_gitlink(entry.sha1, p, &me, entry.path);
+ else if (S_ISDIR(entry.mode))
process_tree(lookup_tree(entry.sha1), p, &me, entry.path);
else
process_blob(lookup_blob(entry.sha1), p, &me, entry.path);
@@ -159,6 +170,16 @@ static void add_cache_refs(struct rev_info *revs)
read_cache();
for (i = 0; i < active_nr; i++) {
+ /*
+ * The index can contain blobs and GITLINKs, GITLINKs are hashes
+ * that don't actually point to objects in the repository, it's
+ * almost guaranteed that they are NOT blobs, so we don't call
+ * lookup_blob() on them, to avoid populating the hash table
+ * with invalid information
+ */
+ if (S_ISGITLINK(ntohl(active_cache[i]->ce_mode)))
+ continue;
+
lookup_blob(active_cache[i]->sha1);
/*
* We could add the blobs to the pending list, but quite
If you think I'm on the right lines with this, I'll make better patches for
Junio.
Andy
--
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@xxxxxxxxx
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
