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

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

[Dana How]

Add our own version of the one in fast-import.c here.
Will need this later to correct an incorrect object
count in the pack header.

Signed-off-by: Dana How <how@deathvalley.cswitch.com>
---
 builtin-pack-objects.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 8415549..7ab0712 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -518,6 +518,46 @@ static off_t write_one(struct sha1file *f,
 	return offset + write_object(f, e);
 }
 
+/*
+ * 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)
+{
+	static const int buf_sz = 128 * 1024;
+	SHA_CTX c;
+	struct pack_header hdr;
+	char *buf;
+
+	if (lseek(pack_fd, 0, SEEK_SET) != 0)
+		die("Failed seeking to start: %s", strerror(errno));
+	if (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
+		die("Unable to reread header of %s", pack_name);
+	if (lseek(pack_fd, 0, SEEK_SET) != 0)
+		die("Failed seeking to start: %s", strerror(errno));
+	hdr.hdr_entries = htonl(object_count);
+	write_or_die(pack_fd, &hdr, sizeof(hdr));
+
+	SHA1_Init(&c);
+	SHA1_Update(&c, &hdr, sizeof(hdr));
+
+	buf = xmalloc(buf_sz);
+	for (;;) {
+		size_t n = xread(pack_fd, buf, buf_sz);
+		if (!n)
+			break;
+		if (n < 0)
+			die("Failed to checksum %s", pack_name);
+		SHA1_Update(&c, buf, n);
+	}
+	free(buf);
+
+	SHA1_Final(pack_file_sha1, &c);
+	write_or_die(pack_fd, pack_file_sha1, 20);
+	close(pack_fd);
+}
+
 typedef int (*entry_sort_t)(const struct object_entry *, const struct object_entry *);
 
 static entry_sort_t current_sort;
-- 
1.5.1.89.g8abf0

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

[Junio C Hamano]

Dana How <danahow@gmail.com> 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?

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

[Nicolas Pitre]

On Sun, 8 Apr 2007, Junio C Hamano wrote:

> Dana How <danahow@gmail.com> 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.


Nicolas

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

[Shawn O. Pearce]

Nicolas Pitre <nico@cam.org> wrote:
> On Sun, 8 Apr 2007, Junio C Hamano wrote:
> 
> > Dana How <danahow@gmail.com> 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.

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

[Nicolas Pitre]

On Mon, 9 Apr 2007, Shawn O. Pearce wrote:

> Nicolas Pitre <nico@cam.org> wrote:
> > 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.

That's easy enough for the nr_objects correction case.

The next obvious candidate would be index generation, but I'd wait a bit 
for all features in progress to be merged and stabilize first.

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

I'd be really tempted to create a pack v4 which only change is to still 
have the pack header at the beginning of the pack like we do today, but 
include the header in the pack SHA1 computation at the end of the stream 
only.  This way the pack SHA1 could be computed as the pack is 
generated, and the header fixed up without having to read the entire 
pack back.  I think it was Geert Bosch who proposed this and it makes 
tons of sense IMHO.


Nicolas

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

[Shawn O. Pearce]

Nicolas Pitre <nico@cam.org> wrote:
> I'd be really tempted to create a pack v4 which only change is to still 
> have the pack header at the beginning of the pack like we do today, but 
> include the header in the pack SHA1 computation at the end of the stream 
> only.  This way the pack SHA1 could be computed as the pack is 
> generated, and the header fixed up without having to read the entire 
> pack back.  I think it was Geert Bosch who proposed this and it makes 
> tons of sense IMHO.

Yes.  If we really are heading in this direction of needing to
correct object counts, we should make that change.  Its trivial
to hang onto that header for the duration of the rest of the data
processing, and tack it onto the end for final SHA-1 computation.

Since pack v4 looks like it will be a dev cycle longer than these
index format changes and pack-splitting changes, I have to say I
agree with you.  Lets move "pack v4" back to "pack v5" and make v4
just a shift of where the header is included in the SHA-1.

We're heading where I said I didn't want to go, which is two
file format changes in 2007.  But I think that ship has already
sailed...  Folks need support for larger repositories now, and pack
v4/v5 (whatever you call our dictionary work) just isn't ready.
Nor does it solve the big packfile problems that this current work
is addressing.

-- 
Shawn.

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

[Dana How]

On 4/9/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> Nicolas Pitre <nico@cam.org> wrote:
> > I'd be really tempted to create a pack v4 which only change is to still
> > have the pack header at the beginning of the pack like we do today, but
> > include the header in the pack SHA1 computation at the end of the stream
> > only.  This way the pack SHA1 could be computed as the pack is
> > generated, and the header fixed up without having to read the entire
> > pack back.  I think it was Geert Bosch who proposed this and it makes
> > tons of sense IMHO.
>
> Yes.  If we really are heading in this direction of needing to
> correct object counts, we should make that change.  Its trivial
> to hang onto that header for the duration of the rest of the data
> processing, and tack it onto the end for final SHA-1 computation.

I like the property that when an SHA-1 appears at the end of a file,
it is a checksum of every byte before it.  The ideas above are a
departure from that.  Do we want this rule to be different for each file type?

Wouldn't the following address the "object count unknown
at the start of sequential pack writing" problem:
  Write 0 for object count in the header. This is a flag to look for
  another header of same format just before the final SHA-1 which
  has the correct count. The SHA-1 is still a checksum of everything
  before it and no seeking/rewriting is needed on generation.  When
  reading the object count from a .pack file, you might need to add
      xread(pack_fd, &header, sizeof(header));
+    if (!header.object_count) {
+      lseek(pack_fd, -20-sizeof(header), SEEK_END);
+      xread(pack_fd, &header, sizeof(header);
+    }
  Or maybe you want this before the object_list_sha1 instead (20->40).

Finally, when I generate several 2GB split packfiles,  I do notice
the slight delay for fixup_header_footer(), and I do think it's a bit
ugly, but in quantitative terms it's an insignificant part of a long
operation that's infrequently performed.  Does this need to be
optimized at all?

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

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

[Nicolas Pitre]

On Mon, 9 Apr 2007, Dana How wrote:

> Wouldn't the following address the "object count unknown
> at the start of sequential pack writing" problem:
>  Write 0 for object count in the header. This is a flag to look for
>  another header of same format just before the final SHA-1 which
>  has the correct count. The SHA-1 is still a checksum of everything
>  before it and no seeking/rewriting is needed on generation.

No.  You really wants to know up front how many objects a pack contains 
when streaming it.  And this is not only for packs written to stdout.

> Finally, when I generate several 2GB split packfiles,  I do notice
> the slight delay for fixup_header_footer(), and I do think it's a bit
> ugly, but in quantitative terms it's an insignificant part of a long
> operation that's infrequently performed.  Does this need to be
> optimized at all?

Maybe, maybe not.  That depends how much data we think GIT could be used 
to manage in the future.  With a 1TB pack file you definitely want to 
optimize that case.

OTOH this could wait for the real pack v4 too.


Nicolas

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

[Dana How]

On 4/9/07, Nicolas Pitre <nico@cam.org> wrote:
> On Mon, 9 Apr 2007, Dana How wrote:
> > Wouldn't the following address the "object count unknown
> > at the start of sequential pack writing" problem:
> >  Write 0 for object count in the header. This is a flag to look for
> >  another header of same format just before the final SHA-1 which
> >  has the correct count. The SHA-1 is still a checksum of everything
> >  before it and no seeking/rewriting is needed on generation.
>
> No.  You really wants to know up front how many objects a pack contains
> when streaming it.  And this is not only for packs written to stdout.
OK, let me ask a dumb question and flog one last additional obvious idea.

Does your wanting to know stem from more than wanting
to stick to one malloc of all the object info at once?

My suggestion quoted above is actually a change to the .pack format.
With all the other ideas for .pack format changes floating around,
let me withdraw that and suggest a simpler one: write a "0" in the header,
and terminate the pack with a sentinel in object format before the final SHA-1s.
The sentinel would be type=OBJ_NONE/length=0, i.e. a null byte.
"Not much" would need to be updated to tolerate it and
you could count objects while looking for it (if header has 0)
during normal processing.  (I'm reacting to your word "streaming".)

> > Finally, when I generate several 2GB split packfiles,  I do notice
> > the slight delay for fixup_header_footer(), and I do think it's a bit
> > ugly, but in quantitative terms it's an insignificant part of a long
> > operation that's infrequently performed.  Does this need to be
> > optimized at all?
> Maybe, maybe not.  That depends how much data we think GIT could be used
> to manage in the future.  With a 1TB pack file you definitely want to
> optimize that case.
OK.  Just FYI, we have a perforce repository near 200GB and
this is not what would concern me right now if we converted all
or part of it to git.  Of course that would depend on the packing schedule.

> OTOH this could wait for the real pack v4 too.
Makes sense to me.  The fewer format changes the better.

BTW,  I've caught up on reading the mailing list archives,
but I don't recall seeing any overview of the objectives of pack v2/v3/v4.
Does that exist any where? I didn't see it in Documentation or
Documentation/technical. It would probably reduce uninformed
questions like the above.  I've deduced rationales for what
miscellaneous details I have seen,
except moving the SHA-1s from .idx to .pack (?).

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

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

[Nicolas Pitre]

On Mon, 9 Apr 2007, Dana How wrote:

> On 4/9/07, Nicolas Pitre <nico@cam.org> wrote:
> > On Mon, 9 Apr 2007, Dana How wrote:
> > > Wouldn't the following address the "object count unknown
> > > at the start of sequential pack writing" problem:
> > >  Write 0 for object count in the header. This is a flag to look for
> > >  another header of same format just before the final SHA-1 which
> > >  has the correct count. The SHA-1 is still a checksum of everything
> > >  before it and no seeking/rewriting is needed on generation.
> > 
> > No.  You really wants to know up front how many objects a pack contains
> > when streaming it.  And this is not only for packs written to stdout.
> OK, let me ask a dumb question and flog one last additional obvious idea.
> 
> Does your wanting to know stem from more than wanting
> to stick to one malloc of all the object info at once?

That, plus progress reporting when fetching.  And progress reporting is 
probably the most important thing for the user experience.

> My suggestion quoted above is actually a change to the .pack format.
> With all the other ideas for .pack format changes floating around,
> let me withdraw that and suggest a simpler one: write a "0" in the header,
> and terminate the pack with a sentinel in object format before the final
> SHA-1s.
> The sentinel would be type=OBJ_NONE/length=0, i.e. a null byte.
> "Not much" would need to be updated to tolerate it and
> you could count objects while looking for it (if header has 0)
> during normal processing.  (I'm reacting to your word "streaming".)

When streaming you really want to know up front how much to expect / 
wait for.

> BTW,  I've caught up on reading the mailing list archives,
> but I don't recall seeing any overview of the objectives of pack v2/v3/v4.
> Does that exist any where? I didn't see it in Documentation or
> Documentation/technical. It would probably reduce uninformed
> questions like the above.  I've deduced rationales for what
> miscellaneous details I have seen,
> except moving the SHA-1s from .idx to .pack (?).

I'm sure this exists in the archive as I recall sending a summary about 
that a while ago.

In short:

 - Pack v1 was the initial implementation from Linus.  It had some flaws
   and didn't exist for more than 2 or 3 days.  It was never used in any 
   official release.

 - Pack v2 is what GIT still produces today.

 - Pack v3 was created when a bit in the delta encoding was redefined.
   See commit d60fc1c8649f8 for the details.  Because it caused too much
   compatibility issues when we attempted to enable pack v3 at the time, 
   we reverted pack generation to v2.

 - Pack v4 has a much larger scope.  This is the WIP from Shawn Pearce 
   and I and I know for sure Shawn already posted a detailed design 
   overview to the list already.


Nicolas

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

[Dana How]

Add our own version of the one in fast-import.c here.
Needed later to correct bad object count in header for split pack.

Signed-off-by: Dana L. How <danahow@gmail.com>
---
 builtin-pack-objects.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index bc45ca6..98066bf 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f,
 	return offset + size;
 }
 
+static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,
+				char *pack_name, uint32_t object_count)
+{
+	static const int buf_sz = 128 * 1024;
+	SHA_CTX c;
+	struct pack_header hdr;
+	char *buf;
+
+	if (lseek(pack_fd, 0, SEEK_SET) != 0)
+		die("Failed seeking to start: %s", strerror(errno));
+	if (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))
+		die("Unable to reread header of %s", pack_name);
+	if (lseek(pack_fd, 0, SEEK_SET) != 0)
+		die("Failed seeking to start: %s", strerror(errno));
+	hdr.hdr_entries = htonl(object_count);
+	write_or_die(pack_fd, &hdr, sizeof(hdr));
+
+	SHA1_Init(&c);
+	SHA1_Update(&c, &hdr, sizeof(hdr));
+
+	buf = xmalloc(buf_sz);
+	for (;;) {
+		size_t n = xread(pack_fd, buf, buf_sz);
+		if (!n)
+			break;
+		if (n < 0)
+			die("Failed to checksum %s", pack_name);
+		SHA1_Update(&c, buf, n);
+	}
+	free(buf);
+
+	SHA1_Final(pack_file_sha1, &c);
+	write_or_die(pack_fd, pack_file_sha1, 20);
+	close(pack_fd);
+}
+
 /* forward declarations for write_pack_file */
 static void write_index_file(off_t last_obj_offset, unsigned char *sha1);
 static int adjust_perm(const char *path, mode_t mode);
-- 
1.5.2.rc0.766.gba60-dirty

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

[Shawn O. Pearce]

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
> index bc45ca6..98066bf 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -562,6 +562,42 @@ static off_t write_one(struct sha1file *f,
>  	return offset + size;
>  }
>  
> +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.  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)?

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.

-- 
Shawn.

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

[Dana How]

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

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

[Shawn O. Pearce]

Dana How <danahow@gmail.com> wrote:
> 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.

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

[Johannes Schindelin]

Hi,

On Tue, 1 May 2007, Shawn O. Pearce wrote:

> Dana How <danahow@gmail.com> wrote:
>
> > 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.

FWIW it is not just a clean up in the sense of making the code more 
elegant. It is about catching bugs. The smaller the code base, and the 
more code is reused for common tasks, the easier it is to

- get at bugs,
- fix bugs, and
- prevent bugs

Some might even argue that the elegance of the code is a direct indicator 
of its quality.

Obviously, I am suggesting to go the slightly slower route. In effect, 
this will turn out to be the faster route, though, since bug fixing 
traditionally makes up for 90% of development time.

Ciao,
Dscho

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

[Nicolas Pitre]

On Tue, 1 May 2007, Shawn O. Pearce wrote:

> Dana How <danahow@gmail.com> wrote:
> > 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.

Well I intended to do more cleanups in the pack code eventually.  That 
included the index writing and pack header fixing.  But I was expecting 
for the pack splitting changes to go in first as it is likely to impose 
some requirements of its own. It is then easier to have a proper 
interface common to all users after everything is in place.

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

It shouldn't be too hard to port the existing code.  Most of it is new 
code that hooks into the existing code in a limited way.

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

While the current pack v4 branch is certainly valuable, I consider it 
more as a proof of concept and a test bench.  In practice it isn't 
really efficient and it won't be able to show its full potential until 
core code like tree walking is better abstracted seamless processing of 
parallel tree representations.

But we're getting there slowly.  My work on index v2 will make the pack 
v4 changes much smaller in that area.  My progress display rework was a 
direct "huh!" reaction to the extra progress reporting the current pack 
v4 code added.  And the general pack-objects.c refactoring was made with 
a look on better and easier pack v4 integration in the future.

So in my mind pack v4 already started to make its way in the main code 
in subtle ways.  ;-)  But since I may do Git work only when I'm bored 
progress doesn't happen as fast as one would have expected.

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

Well well.  OK I'm used to be considered as the bad guy anyway.  ;-)


Nicolas

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

[Dana How]

On 5/1/07, Nicolas Pitre <nico@cam.org> wrote:
> On Tue, 1 May 2007, Shawn O. Pearce wrote:
> > Dana How <danahow@gmail.com> wrote:
> > > 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.
>
> Well I intended to do more cleanups in the pack code eventually.  That
> included the index writing and pack header fixing.  But I was expecting
> for the pack splitting changes to go in first as it is likely to impose
> some requirements of its own. It is then easier to have a proper
> interface common to all users after everything is in place.
I was in the middle of creating pack-write.c at Shawn's suggestion.
It will only contain fixup_header_footer(), to be called by fast-import.c
and builtin-pack-object.c.  index-pack.c also has
readjust_pack_header_and_sha1(),
which is compatible except it doesn't close the file.  I was going to leave it
alone for now.  This new file should be the logical place to put other common
pack-writing-related things.  Please barf now if you don't think I
should do this
tiny refactoring at this point.

> > > 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.
>
> Well well.  OK I'm used to be considered as the bad guy anyway.  ;-)
You *did* tell me about your upcoming patches as I recall ;-)

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

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

[Nicolas Pitre]

On Tue, 1 May 2007, Dana How wrote:

> I was in the middle of creating pack-write.c at Shawn's suggestion. It 
> will only contain fixup_header_footer(), to be called by fast-import.c 
> and builtin-pack-object.c.  index-pack.c also has 
> readjust_pack_header_and_sha1(), which is compatible except it doesn't 
> close the file.  I was going to leave it alone for now.  This new file 
> should be the logical place to put other common pack-writing-related 
> things.  Please barf now if you don't think I should do this tiny 
> refactoring at this point.

That's fine.  Please go ahead.


Nicolas