On Mon, Sep 21, 2015 at 01:11:09PM -0400, Eric Sunshine wrote:
> >> > - 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 think it was more that I looked at other instances of packed_git, and
realized they could not be safely converted. I think "struct
alternate_object_database" has similar problems.
-Peff
PS As I mentioned earlier, I did end up adding a FLEX_ALLOC() macro in
another series that builds on top of this. I haven't posted it yet,
but check out:
https://github.com/peff/git/commit/ba491c527572c763286b4b9519aef3c30482c2d1
and
https://github.com/peff/git/commit/d88444d5ba00bd875ef5291dca3b71dd046186dc
if you are curious.
--
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
