On Mon, 2 Jul 2007, Andy Parkins wrote:
>
> 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; }
No, it's going to be something like "(mode & S_IFMASK) == S_IFDIR", so
that part should be ok.
BUT:
> 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.
Yes, this sounds like the real issue (or the remaining issue, at least).
> 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?
Looks good to me.
> + /*
> + * 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;
Just for clarity, I'd use "process_gitlink()" here (and it would probably
be a good idea to use process_blob() on the blob we look up too), and the
proper tab indentation, but yes, I think you're otherwise exactly right.
Linus
-
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
