Nicolas Pitre <nico@xxxxxxx> wrote:
> On Sun, 8 Apr 2007, Junio C Hamano wrote:
>
> > Dana How <danahow@xxxxxxxxx> writes:
> >
> > > +/*
> > > + * Move this, the version in fast-import.c,
> > > + * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ?
> > > + */
> > > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,
> > > + char *pack_name, uint32_t object_count)
> > > +{
> >
> > Indeed that is a very good point.
> >
> > I admit I did not notice we already had the duplication between
> > fast-import.c and index-pack.c
> >
> > Shawn, Nico, what do you think? Wouldn't it be better to
> > refactor them first, independent of Dana's series?
>
> Probably, yes. But probably not in sha1_file.c though. This file is
> getting a bit large already, and it deals with pack reading only not
> pack writing.
>
> I think another file with common pack writing functions could be
> created. Pack index writing is another item that is currently
> duplicated in pack-objects and index-pack for example.
I agree entirely. And I'd like to see that refactoring occur
before this series, or as part of it. At least for the nr_objects
correction routine. To be honest we should have done that when
fast-import.c and index-pack.c both needed that logic, but we didn't.
I don't remember whose version showed up first in Junio's tree
(I think it was index-pack.c) but the other one (probably me with
fast-import.c) should have done the refactoring then.
We already have *waaay* too many functions that know packfile
structure. I'd like to see that decline, but unfortunately a number
of them are using rather specialized data structures so it makes
things somewhat difficult.
For example, writing objects to a packfile: we have 3
implementations. fast-import.c doesn't use sha1write_compressed
because that was a waste of time to compute the SHA_CTX when we
know we have to go back and fixup nr_objects. It also doesn't use
it because fast-import.c's pack-splitting logic is based on the
final object size, not the starting offset. It does the deflate
itself, decides if the end of the object will overflow, and if so,
jumps to a new packfile.
--
Shawn.
-
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