Re: [PATCH v2] index-pack: always zero-initialize object_entry list
On Tue, Mar 19, 2013 at 12:17 PM, Jeff King p...@peff.net wrote: To ensure that all depths start at 0, that commit changed calls to xmalloc the object_entry list into calls to xcalloc. However, it forgot that we grow the list with xrealloc later. These extra entries are used when we add an object from elsewhere pack to complete a thin pack. If we s/elsewhere pack/pack/ add a non-delta object, its depth value will just be uninitialized heap data. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] index-pack: always zero-initialize object_entry list
On Wed, Mar 20, 2013 at 03:12:07PM -0400, Eric Sunshine wrote: On Tue, Mar 19, 2013 at 12:17 PM, Jeff King p...@peff.net wrote: To ensure that all depths start at 0, that commit changed calls to xmalloc the object_entry list into calls to xcalloc. However, it forgot that we grow the list with xrealloc later. These extra entries are used when we add an object from elsewhere pack to complete a thin pack. If we s/elsewhere pack/pack/ I think it is supposed to be s/elsewhere pack/elsewhere/. Thanks for noticing. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] index-pack: always zero-initialize object_entry list
On Wed, Mar 20, 2013 at 3:13 PM, Jeff King p...@peff.net wrote: On Wed, Mar 20, 2013 at 03:12:07PM -0400, Eric Sunshine wrote: On Tue, Mar 19, 2013 at 12:17 PM, Jeff King p...@peff.net wrote: To ensure that all depths start at 0, that commit changed calls to xmalloc the object_entry list into calls to xcalloc. However, it forgot that we grow the list with xrealloc later. These extra entries are used when we add an object from elsewhere pack to complete a thin pack. If we s/elsewhere pack/pack/ I think it is supposed to be s/elsewhere pack/elsewhere/. Sorry, yes. -- ES -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] index-pack: always zero-initialize object_entry list
Jeff King p...@peff.net writes:
On Tue, Mar 19, 2013 at 11:52:44AM -0400, Jeff King wrote:
Commit 38a4556 (index-pack: start learning to emulate
verify-pack -v, 2011-06-03) added a delta_depth counter
to each struct object_entry. Initially, all object entries
have their depth set to 0; in resolve_delta, we then set the
depth of each delta to base + 1. Base entries never have
their depth touched, and remain at 0.
This patch causes index-pack to fail on the pack that triggered the
whole discussion. More in a minute in another side thread, but
meanwhile: NAK until we understand what is really going on here.
Odd; that's what I was testing with, and it worked fine.
Ah, interesting. I built the fix on top of d1a0ed1, the first commit
that shows the problem. And it works fine there. But when it is
forward-ported to the current master, it breaks as you saw.
More bisection fun.
So after bisecting, I realize that it is indeed broken on top of
d1a0ed1. I have no idea why I didn't notice that before; I'm guessing it
was because I was running it under valgrind and paying attention only to
valgrind errors.
Anyway, the problem is simple and stupid. The original object array is
not nr_objects item long; it is (nr_objects + 1) long, though I'm not
clear why (1-indexing?).
It apparently relates to the use of .idx.offset to compute the next
offset, cf. append_obj_to_pack():
struct object_entry *obj = objects[nr_objects++];
...
obj[1].idx.offset = obj[0].idx.offset + n;
obj[1].idx.offset += write_compressed(f, buf, size);
So you trashed the offset of the first object after all the objects that
are actually *in* the patch.
And with that: ACK.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] index-pack: always zero-initialize object_entry list
Thomas Rast tr...@student.ethz.ch writes: It apparently relates to the use of .idx.offset to compute the next offset, cf. append_obj_to_pack(): struct object_entry *obj = objects[nr_objects++]; ... obj[1].idx.offset = obj[0].idx.offset + n; obj[1].idx.offset += write_compressed(f, buf, size); So you trashed the offset of the first object after all the objects that are actually *in* the patch. And with that: ACK. Ahh, I also was scratching my head about that +1 thing. After all, the +1 in the argument to xrealloc() was already a clue. Thanks both for digging to the bottom of this one. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
