Re: Rebase max-pack-size?

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

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

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

> ----------------------------------------------------------------
> > 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.
> >
> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> > --- a/builtin-pack-objects.c
> > +++ b/builtin-pack-objects.c
> > @@ -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?

> ----------------------------------------------------------------
> > 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.
> >
> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> > --- a/builtin-pack-objects.c
> > +++ b/builtin-pack-objects.c
> > @@ -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);
> > +                     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.

> > diff --git a/git-repack.sh b/git-repack.sh
> > --- a/git-repack.sh
> > +++ b/git-repack.sh
> > @@ -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.