From: Dana How Date: Fri, 25 May 2007 19:49:08 GMT Subject: Re: [PATCH] Enhance unpack-objects for extracting large objects Message-ID: <56b7f5510705251249u74b754f1y4f8cafd5f5c35f19@mail.gmail.com> In-Reply-To: <7vsl9kr9mz.fsf@assigned-by-dhcp.cox.net> On 5/25/07, Junio C Hamano wrote: > Dana How 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=:: 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