Rebase max-pack-size?

Quoted 4 lines · Dana How

Since max-pack-size has appeared, and disappeard, in "pu" twice and never made it into "next", I'm guessing I should rebase/merge it on top of the other recent pack-objects work. I may get to this tonight; perhaps I'll send the patchset in one email with multiple attachments?

I am still holding onto the series on its own topic branch, but merging it to 'pu' and resolving conflicts seem pointless while I am (and hopefully the rest of the list ought to be ;-) primarily concentrating on last-minute bugfixes in preparation for v1.5.2.

A rebase to be queued in 'next' would be good, though.

From 781820f147bcb12dae576734585b27f42faca3ea Mon Sep 17 00:00:00 2001

From 0c75fd66aa6a758e245d85b9304e85a38e977f94 Mon Sep 17 00:00:00 2001

From 40ac2f294ec68cae27f8a9262db4cab84f1d2257 Mon Sep 17 00:00:00 2001

From 9ec3af1f52f41d0737b6271fded5e94527eb0466 Mon Sep 17 00:00:00 2001

From 279d1f9e0e7265c6ccf8759bbe8c5539bf3089fa Mon Sep 17 00:00:00 2001

Let me know if you want them re-sent inline and/or separately.

Quoted 1 line

From 781820f147bcb12dae576734585b27f42faca3ea Mon Sep 17 00:00:00 2001

From: Dana L. How <[email protected]>
Date: Sun, 13 May 2007 11:28:19 -0700
Subject: [PATCH] Alter sha1close() 3rd argument to request flush only

update=0 suppressed writing the final SHA-1 but was not used. Now final=0 suppresses SHA-1 finalization, SHA-1 writing, and closing -- in other words, only flush the buffer.

Signed-off-by Dana L. How <[email protected]>

Quoted 1 line

From 0c75fd66aa6a758e245d85b9304e85a38e977f94 Mon Sep 17 00:00:00 2001

From: Dana L. How <[email protected]>
Date: Sun, 13 May 2007 11:34:56 -0700
Subject: [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring

Add "pack_size_limit", the limit specified by --max-pack-size, "written_list", the list of objects written to the current pack, and "nr_written", the number of objects in written_list. Put "base_name" at file scope again and add forward declarations. Move write_index_file() call from cnd_pack_objects() to write_pack_file() since only the latter will know how many times to call write_index_file().

Signed-off-by Dana L. How <[email protected]>

Quoted 1 line

From 40ac2f294ec68cae27f8a9262db4cab84f1d2257 Mon Sep 17 00:00:00 2001

From: Dana L. How <[email protected]>
Date: Sun, 13 May 2007 12:06:18 -0700
Subject: [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit

With --max-pack-size, generate the appropriate write limit for each object and check against it before each group of writes. Update delta usability rules to handle base being in a previously- written pack. Inline sha1write_compress() so we know the exact size of the written data when it needs to be compressed. Detect and return write "failure".

Signed-off-by Dana L. How <[email protected]>

---
 builtin-pack-objects.c |  131 ++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 104 insertions(+), 27 deletions(-)

Quoted 1 line

From 9ec3af1f52f41d0737b6271fded5e94527eb0466 Mon Sep 17 00:00:00 2001

From: Dana L. How <[email protected]>
Date: Sun, 13 May 2007 12:09:16 -0700
Subject: [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()

Rewrite write_pack_file() to break to a new packfile whenever write_object/write_one request it, and correct the header's object count in the previous packfile. Change write_index_file() to write an index
for just the objects in the most recent packfile.

Signed-off-by Dana L. How <[email protected]>

---
 builtin-pack-objects.c |  120 ++++++++++++++++++++++++++++-------------------
 1 files changed, 71 insertions(+), 49 deletions(-)

builtin-pack-objects.c +8 -0

index 3023aac..ce9eb2d 100644
...

@@ -652,7 +663,26 @@ static void write_pack_file(void)
 				die("unable to rename temporary index file: %s",
 				    strerror(errno));
 			puts(sha1_to_hex(object_list_sha1));
+		}
+
+		/* mark written objects as written to previous pack */
+		for (j = 0; j < nr_written; j++) {
+			written_list[j]->offset = (off_t)-1;
+		}
+		nr_remaining -= nr_written;
+	} while (nr_remaining && i < nr_objects);

Quoted 1 line

From 279d1f9e0e7265c6ccf8759bbe8c5539bf3089fa Mon Sep 17 00:00:00 2001

From: Dana L. How <[email protected]>
Date: Sun, 13 May 2007 12:47:09 -0700
Subject: [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature

Add --max-pack-size parsing and usage messages. Upgrade git-repack.sh to handle multiple packfile names, and build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR. Update documentation.

Signed-off-by Dana L. How <[email protected]>

---
 Documentation/git-pack-objects.txt |    5 +++++
 Documentation/git-repack.txt       |    5 +++++
 builtin-pack-objects.c             |    9 ++++++++-
 git-repack.sh                      |   14 ++++++++------
 4 files changed, 26 insertions(+), 7 deletions(-)

builtin-pack-objects.c +0 -0

index ce9eb2d..930b57a 100644
...

@@ -1713,6 +1713,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			pack_compression_level = level;
 			continue;
 		}

git-repack.sh +0 -0

index 8bf66a4..4ea6e5b 100755
...

Dana How <[email protected]> writes:
----------------------------------------------------------------

Quoted 5 lines

Subject: [PATCH] Alter sha1close() 3rd argument to request flush only

update=0 suppressed writing the final SHA-1 but was not used. Now final=0 suppresses SHA-1 finalization, SHA-1 writing, and closing -- in other words, only flush the buffer.

What it does is understandable but it somehow feels funny that "sha1close(file, hashresult, 0)" does _not_ close it (and does not hash either for obvious reasons ;-). I would say we should let it pass this round, but might want to separate the first part out into a separate "update hash and flush" function if we get more callers.

----------------------------------------------------------------

Quoted 9 lines

Subject: [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring

Add "pack_size_limit", the limit specified by --max-pack-size, "written_list", the list of objects written to the current pack, and "nr_written", the number of objects in written_list. Put "base_name" at file scope again and add forward declarations. Move write_index_file() call from cnd_pack_objects() to write_pack_file() since only the latter will know how many times to call write_index_file().

I would have split this part a bit differently.

This is mostly about restructuring the code so that write_index_file() is called from write_pack_file(), which by itself is a very good change (but then we might have been better off passing basename as a parameter).

You are not using written_list nor limit yet but are introducing them in this step, which feels not quite right. I usually compile stuff with -Werror, and if I ever have to bisect the series, this would bomb out for these unused variables. Not nice.

----------------------------------------------------------------

Quoted 8 lines

Subject: [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit

With --max-pack-size, generate the appropriate write limit for each object and check against it before each group of writes. Update delta usability rules to handle base being in a previously- written pack. Inline sha1write_compress() so we know the exact size of the written data when it needs to be compressed. Detect and return write "failure".

Again, this is split somewhat wrongly, as you do not have a way to set max size from the caller, but more importantly, even though write_one and write_object knows to obey the limit (perhaps somebody bisecting this series later may set the limit under the debugger, to work around the lack of option parser), write_pack_file() does not notice zero return from write_one(); I would have added a check there to do:

die("sorry, limit reached and we do not have code to split the pack yet.")

which obviously can be updated in the next patch.

----------------------------------------------------------------

Quoted 8 lines

Subject: [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()

Rewrite write_pack_file() to break to a new packfile whenever write_object/write_one request it, and correct the header's object count in the previous packfile. Change write_index_file() to write an index
for just the objects in the most recent packfile.

builtin-pack-objects.c static void write_pack_file(void) · line 670

Quoted 12 lines

builtin-pack-objects.c +8 -0

@@ -652,7 +663,26 @@ static void write_pack_file(void)
+     } while (nr_remaining && i < nr_objects);
+
+     for (j = 0; i < nr_objects; i++) {
+             struct object_entry *e = objects + i;
+             j += !e->offset && !e->preferred_base;
+     }
+     if (j)
+             die("wrote %u objects as expected but %u unwritten", written, j);

I am a bit confused by this loop. Don't you have to start with i=0 for this check to be meaningful?

----------------------------------------------------------------

Quoted 16 lines

Subject: [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature

Add --max-pack-size parsing and usage messages. Upgrade git-repack.sh to handle multiple packfile names, and build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR. Update documentation.

builtin-pack-objects.c +5 -0

@@ -1713,6 +1713,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
+             if (!prefixcmp(arg, "--max-pack-size=")) {
+                     char *end;
+                     pack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;
+                     if (!arg[16] || *end)
+                             usage(pack_usage);

builtin-pack-objects.c int cmd_pack_objects(int argc, const char **argv, const char *prefix) · line 1722

Quoted 2 lines

builtin-pack-objects.c +2 -0

+                     continue;
+             }

Hmmm. I was almost going to suggest to have this spelled in bytes, with suffixes like k/m/g. However, because wanting to limit a pack under 1.4MB does not make much sense these days, and because having to spell "up to 2GB" as 2047 is not too much trouble, I think this is Ok.

Shouldn't we have a safety to error out when --stdout and --max-pack-size are both given? Currently it silently ignores the limit, doesn't it?

git-repack.sh true) · line 36

Quoted 6 lines

git-repack.sh +1 -1

@@ -35,7 +36,7 @@ true)
-PACKTMP="$GIT_DIR/.tmp-$$-pack"
+PACKTMP="$GIT_OBJECT_DIRECTORY/.tmp-$$-pack"

Although this is a good change, this hunk does not belong to this.

Overall everything looks good, except some minor details noted above. Separation of the commits into logical steps does not need to be fixed up (they are already in 'next'), but follow-up patches might be needed.

And I have to agree with Linus; responding this way was more cumbersome than it should have been.

Patch 1:
* Pull "sha1flush()" or similar out of sha1close() inside csum-file.c. This will require some edits to callers.

Patch 2:
* Add a comment to "confusing loop" to explain what it's checking.
* Complain about --stdout && --max-pack-size combination.

This will have to happen tomorrow.

Quoted 2 lines

And I have to agree with Linus; responding this way was more cumbersome than it should have been.

Understood.

Thanks,