{"thread":{"id":"7918","subject":"[PATCH 6/8] git-repack --max-pack-size: write_one() implements limits","startedAt":"2007-04-30T23:23:20Z","lastAt":"2007-05-01T06:00:40Z","messageCount":3,"participants":["Dana How","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"40774","messageId":"46367A68.4010008@gmail.com","threadId":"7918","inReplyTo":null,"subject":"[PATCH 6/8] git-repack --max-pack-size: write_one() implements limits","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-30T23:23:20Z","receivedAt":"2007-04-30T23:23:20Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"\nIf --max-pack-size is specified, generate the appropriate\nwrite limit for each object and pass it to write_object().\nDetect and return write \"failure\".\n\nSigned-off-by: Dana L. How \n---\n builtin-pack-objects.c | 10 ++++++++--\n 1 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex d3ebe1d..b50de05 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -612,11 +612,17 @@ static off_t write_one(struct sha1file *f,\n \t\treturn offset;\n \n \t/* if we are deltified, write out base object first. */\n-\tif (e->delta)\n+\tif (e->delta) {\n \t\toffset = write_one(f, e->delta, offset);\n+\t\tif (!offset)\n+\t\t\treturn 0;\n+\t}\n \n \te->offset = offset;\n-\tsize = write_object(f, e, 0);\n+\t/* pass in write limit if limited packsize and not first object */\n+\tsize = write_object(f, e, pack_size_limit && nr_written ? pack_size_limit - offset : 0);\n+\tif (!size)\n+\t\treturn e->offset = 0;\n \n \t/* make sure off_t is sufficiently large not to wrap */\n \tif (offset > offset + size)\n-- \n1.5.2.rc0.766.gba60-dirty\n"},{"id":"40791","messageId":"20070501052517.GA5942@spearce.org","threadId":"7918","inReplyTo":"46367A68.4010008@gmail.com","subject":"Re: [PATCH 6/8] git-repack --max-pack-size: write_one() implements limits","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-05-01T05:25:17Z","receivedAt":"2007-05-01T05:25:17Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Dana How wrote:\n> \n> If --max-pack-size is specified, generate the appropriate\n> write limit for each object and pass it to write_object().\n> Detect and return write \"failure\".\n> \n> Signed-off-by: Dana L. How \n> ---\n> builtin-pack-objects.c | 10 ++++++++--\n> 1 files changed, 8 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index d3ebe1d..b50de05 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -612,11 +612,17 @@ static off_t write_one(struct sha1file *f,\n> \t\treturn offset;\n> \n> \t/* if we are deltified, write out base object first. */\n> -\tif (e->delta)\n> +\tif (e->delta) {\n> \t\toffset = write_one(f, e->delta, offset);\n> +\t\tif (!offset)\n> +\t\t\treturn 0;\n> +\t}\n\nSo offset == 0 means we didn't write this object into this packfile?\nDid I read your changes right?\n\n> \te->offset = offset;\n> -\tsize = write_object(f, e, 0);\n> +\t/* pass in write limit if limited packsize and not first object */\n> +\tsize = write_object(f, e, pack_size_limit && nr_written ? pack_size_limit - offset : 0);\n\nWhy wasn't this in the prior patch? You had almost everything in\nplace, but hardcoded the option to 0, to then just change it here\nin this patch to non-zero under some conditions?\n\nI'd also like to see that line <80 characters, but that's just me\nand my own style preferences.\n\nBut isn't that argument actually kind of silly here? None of\nthe values that are used to compute that 3rd boolean argument to\nwrite_objet depend on things that are local to write_one - they\nare all global values. Can't we spare the poor register-starved\nx86 platform and just let write_object compute that value itself?\n\n> +\tif (!size)\n> +\t\treturn e->offset = 0;\n\nUgh. I don't really like to see assignment in the middle of an\nevaluation of an rvalue, and especially here in a return. Burn the\ncouple of lines to segment it out:\n\n\tif (!size) {\n\t\te->offset = 0;\n\t\treturn 0;\n\t}\n\nor something. Because its a lot more clear and doesn't look like\nyou missed an = to make \"return e->offset == 0\".\n\n-- \nShawn.\n"},{"id":"40795","messageId":"56b7f5510704302300i6c44760dn111729f669493766@mail.gmail.com","threadId":"7918","inReplyTo":"20070501052517.GA5942@spearce.org","subject":"Re: [PATCH 6/8] git-repack --max-pack-size: write_one() implements limits","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-05-01T06:00:40Z","receivedAt":"2007-05-01T06:00:40Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/30/07, Shawn O. Pearce wrote:\n> Dana How wrote:\n> > /* if we are deltified, write out base object first. */\n> > - if (e->delta)\n> > + if (e->delta) {\n> > offset = write_one(f, e->delta, offset);\n> > + if (!offset)\n> > + return 0;\n> > + }\n>\n> So offset == 0 means we didn't write this object into this packfile?\n> Did I read your changes right?\nYes. write_object()/write_one() return 0 to indicate\nthat the write was \"refused\" due to pack size limits.\n\nNote that entry->offset has *2* special values now:\n0 to indicate not yet written (any \"real\" offset is >= 12), and\n(off_t)-1 to indicate written but to previous pack.\nIn my previous patchsets, the latter condition was indicated\nby a new field in object_entry, but since Nicolas Pitre just\noptimized its space usage I figured I wouldn't immediately undo his work...\n\n> > e->offset = offset;\n> > - size = write_object(f, e, 0);\n> > + /* pass in write limit if limited packsize and not first object */\n> > + size = write_object(f, e, pack_size_limit && nr_written ? pack_size_limit - offset : 0);\n>\n> Why wasn't this in the prior patch? You had almost everything in\n> place, but hardcoded the option to 0, to then just change it here\n> in this patch to non-zero under some conditions?\nI broke up the patches by function changed. The goal\nwas to change 1 function per patch, plus whatever minor\nstuff (e.g. ,0 above) was needed to make it compile.\nThat was followed too rigidly: the write_object() and write_one()\nchanges should have been together in 1 patch.\n\n> I'd also like to see that line <80 characters, but that's just me\n> and my own style preferences.\nThe 3rd arg should be on its own line.\nFunny I didn't do that.\n\n> But isn't that argument actually kind of silly here? None of\n> the values that are used to compute that 3rd boolean argument to\n> write_objet depend on things that are local to write_one - they\n> are all global values. Can't we spare the poor register-starved\n> x86 platform and just let write_object compute that value itself?\nThat all makes sense. I left it this way out of laziness and\nbecause Junio suggested putting a limit into sha1write_compressed(),\nso I left the new argument to write_object() as a limit,\nthinking (unclearly?) I might follow his suggestion later.\n\n> > + if (!size)\n> > + return e->offset = 0;\n>\n> Ugh. I don't really like to see assignment in the middle of an\n> evaluation of an rvalue, and especially here in a return. Burn the\n> couple of lines to segment it out:\n>\n> if (!size) {\n> e->offset = 0;\n> return 0;\n> }\n>\n> or something. Because its a lot more clear and doesn't look like\n> you missed an = to make \"return e->offset == 0\".\nHmm, I only worry about the =/== issue when the context\nexpects a condition, and this function was _not_ declared boolean.\nHowever, it's just as easy to do what you suggest.\n\nThanks,\n-- \nDana L. How danahow@gmail.com +1 650 804 5991 cell\n"}]}