From: Dana How <danahow@gmail.com>
Date: Tue, 01 May 2007 05:41:33 GMT
Subject: Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()
Message-ID: <56b7f5510704302241n79601619kda8251a9f7776884@mail.gmail.com>
In-Reply-To: <20070501050633.GZ5942@spearce.org>

On 4/30/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> Dana How <danahow@gmail.com> wrote:
> > Add our own version of the one in fast-import.c here.
> > Needed later to correct bad object count in header for split pack.
> ...
> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> > --- a/builtin-pack-objects.c
> > +++ b/builtin-pack-objects.c
> > @@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f,
> > +static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,
> > +                             char *pack_name, uint32_t object_count)
> > +{
>
> This looks a *lot* like the code in fast-import.c.
I hope so!  That's where I copied it from.

> Why not
> refactor both to use the same implementation and stuff it away in
> say pack-check.c (for lack of a better place), or start a new file
> (pack-write.c)?
Actually I didn't just copy it, I tried to rewrite it for my use
as well as the fast-import.c use (note there is a 3rd copy
in some *index*.c file which I didn't try to merge in yet).
However I didn't yet put it in a new file or change fast-import.c
to call it since I wanted to change as little as possible.

> There is a *lot* of code in fast-import.c (over 2,000 lines) that
> was half-copied from other core code, and that was half created
> on its own.  This is also true in index-pack.c and pack-objects.c;
> I'd like to see these implementations unify more rather than copy
> code from each other.
>
> I know git-blame will identify the original author quite well,
> but I'd really like to avoid adding lots more code to maintain
> if we can avoid it.

I agree with all your arguments.  I had several reasons
to avoid extra rearrangements/refactorings:
(a) First patch to git, not previously known to me;
(b) I prefer to separate new functionality from "clean-up" work;
(c) I didn't really view myself as the person to make the *writing*
    code in git as well organized/minimized as the *reading* code
    [e.g. the sliding mmap stuff -- nice!];
(d) Apparently you and Nicolas Pitre have a lot of pending changes
    affecting the packing code.

I'd have no problem submitting a follow-on patch later containing
some clean-up work if you & NP clear it, so I know I won't have
problems from (d).  Note I had to completely rewrite this patch
when NP submitted some of his pending stuff.

NP wrote that you posted a summary of v3/v4 pack ideas,
but I couldn't find it in the list archives.  Could you either
email it to me, or (re-)post it to the list?

Thanks,
-- 
Dana L. How  danahow@gmail.com  +1 650 804 5991 cell