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

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

[Dana How]

If --max-pack-size (offset_limit) 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 How <how@deathvalley.cswitch.com>
---
 builtin-pack-objects.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 9530008..a088f2e 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -556,16 +556,24 @@ static off_t write_one(struct sha1file *f,
 			       struct object_entry *e,
 			       off_t offset)
 {
+	off_t result;
 	if (e->offset || e->preferred_base)
 		/* offset starts from header size and cannot be zero
 		 * if it is written already.
 		 */
 		return offset;
 	/* if we are deltified, write out its base object first. */
-	if (e->delta)
+	if (e->delta) {
 		offset = write_one(f, e->delta, offset);
+		if (!offset)
+			return offset;
+	}
+	/* pass in write limit if limited packsize and not first object */
+	result = write_object(f, e, offset_limit && nr_written ? offset_limit - offset : 0);
+	if (!result)
+		return result;
 	e->offset = offset;
-	return offset + write_object(f, e, 0);
+	return offset + result;
 }
 
 /*
-- 
1.5.1.89.g8abf0

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

[Dana How]

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;
+	}
 
 	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);
+	if (!size)
+		return e->offset = 0;
 
 	/* make sure off_t is sufficiently large not to wrap */
 	if (offset > offset + size)
-- 
1.5.2.rc0.766.gba60-dirty

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

[Shawn O. Pearce]

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.

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

[Dana How]

On 4/30/07, Shawn O. Pearce <spearce@spearce.org> wrote:
> 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...

> >       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@gmail.com  +1 650 804 5991 cell