From: Dana How Date: Tue, 01 May 2007 06:00:40 GMT Subject: Re: [PATCH 6/8] git-repack --max-pack-size: write_one() implements limits Message-ID: <56b7f5510704302300i6c44760dn111729f669493766@mail.gmail.com> In-Reply-To: <20070501052517.GA5942@spearce.org> On 4/30/07, Shawn O. Pearce wrote: > Dana How wrote: > > /* if we are deltified, write out base object first. */ > > - if (e->delta) > > + if (e->delta) { > > offset = write_one(f, e->delta, offset); > > + if (!offset) > > + return 0; > > + } > > So offset == 0 means we didn't write this object into this packfile? > Did I read your changes right? Yes. write_object()/write_one() return 0 to indicate that the write was "refused" due to pack size limits. Note that entry->offset has *2* special values now: 0 to indicate not yet written (any "real" offset is >= 12), and (off_t)-1 to indicate written but to previous pack. In my previous patchsets, the latter condition was indicated by a new field in object_entry, but since Nicolas Pitre just optimized its space usage I figured I wouldn't immediately undo his work... > > e->offset = offset; > > - size = write_object(f, e, 0); > > + /* pass in write limit if limited packsize and not first object */ > > + size = write_object(f, e, pack_size_limit && nr_written ? pack_size_limit - offset : 0); > > Why wasn't this in the prior patch? You had almost everything in > place, but hardcoded the option to 0, to then just change it here > in this patch to non-zero under some conditions? I broke up the patches by function changed. The goal was to change 1 function per patch, plus whatever minor stuff (e.g. ,0 above) was needed to make it compile. That was followed too rigidly: the write_object() and write_one() changes should have been together in 1 patch. > I'd also like to see that line <80 characters, but that's just me > and my own style preferences. The 3rd arg should be on its own line. Funny I didn't do that. > But isn't that argument actually kind of silly here? None of > the values that are used to compute that 3rd boolean argument to > write_objet depend on things that are local to write_one - they > are all global values. Can't we spare the poor register-starved > x86 platform and just let write_object compute that value itself? That all makes sense. I left it this way out of laziness and because Junio suggested putting a limit into sha1write_compressed(), so I left the new argument to write_object() as a limit, thinking (unclearly?) I might follow his suggestion later. > > + if (!size) > > + return e->offset = 0; > > Ugh. I don't really like to see assignment in the middle of an > evaluation of an rvalue, and especially here in a return. Burn the > couple of lines to segment it out: > > if (!size) { > e->offset = 0; > return 0; > } > > or something. Because its a lot more clear and doesn't look like > you missed an = to make "return e->offset == 0". Hmm, I only worry about the =/== issue when the context expects a condition, and this function was _not_ declared boolean. However, it's just as easy to do what you suggest. Thanks, -- Dana L. How danahow@gmail.com +1 650 804 5991 cell