Re: [PATCH 6/8] git-repack --max-pack-size: write_one() implements limits

From: Dana How <danahow@gmail.com>
Date: May 1, 2007, 06:00 UTC
Message-ID: <56b7f5510704302300i6c44760dn111729f669493766@mail.gmail.com>
In-Reply-To: <20070501052517.GA5942@spearce.org>

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