Re: [PATCH 4/8] git-repack --max-pack-size: add fixup_header_footer()

From: Shawn O. Pearce <spearce@spearce.org>
Date: May 1, 2007, 06:03 UTC
Message-ID: <20070501060340.GD5942@spearce.org>
In-Reply-To: <56b7f5510704302241n79601619kda8251a9f7776884@mail.gmail.com>

Dana How <danahow@gmail.com> wrote:

Show 10 quoted lines

> On 4/30/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> >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.

...

> 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;

A really good reason.  ;-)

But I'd still rather see it done right the first time, then done
partially (copied) and wait for someone to clean it up later.
Sometimes that cleanup doesn't happen.  Anyway, I'm not against
you copying it, I just think there's a better way (refactor at
least fast-import's use of it).

> (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!];

Not sure I follow that argument, but thanks for the compliment on the sliding mmap work. I think I was trying to point out that that one particular function is actually quite simple, and you did a direct copy of it from fast-import.c. Instead a simple refactoring that allows both pack-objects.c and fast-import.c share the same implementation of those 30 or so lines of code would be a good start towards cleaning up the writing code. Yes it doesn't help the index-pack.c usage of same logic, but baby steps will help us to cleanup the mess we have already made!

We like baby steps around here...  ;-)

> (d) Apparently you and Nicolas Pitre have a lot of pending changes
>    affecting the packing code.

Yes, but Nico's work has also destroyed in pack v4 topic. Nico has promised to start working on porting some of that work over, but I don't know if he has been able to start doing so yet. I personally have been too busy this past month and a half to really work on packv4, but I'm hoping to circle back to it before the end of May (if Nico doesn't beat me to it!).

> 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.

Yea, hazard of working in this part of the code when Nico is
also active.  My own sliding mmap stuff was written twice too,
for the same reason - Nico doing much needed improvements right in
the same spot as I was working, at the same time.

> 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?

You can start here:

  http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=40799
  http://article.gmane.org/gmane.comp.version-control.git/42423
  http://article.gmane.org/gmane.comp.version-control.git/45406
  http://news.gmane.org/find-root.php?group=gmane.comp.version-control.git&article=43046

anything by me or Nico in those threads is good reading on pack v4. The last link is probably the best thread available on the subject.

-- 
Shawn.