{"thread":{"id":"55525","subject":"[PATCH] builtin/repack.c: set a default factor for '--geometric'","startedAt":"2021-04-20T01:47:31Z","lastAt":"2021-04-20T21:44:29Z","messageCount":6,"participants":["Taylor Blau","Derrick Stolee","Junio C Hamano","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"422363","messageId":"1ecab817396fae6a1cbafde1ca8b3ebfd9ae4c11.1618883241.git.me@ttaylorr.com","threadId":"55525","inReplyTo":null,"subject":"[PATCH] builtin/repack.c: set a default factor for '--geometric'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-04-20T01:47:26Z","receivedAt":"2021-04-20T01:47:31Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The '--geometric=' argument specifies that each pack must contain at\nleast 'n' times as many objects as the size of the next-largest pack.\nThe factor 'n' is customizable, but setting it to '2' is a sane default.\n\nInstead of making the factor a required argument, make the argument\noptional with a default value of '2'.\n\nTo ensure that the option is setup correctly, modify the most complex\ntest of t7703 to drop the explicit factor.\n\nSigned-off-by: Taylor Blau \n---\n Documentation/git-repack.txt | 8 ++++----\n builtin/repack.c | 5 +++--\n t/t7703-repack-geometric.sh | 2 +-\n 3 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 317d63cf0d..d948a2913d 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -165,11 +165,11 @@ depth is 4095.\n \tPass the `--delta-islands` option to `git-pack-objects`, see\n \tlinkgit:git-pack-objects[1].\n \n--g=::\n---geometric=::\n+-g=[]::\n+--geometric[=]::\n \tArrange resulting pack structure so that each successive pack\n-\tcontains at least `` times the number of objects as the\n-\tnext-largest pack.\n+\tcontains at least `` (`2` if unspecified) times the\n+\tnumber of objects as the next-largest pack.\n +\n `git repack` ensures this by determining a \"cut\" of packfiles that need\n to be repacked into one in order to ensure a geometric progression. It\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2847fdfbab..f2359c9d3a 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -494,8 +494,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n \t\t\t\tN_(\"do not repack this pack\")),\n-\t\tOPT_INTEGER('g', \"geometric\", &geometric_factor,\n-\t\t\t N_(\"find a geometric progression with factor \")),\n+\t\t{ OPTION_INTEGER, 'g', \"geometric\", &geometric_factor, N_(\"n\"),\n+\t\t\t\tN_(\"find a geometric progression with factor \"),\n+\t\t\t\tPARSE_OPT_OPTARG, NULL, 2 },\n \t\tOPT_END()\n \t};\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 5ccaa440e0..77cd5f2284 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -123,7 +123,7 @@ test_expect_success '--geometric with small- and large-pack rollup' '\n \n \t\tfind $objdir/pack -name \"*.pack\" | sort >before &&\n \n-\t\tgit repack --geometric 2 -d &&\n+\t\tgit repack --geometric -d &&\n \n \t\tfind $objdir/pack -name \"*.pack\" | sort >after &&\n \t\tcomm -12 before after >untouched &&\n-- \n2.31.1.163.ga65ce7f831\n"},{"id":"422405","messageId":"60af51b7-b2a3-8acf-f7a0-49a137daf2c6@gmail.com","threadId":"55525","inReplyTo":"1ecab817396fae6a1cbafde1ca8b3ebfd9ae4c11.1618883241.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: set a default factor for '--geometric'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-04-20T12:39:33Z","receivedAt":"2021-04-20T12:39:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/19/2021 9:47 PM, Taylor Blau wrote:\n> The '--geometric=' argument specifies that each pack must contain at\n> least 'n' times as many objects as the size of the next-largest pack.\n> The factor 'n' is customizable, but setting it to '2' is a sane default.\n> \n> Instead of making the factor a required argument, make the argument\n> optional with a default value of '2'.\n\nThis flexibility is nice.\n \n> To ensure that the option is setup correctly, modify the most complex\n> test of t7703 to drop the explicit factor.\n\nGood testing.\n\n> --g=::\n> ---geometric=::\n> +-g=[]::\n> +--geometric[=]::\n> \tArrange resulting pack structure so that each successive pack\n> -\tcontains at least `` times the number of objects as the\n> -\tnext-largest pack.\n> +\tcontains at least `` (`2` if unspecified) times the\n> +\tnumber of objects as the next-largest pack.\n\nThe parenthetical interrupts what `` means. Perhaps\nrearrange:\n\n\tArrange resulting pack structure so that each successive pack\n\tcontains at least `` times the number of objects as the\n\tnext-largest pack. If `` is not specified, then `2` is\n\tused by default.\n\nRest of the diff looks good.\n\nThanks,\n-Stolee\n"},{"id":"422471","messageId":"xmqqv98g4sbl.fsf@gitster.g","threadId":"55525","inReplyTo":"1ecab817396fae6a1cbafde1ca8b3ebfd9ae4c11.1618883241.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: set a default factor for '--geometric'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-20T20:25:02Z","receivedAt":"2021-04-20T20:25:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> --g=::\n> ---geometric=::\n> +-g=[]::\n> +--geometric[=]::\n\nDo you mean \"git repack -g= -d\" is (1) accepted and/or (2)\nrecommended?\n\nIf not, we'd want \"-g[=]\" similar to the longer form, no?\n"},{"id":"422472","messageId":"YH86ENkPrTeWnnBb@nand.local","threadId":"55525","inReplyTo":"xmqqv98g4sbl.fsf@gitster.g","subject":"Re: [PATCH] builtin/repack.c: set a default factor for '--geometric'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-04-20T20:31:12Z","receivedAt":"2021-04-20T20:31:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Apr 20, 2021 at 01:25:02PM -0700, Junio C Hamano wrote:\n> Taylor Blau writes:\n>\n> > --g=::\n> > ---geometric=::\n> > +-g=[]::\n> > +--geometric[=]::\n>\n> Do you mean \"git repack -g= -d\" is (1) accepted and/or (2)\n> recommended?\n>\n> If not, we'd want \"-g[=]\" similar to the longer form, no?\n\nThanks for spotting. The '[' should come before the '=' character, not\nafter. I'll send a reroll with this (and a suggestion from Stolee).\n\nThanks,\nTaylor\n"},{"id":"422473","messageId":"8d8dea2e2c149303891cfe6ae8dbec06b538622c.1618950726.git.me@ttaylorr.com","threadId":"55525","inReplyTo":"1ecab817396fae6a1cbafde1ca8b3ebfd9ae4c11.1618883241.git.me@ttaylorr.com","subject":"[PATCH v2] builtin/repack.c: set a default factor for '--geometric'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-04-20T20:32:13Z","receivedAt":"2021-04-20T20:32:20Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The '--geometric=' argument specifies that each pack must contain at\nleast 'n' times as many objects as the size of the next-largest pack.\nThe factor 'n' is customizable, but setting it to '2' is a sane default.\n\nInstead of making the factor a required argument, make the argument\noptional with a default value of '2'.\n\nTo ensure that the option is setup correctly, modify the most complex\ntest of t7703 to drop the explicit factor.\n\nSigned-off-by: Taylor Blau \n---\nRange-diff against v1:\n1: 1ecab81739 ! 1: 8d8dea2e2c builtin/repack.c: set a default factor for '--geometric'\n @@ Documentation/git-repack.txt: depth is 4095.\n \n --g=::\n ---geometric=::\n -+-g=[]::\n ++-g[=]::\n +--geometric[=]::\n \tArrange resulting pack structure so that each successive pack\n --\tcontains at least `` times the number of objects as the\n + \tcontains at least `` times the number of objects as the\n -\tnext-largest pack.\n -+\tcontains at least `` (`2` if unspecified) times the\n -+\tnumber of objects as the next-largest pack.\n ++\tnext-largest pack. If `` is not specified, then `2` is\n ++\tused by default.\n +\n `git repack` ensures this by determining a \"cut\" of packfiles that need\n to be repacked into one in order to ensure a geometric progression. It\n\n Documentation/git-repack.txt | 7 ++++---\n builtin/repack.c | 5 +++--\n t/t7703-repack-geometric.sh | 2 +-\n 3 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 317d63cf0d..f7c7e0aeae 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -165,11 +165,12 @@ depth is 4095.\n \tPass the `--delta-islands` option to `git-pack-objects`, see\n \tlinkgit:git-pack-objects[1].\n \n--g=::\n---geometric=::\n+-g[=]::\n+--geometric[=]::\n \tArrange resulting pack structure so that each successive pack\n \tcontains at least `` times the number of objects as the\n-\tnext-largest pack.\n+\tnext-largest pack. If `` is not specified, then `2` is\n+\tused by default.\n +\n `git repack` ensures this by determining a \"cut\" of packfiles that need\n to be repacked into one in order to ensure a geometric progression. It\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 2847fdfbab..f2359c9d3a 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -494,8 +494,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_STRING_LIST(0, \"keep-pack\", &keep_pack_list, N_(\"name\"),\n \t\t\t\tN_(\"do not repack this pack\")),\n-\t\tOPT_INTEGER('g', \"geometric\", &geometric_factor,\n-\t\t\t N_(\"find a geometric progression with factor \")),\n+\t\t{ OPTION_INTEGER, 'g', \"geometric\", &geometric_factor, N_(\"n\"),\n+\t\t\t\tN_(\"find a geometric progression with factor \"),\n+\t\t\t\tPARSE_OPT_OPTARG, NULL, 2 },\n \t\tOPT_END()\n \t};\n \ndiff --git a/t/t7703-repack-geometric.sh b/t/t7703-repack-geometric.sh\nindex 5ccaa440e0..77cd5f2284 100755\n--- a/t/t7703-repack-geometric.sh\n+++ b/t/t7703-repack-geometric.sh\n@@ -123,7 +123,7 @@ test_expect_success '--geometric with small- and large-pack rollup' '\n \n \t\tfind $objdir/pack -name \"*.pack\" | sort >before &&\n \n-\t\tgit repack --geometric 2 -d &&\n+\t\tgit repack --geometric -d &&\n \n \t\tfind $objdir/pack -name \"*.pack\" | sort >after &&\n \t\tcomm -12 before after >untouched &&\n-- \n2.31.1.163.ga65ce7f831\n"},{"id":"422480","messageId":"877dkwfx6x.fsf@igel.home","threadId":"55525","inReplyTo":"8d8dea2e2c149303891cfe6ae8dbec06b538622c.1618950726.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/repack.c: set a default factor for '--geometric'","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2021-04-20T21:44:22Z","receivedAt":"2021-04-20T21:44:29Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Apr 20 2021, Taylor Blau wrote:\n\n> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\n> index 317d63cf0d..f7c7e0aeae 100644\n> --- a/Documentation/git-repack.txt\n> +++ b/Documentation/git-repack.txt\n> @@ -165,11 +165,12 @@ depth is 4095.\n> \tPass the `--delta-islands` option to `git-pack-objects`, see\n> \tlinkgit:git-pack-objects[1].\n> \n> --g=::\n> ---geometric=::\n> +-g[=]::\n> +--geometric[=]::\n> \tArrange resulting pack structure so that each successive pack\n> \tcontains at least `` times the number of objects as the\n> -\tnext-largest pack.\n> +\tnext-largest pack. If `` is not specified, then `2` is\n> +\tused by default.\n\nI don't think the short option takes a '='.\n\n$ ./git repack -g=2\nerror: switch `g' expects a numerical value\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"}]}