Re: [PATCH 6/8] git-repack --max-pack-size: write_one() implements limits
On 4/30/07, Shawn O. Pearce <spearce@spearce.org> wrote:
Show 11 quoted lines
> Dana How <danahow@gmail.com> 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...
Show 8 quoted lines
> > 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.
Show 5 quoted lines
> 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.
Show 14 quoted lines
> > + 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
