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

["Shawn O. Pearce" <spearce@spearce.org>]

Dana How <danahow@gmail.com> wrote:
> 
> If --max-pack-size is specified,  generate the appropriate
> write limit for each object and pass it to write_object().
> Detect and return write "failure".
> 
> Signed-off-by: Dana L. How <danahow@gmail.com>
> ---
>  builtin-pack-objects.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index d3ebe1d..b50de05 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -612,11 +612,17 @@ static off_t write_one(struct sha1file *f,
>  		return offset;
>  
>  	/* 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?

>  	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'd also like to see that line <80 characters, but that's just me
and my own style preferences.

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?

> +	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".

-- 
Shawn.