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

On 4/30/07, Shawn O. Pearce [off-list ref] wrote:

Dana How [off-list ref] wrote:

quoted

      /* 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...

quoted

      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.

quoted

+     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