On 4/30/07, Shawn O. Pearce <spearce@xxxxxxxxxxx> wrote:
Dana How <danahow@xxxxxxxxx> 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@xxxxxxxxx +1 650 804 5991 cell
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html