On Mon, Sep 21, 2015 at 11:15 AM, Jeff King <peff@xxxxxxxx> wrote:
> On Sun, Sep 20, 2015 at 06:48:32PM -0400, Eric Sunshine wrote:
>> > diff --git a/archive.c b/archive.c
>> > index 01b0899..4ac86c8 100644
>> > --- a/archive.c
>> > +++ b/archive.c
>> > @@ -171,13 +171,14 @@ static void queue_directory(const unsigned char *sha1,
>> > unsigned mode, int stage, struct archiver_context *c)
>> > {
>> > struct directory *d;
>> > - d = xmallocz(sizeof(*d) + base->len + 1 + strlen(filename));
>> > + size_t len = base->len + 1 + strlen(filename) + 1;
>> > + d = xmalloc(sizeof(*d) + len);
>>
>> Mental note: The new code makes this one longer than the original code...
>>
>> > d->up = c->bottom;
>> > d->baselen = base->len;
>> > d->mode = mode;
>> > d->stage = stage;
>> > c->bottom = d;
>> > - d->len = sprintf(d->path, "%.*s%s/", (int)base->len, base->buf, filename);
>> > + d->len = xsnprintf(d->path, len, "%.*s%s/", (int)base->len, base->buf, filename);
>>
>> Considering that we need space for the '/' and the NUL, the new code
>> seems to be correct, and the old code was under-allocating, right?
>
> Not quite. The original uses xmallocz, which handles the NUL itself. But
> the purpose of this patch is to pull the total length into a variable
> that we can use both for the malloc and for the xsnprintf, so we have
> to account for it ourselves.
Makes sense. I missed the "z" when reading the old code. Thanks for
the explanation.
>> > - p = xcalloc(1, sizeof(*p) + strlen(tmp_file) + 2);
>> > - strcpy(p->pack_name, tmp_file);
>> > + namelen = strlen(tmp_file) + 2;
>>
>> You mentioned this specially in the commit message, but from a brief
>> read of the code, it's still not obvious (to me) why this is +2 rather
>> than +1. Since you're touching the code anyhow, perhaps add an in-code
>> comment explaining it?
>
> To be honest, I'm not sure what's going on with the "+ 2" here.
>
> In many cases with packed_git we allocate with "foo.idx" and want to be
> able to later write "foo.pack" into the same buffer. But here we are
> putting in a tmpfile name. This comes from 8455e48, but I don't see any
> clue there. I wonder if the "+2" was simply cargo-culted from other
> instances.
Ah, ok. I guess I misunderstood the commit message to mean or imply
that the +2 was correct and sensible and well-understood.
> I'm loath to change it in the middle of this patch, because it would be
> hard to see amidst the other changes. I'd rather make this a straight
> conversion, and worry about it in a separate patch.
--
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