Re: [PATCH] Enhance unpack-objects for extracting large objects

["Dana How" <danahow@gmail.com>]

On 5/25/07, Junio C Hamano <junkio@cox.net> wrote:
> Dana How <danahow@gmail.com> writes:
> > Nicolas Pitre wrote:
> >> I wouldn't mind a _separate_ tool that would load a pack index,
> >> determine object sizes from it, and then extract big objects to write
> >> them as loose objects ...
> >
> > Below we add two new options to git-unpack-objects:
> >
> > --min-blob-size=<n>::  Unpacking is only done for objects
> > larger than or equal to n kB (uncompressed size by Junio).
>
> Elsewhere you wanted to use --max-* and that was counted in megs;
> isn't using kilo here and meg there inconsistent?
For git-repack --max-pack-size=N I used MB to be consistent
with git-fast-import.  I think it makes sense to use MB everywhere
we talk about packfile size.

For the old degunking patch's -max-blob-size=N ,  and this patch's
-min-blob-size=N ,  I was using KB to describe blob size.
It looked like I needed finer granularity,  at least for experiments.

I think if we always use MB for packfile sizes, and KB
for blob sizes,  we should be OK.

> > --force::  Loose objects will be created even if they
> > already exist in the repository packed.  This is an option
> > I've wanted before for other reasons.
>
>         ... but if they already exist in the repository as loose
>         objects, do not replace it.
>
> Usually we do not overwrite existing loose objects and it is one
> of the security measure --- if you have an object already, that
> cannot be touched by somebody who maliciously creats a hash
> colliding loose object and tries to inject it into your
> repository via unpack-objects.  It's good that you kept this
> behaviour intact.
I agree.
I'll add the "... but" part to the corrected patch's commit msg.

> > -Objects that already exist in the repository will *not* be unpacked
> > -from the pack-file.  Therefore, nothing will be unpacked if you use
> > -this command on a pack-file that exists within the target repository.
> > +By default,  objects that already exist in the repository will *not*
> > +be unpacked from the pack-file.  Therefore, nothing will be unpacked
> > +if you use this command on a pack-file that exists within the target
> > +repository,  unless you specify --force.
> I would want to add:
>         If an object already exists unpacked in the repository,
>         it will not be replaced with the copy from the pack,
>         with or without `--force`.
OK, that's clearer.

> > -static const char unpack_usage[] = "git-unpack-objects [-n] [-q] [-r] < pack-file";
> > +static const char unpack_usage[] =
> > +"git-unpack-objects [-n] [-q] [-r] [--force] [--min-blob-size=N] < pack-file";
>
> Maybe we would want to call it '-f' for consistency.  Another
> possibility is the other way around, giving others a longer
> synonyms, like --quiet, but this command is plumbing and I do
> not think long options matters that much, so my preference is to
> do '-f' not '--force'.
I picked the longer one only because I didn't view it as frequently used.
But it will be more used than min-blob-size.  I'll change it to -f.

>  * You already have the size here, so if min_blob_size is set
>    and the size is larger, you do not even have to call
>    write_sha1_file() at all.
The way I read the code,  it looks like unpack-objects needs
the last argument always to be initialized with the SHA-1 computed
from the object contents.  Therefore I always need to call
write_sha1_file(),  even if I don't want it to write anything.

> So "ignore" means:
>         negative:       never write it out, even if it does not exist.
>         zero:           do not write it out if it is available (in pack,
>                         or loose, either local or alternate), do
>                         write it out otherwise; it is the same
>                         as the current behaviour of write_sha1_file().
>         positive:       always write it out.
> That does not sound like "ignore".
I agree "ignore" is confusing;
I will change it to "when",  which is more consistent with the
_maybe prefix on the function name.

> My suggestion would be:
I like this suggestion,  but since I need to call the function
to get the SHA-1,  I don't think I can follow it.

I'll send you an updated patch in a moment.

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