git-repack --max-pack-size: write_one() implements limits
PATCH 6/8 git-repack --max-pack-size: write_one() implements limits
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".
--- builtin-pack-objects.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
builtin-pack-objects.c +8 -2
@@ -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)
Signature · 1 line
1.5.2.rc0.766.gba60-dirty
Quoted 15 lines · 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 <[email protected]>--- builtin-pack-objects.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)builtin-pack-objects.c +0
-0
builtin-pack-objects.c static off_t write_one(struct sha1file *f, · line 619
Quoted 9 lines
builtin-pack-objects.c +4
-1@@ -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?
builtin-pack-objects.c static off_t write_one(struct sha1file *f, · line 623
Quoted 4 lines
builtin-pack-objects.c +2
-1e->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?
builtin-pack-objects.c static off_t write_one(struct sha1file *f, · line 625
Quoted 2 lines
builtin-pack-objects.c +2
-0+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".
Signature · 1 line
Shawn.
Quoted 11 lines · Shawn O. Pearce
builtin-pack-objects.c static off_t write_one(struct sha1file *f, · line 619
Quoted 7 lines · Dana How
/* if we are deltified, write out base object first. */builtin-pack-objects.c +4
-1-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 8 lines
builtin-pack-objects.c static off_t write_one(struct sha1file *f, · line 623
Quoted 4 lines
e->offset = offset;builtin-pack-objects.c +2
-1- 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.
Quoted 2 lines
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.
Quoted 5 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_objetdepend on things that are local towrite_one- they are all global values. Can't we spare the poor register-starved x86 platform and just letwrite_objectcompute 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 7 lines
builtin-pack-objects.c static off_t write_one(struct sha1file *f, · line 625
Quoted 2 lines
builtin-pack-objects.c +2
-0+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:
Quoted 7 lines
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,
Signature · 1 line
Dana L. How [email protected] +1 650 804 5991 cell