{"thread":{"id":"54128","subject":"[PATCH] builtin/repack.c: invalidate MIDX only when necessary","startedAt":"2020-08-25T02:01:13Z","lastAt":"2020-12-30T22:00:38Z","messageCount":78,"participants":["Taylor Blau","Jeff King","Son Luong Ngoc","Derrick Stolee","Junio C Hamano","Eric Sunshine","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"404387","messageId":"ef9186a8df0d712c2ecccbe62cb43a7abadb9c96.1598320716.git.me@ttaylorr.com","threadId":"54128","inReplyTo":null,"subject":"[PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-25T02:01:04Z","receivedAt":"2020-08-25T02:01:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\nlearned to remove a multi-pack-index file if it added or removed a pack\nfrom the object store.\n\nThis mechanism is a little over-eager, since it is only necessary to\ndrop a MIDX if 'git repack' removes a pack that the MIDX references.\nAdding a pack outside of the MIDX does not require invalidating the\nMIDX, and likewise for removing a pack the MIDX does not know about.\n\nTeach 'git repack' to check for this by loading the MIDX, and checking\nwhether the to-be-removed pack is known to the MIDX. This requires a\nslightly odd alternation to a test in t5319, which is explained with a\ncomment.\n\nSigned-off-by: Taylor Blau \n---\nSomething I noticed while working on a MIDX-related topic. Not critical\nthat we take this now since it's only dropping the MIDX too\naggressively, but not otherwise doing anything destructive. But, this\nwill make my somewhat large other series a little smaller.\n\n builtin/repack.c | 12 +++++-------\n t/t5319-multi-pack-index.sh | 21 +++++++++++++++++++--\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 04c5ceaf7e..98fac03946 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n+\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n+\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n+\tif (m && midx_contains_pack(m, buf.buf))\n+\t\tclear_midx_file(the_repository);\n+\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n \tunlink_pack_path(buf.buf, 1);\n \tstrbuf_release(&buf);\n }\n@@ -286,7 +290,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tint keep_unreachable = 0;\n \tstruct string_list keep_pack_list = STRING_LIST_INIT_NODUP;\n \tint no_update_server_info = 0;\n-\tint midx_cleared = 0;\n \tstruct pack_objects_args po_args = {NULL};\n\n \tstruct option builtin_repack_options[] = {\n@@ -439,11 +442,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n \t\t\tchar *fname, *fname_old;\n\n-\t\t\tif (!midx_cleared) {\n-\t\t\t\tclear_midx_file(the_repository);\n-\t\t\t\tmidx_cleared = 1;\n-\t\t\t}\n-\n \t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n \t\t\t\t\t\titem->string, exts[ext].name);\n \t\t\tif (!file_exists(fname)) {\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 43b1b5b2af..16a1ad040e 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -382,12 +382,29 @@ test_expect_success 'repack with the --no-progress option' '\n \ttest_line_count = 0 err\n '\n\n-test_expect_success 'repack removes multi-pack-index' '\n+test_expect_success 'repack removes multi-pack-index when deleting packs' '\n \ttest_path_is_file $objdir/pack/multi-pack-index &&\n-\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&\n+\t# Set GIT_TEST_MULTI_PACK_INDEX to 0 to avoid writing a new\n+\t# multi-pack-index after repacking, but set \"core.multiPackIndex\" to\n+\t# true so that \"git repack\" can read the existing MIDX.\n+\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -adf &&\n \ttest_path_is_missing $objdir/pack/multi-pack-index\n '\n\n+test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n+\tgit multi-pack-index write &&\n+\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n+\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n+\n+\t# Write a new pack that is unknown to the multi-pack-index.\n+\tgit hash-object -w blob &&\n+\tgit pack-objects $objdir/pack/pack In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n> learned to remove a multi-pack-index file if it added or removed a pack\n> from the object store.\n> \n> This mechanism is a little over-eager, since it is only necessary to\n> drop a MIDX if 'git repack' removes a pack that the MIDX references.\n> Adding a pack outside of the MIDX does not require invalidating the\n> MIDX, and likewise for removing a pack the MIDX does not know about.\n\nDoes \"git repack\" ever remove just one pack? Obviously \"git repack -ad\"\nor \"git repack -Ad\" is going to pack everything and delete the old\npacks. So I think we'd want to remove a midx there.\n\nAnd \"git repack -d\" I think of as deleting only loose objects that we\njust packed. But I guess it could also remove a pack that has now been\nmade redundant? That seems like a rare case in practice, but I suppose\nis possible.\n\nNot exactly related to your fix, but kind of the flip side of it: would\nwe ever need to retain a midx that mentions some packs that still exist?\n\nE.g., imagine we have a midx that points to packs A and B, and\ngit-repack deletes B. By your logic above, we need to remove the midx\nbecause now it points to objects in B which aren't accessible. But by\ndeleting it, could we be deleting the only thing that mentions the\nobjects in A?\n\nI _think_ the answer is \"no\", because we never went all-in on midx and\nallowed deleting the matching .idx files for contained packs. So we'd\nstill have that A.idx, and we could just use the pack as normal. But\nit's an interesting corner case if we ever do go in that direction.\n\nIf you'll let me muse a bit more on midx-lifetime issues (which I've\nnever really thought about before just now):\n\nI'm also a little curious how bad it is to have a midx whose pack has\ngone away. I guess we'd answer queries for \"yes, we have this object\"\neven if we don't, which is bad. Though in practice we'd only delete\nthose packs if we have their objects elsewhere. And the pack code is\npretty good about retrying other copies of objects that can't be\naccessed. Alternatively, I wonder if the midx-loading code ought to\ncheck that all of the constituent packs are available.\n\nIn that line of thinking, do we even need to delete midx files if one of\ntheir packs goes away? The reading side probably ought to be able to\nhandle that gracefully.\n\nAnd the more interesting case is when you repack everything with \"-ad\"\nor similar, at which point you shouldn't even need to look up what's in\nthe midx to see if you deleted its packs. The point of your operation is\nto put it all-into-one, so you know the old midx should be discarded.\n\n> Teach 'git repack' to check for this by loading the MIDX, and checking\n> whether the to-be-removed pack is known to the MIDX. This requires a\n> slightly odd alternation to a test in t5319, which is explained with a\n> comment.\n\nMy above musings aside, this seems like an obvious improvement.\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 04c5ceaf7e..98fac03946 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n> static void remove_redundant_pack(const char *dir_name, const char *base_name)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> -\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n> +\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n> +\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n> +\tif (m && midx_contains_pack(m, buf.buf))\n> +\t\tclear_midx_file(the_repository);\n> +\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n\nMakes sense. midx_contains_pack() is a binary search, so we'll spend\nO(n log n) effort deleting the packs (I wondered if this might be\naccidentally quadratic over the number of packs).\n\nAnd after we clear, \"m\" will be NULL, so we'll do it at most once. Which\nis why you can get rid of the manual \"midx_cleared\" flag from the\npreimage.\n\nSo the patch looks good to me.\n\n-Peff\n"},{"id":"404389","messageId":"20200825023710.GA98081@syl.lan","threadId":"54128","inReplyTo":"20200825022614.GA1391422@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-25T02:37:10Z","receivedAt":"2020-08-25T02:37:18Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 24, 2020 at 10:26:14PM -0400, Jeff King wrote:\n> On Mon, Aug 24, 2020 at 10:01:04PM -0400, Taylor Blau wrote:\n>\n> > In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n> > learned to remove a multi-pack-index file if it added or removed a pack\n> > from the object store.\n> >\n> > This mechanism is a little over-eager, since it is only necessary to\n> > drop a MIDX if 'git repack' removes a pack that the MIDX references.\n> > Adding a pack outside of the MIDX does not require invalidating the\n> > MIDX, and likewise for removing a pack the MIDX does not know about.\n>\n> Does \"git repack\" ever remove just one pack? Obviously \"git repack -ad\"\n> or \"git repack -Ad\" is going to pack everything and delete the old\n> packs. So I think we'd want to remove a midx there.\n>\n> And \"git repack -d\" I think of as deleting only loose objects that we\n> just packed. But I guess it could also remove a pack that has now been\n> made redundant? That seems like a rare case in practice, but I suppose\n> is possible.\n\nYeah, the patch message makes this sound more likely than it actually\nis, which I agree is very rare. I often write 'git repack' instead of\n'git pack-objects' to slurp up everything loose into a new pack without\nhaving to list loose objects by name.\n\nThat's the case that I really care about here: purely adding a new pack\nshould not invalidate the existing MIDX.\n\n> Not exactly related to your fix, but kind of the flip side of it: would\n> we ever need to retain a midx that mentions some packs that still exist?\n>\n> E.g., imagine we have a midx that points to packs A and B, and\n> git-repack deletes B. By your logic above, we need to remove the midx\n> because now it points to objects in B which aren't accessible. But by\n> deleting it, could we be deleting the only thing that mentions the\n> objects in A?\n>\n> I _think_ the answer is \"no\", because we never went all-in on midx and\n> allowed deleting the matching .idx files for contained packs. So we'd\n> still have that A.idx, and we could just use the pack as normal. But\n> it's an interesting corner case if we ever do go in that direction.\n\nAgreed. Maybe a (admittedly somewhat large) #leftoverbits.\n\n> If you'll let me muse a bit more on midx-lifetime issues (which I've\n> never really thought about before just now):\n>\n> I'm also a little curious how bad it is to have a midx whose pack has\n> gone away. I guess we'd answer queries for \"yes, we have this object\"\n> even if we don't, which is bad. Though in practice we'd only delete\n> those packs if we have their objects elsewhere. And the pack code is\n> pretty good about retrying other copies of objects that can't be\n> accessed. Alternatively, I wonder if the midx-loading code ought to\n> check that all of the constituent packs are available.\n>\n> In that line of thinking, do we even need to delete midx files if one of\n> their packs goes away? The reading side probably ought to be able to\n> handle that gracefully.\n\nI think that this is probably the right direction, although I've only\nspend time in the MIDX code over the past couple of weeks, so I can't\nsay with authority. It seems like it would be pretty annoying, though.\nFor example, code that cares about listing all objects in a MIDX would\nhave to check first whether the pack they're in still exists before\nemitting them. On top of that, there are more corner cases when object X\nexists in more than one pack, but some strict subset of those packs\ncontaining X have gone away.\n\nI don't think that it couldn't be done, though.\n\n> And the more interesting case is when you repack everything with \"-ad\"\n> or similar, at which point you shouldn't even need to look up what's in\n> the midx to see if you deleted its packs. The point of your operation is\n> to put it all-into-one, so you know the old midx should be discarded.\n>\n> > Teach 'git repack' to check for this by loading the MIDX, and checking\n> > whether the to-be-removed pack is known to the MIDX. This requires a\n> > slightly odd alternation to a test in t5319, which is explained with a\n> > comment.\n>\n> My above musings aside, this seems like an obvious improvement.\n>\n> > diff --git a/builtin/repack.c b/builtin/repack.c\n> > index 04c5ceaf7e..98fac03946 100644\n> > --- a/builtin/repack.c\n> > +++ b/builtin/repack.c\n> > @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n> > static void remove_redundant_pack(const char *dir_name, const char *base_name)\n> > {\n> > \tstruct strbuf buf = STRBUF_INIT;\n> > -\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n> > +\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n> > +\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n> > +\tif (m && midx_contains_pack(m, buf.buf))\n> > +\t\tclear_midx_file(the_repository);\n> > +\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n>\n> Makes sense. midx_contains_pack() is a binary search, so we'll spend\n> O(n log n) effort deleting the packs (I wondered if this might be\n> accidentally quadratic over the number of packs).\n\nRight. The MIDX stores packs in lexographic order, so checking them is\nO(log n), which we do at most 'n' times.\n\n> And after we clear, \"m\" will be NULL, so we'll do it at most once. Which\n> is why you can get rid of the manual \"midx_cleared\" flag from the\n> preimage.\n\nYep. I thought briefly about passing 'm' as a parameter, but then you\nhave to worry about a dangling reference to\n'the_repository->objects->multi_pack_index' after calling\n'clear_midx_file()', so it's easier to look it up each time.\n\n> So the patch looks good to me.\n\nThanks.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"404395","messageId":"CB6B70D3-5FC6-43FE-8460-33F6CFC123E6@gmail.com","threadId":"54128","inReplyTo":"ef9186a8df0d712c2ecccbe62cb43a7abadb9c96.1598320716.git.me@ttaylorr.com","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Son Luong Ngoc","fromEmail":"sluongng@gmail.com","sentAt":"2020-08-25T07:55:22Z","receivedAt":"2020-08-25T07:55:28Z","isPatch":true,"sender":{"key":"sluongng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/26684313?v=4"},"body":"Hi Taylor,\n\nThanks for working on this.\n\n> On Aug 25, 2020, at 04:01, Taylor Blau wrote:\n> \n> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n> learned to remove a multi-pack-index file if it added or removed a pack\n> from the object store.\n> \n> This mechanism is a little over-eager, since it is only necessary to\n> drop a MIDX if 'git repack' removes a pack that the MIDX references.\n> Adding a pack outside of the MIDX does not require invalidating the\n> MIDX, and likewise for removing a pack the MIDX does not know about.\n\nI wonder if its worth to trigger write_midx_file() to update the midx instead of\njust removing MIDX?\n\nThat is already the direction we are taking in the 'maintenance' patch series\nwhenever the multi-pack-index file was deemed invalid.\n\nOr perhaps, we can check for 'core.multiPackIndex' value (which recently is \n'true' by default) and determine whether we should remove the MIDX or rewrite\nit?\n\n> \n> Teach 'git repack' to check for this by loading the MIDX, and checking\n> whether the to-be-removed pack is known to the MIDX. This requires a\n> slightly odd alternation to a test in t5319, which is explained with a\n> comment.\n> \n> Signed-off-by: Taylor Blau \n\nCheers,\nSon Luong."},{"id":"404409","messageId":"4caa4d87-d5f8-6f34-025c-0f23506a19fb@gmail.com","threadId":"54128","inReplyTo":"CB6B70D3-5FC6-43FE-8460-33F6CFC123E6@gmail.com","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-25T12:45:50Z","receivedAt":"2020-08-25T12:45:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/25/2020 3:55 AM, Son Luong Ngoc wrote:\n> Hi Taylor,\n> \n> Thanks for working on this.\n> \n>> On Aug 25, 2020, at 04:01, Taylor Blau wrote:\n>>\n>> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n>> learned to remove a multi-pack-index file if it added or removed a pack\n>> from the object store.\n>>\n>> This mechanism is a little over-eager, since it is only necessary to\n>> drop a MIDX if 'git repack' removes a pack that the MIDX references.\n>> Adding a pack outside of the MIDX does not require invalidating the\n>> MIDX, and likewise for removing a pack the MIDX does not know about.\n> \n> I wonder if its worth to trigger write_midx_file() to update the midx instead of\n> just removing MIDX?\n\nIt would be reasonable to have 'git repack' run write_midx_file() after updating\nthe pack-files, but it still needs to delete the existing multi-pack-index after\ndeleting a referenced pack, as the current multi-pack-index will be incorrect,\nleading to possibly invalid data during the write. 'git multi-pack-index expire'\ncarefully handles deleting pack-files that have become redundant.\n\nIt may be possible to change the behavior of write_midx_file() to check for the\nstate of each referenced pack-file, but it would then need to re-scan the\npack-indexes to rebuild the set of objects and which pack-files contain them.\n\n> That is already the direction we are taking in the 'maintenance' patch series\n> whenever the multi-pack-index file was deemed invalid.\n\nThe 'maintenance' builtin definitely adds new MIDX-aware maintenance tasks.\nBy performing a repack using the multi-pack-index as the starting point, we\ncan get around these issues.\n\nOne thing I've noticed by talking with Taylor about this change is that\n'git repack' is a bit like 'git gc' in that it does a LOT of things and it\ncan be hard to understand exactly when certain sub-tasks are run or not.\nThere are likely some interesting operations that could be separated into\nmaintenance tasks. For example, 'git repack -d' is very similar to\n'git maintenance run --task=loose-objects'.\n\n> Or perhaps, we can check for 'core.multiPackIndex' value (which recently is \n> 'true' by default) and determine whether we should remove the MIDX or rewrite\n> it?\n\nWe currently rewrite it during 'git repack' when GIT_TEST_MULTI_PACK_INDEX=1\nto maximize how often we have a multi-pack-index in the test suite. It would\nbe simple to update that to also check a config option. I don't think adding\nthat behavior to 'core.multiPackIndex' is a good direction, though.\n\nThanks,\n-Stolee\n"},{"id":"404410","messageId":"e7eb9fb6-f1ea-f932-efaa-7434ad809989@gmail.com","threadId":"54128","inReplyTo":"20200825023710.GA98081@syl.lan","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-25T13:14:19Z","receivedAt":"2020-08-25T13:14:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2020 10:37 PM, Taylor Blau wrote:\n> On Mon, Aug 24, 2020 at 10:26:14PM -0400, Jeff King wrote:\n>> On Mon, Aug 24, 2020 at 10:01:04PM -0400, Taylor Blau wrote:\n>>\n>>> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n>>> learned to remove a multi-pack-index file if it added or removed a pack\n>>> from the object store.\n>>>\n>>> This mechanism is a little over-eager, since it is only necessary to\n>>> drop a MIDX if 'git repack' removes a pack that the MIDX references.\n>>> Adding a pack outside of the MIDX does not require invalidating the\n>>> MIDX, and likewise for removing a pack the MIDX does not know about.\n>>\n>> Does \"git repack\" ever remove just one pack? Obviously \"git repack -ad\"\n>> or \"git repack -Ad\" is going to pack everything and delete the old\n>> packs. So I think we'd want to remove a midx there.\n>>\n>> And \"git repack -d\" I think of as deleting only loose objects that we\n>> just packed. But I guess it could also remove a pack that has now been\n>> made redundant? That seems like a rare case in practice, but I suppose\n>> is possible.\n> \n> Yeah, the patch message makes this sound more likely than it actually\n> is, which I agree is very rare. I often write 'git repack' instead of\n> 'git pack-objects' to slurp up everything loose into a new pack without\n> having to list loose objects by name.\n> \n> That's the case that I really care about here: purely adding a new pack\n> should not invalidate the existing MIDX.\n> \n>> Not exactly related to your fix, but kind of the flip side of it: would\n>> we ever need to retain a midx that mentions some packs that still exist?\n>>\n>> E.g., imagine we have a midx that points to packs A and B, and\n>> git-repack deletes B. By your logic above, we need to remove the midx\n>> because now it points to objects in B which aren't accessible. But by\n>> deleting it, could we be deleting the only thing that mentions the\n>> objects in A?\n>>\n>> I _think_ the answer is \"no\", because we never went all-in on midx and\n>> allowed deleting the matching .idx files for contained packs. So we'd\n>> still have that A.idx, and we could just use the pack as normal. But\n>> it's an interesting corner case if we ever do go in that direction.\n> \n> Agreed. Maybe a (admittedly somewhat large) #leftoverbits.\n> \n>> If you'll let me muse a bit more on midx-lifetime issues (which I've\n>> never really thought about before just now):\n>>\n>> I'm also a little curious how bad it is to have a midx whose pack has\n>> gone away. I guess we'd answer queries for \"yes, we have this object\"\n>> even if we don't, which is bad. Though in practice we'd only delete\n>> those packs if we have their objects elsewhere. And the pack code is\n>> pretty good about retrying other copies of objects that can't be\n>> accessed. Alternatively, I wonder if the midx-loading code ought to\n>> check that all of the constituent packs are available.\n>>\n>> In that line of thinking, do we even need to delete midx files if one of\n>> their packs goes away? The reading side probably ought to be able to\n>> handle that gracefully.\n> \n> I think that this is probably the right direction, although I've only\n> spend time in the MIDX code over the past couple of weeks, so I can't\n> say with authority. It seems like it would be pretty annoying, though.\n> For example, code that cares about listing all objects in a MIDX would\n> have to check first whether the pack they're in still exists before\n> emitting them. On top of that, there are more corner cases when object X\n> exists in more than one pack, but some strict subset of those packs\n> containing X have gone away.\n> \n> I don't think that it couldn't be done, though.\n> \n>> And the more interesting case is when you repack everything with \"-ad\"\n>> or similar, at which point you shouldn't even need to look up what's in\n>> the midx to see if you deleted its packs. The point of your operation is\n>> to put it all-into-one, so you know the old midx should be discarded.\n>>\n>>> Teach 'git repack' to check for this by loading the MIDX, and checking\n>>> whether the to-be-removed pack is known to the MIDX. This requires a\n>>> slightly odd alternation to a test in t5319, which is explained with a\n>>> comment.\n>>\n>> My above musings aside, this seems like an obvious improvement.\n>>\n>>> diff --git a/builtin/repack.c b/builtin/repack.c\n>>> index 04c5ceaf7e..98fac03946 100644\n>>> --- a/builtin/repack.c\n>>> +++ b/builtin/repack.c\n>>> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n>>> static void remove_redundant_pack(const char *dir_name, const char *base_name)\n>>> {\n>>> \tstruct strbuf buf = STRBUF_INIT;\n>>> -\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n>>> +\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n>>> +\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n>>> +\tif (m && midx_contains_pack(m, buf.buf))\n>>> +\t\tclear_midx_file(the_repository);\n>>> +\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n>>\n>> Makes sense. midx_contains_pack() is a binary search, so we'll spend\n>> O(n log n) effort deleting the packs (I wondered if this might be\n>> accidentally quadratic over the number of packs).\n> \n> Right. The MIDX stores packs in lexographic order, so checking them is\n> O(log n), which we do at most 'n' times.\n> \n>> And after we clear, \"m\" will be NULL, so we'll do it at most once. Which\n>> is why you can get rid of the manual \"midx_cleared\" flag from the\n>> preimage.\n> \n> Yep. I thought briefly about passing 'm' as a parameter, but then you\n> have to worry about a dangling reference to\n> 'the_repository->objects->multi_pack_index' after calling\n> 'clear_midx_file()', so it's easier to look it up each time.\n\nThe discussion in this thread matches my understanding of the\nsituation.\n\n>> So the patch looks good to me.\n\nThe code in builtin/repack.c looks good for sure. I have a quick question\nabout this new test:\n\n+test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n+\tgit multi-pack-index write &&\n+\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n+\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n+\n+\t# Write a new pack that is unknown to the multi-pack-index.\n+\tgit hash-object -w blob &&\n+\tgit pack-objects $objdir/pack/pack blob &&\n+\tHASH=$(git pack-objects $objdir/pack/pack blobs <<-EOF &&\n\t$BLOB1\n\t$BLOB2\n\tEOF\n\tHASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&\n\tHASH2=$(git pack-objects $objdir/pack/pack On 8/24/2020 10:37 PM, Taylor Blau wrote:\n> > On Mon, Aug 24, 2020 at 10:26:14PM -0400, Jeff King wrote:\n> >> On Mon, Aug 24, 2020 at 10:01:04PM -0400, Taylor Blau wrote:\n> >>\n> >>> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n> >>> learned to remove a multi-pack-index file if it added or removed a pack\n> >>> from the object store.\n> >>>\n> >>> This mechanism is a little over-eager, since it is only necessary to\n> >>> drop a MIDX if 'git repack' removes a pack that the MIDX references.\n> >>> Adding a pack outside of the MIDX does not require invalidating the\n> >>> MIDX, and likewise for removing a pack the MIDX does not know about.\n> >>\n> >> Does \"git repack\" ever remove just one pack? Obviously \"git repack -ad\"\n> >> or \"git repack -Ad\" is going to pack everything and delete the old\n> >> packs. So I think we'd want to remove a midx there.\n> >>\n> >> And \"git repack -d\" I think of as deleting only loose objects that we\n> >> just packed. But I guess it could also remove a pack that has now been\n> >> made redundant? That seems like a rare case in practice, but I suppose\n> >> is possible.\n> >\n> > Yeah, the patch message makes this sound more likely than it actually\n> > is, which I agree is very rare. I often write 'git repack' instead of\n> > 'git pack-objects' to slurp up everything loose into a new pack without\n> > having to list loose objects by name.\n> >\n> > That's the case that I really care about here: purely adding a new pack\n> > should not invalidate the existing MIDX.\n> >\n> >> Not exactly related to your fix, but kind of the flip side of it: would\n> >> we ever need to retain a midx that mentions some packs that still exist?\n> >>\n> >> E.g., imagine we have a midx that points to packs A and B, and\n> >> git-repack deletes B. By your logic above, we need to remove the midx\n> >> because now it points to objects in B which aren't accessible. But by\n> >> deleting it, could we be deleting the only thing that mentions the\n> >> objects in A?\n> >>\n> >> I _think_ the answer is \"no\", because we never went all-in on midx and\n> >> allowed deleting the matching .idx files for contained packs. So we'd\n> >> still have that A.idx, and we could just use the pack as normal. But\n> >> it's an interesting corner case if we ever do go in that direction.\n> >\n> > Agreed. Maybe a (admittedly somewhat large) #leftoverbits.\n> >\n> >> If you'll let me muse a bit more on midx-lifetime issues (which I've\n> >> never really thought about before just now):\n> >>\n> >> I'm also a little curious how bad it is to have a midx whose pack has\n> >> gone away. I guess we'd answer queries for \"yes, we have this object\"\n> >> even if we don't, which is bad. Though in practice we'd only delete\n> >> those packs if we have their objects elsewhere. And the pack code is\n> >> pretty good about retrying other copies of objects that can't be\n> >> accessed. Alternatively, I wonder if the midx-loading code ought to\n> >> check that all of the constituent packs are available.\n> >>\n> >> In that line of thinking, do we even need to delete midx files if one of\n> >> their packs goes away? The reading side probably ought to be able to\n> >> handle that gracefully.\n> >\n> > I think that this is probably the right direction, although I've only\n> > spend time in the MIDX code over the past couple of weeks, so I can't\n> > say with authority. It seems like it would be pretty annoying, though.\n> > For example, code that cares about listing all objects in a MIDX would\n> > have to check first whether the pack they're in still exists before\n> > emitting them. On top of that, there are more corner cases when object X\n> > exists in more than one pack, but some strict subset of those packs\n> > containing X have gone away.\n> >\n> > I don't think that it couldn't be done, though.\n> >\n> >> And the more interesting case is when you repack everything with \"-ad\"\n> >> or similar, at which point you shouldn't even need to look up what's in\n> >> the midx to see if you deleted its packs. The point of your operation is\n> >> to put it all-into-one, so you know the old midx should be discarded.\n> >>\n> >>> Teach 'git repack' to check for this by loading the MIDX, and checking\n> >>> whether the to-be-removed pack is known to the MIDX. This requires a\n> >>> slightly odd alternation to a test in t5319, which is explained with a\n> >>> comment.\n> >>\n> >> My above musings aside, this seems like an obvious improvement.\n> >>\n> >>> diff --git a/builtin/repack.c b/builtin/repack.c\n> >>> index 04c5ceaf7e..98fac03946 100644\n> >>> --- a/builtin/repack.c\n> >>> +++ b/builtin/repack.c\n> >>> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n> >>> static void remove_redundant_pack(const char *dir_name, const char *base_name)\n> >>> {\n> >>> \tstruct strbuf buf = STRBUF_INIT;\n> >>> -\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n> >>> +\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n> >>> +\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n> >>> +\tif (m && midx_contains_pack(m, buf.buf))\n> >>> +\t\tclear_midx_file(the_repository);\n> >>> +\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n> >>\n> >> Makes sense. midx_contains_pack() is a binary search, so we'll spend\n> >> O(n log n) effort deleting the packs (I wondered if this might be\n> >> accidentally quadratic over the number of packs).\n> >\n> > Right. The MIDX stores packs in lexographic order, so checking them is\n> > O(log n), which we do at most 'n' times.\n> >\n> >> And after we clear, \"m\" will be NULL, so we'll do it at most once. Which\n> >> is why you can get rid of the manual \"midx_cleared\" flag from the\n> >> preimage.\n> >\n> > Yep. I thought briefly about passing 'm' as a parameter, but then you\n> > have to worry about a dangling reference to\n> > 'the_repository->objects->multi_pack_index' after calling\n> > 'clear_midx_file()', so it's easier to look it up each time.\n>\n> The discussion in this thread matches my understanding of the\n> situation.\n>\n> >> So the patch looks good to me.\n>\n> The code in builtin/repack.c looks good for sure. I have a quick question\n> about this new test:\n>\n> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> +\tgit multi-pack-index write &&\n> +\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n> +\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n> +\n> +\t# Write a new pack that is unknown to the multi-pack-index.\n> +\tgit hash-object -w blob &&\n> +\tgit pack-objects $objdir/pack/pack +\n> +\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> +\ttest_cmp_bin $objdir/pack/multi-pack-index \\\n> +\t\t$objdir/pack/multi-pack-index.bak\n> +'\n> +\n>\n> You create an arbitrary blob, and then add it to a pack-file. Do we\n> know that 'git repack' is definitely creating a new pack-file that makes\n> our manually-created pack-file redundant?\n>\n> My suggestion is to have the test check itself:\n>\n> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> +\tgit multi-pack-index write &&\n> +\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n> +\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n> +\n> +\t# Write a new pack that is unknown to the multi-pack-index.\n> +\tgit hash-object -w blob &&\n> +\tHASH=$(git pack-objects $objdir/pack/pack +\n> +\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> +\ttest_cmp_bin $objdir/pack/multi-pack-index \\\n> +\t\t$objdir/pack/multi-pack-index.bak &&\n> +\ttest_path_is_missing $objdir/pack/pack-$HASH.pack\n> +'\n> +\n>\n> This test fails for me, on the 'test_path_is_missing'. Likely, the\n> blob is seen as already in a pack-file so is just pruned by 'git repack'\n> instead. I thought that perhaps we need to add a new pack ourselves that\n> overrides the small pack. Here is my attempt:\n>\n> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> \tgit multi-pack-index write &&\n> \tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n> \ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n>\n> \t# Write a new pack that is unknown to the multi-pack-index.\n> \tBLOB1=$(echo blob1 | git hash-object -w --stdin) &&\n> \tBLOB2=$(echo blob2 | git hash-object -w --stdin) &&\n> \tcat >blobs <<-EOF &&\n> \t$BLOB1\n> \t$BLOB2\n> \tEOF\n> \tHASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&\n> \tHASH2=$(git pack-objects $objdir/pack/pack \tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> \ttest_cmp_bin $objdir/pack/multi-pack-index \\\n> \t\t$objdir/pack/multi-pack-index.bak &&\n> \ttest_path_is_file $objdir/pack/pack-$HASH2.pack &&\n> \ttest_path_is_missing $objdir/pack/pack-$HASH1.pack\n> '\n>\n> However, this _still_ fails on the \"test_path_is_missing\" line, so I'm not sure\n> how to make sure your logic is tested. I saw that 'git repack' was writing\n> \"nothing new to pack\" in the output, so I also tested adding a few commits and\n> trying to force it to repack reachable data, but I cannot seem to trigger it\n> to create a new pack that overrides only one pack that is not in the MIDX.\n>\n> Likely, I just don't know how 'git rebase' works well enough to trigger this\n> behavior. But the test as-is is not testing what you want it to test.\n\nI think this case might actually be impossible to tickle in a test. I\nthought that 'git repack -d' looked for existing packs whose objects are\na subset of some new pack generated. But, it's much simpler than that:\n'-d' by itself just looks for packs that were already on disk with the\nsame SHA-1 as a new pack, and it removes the old one.\n\nNote that 'git repack' uses 'git pack-objects' internally to find\nobjects and generate a packfile. When calling 'git pack-objects', 'git\nrepack -d' passes '--all' and '--unpacked', which means that there is no\nway we'd generate a new pack with the same SHA-1 as an existing pack\nordinarily.\n\nSo, I think this case is impossible, or at least astronomically\nunlikely. What is more interesting (and untested) is that adding a _new_\npack doesn't cause us to invalidate the MIDX. Here's a patch that does\nthat:\n\n diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n index 16a1ad040e..620f2058d6 100755\n --- a/t/t5319-multi-pack-index.sh\n +++ b/t/t5319-multi-pack-index.sh\n @@ -391,18 +391,27 @@ test_expect_success 'repack removes multi-pack-index when deleting packs' '\n test_path_is_missing $objdir/pack/multi-pack-index\n '\n\n -test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n - git multi-pack-index write &&\n - cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n - test_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n -\n - # Write a new pack that is unknown to the multi-pack-index.\n - git hash-object -w blob &&\n - git pack-objects $objdir/pack/pack before &&\n +\n + cp $midx $midx.bak &&\n +\n + test_commit \"another\" &&\n + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n + ls .git/objects/pack | grep \"\\.pack$\" >after &&\n +\n + test_cmp_bin $midx.bak $midx &&\n + ! test_cmp before after\n + )\n '\n\n compare_results_with_midx \"after repack\"\n\nWhat do you think about applying this on top and then calling it a day?\n\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"404415","messageId":"20200825144515.GB7671@syl.lan","threadId":"54128","inReplyTo":"CB6B70D3-5FC6-43FE-8460-33F6CFC123E6@gmail.com","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-25T14:45:15Z","receivedAt":"2020-08-25T14:45:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 25, 2020 at 09:55:22AM +0200, Son Luong Ngoc wrote:\n> Hi Taylor,\n>\n> Thanks for working on this.\n>\n> > On Aug 25, 2020, at 04:01, Taylor Blau wrote:\n> >\n> > In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n> > learned to remove a multi-pack-index file if it added or removed a pack\n> > from the object store.\n> >\n> > This mechanism is a little over-eager, since it is only necessary to\n> > drop a MIDX if 'git repack' removes a pack that the MIDX references.\n> > Adding a pack outside of the MIDX does not require invalidating the\n> > MIDX, and likewise for removing a pack the MIDX does not know about.\n>\n> I wonder if its worth to trigger write_midx_file() to update the midx instead of\n> just removing MIDX?\n\nThere's no reason that we couldn't do this, but I don't think that it's\na very good idea, especially if the new 'git maintenance' command will\nbe able to do something like this by itself.\n\nI'm hesitant to add yet another option to 'git repack', which I have\nalways thought as a plumbing tool. That's important because callers\n(like 'git maintenance' or user scripts) can 'git multi-pack-index write\n...' after their 'git repack' to generate a new MIDX if they want one.\n\nThis becomes even trickier if 'git multi-pack-index write' were to gain\nnew options, at which point 'git repack' would *also* have to learn\nthose options to plumb them through.\n\nMaybe there's a legitimate case that I'm overlooking, but I don't think\nso. If there is, I'd be happy to come back to this, but this patch is\nreally just focused on fixing a bug.\n\n> That is already the direction we are taking in the 'maintenance' patch series\n> whenever the multi-pack-index file was deemed invalid.\n>\n> Or perhaps, we can check for 'core.multiPackIndex' value (which recently is\n> 'true' by default) and determine whether we should remove the MIDX or rewrite\n> it?\n>\n> >\n> > Teach 'git repack' to check for this by loading the MIDX, and checking\n> > whether the to-be-removed pack is known to the MIDX. This requires a\n> > slightly odd alternation to a test in t5319, which is explained with a\n> > comment.\n> >\n> > Signed-off-by: Taylor Blau \n\nThanks for your feedback.\n\n> Cheers,\n> Son Luong.\n\nThanks,\nTaylor\n"},{"id":"404417","messageId":"6a34d7ee-8c6b-8c6c-93bd-0013dccccafb@gmail.com","threadId":"54128","inReplyTo":"20200825144146.GA7671@syl.lan","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-25T15:14:41Z","receivedAt":"2020-08-25T15:14:50Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/25/2020 10:41 AM, Taylor Blau wrote:\n> On Tue, Aug 25, 2020 at 09:14:19AM -0400, Derrick Stolee wrote:\n>> The code in builtin/repack.c looks good for sure. I have a quick question\n>> about this new test:\n>>\n>> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n>> +\tgit multi-pack-index write &&\n>> +\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n>> +\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n>> +\n>> +\t# Write a new pack that is unknown to the multi-pack-index.\n>> +\tgit hash-object -w blob &&\n>> +\tgit pack-objects $objdir/pack/pack > +\n>> +\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n>> +\ttest_cmp_bin $objdir/pack/multi-pack-index \\\n>> +\t\t$objdir/pack/multi-pack-index.bak\n>> +'\n>> +\n>>\n>> You create an arbitrary blob, and then add it to a pack-file. Do we\n>> know that 'git repack' is definitely creating a new pack-file that makes\n>> our manually-created pack-file redundant?\n>>\n>> My suggestion is to have the test check itself:\n>>\n>> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n>> +\tgit multi-pack-index write &&\n>> +\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n>> +\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n>> +\n>> +\t# Write a new pack that is unknown to the multi-pack-index.\n>> +\tgit hash-object -w blob &&\n>> +\tHASH=$(git pack-objects $objdir/pack/pack > +\n>> +\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n>> +\ttest_cmp_bin $objdir/pack/multi-pack-index \\\n>> +\t\t$objdir/pack/multi-pack-index.bak &&\n>> +\ttest_path_is_missing $objdir/pack/pack-$HASH.pack\n>> +'\n>> +\n>>\n>> This test fails for me, on the 'test_path_is_missing'. Likely, the\n>> blob is seen as already in a pack-file so is just pruned by 'git repack'\n>> instead. I thought that perhaps we need to add a new pack ourselves that\n>> overrides the small pack. Here is my attempt:\n>>\n>> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n>> \tgit multi-pack-index write &&\n>> \tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n>> \ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n>>\n>> \t# Write a new pack that is unknown to the multi-pack-index.\n>> \tBLOB1=$(echo blob1 | git hash-object -w --stdin) &&\n>> \tBLOB2=$(echo blob2 | git hash-object -w --stdin) &&\n>> \tcat >blobs <<-EOF &&\n>> \t$BLOB1\n>> \t$BLOB2\n>> \tEOF\n>> \tHASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&\n>> \tHASH2=$(git pack-objects $objdir/pack/pack > \tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n>> \ttest_cmp_bin $objdir/pack/multi-pack-index \\\n>> \t\t$objdir/pack/multi-pack-index.bak &&\n>> \ttest_path_is_file $objdir/pack/pack-$HASH2.pack &&\n>> \ttest_path_is_missing $objdir/pack/pack-$HASH1.pack\n>> '\n>>\n>> However, this _still_ fails on the \"test_path_is_missing\" line, so I'm not sure\n>> how to make sure your logic is tested. I saw that 'git repack' was writing\n>> \"nothing new to pack\" in the output, so I also tested adding a few commits and\n>> trying to force it to repack reachable data, but I cannot seem to trigger it\n>> to create a new pack that overrides only one pack that is not in the MIDX.\n>>\n>> Likely, I just don't know how 'git rebase' works well enough to trigger this\n>> behavior. But the test as-is is not testing what you want it to test.\n> \n> I think this case might actually be impossible to tickle in a test. I\n> thought that 'git repack -d' looked for existing packs whose objects are\n> a subset of some new pack generated. But, it's much simpler than that:\n> '-d' by itself just looks for packs that were already on disk with the\n> same SHA-1 as a new pack, and it removes the old one.\n\nIf 'git repack' never calls remove_redundant_pack() unless we are doing\na \"full repack\", then we could simplify this logic:\n\n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n+\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n+\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n+\tif (m && midx_contains_pack(m, buf.buf))\n+\t\tclear_midx_file(the_repository);\n+\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n \tunlink_pack_path(buf.buf, 1);\n \tstrbuf_release(&buf);\n }\n\nto\n\n static void remove_redundant_pack(const char *dir_name, const char *base_name)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n+\tclear_midx_file(the_repository);\n \tunlink_pack_path(buf.buf, 1);\n \tstrbuf_release(&buf);\n }\n\nand get the same results as we are showing in these tests. This does\nmove us incrementally to a better situation: don't delete the MIDX\nif we aren't deleting pack files. But, I think we can get around it.\n\n> Note that 'git repack' uses 'git pack-objects' internally to find\n> objects and generate a packfile. When calling 'git pack-objects', 'git\n> repack -d' passes '--all' and '--unpacked', which means that there is no\n> way we'd generate a new pack with the same SHA-1 as an existing pack\n> ordinarily.\n> \n> So, I think this case is impossible, or at least astronomically\n> unlikely. What is more interesting (and untested) is that adding a _new_\n> pack doesn't cause us to invalidate the MIDX. Here's a patch that does\n> that:\n> \n> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> index 16a1ad040e..620f2058d6 100755\n> --- a/t/t5319-multi-pack-index.sh\n> +++ b/t/t5319-multi-pack-index.sh\n> @@ -391,18 +391,27 @@ test_expect_success 'repack removes multi-pack-index when deleting packs' '\n> test_path_is_missing $objdir/pack/multi-pack-index\n> '\n> \n> -test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> - git multi-pack-index write &&\n> - cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n> - test_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n> -\n> - # Write a new pack that is unknown to the multi-pack-index.\n> - git hash-object -w blob &&\n> - git pack-objects $objdir/pack/pack -\n> - GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> - test_cmp_bin $objdir/pack/multi-pack-index \\\n> - $objdir/pack/multi-pack-index.bak\n> +test_expect_success 'repack preserves multi-pack-index when creating packs' '\n> + git init preserve &&\n> + test_when_finished \"rm -fr preserve\" &&\n> + (\n> + cd preserve &&\n> + midx=.git/objects/pack/multi-pack-index &&\n> +\n> + test_commit \"initial\" &&\n> + git repack -ad &&\n> + git multi-pack-index write &&\n> + ls .git/objects/pack | grep \"\\.pack$\" >before &&\n> +\n> + cp $midx $midx.bak &&\n> +\n> + test_commit \"another\" &&\n> + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> + ls .git/objects/pack | grep \"\\.pack$\" >after &&\n> +\n> + test_cmp_bin $midx.bak $midx &&\n> + ! test_cmp before after\n> + )\n> '\n\nAfter looking at the callers to remote_redundant_pack() I noticed that it is only\ncalled after inspecting the \"names\" struct, which contains the names of the packs\nto group into a new pack-file. We can use a .keep file to preserve the pack-file(s) in\nthe MIDX but also to ensure multiple pack-files outside of the MIDX are repacked and\ndeleted. While this is very unlikely in the wild, it is definitely possible.\n\ntest_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n\tgit init preserve &&\n\ttest_when_finished \"rm -fr preserve\" &&\n\t(\n\t\tcd preserve &&\n\t\tmidx=.git/objects/pack/multi-pack-index &&\n\n\t\ttest_commit 1 &&\n\t\tHASH1=$(git pack-objects --all .git/objects/pack/pack) &&\n\t\ttouch .git/objects/pack/pack-$HASH1.keep &&\n\n\t\tcat >pack-input <<-\\EOF &&\n\t\tHEAD\n\t\t^HEAD~1\n\t\tEOF\n\n\t\ttest_commit 2 &&\n\t\tHASH2=$(git pack-objects --revs .git/objects/pack/pack On 8/25/2020 10:41 AM, Taylor Blau wrote:\n> > On Tue, Aug 25, 2020 at 09:14:19AM -0400, Derrick Stolee wrote:\n> >> The code in builtin/repack.c looks good for sure. I have a quick question\n> >> about this new test:\n> >>\n> >> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> >> +\tgit multi-pack-index write &&\n> >> +\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n> >> +\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n> >> +\n> >> +\t# Write a new pack that is unknown to the multi-pack-index.\n> >> +\tgit hash-object -w blob &&\n> >> +\tgit pack-objects $objdir/pack/pack >> +\n> >> +\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> >> +\ttest_cmp_bin $objdir/pack/multi-pack-index \\\n> >> +\t\t$objdir/pack/multi-pack-index.bak\n> >> +'\n> >> +\n> >>\n> >> You create an arbitrary blob, and then add it to a pack-file. Do we\n> >> know that 'git repack' is definitely creating a new pack-file that makes\n> >> our manually-created pack-file redundant?\n> >>\n> >> My suggestion is to have the test check itself:\n> >>\n> >> +test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> >> +\tgit multi-pack-index write &&\n> >> +\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n> >> +\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n> >> +\n> >> +\t# Write a new pack that is unknown to the multi-pack-index.\n> >> +\tgit hash-object -w blob &&\n> >> +\tHASH=$(git pack-objects $objdir/pack/pack >> +\n> >> +\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> >> +\ttest_cmp_bin $objdir/pack/multi-pack-index \\\n> >> +\t\t$objdir/pack/multi-pack-index.bak &&\n> >> +\ttest_path_is_missing $objdir/pack/pack-$HASH.pack\n> >> +'\n> >> +\n> >>\n> >> This test fails for me, on the 'test_path_is_missing'. Likely, the\n> >> blob is seen as already in a pack-file so is just pruned by 'git repack'\n> >> instead. I thought that perhaps we need to add a new pack ourselves that\n> >> overrides the small pack. Here is my attempt:\n> >>\n> >> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> >> \tgit multi-pack-index write &&\n> >> \tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n> >> \ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n> >>\n> >> \t# Write a new pack that is unknown to the multi-pack-index.\n> >> \tBLOB1=$(echo blob1 | git hash-object -w --stdin) &&\n> >> \tBLOB2=$(echo blob2 | git hash-object -w --stdin) &&\n> >> \tcat >blobs <<-EOF &&\n> >> \t$BLOB1\n> >> \t$BLOB2\n> >> \tEOF\n> >> \tHASH1=$(echo $BLOB1 | git pack-objects $objdir/pack/pack) &&\n> >> \tHASH2=$(git pack-objects $objdir/pack/pack >> \tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> >> \ttest_cmp_bin $objdir/pack/multi-pack-index \\\n> >> \t\t$objdir/pack/multi-pack-index.bak &&\n> >> \ttest_path_is_file $objdir/pack/pack-$HASH2.pack &&\n> >> \ttest_path_is_missing $objdir/pack/pack-$HASH1.pack\n> >> '\n> >>\n> >> However, this _still_ fails on the \"test_path_is_missing\" line, so I'm not sure\n> >> how to make sure your logic is tested. I saw that 'git repack' was writing\n> >> \"nothing new to pack\" in the output, so I also tested adding a few commits and\n> >> trying to force it to repack reachable data, but I cannot seem to trigger it\n> >> to create a new pack that overrides only one pack that is not in the MIDX.\n> >>\n> >> Likely, I just don't know how 'git rebase' works well enough to trigger this\n> >> behavior. But the test as-is is not testing what you want it to test.\n> >\n> > I think this case might actually be impossible to tickle in a test. I\n> > thought that 'git repack -d' looked for existing packs whose objects are\n> > a subset of some new pack generated. But, it's much simpler than that:\n> > '-d' by itself just looks for packs that were already on disk with the\n> > same SHA-1 as a new pack, and it removes the old one.\n>\n> If 'git repack' never calls remove_redundant_pack() unless we are doing\n> a \"full repack\", then we could simplify this logic:\n>\n> static void remove_redundant_pack(const char *dir_name, const char *base_name)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> -\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n> +\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n> +\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n> +\tif (m && midx_contains_pack(m, buf.buf))\n> +\t\tclear_midx_file(the_repository);\n> +\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n> \tunlink_pack_path(buf.buf, 1);\n> \tstrbuf_release(&buf);\n> }\n>\n> to\n>\n> static void remove_redundant_pack(const char *dir_name, const char *base_name)\n> {\n> \tstruct strbuf buf = STRBUF_INIT;\n> \tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n> +\tclear_midx_file(the_repository);\n> \tunlink_pack_path(buf.buf, 1);\n> \tstrbuf_release(&buf);\n> }\n>\n> and get the same results as we are showing in these tests. This does\n> move us incrementally to a better situation: don't delete the MIDX\n> if we aren't deleting pack files. But, I think we can get around it.\n\nMakes sense, but reading your whole email we are better off leaving this\nas-is and changing the test to exercise it more often.\n\n> > Note that 'git repack' uses 'git pack-objects' internally to find\n> > objects and generate a packfile. When calling 'git pack-objects', 'git\n> > repack -d' passes '--all' and '--unpacked', which means that there is no\n> > way we'd generate a new pack with the same SHA-1 as an existing pack\n> > ordinarily.\n> >\n> > So, I think this case is impossible, or at least astronomically\n> > unlikely. What is more interesting (and untested) is that adding a _new_\n> > pack doesn't cause us to invalidate the MIDX. Here's a patch that does\n> > that:\n> >\n> > diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\n> > index 16a1ad040e..620f2058d6 100755\n> > --- a/t/t5319-multi-pack-index.sh\n> > +++ b/t/t5319-multi-pack-index.sh\n> > @@ -391,18 +391,27 @@ test_expect_success 'repack removes multi-pack-index when deleting packs' '\n> > test_path_is_missing $objdir/pack/multi-pack-index\n> > '\n> >\n> > -test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> > - git multi-pack-index write &&\n> > - cp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n> > - test_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n> > -\n> > - # Write a new pack that is unknown to the multi-pack-index.\n> > - git hash-object -w blob &&\n> > - git pack-objects $objdir/pack/pack > -\n> > - GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> > - test_cmp_bin $objdir/pack/multi-pack-index \\\n> > - $objdir/pack/multi-pack-index.bak\n> > +test_expect_success 'repack preserves multi-pack-index when creating packs' '\n> > + git init preserve &&\n> > + test_when_finished \"rm -fr preserve\" &&\n> > + (\n> > + cd preserve &&\n> > + midx=.git/objects/pack/multi-pack-index &&\n> > +\n> > + test_commit \"initial\" &&\n> > + git repack -ad &&\n> > + git multi-pack-index write &&\n> > + ls .git/objects/pack | grep \"\\.pack$\" >before &&\n> > +\n> > + cp $midx $midx.bak &&\n> > +\n> > + test_commit \"another\" &&\n> > + GIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -d &&\n> > + ls .git/objects/pack | grep \"\\.pack$\" >after &&\n> > +\n> > + test_cmp_bin $midx.bak $midx &&\n> > + ! test_cmp before after\n> > + )\n> > '\n>\n> After looking at the callers to remote_redundant_pack() I noticed that it is only\n> called after inspecting the \"names\" struct, which contains the names of the packs\n> to group into a new pack-file. We can use a .keep file to preserve the pack-file(s) in\n> the MIDX but also to ensure multiple pack-files outside of the MIDX are repacked and\n> deleted. While this is very unlikely in the wild, it is definitely possible.\n\nGreat idea.\n\n> test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> \tgit init preserve &&\n> \ttest_when_finished \"rm -fr preserve\" &&\n> \t(\n> \t\tcd preserve &&\n> \t\tmidx=.git/objects/pack/multi-pack-index &&\n>\n> \t\ttest_commit 1 &&\n> \t\tHASH1=$(git pack-objects --all .git/objects/pack/pack) &&\n> \t\ttouch .git/objects/pack/pack-$HASH1.keep &&\n>\n> \t\tcat >pack-input <<-\\EOF &&\n\nEscaping the heredoc shouldn't be necessary, so this can be written\ninstead as '<<-EOF'.\n\n> \t\tHEAD\n> \t\t^HEAD~1\n> \t\tEOF\n>\n> \t\ttest_commit 2 &&\n> \t\tHASH2=$(git pack-objects --revs .git/objects/pack/pack \t\ttouch .git/objects/pack/pack-$HASH2.keep &&\n>\n> \t\tgit multi-pack-index write &&\n> \t\tcp $midx $midx.bak &&\n>\n> \t\ttest_commit 3 &&\n> \t\tHASH3=$(git pack-objects --revs .git/objects/pack/pack \n> \t\ttest_commit 4 &&\n> \t\tHASH4=$(git pack-objects --revs .git/objects/pack/pack \n> \t\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -ad &&\n> \t\ttest_path_is_file .git/objects/pack/pack-$HASH1.pack &&\n> \t\ttest_path_is_file .git/objects/pack/pack-$HASH2.pack &&\n> \t\ttest_path_is_missing .git/objects/pack/pack-$HASH3.pack &&\n> \t\ttest_path_is_missing .git/objects/pack/pack-$HASH4.pack\n\n...and we should check that 'test_cmp $midx.bak $midx' is clean, i.e.,\nthat we didn't touch the MIDX.\n\n> )\n> '\n>\n> I believe this checks your condition properly enough.\n\nOtherwise, I think that this test looks great. Thanks for suggesting\nit. I'll send a new patch now...\n\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"404421","messageId":"xmqqtuwq1zux.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200825022614.GA1391422@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T15:58:46Z","receivedAt":"2020-08-25T15:58:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Does \"git repack\" ever remove just one pack? Obviously \"git repack -ad\"\n> or \"git repack -Ad\" is going to pack everything and delete the old\n> packs. So I think we'd want to remove a midx there.\n\nAFAIK, the pack-redundant subcommand is used by nobody in-tree, so\nnobody is doing \"there are three packfiles, but all the objects in\none of them are contained in the other two, so let's remove that\nredundant one\".\n\n> And \"git repack -d\" I think of as deleting only loose objects that we\n> just packed. But I guess it could also remove a pack that has now been\n> made redundant? That seems like a rare case in practice, but I suppose\n> is possible.\n\nMeaning it can become reality? Yes. Or it already can happen? I\ndoubt it.\n\n> E.g., imagine we have a midx that points to packs A and B, and\n> git-repack deletes B. By your logic above, we need to remove the midx\n> because now it points to objects in B which aren't accessible. But by\n> deleting it, could we be deleting the only thing that mentions the\n> objects in A?\n>\n> I _think_ the answer is \"no\", because we never went all-in on midx and\n> allowed deleting the matching .idx files for contained packs.\n\nYeah, it has been my assumption that that part of the design would\nnever change.\n\n> I'm also a little curious how bad it is to have a midx whose pack has\n> gone away. I guess we'd answer queries for \"yes, we have this object\"\n> even if we don't, which is bad. Though in practice we'd only delete\n> those packs if we have their objects elsewhere.\n\nHmph, object O used to be in pack A and pack B, midx points at the\ncopy in pack B but we deleted it because the pack is redundant.\nNow, midx says O exists, but midx cannot be used to retrieve O. We\nneed to ask A.idx about O to locate it.\n\nThat sounds brittle. I am not sure our error fallback is all that\npatient.\n\n\n> And the pack code is\n> pretty good about retrying other copies of objects that can't be\n> accessed. Alternatively, I wonder if the midx-loading code ought to\n> check that all of the constituent packs are available.\n>\n> In that line of thinking, do we even need to delete midx files if one of\n> their packs goes away? The reading side probably ought to be able to\n> handle that gracefully.\n\nBut at that point, is there even a point to have that midx file that\nknows about objects (so it can answer has_object()? correctly and\nquickly) but does not know the correct location of half of the objects?\nInstead of going directly to A.idx to locate O, we need to go to midx\nto learn the location of O in B (which no longer exists), and then\nfall back to it, that is a pure overhead.\n\n> And the more interesting case is when you repack everything with \"-ad\"\n> or similar, at which point you shouldn't even need to look up what's in\n> the midx to see if you deleted its packs. The point of your operation is\n> to put it all-into-one, so you know the old midx should be discarded.\n\nOld one, yes. Do we need to have the new one in that case?\n\n>> Teach 'git repack' to check for this by loading the MIDX, and checking\n>> whether the to-be-removed pack is known to the MIDX. This requires a\n>> slightly odd alternation to a test in t5319, which is explained with a\n>> comment.\n>\n> My above musings aside, this seems like an obvious improvement.\n\nYes.\n\n>> diff --git a/builtin/repack.c b/builtin/repack.c\n>> index 04c5ceaf7e..98fac03946 100644\n>> --- a/builtin/repack.c\n>> +++ b/builtin/repack.c\n>> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n>> static void remove_redundant_pack(const char *dir_name, const char *base_name)\n>> {\n>> \tstruct strbuf buf = STRBUF_INIT;\n>> -\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n>> +\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n>> +\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n>> +\tif (m && midx_contains_pack(m, buf.buf))\n>> +\t\tclear_midx_file(the_repository);\n>> +\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n>\n> Makes sense. midx_contains_pack() is a binary search, so we'll spend\n> O(n log n) effort deleting the packs (I wondered if this might be\n> accidentally quadratic over the number of packs).\n>\n> And after we clear, \"m\" will be NULL, so we'll do it at most once. Which\n> is why you can get rid of the manual \"midx_cleared\" flag from the\n> preimage.\n>\n> So the patch looks good to me.\n\nThanks.\n"},{"id":"404423","messageId":"87a3b7a5a2f091e2a23c163a7d86effbbbedfa3a.1598371475.git.me@ttaylorr.com","threadId":"54128","inReplyTo":"20200825144515.GB7671@syl.lan","subject":"[PATCH v2] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-25T16:04:36Z","receivedAt":"2020-08-25T16:04:46Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\nlearned to remove a multi-pack-index file if it added or removed a pack\nfrom the object store.\n\nThis mechanism is a little over-eager, since it is only necessary to\ndrop a MIDX if 'git repack' removes a pack that the MIDX references.\nAdding a pack outside of the MIDX does not require invalidating the\nMIDX, and likewise for removing a pack the MIDX does not know about.\n\nTeach 'git repack' to check for this by loading the MIDX, and checking\nwhether the to-be-removed pack is known to the MIDX. This requires a\nslightly odd alternation to a test in t5319, which is explained with a\ncomment. A new test is added to show that the MIDX is left alone when\nboth packs known to it are marked as .keep, but two packs unknown to it\nare removed and combined into one new pack.\n\nHelped-by: Derrick Stolee \nSigned-off-by: Taylor Blau \n---\nRange-diff against v1:\n1: ef9186a8df ! 1: 87a3b7a5a2 builtin/repack.c: invalidate MIDX only when necessary\n @@ Commit message\n Teach 'git repack' to check for this by loading the MIDX, and checking\n whether the to-be-removed pack is known to the MIDX. This requires a\n slightly odd alternation to a test in t5319, which is explained with a\n - comment.\n + comment. A new test is added to show that the MIDX is left alone when\n + both packs known to it are marked as .keep, but two packs unknown to it\n + are removed and combined into one new pack.\n \n + Helped-by: Derrick Stolee \n Signed-off-by: Taylor Blau \n \n ## builtin/repack.c ##\n @@ t/t5319-multi-pack-index.sh: test_expect_success 'repack with the --no-progress\n \ttest_path_is_missing $objdir/pack/multi-pack-index\n '\n \n -+test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n -+\tgit multi-pack-index write &&\n -+\tcp $objdir/pack/multi-pack-index $objdir/pack/multi-pack-index.bak &&\n -+\ttest_when_finished \"rm -f $objdir/pack/multi-pack-index.bak\" &&\n -+\n -+\t# Write a new pack that is unknown to the multi-pack-index.\n -+\tgit hash-object -w blob &&\n -+\tgit pack-objects $objdir/pack/pack pack-input <<-EOF &&\n ++\t\tHEAD\n ++\t\t^HEAD~1\n ++\t\tEOF\n ++\t\ttest_commit 3 &&\n ++\t\tpack3=$(git pack-objects --revs $packdir/pack string, exts[ext].name);\n \t\t\tif (!file_exists(fname)) {\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 43b1b5b2af..f340b376bc 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -382,12 +382,52 @@ test_expect_success 'repack with the --no-progress option' '\n \ttest_line_count = 0 err\n '\n \n-test_expect_success 'repack removes multi-pack-index' '\n+test_expect_success 'repack removes multi-pack-index when deleting packs' '\n \ttest_path_is_file $objdir/pack/multi-pack-index &&\n-\tGIT_TEST_MULTI_PACK_INDEX=0 git repack -adf &&\n+\t# Set GIT_TEST_MULTI_PACK_INDEX to 0 to avoid writing a new\n+\t# multi-pack-index after repacking, but set \"core.multiPackIndex\" to\n+\t# true so that \"git repack\" can read the existing MIDX.\n+\tGIT_TEST_MULTI_PACK_INDEX=0 git -c core.multiPackIndex repack -adf &&\n \ttest_path_is_missing $objdir/pack/multi-pack-index\n '\n \n+test_expect_success 'repack preserves multi-pack-index when creating packs' '\n+\tgit init preserve &&\n+\ttest_when_finished \"rm -fr preserve\" &&\n+\t(\n+\t\tcd preserve &&\n+\t\tpackdir=.git/objects/pack &&\n+\t\tmidx=$packdir/multi-pack-index &&\n+\n+\t\ttest_commit 1 &&\n+\t\tpack1=$(git pack-objects --all $packdir/pack) &&\n+\t\ttouch $packdir/pack-$pack1.keep &&\n+\t\ttest_commit 2 &&\n+\t\tpack2=$(git pack-objects --revs $packdir/pack) &&\n+\t\ttouch $packdir/pack-$pack2.keep &&\n+\n+\t\tgit multi-pack-index write &&\n+\t\tcp $midx $midx.bak &&\n+\n+\t\tcat >pack-input <<-EOF &&\n+\t\tHEAD\n+\t\t^HEAD~1\n+\t\tEOF\n+\t\ttest_commit 3 &&\n+\t\tpack3=$(git pack-objects --revs $packdir/pack Jeff King writes:\n>\n> > Does \"git repack\" ever remove just one pack? Obviously \"git repack -ad\"\n> > or \"git repack -Ad\" is going to pack everything and delete the old\n> > packs. So I think we'd want to remove a midx there.\n>\n> AFAIK, the pack-redundant subcommand is used by nobody in-tree, so\n> nobody is doing \"there are three packfiles, but all the objects in\n> one of them are contained in the other two, so let's remove that\n> redundant one\".\n\nNot AFAICT either.\n\n> > And \"git repack -d\" I think of as deleting only loose objects that we\n> > just packed. But I guess it could also remove a pack that has now been\n> > made redundant? That seems like a rare case in practice, but I suppose\n> > is possible.\n>\n> Meaning it can become reality? Yes. Or it already can happen? I\n> doubt it.\n>\n> > E.g., imagine we have a midx that points to packs A and B, and\n> > git-repack deletes B. By your logic above, we need to remove the midx\n> > because now it points to objects in B which aren't accessible. But by\n> > deleting it, could we be deleting the only thing that mentions the\n> > objects in A?\n> >\n> > I _think_ the answer is \"no\", because we never went all-in on midx and\n> > allowed deleting the matching .idx files for contained packs.\n>\n> Yeah, it has been my assumption that that part of the design would\n> never change.\n>\n> > I'm also a little curious how bad it is to have a midx whose pack has\n> > gone away. I guess we'd answer queries for \"yes, we have this object\"\n> > even if we don't, which is bad. Though in practice we'd only delete\n> > those packs if we have their objects elsewhere.\n>\n> Hmph, object O used to be in pack A and pack B, midx points at the\n> copy in pack B but we deleted it because the pack is redundant.\n> Now, midx says O exists, but midx cannot be used to retrieve O. We\n> need to ask A.idx about O to locate it.\n>\n> That sounds brittle. I am not sure our error fallback is all that\n> patient.\n\nMe either. Like I said, I think that all of this is possible at least in\ntheory, but in practice it may be somewhat annoying in cases like these.\n\n>\n> > And the pack code is\n> > pretty good about retrying other copies of objects that can't be\n> > accessed. Alternatively, I wonder if the midx-loading code ought to\n> > check that all of the constituent packs are available.\n> >\n> > In that line of thinking, do we even need to delete midx files if one of\n> > their packs goes away? The reading side probably ought to be able to\n> > handle that gracefully.\n>\n> But at that point, is there even a point to have that midx file that\n> knows about objects (so it can answer has_object()? correctly and\n> quickly) but does not know the correct location of half of the objects?\n> Instead of going directly to A.idx to locate O, we need to go to midx\n> to learn the location of O in B (which no longer exists), and then\n> fall back to it, that is a pure overhead.\n\nWell put.\n\n> > And the more interesting case is when you repack everything with \"-ad\"\n> > or similar, at which point you shouldn't even need to look up what's in\n> > the midx to see if you deleted its packs. The point of your operation is\n> > to put it all-into-one, so you know the old midx should be discarded.\n>\n> Old one, yes. Do we need to have the new one in that case?\n>\n> >> Teach 'git repack' to check for this by loading the MIDX, and checking\n> >> whether the to-be-removed pack is known to the MIDX. This requires a\n> >> slightly odd alternation to a test in t5319, which is explained with a\n> >> comment.\n> >\n> > My above musings aside, this seems like an obvious improvement.\n>\n> Yes.\n>\n> >> diff --git a/builtin/repack.c b/builtin/repack.c\n> >> index 04c5ceaf7e..98fac03946 100644\n> >> --- a/builtin/repack.c\n> >> +++ b/builtin/repack.c\n> >> @@ -133,7 +133,11 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list,\n> >> static void remove_redundant_pack(const char *dir_name, const char *base_name)\n> >> {\n> >> \tstruct strbuf buf = STRBUF_INIT;\n> >> -\tstrbuf_addf(&buf, \"%s/%s.pack\", dir_name, base_name);\n> >> +\tstruct multi_pack_index *m = get_multi_pack_index(the_repository);\n> >> +\tstrbuf_addf(&buf, \"%s.pack\", base_name);\n> >> +\tif (m && midx_contains_pack(m, buf.buf))\n> >> +\t\tclear_midx_file(the_repository);\n> >> +\tstrbuf_insertf(&buf, 0, \"%s/\", dir_name);\n> >\n> > Makes sense. midx_contains_pack() is a binary search, so we'll spend\n> > O(n log n) effort deleting the packs (I wondered if this might be\n> > accidentally quadratic over the number of packs).\n> >\n> > And after we clear, \"m\" will be NULL, so we'll do it at most once. Which\n> > is why you can get rid of the manual \"midx_cleared\" flag from the\n> > preimage.\n> >\n> > So the patch looks good to me.\n>\n> Thanks.\n\nThanks, both. If you're ready, let's use the version in [1] for\nqueueing.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/87a3b7a5a2f091e2a23c163a7d86effbbbedfa3a.1598371475.git.me@ttaylorr.com/\n"},{"id":"404426","messageId":"c78b3108-b760-d252-9428-6f03549fea11@gmail.com","threadId":"54128","inReplyTo":"xmqqtuwq1zux.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-25T16:18:42Z","receivedAt":"2020-08-25T16:18:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/25/2020 11:58 AM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> Does \"git repack\" ever remove just one pack? Obviously \"git repack -ad\"\n>> or \"git repack -Ad\" is going to pack everything and delete the old\n>> packs. So I think we'd want to remove a midx there.\n> \n> AFAIK, the pack-redundant subcommand is used by nobody in-tree, so\n> nobody is doing \"there are three packfiles, but all the objects in\n> one of them are contained in the other two, so let's remove that\n> redundant one\".\n> \n>> And \"git repack -d\" I think of as deleting only loose objects that we\n>> just packed. But I guess it could also remove a pack that has now been\n>> made redundant? That seems like a rare case in practice, but I suppose\n>> is possible.\n> \n> Meaning it can become reality? Yes. Or it already can happen? I\n> doubt it.\n> \n>> E.g., imagine we have a midx that points to packs A and B, and\n>> git-repack deletes B. By your logic above, we need to remove the midx\n>> because now it points to objects in B which aren't accessible. But by\n>> deleting it, could we be deleting the only thing that mentions the\n>> objects in A?\n>>\n>> I _think_ the answer is \"no\", because we never went all-in on midx and\n>> allowed deleting the matching .idx files for contained packs.\n> \n> Yeah, it has been my assumption that that part of the design would\n> never change.\n\nYes, we should intend to keep the .idx files around forever even when\nusing the multi-pack-index. The duplicated data overhead is not typically\na real problem.\n\nThe one caveat I would consider is if we wanted to let a multi-pack-index\ncover thin packs, but that would be a substantial change to the object\ndatabase that I do not plan to tackle any time soon.\n\n>> I'm also a little curious how bad it is to have a midx whose pack has\n>> gone away. I guess we'd answer queries for \"yes, we have this object\"\n>> even if we don't, which is bad. Though in practice we'd only delete\n>> those packs if we have their objects elsewhere.\n> \n> Hmph, object O used to be in pack A and pack B, midx points at the\n> copy in pack B but we deleted it because the pack is redundant.\n> Now, midx says O exists, but midx cannot be used to retrieve O. We\n> need to ask A.idx about O to locate it.\n> \n> That sounds brittle. I am not sure our error fallback is all that\n> patient.\n\nThe best I can see is that prepare_midx_pack() will return 1 if the\npack no longer exists, and this would cause a die(\"error preparing\npackfile from multi-pack-index\") in nth_midxed_pack_entry(), killing\nthe following stack trace:\n\n+ find_pack_entry():packfile.c\n + fill_midx_entry():midx.c\n + nth_midxed_pack_entry():midx.c\n\nPerhaps that die() is a bit over-zealous since we could return 1\nthrough this stack and find_pack_entry() could continue searching\nfor the object in the packed_git list. However, it could start\nreturning false _negatives_ if there were duplicates of the object\nin the multi-pack-index but only the latest copy was deleted (and\nthe object does not appear in a pack-file outside of the multi-\npack-index).\n\nThanks,\n-Stolee\n"},{"id":"404427","messageId":"20200825164721.GA1414394@coredump.intra.peff.net","threadId":"54128","inReplyTo":"20200825144515.GB7671@syl.lan","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T16:47:21Z","receivedAt":"2020-08-25T16:47:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 10:45:15AM -0400, Taylor Blau wrote:\n\n> > I wonder if its worth to trigger write_midx_file() to update the midx instead of\n> > just removing MIDX?\n> \n> There's no reason that we couldn't do this, but I don't think that it's\n> a very good idea, especially if the new 'git maintenance' command will\n> be able to do something like this by itself.\n> \n> I'm hesitant to add yet another option to 'git repack', which I have\n> always thought as a plumbing tool. That's important because callers\n> (like 'git maintenance' or user scripts) can 'git multi-pack-index write\n> ...' after their 'git repack' to generate a new MIDX if they want one.\n\nIt may be worth thinking a bit about atomicity here, though. Rather than\nseparate delete and write steps, would somebody want a sequence like:\n\n - we have a midx with packs A, B, C\n\n - we find out that pack C is redundant and want to drop it\n\n - we create a new midx with A and B in a tempfile\n\n - we atomically rename the new midx over the old\n\n - we delete pack C\n\nA simultaneous reader always sees a consistent midx. Whereas in a\ndelete-then-rewrite scenario there is a moment where there is no midx,\nand they'd fall back to reading the individual idx files.\n\nIt may not matter all that much, though, for two reasons:\n\n - reading individual idx files should still give a correct answer.\n It just may be a bit slower.\n\n - even with an atomic replacement, I think caching on the reader side\n may cause interesting effects. For instance, if a reader process\n opens the old midx, it will generally cache that knowledge in memory\n (including mmap the content). So even after the replacement and\n deletion of C, it may still try to use the old midx that references\n C.\n\nIf we do care, though, that implies some cooperation between the\ndeletion process and the midx code. Perhaps it even argues that repack\nshould refuse to delete such a single pack at all, since it _isn't_\nredundant. It's part of a midx, and the caller should rewrite the midx\nfirst itself, and _then_ look for redundant packs.\n\n-Peff\n"},{"id":"404428","messageId":"20200825165642.GB1414394@coredump.intra.peff.net","threadId":"54128","inReplyTo":"20200825154224.GA9116@syl.lan","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T16:56:42Z","receivedAt":"2020-08-25T16:56:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 11:42:24AM -0400, Taylor Blau wrote:\n\n> > test_expect_success 'repack preserves multi-pack-index when deleting unknown packs' '\n> > \tgit init preserve &&\n> > \ttest_when_finished \"rm -fr preserve\" &&\n> > \t(\n> > \t\tcd preserve &&\n> > \t\tmidx=.git/objects/pack/multi-pack-index &&\n> >\n> > \t\ttest_commit 1 &&\n> > \t\tHASH1=$(git pack-objects --all .git/objects/pack/pack) &&\n> > \t\ttouch .git/objects/pack/pack-$HASH1.keep &&\n> >\n> > \t\tcat >pack-input <<-\\EOF &&\n> \n> Escaping the heredoc shouldn't be necessary, so this can be written\n> instead as '<<-EOF'.\n\nWe usually go the opposite way: if a here-doc doesn't need\ninterpolation, then we prefer to mark it as such to avoid surprising\nsomebody who adds lines later (and might need quoting).\n\nCertainly you can argue that least-surprise would be in the other\ndirection (i.e., people expect to interpolate by default, and you\nsurprise anybody adding a variable reference), but for Git's test suite,\nthe convention usually runs the other way.\n\n-Peff\n"},{"id":"404429","messageId":"45921233-ac6c-05f2-e108-0ab2aeb56104@gmail.com","threadId":"54128","inReplyTo":"20200825164721.GA1414394@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-25T17:10:13Z","receivedAt":"2020-08-25T17:10:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/25/2020 12:47 PM, Jeff King wrote:\n> It may be worth thinking a bit about atomicity here, though. Rather than\n> separate delete and write steps, would somebody want a sequence like:\n\n...\n> If we do care, though, that implies some cooperation between the\n> deletion process and the midx code. Perhaps it even argues that repack\n> should refuse to delete such a single pack at all, since it _isn't_\n> redundant. It's part of a midx, and the caller should rewrite the midx\n> first itself, and _then_ look for redundant packs.\n\nIt is worth noting that we _do_ have a way to integrate the delete and\nwrite code using 'git multi-pack-index expire'. One way to resolve this\natomicity would be to do the following inside the repack command:\n\n 1. Create and index the new pack.\n 2. git multi-pack-index write\n 3. git multi-pack-index expire\n\nWhile this _mostly_ works, we still have issues around a concurrent\nprocess opening a multi-pack-index before step (2) and trying to\nread from the pack-file deleted in step (3). For this reason, the\n'incremental-repack' task in the maintenance builtin series [1] runs\n'expire' then 'repack' (and not the opposite order). To be completely\nsafe, we'd want to make sure that no Git command that started before\nthe 'repack' command is still running when we run 'expire'. In practice,\nit suffices to run the 'incremental-repack' step at most once a day.\n\n[1] https://lore.kernel.org/git/a8d956dad6b3a81d0f1b63cbd48f36a5e1195ae8.1597760730.git.gitgitgadget@gmail.com/\n\nThis discussion points out a big reason why the multi-pack-index\nwasn't fully integrated with 'git repack': it can be tricky. Using\na repacking strategy that is focused on the multi-pack-index is\nless tricky.\n\nI still think that this patch is moving us in a better direction.\n\nI'm making note of these possible improvements:\n\n1. Have 'git repack' integrate with 'git multi-pack-index expire'\n when deleting pack-files after creating a new one (if a\n multi-pack-index exists).\n\n2. Handle \"missing packs\" referenced by the multi-pack-index more\n gracefully, likely by disabling the multi-pack-index and\n calling reprepare_packed_git().\n\nThanks,\n-Stolee\n\n\n"},{"id":"404430","messageId":"20200825172214.GC1414394@coredump.intra.peff.net","threadId":"54128","inReplyTo":"xmqqtuwq1zux.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T17:22:14Z","receivedAt":"2020-08-25T17:22:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 08:58:46AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Does \"git repack\" ever remove just one pack? Obviously \"git repack -ad\"\n> > or \"git repack -Ad\" is going to pack everything and delete the old\n> > packs. So I think we'd want to remove a midx there.\n> \n> AFAIK, the pack-redundant subcommand is used by nobody in-tree, so\n> nobody is doing \"there are three packfiles, but all the objects in\n> one of them are contained in the other two, so let's remove that\n> redundant one\".\n\nOK, that's the part I was missing. The discussion here and the statement\nfrom git-repack(1):\n\n -d\n After packing, if the newly created packs make some existing packs\n redundant, remove the redundant packs. Also run git prune-packed\n to remove redundant loose object files.\n\nmade me think that it was running pack-redundant. But it doesn't seem\nto. It looks like we stopped doing so in 6ed64058e1 (git-repack: do not\ndo complex redundancy check., 2005-11-19).\n\nAs an aside, we tried using pack-redundant at GitHub several years ago\nfor dropping packs that were replicated in alternates storage. It\nperforms very poorly (quadratically, perhaps?) to the point that we\nfound it unusable, and I implemented a \"local-redundant\" command to do\nthe same thing more quickly. We ended up dropping that as we now migrate\npacks wholesale (rather than via fetch), so I never upstreamed it.\n\nI mention that here to warn people away from pack-redundant (I wondered\nat the time why nobody had noticed its terrible performance, but now I\nthink the answer is just that nobody ever runs it). I wonder if we\nshould even consider deprecating and removing it.\n\nI'm also happy to share local-redundant if anybody is interested (though\nas I've mentioned elsewhere, I think the larger scheme it was part of it\nalso performed poorly and isn't worth pursuing).\n\n> > And \"git repack -d\" I think of as deleting only loose objects that we\n> > just packed. But I guess it could also remove a pack that has now been\n> > made redundant? That seems like a rare case in practice, but I suppose\n> > is possible.\n> \n> Meaning it can become reality? Yes. Or it already can happen? I\n> doubt it.\n\nI thought the latter, but after this thread I agree that it can't.\n\n> > I'm also a little curious how bad it is to have a midx whose pack has\n> > gone away. I guess we'd answer queries for \"yes, we have this object\"\n> > even if we don't, which is bad. Though in practice we'd only delete\n> > those packs if we have their objects elsewhere.\n> \n> Hmph, object O used to be in pack A and pack B, midx points at the\n> copy in pack B but we deleted it because the pack is redundant.\n> Now, midx says O exists, but midx cannot be used to retrieve O. We\n> need to ask A.idx about O to locate it.\n> \n> That sounds brittle. I am not sure our error fallback is all that\n> patient.\n\nIt has to be that patient even without a midx, I think. We might open\nA.idx and mmap its contents, without opening the matching A.pack (or we\nmight open it and later close it due to memory or descriptor\nconstraints). And we have two layers there:\n\n - when object_info_extended sees a bad return from\n packed_object_info(), it will mark the entry as bad and recurse,\n giving us an opportunity to find another one\n\n - when we can't find the object at all (e.g., because we marked that\n particular pack's version as inaccessible), we'll hit the\n reprepare_packed_git() path and look for a new idx\n\nThose same things should be kicking in for midx lookups, too (I didn't\ntest them, but from a cursory look at the organization of the code, I\nthink they will).\n\n> > In that line of thinking, do we even need to delete midx files if one of\n> > their packs goes away? The reading side probably ought to be able to\n> > handle that gracefully.\n> \n> But at that point, is there even a point to have that midx file that\n> knows about objects (so it can answer has_object()? correctly and\n> quickly) but does not know the correct location of half of the objects?\n> Instead of going directly to A.idx to locate O, we need to go to midx\n> to learn the location of O in B (which no longer exists), and then\n> fall back to it, that is a pure overhead.\n\nIt is overhead for sure. But my thinking was that you're trading one\nefficiency for another:\n\n - the midx may incur an extra lookup for objects in the redundant\n pack, but that pack may be a small portion of what the midx indexes\n (and this is likely in the usual case that you have one big\n cumulative pack and many recently-created smaller ones; the big one\n will never be made redundant and dominates the object count of the\n midx)\n\n - by keeping the midx, you're improving lookup speed for all of the\n other objects, which don't have to hunt through separate pack idx\n files\n\nSo my guess is that it would usually be a win. Though really the correct\nanswer is: if you are mucking with packs you should just generate a new\nmidx (or you should pack all-into-one, which generates a single idx\naccomplishing the same thing).\n\n> > And the more interesting case is when you repack everything with \"-ad\"\n> > or similar, at which point you shouldn't even need to look up what's in\n> > the midx to see if you deleted its packs. The point of your operation is\n> > to put it all-into-one, so you know the old midx should be discarded.\n> \n> Old one, yes. Do we need to have the new one in that case?\n\nYou shouldn't need one since you'd only have a single pack now.\n\n-Peff\n"},{"id":"404431","messageId":"20200825172901.GD1414394@coredump.intra.peff.net","threadId":"54128","inReplyTo":"45921233-ac6c-05f2-e108-0ab2aeb56104@gmail.com","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T17:29:01Z","receivedAt":"2020-08-25T17:29:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 01:10:13PM -0400, Derrick Stolee wrote:\n\n> > If we do care, though, that implies some cooperation between the\n> > deletion process and the midx code. Perhaps it even argues that repack\n> > should refuse to delete such a single pack at all, since it _isn't_\n> > redundant. It's part of a midx, and the caller should rewrite the midx\n> > first itself, and _then_ look for redundant packs.\n> \n> It is worth noting that we _do_ have a way to integrate the delete and\n> write code using 'git multi-pack-index expire'. One way to resolve this\n> atomicity would be to do the following inside the repack command:\n> \n> 1. Create and index the new pack.\n> 2. git multi-pack-index write\n> 3. git multi-pack-index expire\n\nGiven that discussion elsewhere points to git-repack only really\ndeleting packs in all-in-one mode (and not ever a single pack), it seems\nlike we can really be much simpler here. If we're not deleting packs via\nall-in-one, there's no need to touch the midx at all. If we are, then\nit's reasonable to delete the midx immediately (after having written our\nnew pack but before deleting) since our new single pack idx is as good\nor better.\n\nI.e., drop step 2 above, and make step 3 just clear_midx_file(). Which\nis roughly what the code does now, isn't it? Or is there some reason\nthat \"expire\" is more interesting than just clearing?\n\nAnd if anybody does want to drop single packs, etc, they can do so by\ngenerating a sensible midx separately from the repack operation (and\nprobably doing so before dropping the packs for atomicity).\n\n-Peff\n"},{"id":"404432","messageId":"20200825173425.GA16844@syl.lan","threadId":"54128","inReplyTo":"20200825172901.GD1414394@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-25T17:34:25Z","receivedAt":"2020-08-25T17:34:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 25, 2020 at 01:29:01PM -0400, Jeff King wrote:\n> On Tue, Aug 25, 2020 at 01:10:13PM -0400, Derrick Stolee wrote:\n>\n> > > If we do care, though, that implies some cooperation between the\n> > > deletion process and the midx code. Perhaps it even argues that repack\n> > > should refuse to delete such a single pack at all, since it _isn't_\n> > > redundant. It's part of a midx, and the caller should rewrite the midx\n> > > first itself, and _then_ look for redundant packs.\n> >\n> > It is worth noting that we _do_ have a way to integrate the delete and\n> > write code using 'git multi-pack-index expire'. One way to resolve this\n> > atomicity would be to do the following inside the repack command:\n> >\n> > 1. Create and index the new pack.\n> > 2. git multi-pack-index write\n> > 3. git multi-pack-index expire\n>\n> Given that discussion elsewhere points to git-repack only really\n> deleting packs in all-in-one mode (and not ever a single pack), it seems\n> like we can really be much simpler here. If we're not deleting packs via\n> all-in-one, there's no need to touch the midx at all. If we are, then\n> it's reasonable to delete the midx immediately (after having written our\n> new pack but before deleting) since our new single pack idx is as good\n> or better.\n>\n> I.e., drop step 2 above, and make step 3 just clear_midx_file(). Which\n> is roughly what the code does now, isn't it? Or is there some reason\n> that \"expire\" is more interesting than just clearing?\n\nIt's not clear to me whether you're talking about my patch, or what a\nmore full integration with 'git repack' looks like.\n\nIf you are talking about my patch, I disagree: checking that the MIDX\ndoesn't know about a pack we're dropping *is* useful even without\nall-in-one, because of '.keep' packs (as demonstrated by the new test in\nmy patch).\n\nTo me, this patch seems like an incremental step in the direction that\nwe ultimately want to be going, but it's hard to untangle whether the\nensuing discussion is targeted at my patch, or the ultimate goal.\n\n> And if anybody does want to drop single packs, etc, they can do so by\n> generating a sensible midx separately from the repack operation (and\n> probably doing so before dropping the packs for atomicity).\n>\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"404433","messageId":"20200825173446.GE1414394@coredump.intra.peff.net","threadId":"54128","inReplyTo":"c78b3108-b760-d252-9428-6f03549fea11@gmail.com","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T17:34:46Z","receivedAt":"2020-08-25T17:34:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 12:18:42PM -0400, Derrick Stolee wrote:\n\n> The best I can see is that prepare_midx_pack() will return 1 if the\n> pack no longer exists, and this would cause a die(\"error preparing\n> packfile from multi-pack-index\") in nth_midxed_pack_entry(), killing\n> the following stack trace:\n> \n> + find_pack_entry():packfile.c\n> + fill_midx_entry():midx.c\n> + nth_midxed_pack_entry():midx.c\n> \n> Perhaps that die() is a bit over-zealous since we could return 1\n> through this stack and find_pack_entry() could continue searching\n> for the object in the packed_git list. However, it could start\n> returning false _negatives_ if there were duplicates of the object\n> in the multi-pack-index but only the latest copy was deleted (and\n> the object does not appear in a pack-file outside of the multi-\n> pack-index).\n\nHmm, yeah.\n\nI thought this code is already doing the right thing, because I'd expect\nthe is_pack_valid() call later in nth_midxed_pack_entry() to be where we\nnotice the problem. But add_packed_git() does stat the packfile and\nreturn an error.\n\nSo that die() really ought to be just \"return 0\". The caller already has\nto (and does) handle similar errors (including that the pack went away\nafter we added it to the packed_git list, or that it exists but has\nbogus data, etc).\n\n-Peff\n"},{"id":"404434","messageId":"20200825174218.GF1414394@coredump.intra.peff.net","threadId":"54128","inReplyTo":"20200825173425.GA16844@syl.lan","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T17:42:18Z","receivedAt":"2020-08-25T17:42:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 01:34:25PM -0400, Taylor Blau wrote:\n\n> > I.e., drop step 2 above, and make step 3 just clear_midx_file(). Which\n> > is roughly what the code does now, isn't it? Or is there some reason\n> > that \"expire\" is more interesting than just clearing?\n> \n> It's not clear to me whether you're talking about my patch, or what a\n> more full integration with 'git repack' looks like.\n> \n> If you are talking about my patch, I disagree: checking that the MIDX\n> doesn't know about a pack we're dropping *is* useful even without\n> all-in-one, because of '.keep' packs (as demonstrated by the new test in\n> my patch).\n\nI hadn't considered .keep. So all-in-one may still involve selectively\ndeleting some packs. It makes sense, then, for repack to consider\nwhether the midx is actually redundant or not rather than just always\nclearing it (i.e., what your patch does).\n\nIn general I consider .keep packs kind of an awful feature that\nintroduces a lot of confusion and works against other features like\nbitmaps. But I guess that they're the only thing that allows you to have\na gigantic Windows-like repo where you never fully pack it, but just\nkeep acquiring a string of big packs. Which is the exact case that the\nmidx is most useful for. So they're definitely worth considering and\nsupporting here.\n\n> To me, this patch seems like an incremental step in the direction that\n> we ultimately want to be going, but it's hard to untangle whether the\n> ensuing discussion is targeted at my patch, or the ultimate goal.\n\nI wasn't sure of the answer to that until we untangled more. I.e., it\nwasn't clear to me if your incremental step was even in the right\ndirection if we weren't in fact ever selectively deleting packs in\ngit-repack. (And now it sounds like we aren't via \"git repack -d\", which\nwas my original question, but we are via .keep).\n\n-Peff\n"},{"id":"404435","messageId":"xmqqh7sq1u0a.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200825172214.GC1414394@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T18:05:09Z","receivedAt":"2020-08-25T18:05:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> OK, that's the part I was missing. The discussion here and the statement\n> from git-repack(1):\n>\n> -d\n> After packing, if the newly created packs make some existing packs\n> redundant, remove the redundant packs. Also run git prune-packed\n> to remove redundant loose object files.\n>\n> made me think that it was running pack-redundant. But it doesn't seem\n> to. It looks like we stopped doing so in 6ed64058e1 (git-repack: do not\n> do complex redundancy check., 2005-11-19).\n\nThanks for digging. A good opportunity for a #leftoverbits\ndocumentation update from new people is here.\n\n> As an aside, we tried using pack-redundant at GitHub several years ago\n> for dropping packs that were replicated in alternates storage. It\n> performs very poorly (quadratically, perhaps?) to the point that we\n> found it unusable,...\n\nYes, I originally wrote \"the pack-redundant subcommand\" in the\nmessage you are responding to with a bit more colourful adjectives,\nbut rewrote it ;-) My recollection from the last time I looked at\nit is that it is quadratic or even worse---that was long time ago,\nbut on the other hand I think the subcommand had no significant\nimprovement over the course of its life.\n\nPerhaps it is time to drop it.\n\n-- >8 --\nSubject: [RFC] pack-redundant: gauge the usage before proposing its removal\n\nThe subcommand is unusably slow and the reason why nobody reports it\nas a performance bug is suspected to be the absense of users. Let's\ndisable the normal use of command by making it error out with a big\nmessage that asks the user to tell us that they still care about the\ncommand, with an escape hatch to override it with a command line\noption.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/pack-redundant.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 178e3409b7..97cf3df79b 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -554,6 +554,7 @@ static void load_all(void)\n int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n+\tint i_still_use_this = 0;\n \tstruct pack_list *min = NULL, *red, *pl;\n \tstruct llist *ignore;\n \tstruct object_id *oid;\n@@ -580,12 +581,25 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\n \t\t\talt_odb = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--i-still-use-this\")) {\n+\t\t\ti_still_use_this = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (*arg == '-')\n \t\t\tusage(pack_redundant_usage);\n \t\telse\n \t\t\tbreak;\n \t}\n \n+\tif (!i_still_use_this) {\n+\t\tputs(_(\"'git pack-redundant' is nominated for removal.\\n\"\n+\t\t \"If you still use this command, please add an extra\\n\"\n+\t\t \"option, '--i-still-use-this', on the command line\\n\"\n+\t\t \"and let us know you still use it by sending an e-mail\\n\"\n+\t\t \"to . Thanks\\n\"));\n+\t\texit(1);\n+\t}\n+\n \tif (load_all_packs)\n \t\tload_all();\n \telse\n"},{"id":"404438","messageId":"20200825182745.GA1417288@coredump.intra.peff.net","threadId":"54128","inReplyTo":"xmqqh7sq1u0a.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-25T18:27:45Z","receivedAt":"2020-08-25T18:27:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 11:05:09AM -0700, Junio C Hamano wrote:\n\n> Subject: [RFC] pack-redundant: gauge the usage before proposing its removal\n> \n> The subcommand is unusably slow and the reason why nobody reports it\n> as a performance bug is suspected to be the absense of users. Let's\n> disable the normal use of command by making it error out with a big\n> message that asks the user to tell us that they still care about the\n> command, with an escape hatch to override it with a command line\n> option.\n\nI like this direction. And I'm tempted to say it's OK to go straight\nhere given how generally useless the command is. But it feels like a big\njump for the initial change. While we give the user an easy escape\nhatch, it might still break their scripts.\n\nA more gentle transition would I guess be:\n\n 1. Mention deprecation in release notes (hah, as if anybody reads\n them).\n\n 2. Issue a warning but continue to behave as normal. That might break\n scripts that care a lot about stderr, but otherwise is harmless. No\n clue if anybody would actually see the message or not.\n\n 3. Flip warning to error, as your patch does.\n\n 4. Removal.\n\nI'd guess we could do 1+2 in one release, then 3 a release or two later,\nand then finally 4. I dunno. That's more tedious and I'll be surprised\nif we get any bites, but maybe we ought to do it out of principle.\n\n-Peff\n"},{"id":"404487","messageId":"xmqq1rjuz6n3.fsf_-_@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200825182745.GA1417288@coredump.intra.peff.net","subject":"[PATCH] pack-redundant: gauge the usage before proposing its removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T22:45:52Z","receivedAt":"2020-08-25T22:45:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The subcommand is unusably slow and the reason why nobody reports it\nas a performance bug is suspected to be the absense of users. Let's\nshow a big message that asks the user to tell us that they still\ncare about the command when an attempt is made to run the command,\nwith an escape hatch to override it with a command line option.\n\nIn a few releases, we may turn it into an error and keep it for a\nfew more releases before finally removing it (during the whole time,\nthe plan to remove it would be interrupted by end user raising hand).\n\nSigned-off-by: Junio C Hamano \n---\n\n Jeff King writes:\n\n > A more gentle transition would I guess be:\n >\n > 1. Mention deprecation in release notes (hah, as if anybody reads\n > them).\n >\n > 2. Issue a warning but continue to behave as normal. That might break\n > scripts that care a lot about stderr, but otherwise is harmless. No\n > clue if anybody would actually see the message or not.\n\n OK, so here is an update for the above.\n\n builtin/pack-redundant.c | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 178e3409b7..b94c2f2423 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -554,6 +554,7 @@ static void load_all(void)\n int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n+\tint i_still_use_this = 0;\n \tstruct pack_list *min = NULL, *red, *pl;\n \tstruct llist *ignore;\n \tstruct object_id *oid;\n@@ -580,12 +581,24 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\n \t\t\talt_odb = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--i-still-use-this\")) {\n+\t\t\ti_still_use_this = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (*arg == '-')\n \t\t\tusage(pack_redundant_usage);\n \t\telse\n \t\t\tbreak;\n \t}\n \n+\tif (!i_still_use_this) {\n+\t\tfputs(_(\"'git pack-redundant' is nominated for removal.\\n\"\n+\t\t\t\"If you still use this command, please add an extra\\n\"\n+\t\t\t\"option, '--i-still-use-this', on the command line\\n\"\n+\t\t\t\"and let us know you still use it by sending an e-mail\\n\"\n+\t\t\t\"to . Thanks.\\n\"), stderr);\n+\t}\n+\n \tif (load_all_packs)\n \t\tload_all();\n \telse\n-- \n2.28.0-454-g5f859b1948\n\n"},{"id":"404488","messageId":"20200825230904.GA23203@syl.lan","threadId":"54128","inReplyTo":"xmqq1rjuz6n3.fsf_-_@gitster.c.googlers.com","subject":"Re: [PATCH] pack-redundant: gauge the usage before proposing its removal","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-25T23:09:04Z","receivedAt":"2020-08-25T23:09:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi Junio,\n\nOn Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:\n> The subcommand is unusably slow and the reason why nobody reports it\n> as a performance bug is suspected to be the absense of users. Let's\n> show a big message that asks the user to tell us that they still\n> care about the command when an attempt is made to run the command,\n> with an escape hatch to override it with a command line option.\n>\n> In a few releases, we may turn it into an error and keep it for a\n> few more releases before finally removing it (during the whole time,\n> the plan to remove it would be interrupted by end user raising hand).\n>\n> Signed-off-by: Junio C Hamano \n> ---\n\nThanks. Peff's plan seems reasonable to me (and I'd like to add that I\nam a frequent reader of the release notes ;-)), as does this patch.\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"},{"id":"404489","messageId":"xmqqwo1mxqeb.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200825230904.GA23203@syl.lan","subject":"Re: [PATCH] pack-redundant: gauge the usage before proposing its removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-25T23:22:04Z","receivedAt":"2020-08-25T23:22:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:\n>> The subcommand is unusably slow and the reason why nobody reports it\n>> as a performance bug is suspected to be the absense of users. Let's\n>> show a big message that asks the user to tell us that they still\n>> care about the command when an attempt is made to run the command,\n>> with an escape hatch to override it with a command line option.\n>>\n>> In a few releases, we may turn it into an error and keep it for a\n>> few more releases before finally removing it (during the whole time,\n>> the plan to remove it would be interrupted by end user raising hand).\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>\n> Thanks. Peff's plan seems reasonable to me (and I'd like to add that I\n> am a frequent reader of the release notes ;-)), as does this patch.\n>\n> Reviewed-by: Taylor Blau \n\nThanks. It needs updates to a test script, though.\n\ndiff --git a/t/t5323-pack-redundant.sh b/t/t5323-pack-redundant.sh\nindex 6b4d1ca353..2dd2d67b9e 100755\n--- a/t/t5323-pack-redundant.sh\n+++ b/t/t5323-pack-redundant.sh\n@@ -39,6 +39,8 @@ relationship between packs and objects is as follows:\n master_repo=master.git\n shared_repo=shared.git\n \n+git_pack_redundant='git pack-redundant --i-still-use-this'\n+\n # Create commits in and assign each commit's oid to shell variables\n # given in the arguments (A, B, and C). E.g.:\n #\n@@ -154,7 +156,7 @@ test_expect_success 'master: no redundant for pack 1, 2, 3' '\n \t\tEOF\n \t(\n \t\tcd \"$master_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -192,7 +194,7 @@ test_expect_success 'master: one of pack-2/pack-3 is redundant' '\n \t\tcat >expect <<-EOF &&\n \t\t\tP3:$P3\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -231,7 +233,7 @@ test_expect_success 'master: pack 2, 4, and 6 are redundant' '\n \t\t\tP4:$P4\n \t\t\tP6:$P6\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -266,7 +268,7 @@ test_expect_success 'master: pack-8 (subset of pack-1) is also redundant' '\n \t\t\tP6:$P6\n \t\t\tP8:$P8\n \t\t\tEOF\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\tformat_packfiles actual &&\n \t\ttest_cmp expect actual\n \t)\n@@ -284,9 +286,9 @@ test_expect_success 'master: clean loose objects' '\n test_expect_success 'master: remove redundant packs and pass fsck' '\n \t(\n \t\tcd \"$master_repo\" &&\n-\t\tgit pack-redundant --all | xargs rm &&\n+\t\t$git_pack_redundant --all | xargs rm &&\n \t\tgit fsck &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -304,7 +306,7 @@ test_expect_success 'setup shared.git' '\n test_expect_success 'shared: all packs are redundant, but no output without --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -343,7 +345,7 @@ test_expect_success 'shared: show redundant packs in stderr for verbose mode' '\n \t\t\tP5:$P5\n \t\t\tP7:$P7\n \t\t\tEOF\n-\t\tgit pack-redundant --all --verbose >out 2>out.err &&\n+\t\t$git_pack_redundant --all --verbose >out 2>out.err &&\n \t\ttest_must_be_empty out &&\n \t\tgrep \"pack$\" out.err | format_packfiles >actual &&\n \t\ttest_cmp expect actual\n@@ -356,9 +358,9 @@ test_expect_success 'shared: remove redundant packs, no packs left' '\n \t\tcat >expect <<-EOF &&\n \t\t\tfatal: Zero packs found!\n \t\t\tEOF\n-\t\tgit pack-redundant --all --alt-odb | xargs rm &&\n+\t\t$git_pack_redundant --all --alt-odb | xargs rm &&\n \t\tgit fsck &&\n-\t\ttest_must_fail git pack-redundant --all --alt-odb >actual 2>&1 &&\n+\t\ttest_must_fail $git_pack_redundant --all --alt-odb >actual 2>&1 &&\n \t\ttest_cmp expect actual\n \t)\n '\n@@ -386,7 +388,7 @@ test_expect_success 'shared: create new objects and packs' '\n test_expect_success 'shared: no redundant without --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all >out &&\n+\t\t$git_pack_redundant --all >out &&\n \t\ttest_must_be_empty out\n \t)\n '\n@@ -417,7 +419,7 @@ test_expect_success 'shared: no redundant without --alt-odb' '\n test_expect_success 'shared: one pack is redundant with --alt-odb' '\n \t(\n \t\tcd \"$shared_repo\" &&\n-\t\tgit pack-redundant --all --alt-odb >out &&\n+\t\t$git_pack_redundant --all --alt-odb >out &&\n \t\tformat_packfiles actual &&\n \t\ttest_line_count = 1 actual\n \t)\n@@ -454,7 +456,7 @@ test_expect_success 'shared: ignore unique objects and all two packs are redunda\n \t\t\tPx1:$Px1\n \t\t\tPx2:$Px2\n \t\t\tEOF\n-\t\tgit pack-redundant --all --alt-odb >out <<-EOF &&\n+\t\t$git_pack_redundant --all --alt-odb >out <<-EOF &&\n \t\t\t$X\n \t\t\t$Y\n \t\t\t$Z\n"},{"id":"404490","messageId":"20200826011718.3186597-1-gitster@pobox.com","threadId":"54128","inReplyTo":"xmqq1rjuz6n3.fsf_-_@gitster.c.googlers.com","subject":"[PATCH v1 0/3] War on dashed-git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T01:17:15Z","receivedAt":"2020-08-26T01:17:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If we were to propose breaking the 12-year old promise to our users\nthat we will keep \"git-foo\" binaries on disk where \"git --exec-path\"\ntells them, we first need to gauge how big a population we would be\nhurting with such a move.\n\nSo here is a miniseries towards that. The third one hooks to the\ncodepath in git.c::cmd_main() where av[0] is of \"git-foo\" form and\nwe dispatch to \"foo\" as a builtin command. In the original code, we\nwill die() if \"foo\" is not a built-in command in this codepath, so\nit is exactly the place we want to catch remaining uses of \"git-foo\"\ninvoking built-in commands.\n\nThere are a few legitimate \"git-foo\" calls made even for built-ins\nand those exceptions are marked in the command-list mechanism, which\nis shared with the help subsystem. We might want to see if we can\nunify this exception list with what we have in the Makefile as\nBIN_PROGRAMS and what Dscho introduces as ALL_COMMANDS_TO_INSTALL\nin his series. These have large overlaps in what they mean, but\nthey are not exactly identical.\n\nJunio C Hamano (3):\n transport-helper: do not run git-remote-ext etc. in dashed form\n cvsexportcommit: do not run git programs in dashed form\n git: catch an attempt to run \"git-foo\"\n\n command-list.txt | 11 +++++++----\n git-cvsexportcommit.perl | 16 ++++++++--------\n git.c | 2 ++\n help.c | 34 ++++++++++++++++++++++++++++++++++\n help.h | 3 +++\n transport-helper.c | 3 ++-\n 6 files changed, 56 insertions(+), 13 deletions(-)\n\n-- \n2.28.0-454-g5f859b1948\n\n"},{"id":"404491","messageId":"20200826011718.3186597-2-gitster@pobox.com","threadId":"54128","inReplyTo":"20200826011718.3186597-1-gitster@pobox.com","subject":"[PATCH v1 1/3] transport-helper: do not run git-remote-ext etc. in dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T01:17:16Z","receivedAt":"2020-08-26T01:17:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Runing them as \"git remote-ext\" and letting \"git\" dispatch to\n\"remote-ext\" would just be fine and is more idiomatic.\n\nSigned-off-by: Junio C Hamano \n---\n transport-helper.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex defafbf4c1..fae3d99500 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -128,7 +128,8 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->in = -1;\n \thelper->out = -1;\n \thelper->err = 0;\n-\tstrvec_pushf(&helper->args, \"git-remote-%s\", data->name);\n+\tstrvec_push(&helper->args, \"git\");\n+\tstrvec_pushf(&helper->args, \"remote-%s\", data->name);\n \tstrvec_push(&helper->args, transport->remote->name);\n \tstrvec_push(&helper->args, remove_ext_force(transport->url));\n \thelper->git_cmd = 0;\n-- \n2.28.0-454-g5f859b1948\n\n"},{"id":"404492","messageId":"20200826011718.3186597-3-gitster@pobox.com","threadId":"54128","inReplyTo":"20200826011718.3186597-1-gitster@pobox.com","subject":"[PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T01:17:17Z","receivedAt":"2020-08-26T01:17:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This ancient script runs \"git-foo\" all over the place. A strange\nthing is that it has t9200 tests successfully running, even though\nit does not seem to futz with PATH to prepend $(git --exec-path)\noutput. It is tempting to declare that the command must be unused,\nbut that is left to another topic.\n\nSigned-off-by: Junio C Hamano \n---\n git-cvsexportcommit.perl | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 0ae8bce3fb..289d4bc684 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -30,7 +30,7 @@\n \t# Remember where GIT_DIR is before changing to CVS checkout\n \tunless ($ENV{GIT_DIR}) {\n \t\t# No GIT_DIR set. Figure it out for ourselves\n-\t\tmy $gd =`git-rev-parse --git-dir`;\n+\t\tmy $gd =`git rev-parse --git-dir`;\n \t\tchomp($gd);\n \t\t$ENV{GIT_DIR} = $gd;\n \t}\n@@ -66,7 +66,7 @@\n # resolve target commit\n my $commit;\n $commit = pop @ARGV;\n-$commit = safe_pipe_capture('git-rev-parse', '--verify', \"$commit^0\");\n+$commit = safe_pipe_capture('git', 'rev-parse', '--verify', \"$commit^0\");\n chomp $commit;\n if ($?) {\n die \"The commit reference $commit did not resolve!\";\n@@ -76,7 +76,7 @@\n my $parent;\n if (@ARGV) {\n $parent = pop @ARGV;\n- $parent = safe_pipe_capture('git-rev-parse', '--verify', \"$parent^0\");\n+ $parent = safe_pipe_capture('git', 'rev-parse', '--verify', \"$parent^0\");\n chomp $parent;\n if ($?) {\n \tdie \"The parent reference did not resolve!\";\n@@ -84,7 +84,7 @@\n }\n \n # find parents from the commit itself\n-my @commit = safe_pipe_capture('git-cat-file', 'commit', $commit);\n+my @commit = safe_pipe_capture('git', 'cat-file', 'commit', $commit);\n my @parents;\n my $committer;\n my $author;\n@@ -162,9 +162,9 @@\n close MSG;\n \n if ($parent eq $noparent) {\n- `git-diff-tree --binary -p --root $commit >.cvsexportcommit.diff`;# || die \"Cannot diff\";\n+ `git diff-tree --binary -p --root $commit >.cvsexportcommit.diff`;# || die \"Cannot diff\";\n } else {\n- `git-diff-tree --binary -p $parent $commit >.cvsexportcommit.diff`;# || die \"Cannot diff\";\n+ `git diff-tree --binary -p $parent $commit >.cvsexportcommit.diff`;# || die \"Cannot diff\";\n }\n \n ## apply non-binary changes\n@@ -178,7 +178,7 @@\n print \"Checking if patch will apply\\n\";\n \n my @stat;\n-open APPLY, \"GIT_INDEX_FILE=$tmpdir/index git-apply $context --summary --numstat<.cvsexportcommit.diff|\" || die \"cannot patch\";\n+open APPLY, \"GIT_INDEX_FILE=$tmpdir/index git apply $context --summary --numstat<.cvsexportcommit.diff|\" || die \"cannot patch\";\n @stat=;\n close APPLY || die \"Cannot patch\";\n my (@bfiles,@files,@afiles,@dfiles);\n@@ -333,7 +333,7 @@\n if ($opt_W) {\n system(\"git checkout -q $commit^0\") && die \"cannot patch\";\n } else {\n- `GIT_INDEX_FILE=$tmpdir/index git-apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die \"cannot patch\";\n+ `GIT_INDEX_FILE=$tmpdir/index git apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die \"cannot patch\";\n }\n \n print \"Patch applied successfully. Adding new files and directories to CVS\\n\";\n-- \n2.28.0-454-g5f859b1948\n\n"},{"id":"404493","messageId":"20200826011718.3186597-4-gitster@pobox.com","threadId":"54128","inReplyTo":"20200826011718.3186597-1-gitster@pobox.com","subject":"[PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T01:17:18Z","receivedAt":"2020-08-26T01:17:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"If we were to propose removing \"git-foo\" binaries from the\nfilesystem for built-in commands, we should first see if there are\nusers who will be affected by such a move. When cmd_main() detects\nwe were called not as \"git\", but as \"git-foo\", give an error message\nto ask the user to let us know and stop our removal plan, unless we\nare running a selected few programs that MUST be callable in the\ndashed form (e.g. \"git-upload-pack\").\n\nThose who are always using \"git foo\" form will not be affected, but\nthose who trusted the promise we made to them 12 years ago that by\nprepending the output of $(git --exec-path) to the list of\ndirectories on their $PATH, they can safely keep writing\n\"git-cat-file\" will be negatively affected as all their scripts\nassuming the promise will be kept are now broken.\n\nSigned-off-by: Junio C Hamano \n---\n command-list.txt | 11 +++++++----\n git.c | 2 ++\n help.c | 34 ++++++++++++++++++++++++++++++++++\n help.h | 3 +++\n 4 files changed, 46 insertions(+), 4 deletions(-)\n\ndiff --git a/command-list.txt b/command-list.txt\nindex e5901f2213..1238f6ec8b 100644\n--- a/command-list.txt\n+++ b/command-list.txt\n@@ -39,6 +39,9 @@\n # mainporcelain commands are completable so you don't need this\n # attribute.\n #\n+# \"onpath\" attribute is used to mark that the command MUST appear\n+# on $PATH (typically in /usr/bin) due to protocol requirement.\n+#\n # As part of the Git man page list, the man(5/7) guides are also\n # specified here, which can only have \"guide\" attribute and nothing\n # else.\n@@ -144,7 +147,7 @@ git-quiltimport foreignscminterface\n git-range-diff mainporcelain\n git-read-tree plumbingmanipulators\n git-rebase mainporcelain history\n-git-receive-pack synchelpers\n+git-receive-pack synchelpers onpath\n git-reflog ancillarymanipulators complete\n git-remote ancillarymanipulators complete\n git-repack ancillarymanipulators complete\n@@ -159,7 +162,7 @@ git-rev-parse plumbinginterrogators\n git-rm mainporcelain worktree\n git-send-email foreignscminterface complete\n git-send-pack synchingrepositories\n-git-shell synchelpers\n+git-shell synchelpers onpath\n git-shortlog mainporcelain\n git-show mainporcelain info\n git-show-branch ancillaryinterrogators complete\n@@ -182,8 +185,8 @@ git-unpack-objects plumbingmanipulators\n git-update-index plumbingmanipulators\n git-update-ref plumbingmanipulators\n git-update-server-info synchingrepositories\n-git-upload-archive synchelpers\n-git-upload-pack synchelpers\n+git-upload-archive synchelpers onpath\n+git-upload-pack synchelpers onpath\n git-var plumbinginterrogators\n git-verify-commit ancillaryinterrogators\n git-verify-pack plumbinginterrogators\ndiff --git a/git.c b/git.c\nindex 8bd1d7551d..927018bda7 100644\n--- a/git.c\n+++ b/git.c\n@@ -839,6 +839,8 @@ int cmd_main(int argc, const char **argv)\n \t * that one cannot handle it.\n \t */\n \tif (skip_prefix(cmd, \"git-\", &cmd)) {\n+\t\twarn_on_dashed_git(argv[0]);\n+\n \t\targv[0] = cmd;\n \t\thandle_builtin(argc, argv);\n \t\tdie(_(\"cannot handle %s as a builtin\"), cmd);\ndiff --git a/help.c b/help.c\nindex d478afb2af..490d2bc3ae 100644\n--- a/help.c\n+++ b/help.c\n@@ -720,3 +720,37 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n \tstring_list_clear(&suggested_refs, 0);\n \texit(1);\n }\n+\n+static struct cmdname_help *find_cmdname_help(const char *name)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(command_list); i++) {\n+\t\tif (!strcmp(command_list[i].name, name))\n+\t\t\treturn &command_list[i];\n+\t}\n+\treturn NULL;\n+}\n+\n+void warn_on_dashed_git(const char *cmd)\n+{\n+\tstruct cmdname_help *cmdname;\n+\tstatic const char *still_in_use_var = \"GIT_I_STILL_USE_DASHED_GIT\";\n+\tstatic const char *still_in_use_msg =\n+\t\tN_(\"Use of '%s' in the dashed-form is nominated for removal.\\n\"\n+\t\t \"If you still use it, export '%s=true'\\n\"\n+\t\t \"and send an e-mail to \\n\"\n+\t\t \"to let us know and stop our removal plan. Thanks.\\n\");\n+\n+\tif (!cmd)\n+\t\treturn; /* git-help is OK */\n+\n+\tcmdname = find_cmdname_help(cmd);\n+\tif (cmdname && (cmdname->category & CAT_onpath))\n+\t\treturn; /* git-upload-pack and friends are OK */\n+\n+\tif (!git_env_bool(still_in_use_var, 0)) {\n+\t\tfprintf(stderr, _(still_in_use_msg), cmd, still_in_use_var);\n+\t\texit(1);\n+\t}\n+}\ndiff --git a/help.h b/help.h\nindex dc02458855..d3de5e0d69 100644\n--- a/help.h\n+++ b/help.h\n@@ -45,6 +45,9 @@ void get_version_info(struct strbuf *buf, int show_build_options);\n */\n NORETURN void help_unknown_ref(const char *ref, const char *cmd, const char *error);\n \n+/* When the cmd_main() sees \"git-foo\", check if the user intended */\n+void warn_on_dashed_git(const char *);\n+\n static inline void list_config_item(struct string_list *list,\n \t\t\t\t const char *prefix,\n \t\t\t\t const char *str)\n-- \n2.28.0-454-g5f859b1948\n\n"},{"id":"404494","messageId":"xmqqimd6xkyt.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200826011718.3186597-4-gitster@pobox.com","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T01:19:22Z","receivedAt":"2020-08-26T01:19:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If we were to propose removing \"git-foo\" binaries from the\n> filesystem for built-in commands, we should first see if there are\n> users who will be affected by such a move. When cmd_main() detects\n> we were called not as \"git\", but as \"git-foo\", give an error message\n> to ask the user to let us know and stop our removal plan, unless we\n> are running a selected few programs that MUST be callable in the\n> dashed form (e.g. \"git-upload-pack\").\n>\n> Those who are always using \"git foo\" form will not be affected, but\n> those who trusted the promise we made to them 12 years ago that by\n> prepending the output of $(git --exec-path) to the list of\n> directories on their $PATH, they can safely keep writing\n> \"git-cat-file\" will be negatively affected as all their scripts\n> assuming the promise will be kept are now broken.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n\nThe same idea as the one for pack-redundant. I do not use the\ntechnique to inspect $PATH and see if $GIT_EXEC_PATH is on it, as\nthat would mean we will *not* bug users with legitimate need to keep\nthe feature working, hence will not get \"don't do that\" objections.\n\nWe may want to ensure command_list[] is sorted by name and run\nbinary search on it if running find_cmdname_help() for each and\nevery dashed \"git-foo\" invocations turns out to be costly. Our\nconjecture behind this patch is that the form is rarely if ever\nused, so it may not matter at all, though.\n"},{"id":"404495","messageId":"CAPig+cTvLaOD1idfB2M0-QSfXXKBe5-FnWSU9E0PaUMHAoGj1w@mail.gmail.com","threadId":"54128","inReplyTo":"20200826011718.3186597-2-gitster@pobox.com","subject":"Re: [PATCH v1 1/3] transport-helper: do not run git-remote-ext etc. in dashed form","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-26T01:24:11Z","receivedAt":"2020-08-26T01:24:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano wrote:\n> Runing them as \"git remote-ext\" and letting \"git\" dispatch to\n\ns/Runing/Running/\ns/them/it/\n\n> \"remote-ext\" would just be fine and is more idiomatic.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/transport-helper.c b/transport-helper.c\n> @@ -128,7 +128,8 @@ static struct child_process *get_helper(struct transport *transport)\n> - strvec_pushf(&helper->args, \"git-remote-%s\", data->name);\n> + strvec_push(&helper->args, \"git\");\n> + strvec_pushf(&helper->args, \"remote-%s\", data->name);\n\nRather than pushing \"git\" as the first argument, would it instead be\nmore idiomatic to set `helper->git_cmd = 1` (or would that not work\ncorrectly for some reason)?\n"},{"id":"404496","messageId":"CAPig+cR-eYCVQLRa0rVhgJ8L60-zCS_aK6_nVERcrXSyApdihw@mail.gmail.com","threadId":"54128","inReplyTo":"20200826011718.3186597-3-gitster@pobox.com","subject":"Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-26T01:28:29Z","receivedAt":"2020-08-26T01:28:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano wrote:\n> This ancient script runs \"git-foo\" all over the place. A strange\n> thing is that it has t9200 tests successfully running, even though\n> it does not seem to futz with PATH to prepend $(git --exec-path)\n> output.\n\nt/test-lib.sh takes care of that for us, doesn't it?\n\n> It is tempting to declare that the command must be unused,\n> but that is left to another topic.\n>\n> Signed-off-by: Junio C Hamano \n"},{"id":"404497","messageId":"xmqqeenuxjwo.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"CAPig+cR-eYCVQLRa0rVhgJ8L60-zCS_aK6_nVERcrXSyApdihw@mail.gmail.com","subject":"Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T01:42:15Z","receivedAt":"2020-08-26T01:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano wrote:\n>> This ancient script runs \"git-foo\" all over the place. A strange\n>> thing is that it has t9200 tests successfully running, even though\n>> it does not seem to futz with PATH to prepend $(git --exec-path)\n>> output.\n>\n> t/test-lib.sh takes care of that for us, doesn't it?\n\nIt generally shouldn't, I think.\n\nWe have bin-wrappers directory and we shouldn't be copying things we\nwould install in libexec/git-core/ in real life.\n"},{"id":"404520","messageId":"nycvar.QRO.7.76.6.2008260954430.56@tvgsbejvaqbjf.bet","threadId":"54128","inReplyTo":"CAPig+cTvLaOD1idfB2M0-QSfXXKBe5-FnWSU9E0PaUMHAoGj1w@mail.gmail.com","subject":"Re: [PATCH v1 1/3] transport-helper: do not run git-remote-ext etc. in dashed form","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-26T07:55:29Z","receivedAt":"2020-08-26T14:55:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 25 Aug 2020, Eric Sunshine wrote:\n\n> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano wrote:\n> > Runing them as \"git remote-ext\" and letting \"git\" dispatch to\n>\n> s/Runing/Running/\n> s/them/it/\n>\n> > \"remote-ext\" would just be fine and is more idiomatic.\n> >\n> > Signed-off-by: Junio C Hamano \n> > ---\n> > diff --git a/transport-helper.c b/transport-helper.c\n> > @@ -128,7 +128,8 @@ static struct child_process *get_helper(struct transport *transport)\n> > - strvec_pushf(&helper->args, \"git-remote-%s\", data->name);\n> > + strvec_push(&helper->args, \"git\");\n> > + strvec_pushf(&helper->args, \"remote-%s\", data->name);\n>\n> Rather than pushing \"git\" as the first argument, would it instead be\n> more idiomatic to set `helper->git_cmd = 1` (or would that not work\n> correctly for some reason)?\n\nThat is precisely what I did in Git for Windows:\n\n-- snipsnap --\nFrom: Johannes Schindelin \nSubject: [PATCH] transport-helper: prefer Git's builtins over dashed form\n\nThis helps with minimal installations such as MinGit that refuse to\nwaste .zip real estate by shipping identical copies of builtins (.zip\nfiles do not support hard links).\n\nSigned-off-by: Johannes Schindelin \n---\n transport-helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 35d6437b557..2c4b7a023db 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -130,10 +130,10 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->in = -1;\n \thelper->out = -1;\n \thelper->err = 0;\n-\targv_array_pushf(&helper->args, \"git-remote-%s\", data->name);\n+\targv_array_pushf(&helper->args, \"remote-%s\", data->name);\n \targv_array_push(&helper->args, transport->remote->name);\n \targv_array_push(&helper->args, remove_ext_force(transport->url));\n-\thelper->git_cmd = 0;\n+\thelper->git_cmd = 1;\n \thelper->silent_exec_failure = 1;\n\n \tif (have_git_dir())\n--\n2.28.0.windows.1.18.g5300e52e185\n\n"},{"id":"404522","messageId":"nycvar.QRO.7.76.6.2008260956380.56@tvgsbejvaqbjf.bet","threadId":"54128","inReplyTo":"20200826011718.3186597-3-gitster@pobox.com","subject":"Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-26T08:02:18Z","receivedAt":"2020-08-26T15:02:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 25 Aug 2020, Junio C Hamano wrote:\n\n> This ancient script runs \"git-foo\" all over the place. A strange\n> thing is that it has t9200 tests successfully running, even though\n> it does not seem to futz with PATH to prepend $(git --exec-path)\n> output. It is tempting to declare that the command must be unused,\n> but that is left to another topic.\n\nNot surprising at all: when t9200 runs `git cvsexportcommit`, it actually\nruns `bin-wrappers/git`, which sets `GIT_EXEC_PATH` to the top-level\ndirectory of the Git source code, then calls the `git` executable which in\nturn will set up the `PATH` to prepend `GIT_EXEC_PATH`, and then look for\n`git-cvsexportcommit` (which does not exist in `bin-wrappers/`, but in\n`GIT_EXEC_PATH`). And of course then `git-rev-parse` is found on the\n`PATH`, too.\n\nSlightly more surprising is that my PR build did not fail. I guess we do\ntest `git svn`, but we skip all CVS-related tests, eh?\n\nSlightly related: it might be a good time to deprecate the CVS-related Git\ncommands...\n\nCiao,\nDscho\n"},{"id":"404523","messageId":"nycvar.QRO.7.76.6.2008261004120.56@tvgsbejvaqbjf.bet","threadId":"54128","inReplyTo":"20200826011718.3186597-4-gitster@pobox.com","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-26T08:06:20Z","receivedAt":"2020-08-26T15:06:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 25 Aug 2020, Junio C Hamano wrote:\n\n> If we were to propose removing \"git-foo\" binaries from the\n> filesystem for built-in commands, we should first see if there are\n> users who will be affected by such a move. When cmd_main() detects\n> we were called not as \"git\", but as \"git-foo\", give an error message\n> to ask the user to let us know and stop our removal plan, unless we\n> are running a selected few programs that MUST be callable in the\n> dashed form (e.g. \"git-upload-pack\").\n>\n> Those who are always using \"git foo\" form will not be affected, but\n> those who trusted the promise we made to them 12 years ago that by\n> prepending the output of $(git --exec-path) to the list of\n> directories on their $PATH, they can safely keep writing\n> \"git-cat-file\" will be negatively affected as all their scripts\n> assuming the promise will be kept are now broken.\n\nIt might be a good idea to also instrument the existing scripts, to show\nthe same warning unless they were called through the `git` binary.\n\n_If_ we were to do this ;-)\n\nCiao,\nDscho\n"},{"id":"404524","messageId":"nycvar.QRO.7.76.6.2008261007100.56@tvgsbejvaqbjf.bet","threadId":"54128","inReplyTo":"20200826011718.3186597-1-gitster@pobox.com","subject":"Re: [PATCH v1 0/3] War on dashed-git","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-26T08:09:25Z","receivedAt":"2020-08-26T15:09:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 25 Aug 2020, Junio C Hamano wrote:\n\n> If we were to propose breaking the 12-year old promise to our users\n> that we will keep \"git-foo\" binaries on disk where \"git --exec-path\"\n> tells them, we first need to gauge how big a population we would be\n> hurting with such a move.\n>\n> So here is a miniseries towards that. The third one hooks to the\n> codepath in git.c::cmd_main() where av[0] is of \"git-foo\" form and\n> we dispatch to \"foo\" as a builtin command. In the original code, we\n> will die() if \"foo\" is not a built-in command in this codepath, so\n> it is exactly the place we want to catch remaining uses of \"git-foo\"\n> invoking built-in commands.\n>\n> There are a few legitimate \"git-foo\" calls made even for built-ins\n> and those exceptions are marked in the command-list mechanism, which\n> is shared with the help subsystem. We might want to see if we can\n> unify this exception list with what we have in the Makefile as\n> BIN_PROGRAMS and what Dscho introduces as ALL_COMMANDS_TO_INSTALL\n> in his series. These have large overlaps in what they mean, but\n> they are not exactly identical.\n\nAs it happens, I discussed a scenario the other day where dashed\ninvocations might still happen, and the consensus was that nobody looks at\nthe output unless things are broken.\n\nSo maybe this patch series would be a good first step, but if we truly\nwanted to break that 12-year old promise, we might need to have another\npatch on top that _does_ install the dashed commands, but into a\nsubdirectory of `libexec/git-core/` that is only added to the `PATH` when\nan \"escape hatch\"-style config setting is set.\n\nCiao,\nDscho\n"},{"id":"404529","messageId":"xmqqa6yhxucq.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"CAPig+cR-eYCVQLRa0rVhgJ8L60-zCS_aK6_nVERcrXSyApdihw@mail.gmail.com","subject":"Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T16:08:53Z","receivedAt":"2020-08-26T16:09:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano wrote:\n>> This ancient script runs \"git-foo\" all over the place. A strange\n>> thing is that it has t9200 tests successfully running, even though\n>> it does not seem to futz with PATH to prepend $(git --exec-path)\n>> output.\n>\n> t/test-lib.sh takes care of that for us, doesn't it?\n\nActually, it is \"git\" itself. \n\nThe tests spawn \"git cvsexportcommit\" via the \"git\" dispatcher. The\ndispatcher adds GIT_EXEC_PATH to PATH in exec-cmd.c::setup_path()\nand that is used to (1) locate \"git-foo\" from /usr/libexec/git-core/\nthat are not builtin and (2) passed to any processes we invoke. I\nthink the latter was originally done primarily for not breaking\nhooks, but that is what allows this script going in this particular\ncase.\n\nIf this were only a fluke in the test that kept otherwise unrunnable\nscript passing, I'd say it is an evidence enough that lets us drop\nthe cvsexportcommit immediately, but now I rediscovered how it was\nsupposed to work and saw how it actually does work as designed, I\nwould not be surprised if it is still used in the wild.\n\nThanks.\n\n\n\n"},{"id":"404534","messageId":"xmqqpn7dwews.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"CAPig+cTvLaOD1idfB2M0-QSfXXKBe5-FnWSU9E0PaUMHAoGj1w@mail.gmail.com","subject":"Re: [PATCH v1 1/3] transport-helper: do not run git-remote-ext etc. in dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T16:27:47Z","receivedAt":"2020-08-26T16:27:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano wrote:\n>> Runing them as \"git remote-ext\" and letting \"git\" dispatch to\n>\n> s/Runing/Running/\n> s/them/it/\n\nI wrote 'them' because it is not only 'ext' but other helpers are\nalso run with the same mechanism, but the sentence uses remote-ext\nas a single concrete example, so 'it' would be more appropriate.\nThanks.\n\n>> \"remote-ext\" would just be fine and is more idiomatic.\n>>\n>> Signed-off-by: Junio C Hamano \n>> ---\n>> diff --git a/transport-helper.c b/transport-helper.c\n>> @@ -128,7 +128,8 @@ static struct child_process *get_helper(struct transport *transport)\n>> - strvec_pushf(&helper->args, \"git-remote-%s\", data->name);\n>> + strvec_push(&helper->args, \"git\");\n>> + strvec_pushf(&helper->args, \"remote-%s\", data->name);\n>\n> Rather than pushing \"git\" as the first argument, would it instead be\n> more idiomatic to set `helper->git_cmd = 1` (or would that not work\n> correctly for some reason)?\n\nI was aiming for minimum change and did not think too deeply. \n\nIf .git_cmd=1 works here (and offhand I do not see a reason why\nnot), then that would be simpler. Thanks.\n"},{"id":"404535","messageId":"xmqqlfi1wevq.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"xmqqa6yhxucq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v1 2/3] cvsexportcommit: do not run git programs in dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T16:28:25Z","receivedAt":"2020-08-26T16:28:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> On Tue, Aug 25, 2020 at 9:17 PM Junio C Hamano wrote:\n>>> This ancient script runs \"git-foo\" all over the place. A strange\n>>> thing is that it has t9200 tests successfully running, even though\n>>> it does not seem to futz with PATH to prepend $(git --exec-path)\n>>> output.\n>>\n>> t/test-lib.sh takes care of that for us, doesn't it?\n>\n> Actually, it is \"git\" itself. \n>\n> The tests spawn \"git cvsexportcommit\" via the \"git\" dispatcher. The\n> dispatcher adds GIT_EXEC_PATH to PATH in exec-cmd.c::setup_path()\n> and that is used to (1) locate \"git-foo\" from /usr/libexec/git-core/\n> that are not builtin and (2) passed to any processes we invoke. I\n> think the latter was originally done primarily for not breaking\n> hooks, but that is what allows this script going in this particular\n> case.\n>\n> If this were only a fluke in the test that kept otherwise unrunnable\n> script passing, I'd say it is an evidence enough that lets us drop\n> the cvsexportcommit immediately, but now I rediscovered how it was\n> supposed to work and saw how it actually does work as designed, I\n> would not be surprised if it is still used in the wild.\n\nSo, the conclusion: the proposed log message needs a large revamp.\nThanks.\n"},{"id":"404536","messageId":"xmqqh7spwess.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"nycvar.QRO.7.76.6.2008261004120.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T16:30:11Z","receivedAt":"2020-08-26T16:30:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Tue, 25 Aug 2020, Junio C Hamano wrote:\n>\n>> If we were to propose removing \"git-foo\" binaries from the\n>> filesystem for built-in commands, we should first see if there are\n>> users who will be affected by such a move. When cmd_main() detects\n>> we were called not as \"git\", but as \"git-foo\", give an error message\n>> to ask the user to let us know and stop our removal plan, unless we\n>> are running a selected few programs that MUST be callable in the\n>> dashed form (e.g. \"git-upload-pack\").\n>>\n>> Those who are always using \"git foo\" form will not be affected, but\n>> those who trusted the promise we made to them 12 years ago that by\n>> prepending the output of $(git --exec-path) to the list of\n>> directories on their $PATH, they can safely keep writing\n>> \"git-cat-file\" will be negatively affected as all their scripts\n>> assuming the promise will be kept are now broken.\n>\n> It might be a good idea to also instrument the existing scripts, to show\n> the same warning unless they were called through the `git` binary.\n>\n> _If_ we were to do this ;-)\n\nSure. \n\nI am not the advocate for removing builtins from on-disk, though.\nThe burden of proof... ;-)\n\n\n"},{"id":"404538","messageId":"xmqqd03dwe2x.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"nycvar.QRO.7.76.6.2008261007100.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 0/3] War on dashed-git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T16:45:42Z","receivedAt":"2020-08-26T16:48:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> So maybe this patch series would be a good first step, but if we truly\n> wanted to break that 12-year old promise, we might need to have another\n> patch on top that _does_ install the dashed commands, but into a\n> subdirectory of `libexec/git-core/` that is only added to the `PATH` when\n> an \"escape hatch\"-style config setting is set.\n\nYes, I think that is a much more reasonable approach. Instead of\nchecking another environment variable that does not do anything but\nbypass the \"you used dashed form for builtin\" check, we install non\nbuiltins in libexec/git-core and leave the exec-cmd.c::setup_path()\nas-is to add it to the PATH. A new location will have the buitlin\nbinaries, say in libexec/git-core/builtins, and it is not added to\nthe $PATH by us. The scripts that the users updated 12-years ago by\nadding the former to the $PATH now needs to also add the latter,\ntoo, and those users will loudly complain (which is what we want to\nsee happen [*1*]), but doing so is an easy way to unbreak them while\nwe reverse the course.\n\nI think the cvsexportcommit and transport-helper changes are worth\nsalvaging even if we don't remove builtin binaries, so I'll split\nthem and whip them a bit more into a reasonable shape to be merged\nto 'next'. The \"break those who say 'git-cat-file'\" can be left for\nfuture.\n\nThanks.\n\n\n[Footnote]\n\n*1* In the ancient thread, I was sick of hearing complaints that\nbeat the dead horse, but in this particular case, we do want to hear\nfrom them---that is the primary reason why we are doing it.\n\nhttps://public-inbox.org/git/7vr68b8q9p.fsf@gitster.siamese.dyndns.org/\n\n"},{"id":"404544","messageId":"20200826194650.4031087-1-gitster@pobox.com","threadId":"54128","inReplyTo":"xmqqd03dwe2x.fsf@gitster.c.googlers.com","subject":"[PATCH v2 0/2] avoid running \"git-subcmd\" in the dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T19:46:48Z","receivedAt":"2020-08-26T19:46:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I think the cvsexportcommit and transport-helper changes are worth\n> salvaging even if we don't remove builtin binaries, so I'll split\n> them and whip them a bit more into a reasonable shape to be merged\n> to 'next'. The \"break those who say 'git-cat-file'\" can be left for\n> future.\n\nAnd here it is.\n\nThese two patches are clean-ups that are worth doing whether we\ndecide to remove on-disk binaries for the builtin commands. \n\nI droped the third one, that actually stops users from running\nbuilt-in commands using the dashed form, at least for now. It can\nbe resurrected later if we really wants to propose removal to the\nusers, but I am not inclined to make such a proposal right now.\n\nJunio C Hamano (2):\n transport-helper: do not run git-remote-ext etc. in dashed form\n cvsexportcommit: do not run git programs in dashed form\n\n git-cvsexportcommit.perl | 16 ++++++++--------\n transport-helper.c | 4 ++--\n 2 files changed, 10 insertions(+), 10 deletions(-)\n\n-- \n2.28.0-454-g5f859b1948\n\n"},{"id":"404545","messageId":"20200826194650.4031087-2-gitster@pobox.com","threadId":"54128","inReplyTo":"20200826194650.4031087-1-gitster@pobox.com","subject":"[PATCH v2 1/2] transport-helper: do not run git-remote-ext etc. in dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T19:46:49Z","receivedAt":"2020-08-26T19:47:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Running it as \"git remote-ext\" and letting \"git\" dispatch to\n\"remote-ext\" would just be fine and is more idiomatic.\n\nSigned-off-by: Junio C Hamano \n---\n transport-helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex defafbf4c1..c52c99d829 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -128,10 +128,10 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->in = -1;\n \thelper->out = -1;\n \thelper->err = 0;\n-\tstrvec_pushf(&helper->args, \"git-remote-%s\", data->name);\n+\tstrvec_pushf(&helper->args, \"remote-%s\", data->name);\n \tstrvec_push(&helper->args, transport->remote->name);\n \tstrvec_push(&helper->args, remove_ext_force(transport->url));\n-\thelper->git_cmd = 0;\n+\thelper->git_cmd = 1;\n \thelper->silent_exec_failure = 1;\n \n \tif (have_git_dir())\n-- \n2.28.0-454-g5f859b1948\n\n"},{"id":"404546","messageId":"20200826194650.4031087-3-gitster@pobox.com","threadId":"54128","inReplyTo":"20200826194650.4031087-1-gitster@pobox.com","subject":"[PATCH v2 2/2] cvsexportcommit: do not run git programs in dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T19:46:50Z","receivedAt":"2020-08-26T19:47:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This ancient script runs \"git-foo\" all over the place, which is\nOK for a scripted Porcelain in the Git suite, but asking \"git\" to\ndispatch to subcommands is the usual way these days.\n\nSigned-off-by: Junio C Hamano \n---\n git-cvsexportcommit.perl | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 0ae8bce3fb..289d4bc684 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -30,7 +30,7 @@\n \t# Remember where GIT_DIR is before changing to CVS checkout\n \tunless ($ENV{GIT_DIR}) {\n \t\t# No GIT_DIR set. Figure it out for ourselves\n-\t\tmy $gd =`git-rev-parse --git-dir`;\n+\t\tmy $gd =`git rev-parse --git-dir`;\n \t\tchomp($gd);\n \t\t$ENV{GIT_DIR} = $gd;\n \t}\n@@ -66,7 +66,7 @@\n # resolve target commit\n my $commit;\n $commit = pop @ARGV;\n-$commit = safe_pipe_capture('git-rev-parse', '--verify', \"$commit^0\");\n+$commit = safe_pipe_capture('git', 'rev-parse', '--verify', \"$commit^0\");\n chomp $commit;\n if ($?) {\n die \"The commit reference $commit did not resolve!\";\n@@ -76,7 +76,7 @@\n my $parent;\n if (@ARGV) {\n $parent = pop @ARGV;\n- $parent = safe_pipe_capture('git-rev-parse', '--verify', \"$parent^0\");\n+ $parent = safe_pipe_capture('git', 'rev-parse', '--verify', \"$parent^0\");\n chomp $parent;\n if ($?) {\n \tdie \"The parent reference did not resolve!\";\n@@ -84,7 +84,7 @@\n }\n \n # find parents from the commit itself\n-my @commit = safe_pipe_capture('git-cat-file', 'commit', $commit);\n+my @commit = safe_pipe_capture('git', 'cat-file', 'commit', $commit);\n my @parents;\n my $committer;\n my $author;\n@@ -162,9 +162,9 @@\n close MSG;\n \n if ($parent eq $noparent) {\n- `git-diff-tree --binary -p --root $commit >.cvsexportcommit.diff`;# || die \"Cannot diff\";\n+ `git diff-tree --binary -p --root $commit >.cvsexportcommit.diff`;# || die \"Cannot diff\";\n } else {\n- `git-diff-tree --binary -p $parent $commit >.cvsexportcommit.diff`;# || die \"Cannot diff\";\n+ `git diff-tree --binary -p $parent $commit >.cvsexportcommit.diff`;# || die \"Cannot diff\";\n }\n \n ## apply non-binary changes\n@@ -178,7 +178,7 @@\n print \"Checking if patch will apply\\n\";\n \n my @stat;\n-open APPLY, \"GIT_INDEX_FILE=$tmpdir/index git-apply $context --summary --numstat<.cvsexportcommit.diff|\" || die \"cannot patch\";\n+open APPLY, \"GIT_INDEX_FILE=$tmpdir/index git apply $context --summary --numstat<.cvsexportcommit.diff|\" || die \"cannot patch\";\n @stat=;\n close APPLY || die \"Cannot patch\";\n my (@bfiles,@files,@afiles,@dfiles);\n@@ -333,7 +333,7 @@\n if ($opt_W) {\n system(\"git checkout -q $commit^0\") && die \"cannot patch\";\n } else {\n- `GIT_INDEX_FILE=$tmpdir/index git-apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die \"cannot patch\";\n+ `GIT_INDEX_FILE=$tmpdir/index git apply $context --summary --numstat --apply <.cvsexportcommit.diff` || die \"cannot patch\";\n }\n \n print \"Patch applied successfully. Adding new files and directories to CVS\\n\";\n-- \n2.28.0-454-g5f859b1948\n\n"},{"id":"404551","messageId":"a9997a0b-9ba9-4614-2081-fc74e4c2171a@gmail.com","threadId":"54128","inReplyTo":"87a3b7a5a2f091e2a23c163a7d86effbbbedfa3a.1598371475.git.me@ttaylorr.com","subject":"Re: [PATCH v2] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-26T20:51:29Z","receivedAt":"2020-08-26T20:51:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/25/2020 12:04 PM, Taylor Blau wrote:\n> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n> learned to remove a multi-pack-index file if it added or removed a pack\n> from the object store.\n> \n> This mechanism is a little over-eager, since it is only necessary to\n> drop a MIDX if 'git repack' removes a pack that the MIDX references.\n> Adding a pack outside of the MIDX does not require invalidating the\n> MIDX, and likewise for removing a pack the MIDX does not know about.\n> \n> Teach 'git repack' to check for this by loading the MIDX, and checking\n> whether the to-be-removed pack is known to the MIDX. This requires a\n> slightly odd alternation to a test in t5319, which is explained with a\n> comment. A new test is added to show that the MIDX is left alone when\n> both packs known to it are marked as .keep, but two packs unknown to it\n> are removed and combined into one new pack.\n> \n> Helped-by: Derrick Stolee \n> Signed-off-by: Taylor Blau \n> ---\n> Range-diff against v1:\n\nI know this thread went in a new direction about pack-redundant and\ndashed-git, but this version looks good to me. I wanted to make sure\nit wasn't lost in the shuffle.\n\nThanks,\n-Stolee\n\n"},{"id":"404552","messageId":"xmqq5z95unzt.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"a9997a0b-9ba9-4614-2081-fc74e4c2171a@gmail.com","subject":"Re: [PATCH v2] builtin/repack.c: invalidate MIDX only when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T20:54:30Z","receivedAt":"2020-08-26T20:54:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 8/25/2020 12:04 PM, Taylor Blau wrote:\n>> In 525e18c04b (midx: clear midx on repack, 2018-07-12), 'git repack'\n>> learned to remove a multi-pack-index file if it added or removed a pack\n>> from the object store.\n>> \n>> This mechanism is a little over-eager, since it is only necessary to\n>> drop a MIDX if 'git repack' removes a pack that the MIDX references.\n>> Adding a pack outside of the MIDX does not require invalidating the\n>> MIDX, and likewise for removing a pack the MIDX does not know about.\n>> \n>> Teach 'git repack' to check for this by loading the MIDX, and checking\n>> whether the to-be-removed pack is known to the MIDX. This requires a\n>> slightly odd alternation to a test in t5319, which is explained with a\n>> comment. A new test is added to show that the MIDX is left alone when\n>> both packs known to it are marked as .keep, but two packs unknown to it\n>> are removed and combined into one new pack.\n>> \n>> Helped-by: Derrick Stolee \n>> Signed-off-by: Taylor Blau \n>> ---\n>> Range-diff against v1:\n>\n> I know this thread went in a new direction about pack-redundant and\n> dashed-git, but this version looks good to me. I wanted to make sure\n> it wasn't lost in the shuffle.\n\nVery much appreciated. I was wondering if I'll see a final reroll\nor v2 already had all we wanted.\n\nThanks.\n"},{"id":"404553","messageId":"xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200826194650.4031087-3-gitster@pobox.com","subject":"[PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T21:37:39Z","receivedAt":"2020-08-26T21:37:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As child_process structure has an embedded strvec args for\nformulating the command line, let's use it instead of using\nan out-of-line argv[] whose length needs to be maintained\ncorrectly. \n\nSigned-off-by: Junio C Hamano \n---\n builtin/credential-cache.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\nindex d0fafdeb9e..195335a783 100644\n--- a/builtin/credential-cache.c\n+++ b/builtin/credential-cache.c\n@@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out)\n static void spawn_daemon(const char *socket)\n {\n \tstruct child_process daemon = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = { NULL, NULL, NULL };\n \tchar buf[128];\n \tint r;\n \n-\targv[0] = \"git-credential-cache--daemon\";\n-\targv[1] = socket;\n-\tdaemon.argv = argv;\n+\tstrvec_pushl(&daemon.args, \n+\t\t \"credential-cache--daemon\", socket,\n+\t\t NULL);\n+\tdaemon.git_cmd = 1;\n \tdaemon.no_stdin = 1;\n \tdaemon.out = -1;\n \n"},{"id":"404557","messageId":"xmqqmu2ht58g.fsf_-_@gitster.c.googlers.com","threadId":"54128","inReplyTo":"xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com","subject":"[PATCH] run_command: teach API users to use embedded 'args' more","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-26T22:25:03Z","receivedAt":"2020-08-26T22:25:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The child_process structure has an embedded strvec for formulating\nthe command line argument list these days, but code that predates\nthe wide use of it prepared a separate char *argv[] array and\nmanually set the child_process.argv pointer point at it.\n\nTeach these old-style code to lose the separate argv[] array.\n\nSigned-off-by: Junio C Hamano \n---\n convert.c | 5 +----\n credential.c | 4 +---\n submodule.c | 13 ++++---------\n trailer.c | 4 +---\n 4 files changed, 7 insertions(+), 19 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 572449825c..8e6c292421 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -638,7 +638,6 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tstruct child_process child_process = CHILD_PROCESS_INIT;\n \tstruct filter_params *params = (struct filter_params *)data;\n \tint write_err, status;\n-\tconst char *argv[] = { NULL, NULL };\n \n \t/* apply % substitution to cmd */\n \tstruct strbuf cmd = STRBUF_INIT;\n@@ -656,9 +655,7 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tstrbuf_expand(&cmd, params->cmd, strbuf_expand_dict_cb, &dict);\n \tstrbuf_release(&path);\n \n-\targv[0] = cmd.buf;\n-\n-\tchild_process.argv = argv;\n+\tstrvec_push(&child_process.args, cmd.buf);\n \tchild_process.use_shell = 1;\n \tchild_process.in = -1;\n \tchild_process.out = out;\ndiff --git a/credential.c b/credential.c\nindex d8d226b97e..efc29dc5e1 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -274,11 +274,9 @@ static int run_credential_helper(struct credential *c,\n \t\t\t\t int want_output)\n {\n \tstruct child_process helper = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = { NULL, NULL };\n \tFILE *fp;\n \n-\targv[0] = cmd;\n-\thelper.argv = argv;\n+\tstrvec_push(&helper.args, cmd);\n \thelper.use_shell = 1;\n \thelper.in = -1;\n \tif (want_output)\ndiff --git a/submodule.c b/submodule.c\nindex 01697848be..e6086faff7 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1727,14 +1727,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n int submodule_uses_gitfile(const char *path)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {\n-\t\t\"submodule\",\n-\t\t\"foreach\",\n-\t\t\"--quiet\",\n-\t\t\"--recursive\",\n-\t\t\"test -f .git\",\n-\t\tNULL,\n-\t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *git_dir;\n \n@@ -1747,7 +1739,10 @@ int submodule_uses_gitfile(const char *path)\n \tstrbuf_release(&buf);\n \n \t/* Now test that all nested submodules use a gitfile too */\n-\tcp.argv = argv;\n+\tstrvec_pushl(&cp.args,\n+\t\t \"submodule\", \"foreach\", \"--quiet\",\t\"--recursive\",\n+\t\t \"test -f .git\", NULL);\n+\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\ndiff --git a/trailer.c b/trailer.c\nindex 0c414f2fed..68dabc2556 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -221,15 +221,13 @@ static char *apply_command(const char *command, const char *arg)\n \tstruct strbuf cmd = STRBUF_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {NULL, NULL};\n \tchar *result;\n \n \tstrbuf_addstr(&cmd, command);\n \tif (arg)\n \t\tstrbuf_replace(&cmd, TRAILER_ARG_STRING, arg);\n \n-\targv[0] = cmd.buf;\n-\tcp.argv = argv;\n+\tstrvec_push(&cp.args, cmd.buf);\n \tcp.env = local_repo_env;\n \tcp.no_stdin = 1;\n \tcp.use_shell = 1;\n"},{"id":"404566","messageId":"07f26226-c1cd-494d-899e-d6452ad2751f@gmail.com","threadId":"54128","inReplyTo":"20200826194650.4031087-1-gitster@pobox.com","subject":"Re: [PATCH v2 0/2] avoid running \"git-subcmd\" in the dashed form","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-27T00:57:45Z","receivedAt":"2020-08-27T00:57:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/26/2020 3:46 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> I think the cvsexportcommit and transport-helper changes are worth\n>> salvaging even if we don't remove builtin binaries, so I'll split\n>> them and whip them a bit more into a reasonable shape to be merged\n>> to 'next'. The \"break those who say 'git-cat-file'\" can be left for\n>> future.\n> \n> And here it is.\n> \n> These two patches are clean-ups that are worth doing whether we\n> decide to remove on-disk binaries for the builtin commands. \n\nLGTM.\n\n> I droped the third one, that actually stops users from running\n> built-in commands using the dashed form, at least for now. It can\n> be resurrected later if we really wants to propose removal to the\n> users, but I am not inclined to make such a proposal right now.\n\nI think that's fine for now. A plan to fully deprecate the dashed\nforms can come later.\n\nWould an interesting in-between step include removing the dashed\nforms for builtins that didn't exist in Git 2.0?\n\nThanks,\n-Stolee\n"},{"id":"404567","messageId":"xmqqy2m0swzz.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"07f26226-c1cd-494d-899e-d6452ad2751f@gmail.com","subject":"Re: [PATCH v2 0/2] avoid running \"git-subcmd\" in the dashed form","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T01:22:56Z","receivedAt":"2020-08-27T01:23:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Would an interesting in-between step include removing the dashed\n> forms for builtins that didn't exist in Git 2.0?\n\nI am not sure if it makes much sense to treat newer and older\nbuilt-in commands differently.\n\nImagine that an old-timer wrote a script by somebody who trusted the\n\"futz with PATH and you can use git-foo\" promise before Git 2.0 and\nthen the script was inherited by relatively new users. Adhering to\nthe \"when in doubt, mimic the surrounding code\", which is usually a\ngood discipline to follow, these new users, who are now in charge of\nmaintaining the script, would add any new calls in \"git-foo\" form to\nmatch the local convention in good faith. And the resulting code\nwould have been working just fine.\n\nBefore such a \"in-between step\" is thrown at them, that is, at which\npoint it stops working if they were unlucky that they used a\nrelatively new built-ins. Typically new end-users would not know\nwhich ones are old built-ins and which ones are new, I suspect.\n\nI do agree with Dscho that \"we won't let you use builtin in dashed\nform before you export an enviornment\" I wrote was not a good way to\ngauge the usage of on-disk builtins. We should move the on-disk\nbuiltins to a different directory and have them point at the\nlocation with their $PATH as the escape hatch, as Dscho suggested,\nif we were to do this for real, I'd think.\n\nThanks.\n"},{"id":"404571","messageId":"20200827041328.GA3346457@coredump.intra.peff.net","threadId":"54128","inReplyTo":"xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-27T04:13:28Z","receivedAt":"2020-08-27T04:13:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote:\n\n> diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\n> index d0fafdeb9e..195335a783 100644\n> --- a/builtin/credential-cache.c\n> +++ b/builtin/credential-cache.c\n> @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out)\n> static void spawn_daemon(const char *socket)\n> {\n> \tstruct child_process daemon = CHILD_PROCESS_INIT;\n> -\tconst char *argv[] = { NULL, NULL, NULL };\n> \tchar buf[128];\n> \tint r;\n> \n> -\targv[0] = \"git-credential-cache--daemon\";\n> -\targv[1] = socket;\n> -\tdaemon.argv = argv;\n> +\tstrvec_pushl(&daemon.args, \n> +\t\t \"credential-cache--daemon\", socket,\n> +\t\t NULL);\n> +\tdaemon.git_cmd = 1;\n\nYep, this makes sense. I don't recall any reason to use the dashed form\nback then, but probably it was just that I knew it was a separate\nprogram. Doing it this way will mean an extra parent \"git\" process\nhanging around, but I don't think it's that big a deal. We never try to\nkill it by PID, etc (instead we connect to the socket and ask it to\nexit). And anyway, it is becoming a builtin in a parallel topic, so that\nextra process will go away. :)\n\n-Peff\n"},{"id":"404572","messageId":"20200827041456.GB3346457@coredump.intra.peff.net","threadId":"54128","inReplyTo":"xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-27T04:14:56Z","receivedAt":"2020-08-27T04:15:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote:\n\n> As child_process structure has an embedded strvec args for\n> formulating the command line, let's use it instead of using\n> an out-of-line argv[] whose length needs to be maintained\n> correctly.\n\nI forgot to mention in my other reply: I think this fails to mention the\nactual functional change, which is switching from the dashed form to\nusing the \"git\" wrapper.\n\n-Peff\n"},{"id":"404574","messageId":"20200827042157.GC3346457@coredump.intra.peff.net","threadId":"54128","inReplyTo":"xmqqmu2ht58g.fsf_-_@gitster.c.googlers.com","subject":"Re: [PATCH] run_command: teach API users to use embedded 'args' more","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-27T04:21:57Z","receivedAt":"2020-08-27T04:22:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 26, 2020 at 03:25:03PM -0700, Junio C Hamano wrote:\n\n> The child_process structure has an embedded strvec for formulating\n> the command line argument list these days, but code that predates\n> the wide use of it prepared a separate char *argv[] array and\n> manually set the child_process.argv pointer point at it.\n> \n> Teach these old-style code to lose the separate argv[] array.\n\nI think the result is much nicer and less error-prone (especially the\nones that pre-size the array with NULLs and fill in the components\nlater). It incurs a few extra mallocs at run-time, but on top of an\nexecve(), that's a drop in the bucket.\n\nI've actually considered dropping child_process.argv entirely. Having\ntwo separate ways to do the same thing gives the potential for\nconfusion. But I never dug into whether any existing callers would be\nmade worse for it (I kind of doubt it, though; worst case they can use\nstrvec_pushv). There are still several left after this patch, it seems.\n\nLikewise for child_process.env_array.\n\n-Peff\n"},{"id":"404575","messageId":"20200827042234.GD3346457@coredump.intra.peff.net","threadId":"54128","inReplyTo":"20200827041328.GA3346457@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-27T04:22:34Z","receivedAt":"2020-08-27T04:22:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 27, 2020 at 12:13:28AM -0400, Jeff King wrote:\n\n> On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote:\n> \n> > diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\n> > index d0fafdeb9e..195335a783 100644\n> > --- a/builtin/credential-cache.c\n> > +++ b/builtin/credential-cache.c\n> > @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out)\n> > static void spawn_daemon(const char *socket)\n> > {\n> > \tstruct child_process daemon = CHILD_PROCESS_INIT;\n> > -\tconst char *argv[] = { NULL, NULL, NULL };\n> > \tchar buf[128];\n> > \tint r;\n> > \n> > -\targv[0] = \"git-credential-cache--daemon\";\n> > -\targv[1] = socket;\n> > -\tdaemon.argv = argv;\n> > +\tstrvec_pushl(&daemon.args, \n> > +\t\t \"credential-cache--daemon\", socket,\n> > +\t\t NULL);\n> > +\tdaemon.git_cmd = 1;\n> \n> Yep, this makes sense. I don't recall any reason to use the dashed form\n> back then, but probably it was just that I knew it was a separate\n> program. Doing it this way will mean an extra parent \"git\" process\n> hanging around, but I don't think it's that big a deal. We never try to\n> kill it by PID, etc (instead we connect to the socket and ask it to\n> exit). And anyway, it is becoming a builtin in a parallel topic, so that\n> extra process will go away. :)\n\n...and if I had read the diff header more carefully, I'd see that you\ndid this on top of that topic. :)\n\n-Peff\n"},{"id":"404577","messageId":"xmqqlfi0sobu.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200827042157.GC3346457@coredump.intra.peff.net","subject":"Re: [PATCH] run_command: teach API users to use embedded 'args' more","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T04:30:13Z","receivedAt":"2020-08-27T04:30:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I've actually considered dropping child_process.argv entirely. Having\n> two separate ways to do the same thing gives the potential for\n> confusion. But I never dug into whether any existing callers would be\n> made worse for it (I kind of doubt it, though; worst case they can use\n> strvec_pushv). There are still several left after this patch, it seems.\n>\n> Likewise for child_process.env_array.\n\nYup, conversion similar to what I did in this patch may be too\ntrivial for #microproject, but would nevertheless be a good\n#leftoverbits task. The removal of .argv/.env is not entirely\ntrivial but a good candidate for #microproject.\n\nThanks.\n\n"},{"id":"404578","messageId":"xmqqh7soso9v.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200827041328.GA3346457@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T04:31:24Z","receivedAt":"2020-08-27T04:31:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yep, this makes sense. I don't recall any reason to use the dashed form\n> back then, but probably it was just that I knew it was a separate\n> program. Doing it this way will mean an extra parent \"git\" process\n> hanging around, but I don't think it's that big a deal. We never try to\n> kill it by PID, etc (instead we connect to the socket and ask it to\n> exit). And anyway, it is becoming a builtin in a parallel topic, so that\n> extra process will go away. :)\n\nYeah, this was discovered when I tentatively merged that \"don't run\nbuilt-in as git-foo\" to the tip of seen. Without your slimmed-down\ntopic, it wouldn't have been caught. Likewise for remote-ext.\n\n"},{"id":"404579","messageId":"CAPig+cS1uMw6YDVjzb8FbBmC=iVjged-wHu0LF2+trmW-4ZfVw@mail.gmail.com","threadId":"54128","inReplyTo":"20200827042157.GC3346457@coredump.intra.peff.net","subject":"Re: [PATCH] run_command: teach API users to use embedded 'args' more","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-27T04:31:52Z","receivedAt":"2020-08-27T04:32:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 27, 2020 at 12:22 AM Jeff King wrote:\n> I've actually considered dropping child_process.argv entirely. Having\n> two separate ways to do the same thing gives the potential for\n> confusion. [...]\n>\n> Likewise for child_process.env_array.\n\nbuiltin/worktree.c:add_worktree() is a case in which an environment\nstrvec is built once and re-used for multiple run_command()\ninvocations by re-assigning it to child_process.env before each\nrun_command(). It uses child_process.env rather than\nchild_process.env_array because run_command() clears\nchild_process.env_array upon completion, which makes it difficult to\nreuse a prepared environment strvec repeatedly.\n\nNevertheless, that isn't much of a reason to keep child_process.env.\nRefactoring add_worktree() a bit to rebuild the environment strvec\non-demand wouldn't be very difficult.\n"},{"id":"404581","messageId":"20200827044420.GA3360616@coredump.intra.peff.net","threadId":"54128","inReplyTo":"CAPig+cS1uMw6YDVjzb8FbBmC=iVjged-wHu0LF2+trmW-4ZfVw@mail.gmail.com","subject":"Re: [PATCH] run_command: teach API users to use embedded 'args' more","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-27T04:44:20Z","receivedAt":"2020-08-27T04:44:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 27, 2020 at 12:31:52AM -0400, Eric Sunshine wrote:\n\n> On Thu, Aug 27, 2020 at 12:22 AM Jeff King wrote:\n> > I've actually considered dropping child_process.argv entirely. Having\n> > two separate ways to do the same thing gives the potential for\n> > confusion. [...]\n> >\n> > Likewise for child_process.env_array.\n> \n> builtin/worktree.c:add_worktree() is a case in which an environment\n> strvec is built once and re-used for multiple run_command()\n> invocations by re-assigning it to child_process.env before each\n> run_command(). It uses child_process.env rather than\n> child_process.env_array because run_command() clears\n> child_process.env_array upon completion, which makes it difficult to\n> reuse a prepared environment strvec repeatedly.\n> \n> Nevertheless, that isn't much of a reason to keep child_process.env.\n> Refactoring add_worktree() a bit to rebuild the environment strvec\n> on-demand wouldn't be very difficult.\n\nI think they'd still be one-liner changes, like:\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 378f332b5d..b40f0d4cea 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -422,7 +422,7 @@ static int add_worktree(const char *path, const char *refname,\n \t\t\tstrvec_push(&cp.args, \"--quiet\");\n \t}\n \n-\tcp.env = child_env.v;\n+\tstrvec_pushv(&cp.env_array, child_env.v);\n \tret = run_command(&cp);\n \tif (ret)\n \t\tgoto done;\n@@ -433,7 +433,7 @@ static int add_worktree(const char *path, const char *refname,\n \t\tstrvec_pushl(&cp.args, \"reset\", \"--hard\", \"--no-recurse-submodules\", NULL);\n \t\tif (opts->quiet)\n \t\t\tstrvec_push(&cp.args, \"--quiet\");\n-\t\tcp.env = child_env.v;\n+\t\tstrvec_pushv(&cp.env_array, child_env.v);\n \t\tret = run_command(&cp);\n \t\tif (ret)\n \t\t\tgoto done;\n\nI think there are other opportunities for cleanup, too:\n\n - the code right above the second hunk clears cp.args manually. That\n shouldn't be necessary because run_command() will leave it in a\n blank state (and we're already relying on that, since otherwise we'd\n be left with cruft in other fields from the previous run).\n\n - check_clean_worktree() only uses it once, and could drop the\n separate child_env (and in fact appears to leak it)\n\n-Peff\n"},{"id":"404582","messageId":"CAPig+cSA56xgNN0WP4t+YoyNU8fGf5eaz__=4Vh+s=He-tG=DA@mail.gmail.com","threadId":"54128","inReplyTo":"20200827044420.GA3360616@coredump.intra.peff.net","subject":"Re: [PATCH] run_command: teach API users to use embedded 'args' more","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-27T05:03:43Z","receivedAt":"2020-08-27T05:06:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 27, 2020 at 12:44 AM Jeff King wrote:\n> On Thu, Aug 27, 2020 at 12:31:52AM -0400, Eric Sunshine wrote:\n> > Nevertheless, that isn't much of a reason to keep child_process.env.\n> > Refactoring add_worktree() a bit to rebuild the environment strvec\n> > on-demand wouldn't be very difficult.\n>\n> I think they'd still be one-liner changes, like:\n>\n> - cp.env = child_env.v;\n> + strvec_pushv(&cp.env_array, child_env.v);\n\nNice and simple.\n\n> I think there are other opportunities for cleanup, too:\n\nTrue on both counts.\n\n> - the code right above the second hunk clears cp.args manually. That\n> shouldn't be necessary because run_command() will leave it in a\n> blank state (and we're already relying on that, since otherwise we'd\n> be left with cruft in other fields from the previous run).\n\nRight. I wonder why the author of 7f44e3d1de (worktree: make setup of\nnew HEAD distinct from worktree population, 2015-07-17) chose to clear\ncp.args manually like that.\n\n> - check_clean_worktree() only uses it once, and could drop the\n> separate child_env (and in fact appears to leak it)\n\nPerhaps this unnecessary 'child_env' strvec was a copy/paste from\nadd_worktree()? But certainly cp.env_array would be simpler and avoid\nthe leak.\n"},{"id":"404583","messageId":"20200827052504.GA3360984@coredump.intra.peff.net","threadId":"54128","inReplyTo":"CAPig+cSA56xgNN0WP4t+YoyNU8fGf5eaz__=4Vh+s=He-tG=DA@mail.gmail.com","subject":"[PATCH] worktree: fix leak in check_clean_worktree()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-27T05:25:04Z","receivedAt":"2020-08-27T05:25:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 27, 2020 at 01:03:43AM -0400, Eric Sunshine wrote:\n\n> > - the code right above the second hunk clears cp.args manually. That\n> > shouldn't be necessary because run_command() will leave it in a\n> > blank state (and we're already relying on that, since otherwise we'd\n> > be left with cruft in other fields from the previous run).\n> \n> Right. I wonder why the author of 7f44e3d1de (worktree: make setup of\n> new HEAD distinct from worktree population, 2015-07-17) chose to clear\n> cp.args manually like that.\n\nI wondered if we might not have cleared the array automatically back\nthen, but it looks like we did.\n\nI do think this kind of child_process struct reuse is slightly sketchy\nin general. Looking at child_process_clear(), we only free the memory,\nbut leave other fields set. And in fact we rely on that here; git_cmd\nneeds to remain set for both commands to work. But if the first command\nhad used, say, cp.in, then we'd be left with a bogus descriptor.\n\n> > - check_clean_worktree() only uses it once, and could drop the\n> > separate child_env (and in fact appears to leak it)\n> \n> Perhaps this unnecessary 'child_env' strvec was a copy/paste from\n> add_worktree()? But certainly cp.env_array would be simpler and avoid\n> the leak.\n\nYeah, that was my guess, too.\n\nMost of these issues are more complex and/or should be part of a larger\ncleanup effort. But let's fix the leak while we're thinking about it.\n\n-- >8 --\nSubject: [PATCH] worktree: fix leak in check_clean_worktree()\n\nWe allocate a child_env strvec but never free its memory. Instead, let's\njust use the strvec that our child_process struct provides, which is\ncleaned up automatically when we run the command.\n\nAnd while we're moving the initialization of the child_process around,\nlet's switch it to use the official init function (zero-initializing it\nworks OK, since strvec is happy enough with that, but it sets a bad\nexample).\n\nSigned-off-by: Jeff King \n---\n builtin/worktree.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 378f332b5d..df214697d2 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -924,7 +924,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n static void check_clean_worktree(struct worktree *wt,\n \t\t\t\t const char *original_path)\n {\n-\tstruct strvec child_env = STRVEC_INIT;\n \tstruct child_process cp;\n \tchar buf[1];\n \tint ret;\n@@ -935,15 +934,14 @@ static void check_clean_worktree(struct worktree *wt,\n \t */\n \tvalidate_no_submodules(wt);\n \n-\tstrvec_pushf(&child_env, \"%s=%s/.git\",\n+\tchild_process_init(&cp);\n+\tstrvec_pushf(&cp.env_array, \"%s=%s/.git\",\n \t\t GIT_DIR_ENVIRONMENT, wt->path);\n-\tstrvec_pushf(&child_env, \"%s=%s\",\n+\tstrvec_pushf(&cp.env_array, \"%s=%s\",\n \t\t GIT_WORK_TREE_ENVIRONMENT, wt->path);\n-\tmemset(&cp, 0, sizeof(cp));\n \tstrvec_pushl(&cp.args, \"status\",\n \t\t \"--porcelain\", \"--ignore-submodules=none\",\n \t\t NULL);\n-\tcp.env = child_env.v;\n \tcp.git_cmd = 1;\n \tcp.dir = wt->path;\n \tcp.out = -1;\n-- \n2.28.0.751.g0834cceced\n\n"},{"id":"404585","messageId":"CAPig+cQxvq3MzyB3e8-ZeVSdCot04=9p4L8CZRnpYbrmnR70_g@mail.gmail.com","threadId":"54128","inReplyTo":"20200827052504.GA3360984@coredump.intra.peff.net","subject":"Re: [PATCH] worktree: fix leak in check_clean_worktree()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-27T05:56:47Z","receivedAt":"2020-08-27T06:00:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 27, 2020 at 1:25 AM Jeff King wrote:\n> On Thu, Aug 27, 2020 at 01:03:43AM -0400, Eric Sunshine wrote:\n> > Right. I wonder why the author of 7f44e3d1de (worktree: make setup of\n> > new HEAD distinct from worktree population, 2015-07-17) chose to clear\n> > cp.args manually like that.\n>\n> I wondered if we might not have cleared the array automatically back\n> then, but it looks like we did.\n\nI had the same thought and came to the same conclusion. It's possible\nthat the manual clearing of the array was leftover cruft as the\nimplementation matured before the patch was submitted. I have a vague\n(perhaps false) recollection that a local argv_array was populated and\nassigned to cp.argv originally, until cp.args was discovered as a\ncleaner alternative and used instead. If that was the case, then the\nlocal argv_array wouldn't have been cleared automatically by\nrun_command(), which would account for clearing it manually.\n\n> I do think this kind of child_process struct reuse is slightly sketchy\n> in general. Looking at child_process_clear(), we only free the memory,\n> but leave other fields set. And in fact we rely on that here; git_cmd\n> needs to remain set for both commands to work. But if the first command\n> had used, say, cp.in, then we'd be left with a bogus descriptor.\n\nAgreed. The current usage in worktree.c is a bit too familiar with the\ncurrent internal implementation of run_command(). Reinitializing the\nchild_process struct or using a separate one would be a good cleanup.\n\n> -- >8 --\n> Subject: [PATCH] worktree: fix leak in check_clean_worktree()\n>\n> We allocate a child_env strvec but never free its memory. Instead, let's\n> just use the strvec that our child_process struct provides, which is\n> cleaned up automatically when we run the command.\n>\n> And while we're moving the initialization of the child_process around,\n> let's switch it to use the official init function (zero-initializing it\n> works OK, since strvec is happy enough with that, but it sets a bad\n> example).\n\nThe various memset()'s in worktree.c seem to have been inherited (and\nmultiplied) from Duy's original \"git checkout --to\" implementation\n(which later became the basis for \"git worktree add\" after which it\nmutated significantly), and \"git checkout --to\" predates introduction\nof child_process_init().\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -924,7 +924,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n> - struct strvec child_env = STRVEC_INIT;\n> @@ -935,15 +934,14 @@ static void check_clean_worktree(struct worktree *wt,\n> - strvec_pushf(&child_env, \"%s=%s/.git\",\n> + child_process_init(&cp);\n> + strvec_pushf(&cp.env_array, \"%s=%s/.git\",\n> GIT_DIR_ENVIRONMENT, wt->path);\n> - strvec_pushf(&child_env, \"%s=%s\",\n> + strvec_pushf(&cp.env_array, \"%s=%s\",\n> GIT_WORK_TREE_ENVIRONMENT, wt->path);\n> - memset(&cp, 0, sizeof(cp));\n> - cp.env = child_env.v;\n\nLooks good to me. For what it's worth:\n\nReviewed-by: Eric Sunshine \n"},{"id":"404607","messageId":"xmqq8se0rtqi.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"CAPig+cQxvq3MzyB3e8-ZeVSdCot04=9p4L8CZRnpYbrmnR70_g@mail.gmail.com","subject":"Re: [PATCH] worktree: fix leak in check_clean_worktree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T15:31:01Z","receivedAt":"2020-08-27T15:31:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Agreed. The current usage in worktree.c is a bit too familiar with the\n> current internal implementation of run_command(). Reinitializing the\n> child_process struct or using a separate one would be a good cleanup.\n>\n>> -- >8 --\n>> Subject: [PATCH] worktree: fix leak in check_clean_worktree()\n>>\n>> We allocate a child_env strvec but never free its memory. Instead, let's\n>> just use the strvec that our child_process struct provides, which is\n>> cleaned up automatically when we run the command.\n>>\n>> And while we're moving the initialization of the child_process around,\n>> let's switch it to use the official init function (zero-initializing it\n>> works OK, since strvec is happy enough with that, but it sets a bad\n>> example).\n>\n> The various memset()'s in worktree.c seem to have been inherited (and\n> multiplied) from Duy's original \"git checkout --to\" implementation\n> (which later became the basis for \"git worktree add\" after which it\n> mutated significantly), and \"git checkout --to\" predates introduction\n> of child_process_init().\n>\n>> diff --git a/builtin/worktree.c b/builtin/worktree.c\n>> @@ -924,7 +924,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n>> - struct strvec child_env = STRVEC_INIT;\n>> @@ -935,15 +934,14 @@ static void check_clean_worktree(struct worktree *wt,\n>> - strvec_pushf(&child_env, \"%s=%s/.git\",\n>> + child_process_init(&cp);\n>> + strvec_pushf(&cp.env_array, \"%s=%s/.git\",\n>> GIT_DIR_ENVIRONMENT, wt->path);\n>> - strvec_pushf(&child_env, \"%s=%s\",\n>> + strvec_pushf(&cp.env_array, \"%s=%s\",\n>> GIT_WORK_TREE_ENVIRONMENT, wt->path);\n>> - memset(&cp, 0, sizeof(cp));\n>> - cp.env = child_env.v;\n>\n> Looks good to me. For what it's worth:\n>\n> Reviewed-by: Eric Sunshine \n\nThanks, both. This looks good.\n"},{"id":"404608","messageId":"xmqq4koortkl.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200827041456.GB3346457@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T15:34:34Z","receivedAt":"2020-08-27T15:34:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Aug 26, 2020 at 02:37:39PM -0700, Junio C Hamano wrote:\n>\n>> As child_process structure has an embedded strvec args for\n>> formulating the command line, let's use it instead of using\n>> an out-of-line argv[] whose length needs to be maintained\n>> correctly.\n>\n> I forgot to mention in my other reply: I think this fails to mention the\n> actual functional change, which is switching from the dashed form to\n> using the \"git\" wrapper.\n\nTrue. With an extra paragraph.\n\n-- >8 --\nSubject: [PATCH] credential-cache: use child_process.args\n\nAs child_process structure has an embedded strvec args for\nformulating the command line, let's use it instead of using\nan out-of-line argv[] whose length needs to be maintained\ncorrectly.\n\nAlso, when spawning a git subcommand, omit it from the command list\nand instead use the .git_cmd bit in the child_process structure.\n\nSigned-off-by: Junio C Hamano \n---\n credential-cache.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex 1cccc3a0b9..04df61cf02 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -39,13 +39,13 @@ static int send_request(const char *socket, const struct strbuf *out)\n static void spawn_daemon(const char *socket)\n {\n \tstruct child_process daemon = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = { NULL, NULL, NULL };\n \tchar buf[128];\n \tint r;\n \n-\targv[0] = \"git-credential-cache--daemon\";\n-\targv[1] = socket;\n-\tdaemon.argv = argv;\n+\tstrvec_pushl(&daemon.args,\n+\t\t \"credential-cache--daemon\", socket,\n+\t\t NULL);\n+\tdaemon.git_cmd = 1;\n \tdaemon.no_stdin = 1;\n \tdaemon.out = -1;\n \n-- \n2.28.0-462-gf84ddd074d\n\n"},{"id":"404679","messageId":"20200828091454.GA2140991@coredump.intra.peff.net","threadId":"54128","inReplyTo":"xmqq1rjuz6n3.fsf_-_@gitster.c.googlers.com","subject":"Re: [PATCH] pack-redundant: gauge the usage before proposing its removal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-28T09:14:54Z","receivedAt":"2020-08-28T09:14:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:\n\n> The subcommand is unusably slow and the reason why nobody reports it\n> as a performance bug is suspected to be the absense of users. Let's\n> show a big message that asks the user to tell us that they still\n> care about the command when an attempt is made to run the command,\n> with an escape hatch to override it with a command line option.\n\nI was looking at the history here and noticed this topic, which I\nsomehow missed when it happened:\n\n $ git show -s cf0879f7e98d2213503622f780d2ab0dd3f93477\n commit cf0879f7e98d2213503622f780d2ab0dd3f93477\n Merge: 3710f60a80 0e37abd2e8\n Author: Junio C Hamano \n Date: Thu Mar 7 09:59:54 2019 +0900\n \n Merge branch 'sc/pack-redundant'\n \n Update the implementation of pack-redundant for performance in a\n repository with many packfiles.\n \n * sc/pack-redundant:\n pack-redundant: consistent sort method\n pack-redundant: rename pack_list.all_objects\n pack-redundant: new algorithm to find min packs\n pack-redundant: delete redundant code\n pack-redundant: delay creation of unique_objects\n t5323: test cases for git-pack-redundant\n\nSo it sounds like:\n\n - somebody does care enough to use it\n\n - it may not be horrifically slow anymore\n\nSo it may not be worth trying to follow through on the deprecation\n(though the fact that neither of us realized this makes me worried for\nthe general state of maintenance of this code).\n\n-Peff\n"},{"id":"404697","messageId":"nycvar.QRO.7.76.6.2008280412030.56@tvgsbejvaqbjf.bet","threadId":"54128","inReplyTo":"20200826011718.3186597-4-gitster@pobox.com","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-28T02:13:15Z","receivedAt":"2020-08-28T12:53:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 25 Aug 2020, Junio C Hamano wrote:\n\n> diff --git a/git.c b/git.c\n> index 8bd1d7551d..927018bda7 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -839,6 +839,8 @@ int cmd_main(int argc, const char **argv)\n> \t * that one cannot handle it.\n> \t */\n> \tif (skip_prefix(cmd, \"git-\", &cmd)) {\n> +\t\twarn_on_dashed_git(argv[0]);\n> +\n> \t\targv[0] = cmd;\n> \t\thandle_builtin(argc, argv);\n> \t\tdie(_(\"cannot handle %s as a builtin\"), cmd);\n> diff --git a/help.c b/help.c\n> index d478afb2af..490d2bc3ae 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -720,3 +720,37 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n> \tstring_list_clear(&suggested_refs, 0);\n> \texit(1);\n> }\n> +\n> +static struct cmdname_help *find_cmdname_help(const char *name)\n> +{\n> +\tint i;\n> +\n> +\tfor (i = 0; i < ARRAY_SIZE(command_list); i++) {\n> +\t\tif (!strcmp(command_list[i].name, name))\n> +\t\t\treturn &command_list[i];\n> +\t}\n> +\treturn NULL;\n> +}\n> +\n> +void warn_on_dashed_git(const char *cmd)\n> +{\n> +\tstruct cmdname_help *cmdname;\n> +\tstatic const char *still_in_use_var = \"GIT_I_STILL_USE_DASHED_GIT\";\n> +\tstatic const char *still_in_use_msg =\n> +\t\tN_(\"Use of '%s' in the dashed-form is nominated for removal.\\n\"\n> +\t\t \"If you still use it, export '%s=true'\\n\"\n> +\t\t \"and send an e-mail to \\n\"\n> +\t\t \"to let us know and stop our removal plan. Thanks.\\n\");\n> +\n> +\tif (!cmd)\n> +\t\treturn; /* git-help is OK */\n> +\n> +\tcmdname = find_cmdname_help(cmd);\n> +\tif (cmdname && (cmdname->category & CAT_onpath))\n> +\t\treturn; /* git-upload-pack and friends are OK */\n> +\n> +\tif (!git_env_bool(still_in_use_var, 0)) {\n> +\t\tfprintf(stderr, _(still_in_use_msg), cmd, still_in_use_var);\n> +\t\texit(1);\n> +\t}\n> +}\n\nI need this on top, to make it work on Windows:\n\n-- snipsnap --\nFrom: Johannes Schindelin \nSubject: [PATCH] fixup??? git: catch an attempt to run \"git-foo\"\n\nThis is needed to handle the case where `argv[0]` contains the full path\n(which is the case on Windows) and the suffix `.exe` (which is also the\ncase on Windows).\n\nSigned-off-by: Johannes Schindelin \n---\n git.c | 3 ++-\n help.c | 5 ++++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 71ef4835b20e..863fd0c58a66 100644\n--- a/git.c\n+++ b/git.c\n@@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv)\n \t * that one cannot handle it.\n \t */\n \tif (skip_prefix(cmd, \"git-\", &cmd)) {\n-\t\twarn_on_dashed_git(argv[0]);\n+\t\tstrip_extension(&cmd);\n+\t\twarn_on_dashed_git(cmd);\n\n \t\targv[0] = cmd;\n \t\thandle_builtin(argc, argv);\ndiff --git a/help.c b/help.c\nindex c93a76944b00..27b1b26890be 100644\n--- a/help.c\n+++ b/help.c\n@@ -724,9 +724,12 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n static struct cmdname_help *find_cmdname_help(const char *name)\n {\n \tint i;\n+\tconst char *p;\n\n+\tskip_prefix(name, \"git-\", &name);\n \tfor (i = 0; i < ARRAY_SIZE(command_list); i++) {\n-\t\tif (!strcmp(command_list[i].name, name))\n+\t\tif (skip_prefix(command_list[i].name, \"git-\", &p) &&\n+\t\t !strcmp(p, name))\n \t\t\treturn &command_list[i];\n \t}\n \treturn NULL;\n--\n2.28.0.windows.1\n\n"},{"id":"404736","messageId":"xmqq1rjqo2bp.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"nycvar.QRO.7.76.6.2008280412030.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-28T22:03:38Z","receivedAt":"2020-08-28T22:03:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> This is needed to handle the case where `argv[0]` contains the full path\n> (which is the case on Windows) and the suffix `.exe` (which is also the\n> case on Windows).\n\nOy. Yeah, I totally forgot about \".exe\" thing.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> git.c | 3 ++-\n> help.c | 5 ++++-\n> 2 files changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 71ef4835b20e..863fd0c58a66 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv)\n> \t * that one cannot handle it.\n> \t */\n> \tif (skip_prefix(cmd, \"git-\", &cmd)) {\n> -\t\twarn_on_dashed_git(argv[0]);\n> +\t\tstrip_extension(&cmd);\n> +\t\twarn_on_dashed_git(cmd);\n\nThe argv[0] may have been NULL from the beginning of cmd_main(), and\ncmd would be \"git-help\" in such a case. We used to pass NULL to\nwarn_on_dashed_git() in such a case because \"git-help\" is not what\nthe user typed, but what we internally trigger, so we didn't want\nwarn_on_dashed_git() to do anything based on that internal string.\n\nAs there is no special casing of \"help\" in warn_on_dashed_git()\nmechanism, it probably would start triggering a warning/die when\n\"git\" is typed, no?\n\n+\tif (argv[0]) {\n\t\tstrip_extension(&cmd);\n\t\twarn_on_dashed_git(cmd);\n\nmay be the minimum fix, but I would strongly prefer to keep the\ninterface into warn_on_dashed_git() (eh, the most important is the\ninterface into find_cmdname_help() helper function, which is\ndesigned to be reused by other parts of help.c) to take the full\ncommand name, not without \"git-\" prefix. This is primarily because\nthe entirety of the help.c API is driven by full command names,\nwithout removing \"git-\" prefix, and it has to, because the help.c\nAPI needs to handle \"gitk\", from which you cannot remove any \"git-\"\nprefix.\n\nPerhaps\n\n\tif (starts_with(cmd, \"git-\")) {\n \tstrip_extension(&cmd);\n\t\tif (argv[0])\n\t\t\twarn_on_dashed_git(cmd);\n\t\targv[0] = cmd + 4;\n handle_builtin(argc, argv);\n\t\tdie(...);\n\nHow does your handle_builtin() work, by the way? \n\nThe original code (even before we added warn_on_dashed_git() in this\ncodepath) did not do any strip_extension(), so handle_builtin() can\ntake commands with \".exe\" suffix, but now we are feeding the result\nof strip_extension() to it, so it can deal with both? \n\nSounds convenient and sloppy (not the handle_builtin's\nimplementation, but its callers that sometimes feeds the full\nexecutable name, and feeds only the basename some other times) at\nthe same time.\n\n> \t\targv[0] = cmd;\n> \t\thandle_builtin(argc, argv);\n> diff --git a/help.c b/help.c\n> index c93a76944b00..27b1b26890be 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -724,9 +724,12 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n> static struct cmdname_help *find_cmdname_help(const char *name)\n> {\n> \tint i;\n> +\tconst char *p;\n>\n> +\tskip_prefix(name, \"git-\", &name);\n> \tfor (i = 0; i < ARRAY_SIZE(command_list); i++) {\n> -\t\tif (!strcmp(command_list[i].name, name))\n> +\t\tif (skip_prefix(command_list[i].name, \"git-\", &p) &&\n> +\t\t !strcmp(p, name))\n> \t\t\treturn &command_list[i];\n> \t}\n> \treturn NULL;\n> --\n> 2.28.0.windows.1\n"},{"id":"404738","messageId":"xmqqwo1imltm.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200828091454.GA2140991@coredump.intra.peff.net","subject":"Re: [PATCH] pack-redundant: gauge the usage before proposing its removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-28T22:45:25Z","receivedAt":"2020-08-28T22:45:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Aug 25, 2020 at 03:45:52PM -0700, Junio C Hamano wrote:\n>\n>> The subcommand is unusably slow and the reason why nobody reports it\n>> as a performance bug is suspected to be the absense of users. Let's\n>> show a big message that asks the user to tell us that they still\n>> care about the command when an attempt is made to run the command,\n>> with an escape hatch to override it with a command line option.\n>\n> I was looking at the history here and noticed this topic, which I\n> somehow missed when it happened:\n>\n> $ git show -s cf0879f7e98d2213503622f780d2ab0dd3f93477\n> commit cf0879f7e98d2213503622f780d2ab0dd3f93477\n> Merge: 3710f60a80 0e37abd2e8\n> Author: Junio C Hamano \n> Date: Thu Mar 7 09:59:54 2019 +0900\n> \n> Merge branch 'sc/pack-redundant'\n> \n> Update the implementation of pack-redundant for performance in a\n> repository with many packfiles.\n> \n> * sc/pack-redundant:\n> pack-redundant: consistent sort method\n> pack-redundant: rename pack_list.all_objects\n> pack-redundant: new algorithm to find min packs\n> pack-redundant: delete redundant code\n> pack-redundant: delay creation of unique_objects\n> t5323: test cases for git-pack-redundant\n>\n> So it sounds like:\n>\n> - somebody does care enough to use it\n>\n> - it may not be horrifically slow anymore\n>\n> So it may not be worth trying to follow through on the deprecation\n> (though the fact that neither of us realized this makes me worried for\n> the general state of maintenance of this code).\n\nOK. Just dropping the topic is the easiest ;-) Thanks.\n"},{"id":"404795","messageId":"nycvar.QRO.7.76.6.2008311156020.56@tvgsbejvaqbjf.bet","threadId":"54128","inReplyTo":"xmqq1rjqo2bp.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-08-31T09:59:02Z","receivedAt":"2020-08-31T09:59:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 28 Aug 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > git.c | 3 ++-\n> > help.c | 5 ++++-\n> > 2 files changed, 6 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/git.c b/git.c\n> > index 71ef4835b20e..863fd0c58a66 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv)\n> > \t * that one cannot handle it.\n> > \t */\n> > \tif (skip_prefix(cmd, \"git-\", &cmd)) {\n> > -\t\twarn_on_dashed_git(argv[0]);\n> > +\t\tstrip_extension(&cmd);\n> > +\t\twarn_on_dashed_git(cmd);\n>\n> The argv[0] may have been NULL from the beginning of cmd_main(), and\n> cmd would be \"git-help\" in such a case. We used to pass NULL to\n> warn_on_dashed_git() in such a case because \"git-help\" is not what the\n> user typed, but what we internally trigger, so we didn't want\n> warn_on_dashed_git() to do anything based on that internal string.\n\nTrue. The test suite passes with this, though, which means we haven't\ncovered that case.\n\n> As there is no special casing of \"help\" in warn_on_dashed_git()\n> mechanism, it probably would start triggering a warning/die when\n> \"git\" is typed, no?\n>\n> +\tif (argv[0]) {\n> \t\tstrip_extension(&cmd);\n> \t\twarn_on_dashed_git(cmd);\n>\n> may be the minimum fix, but I would strongly prefer to keep the\n> interface into warn_on_dashed_git() (eh, the most important is the\n> interface into find_cmdname_help() helper function, which is\n> designed to be reused by other parts of help.c) to take the full\n> command name, not without \"git-\" prefix. This is primarily because\n> the entirety of the help.c API is driven by full command names,\n> without removing \"git-\" prefix, and it has to, because the help.c\n> API needs to handle \"gitk\", from which you cannot remove any \"git-\"\n> prefix.\n>\n> Perhaps\n>\n> \tif (starts_with(cmd, \"git-\")) {\n> \tstrip_extension(&cmd);\n> \t\tif (argv[0])\n> \t\t\twarn_on_dashed_git(cmd);\n> \t\targv[0] = cmd + 4;\n> handle_builtin(argc, argv);\n> \t\tdie(...);\n\nSure.\n\n> How does your handle_builtin() work, by the way?\n>\n> The original code (even before we added warn_on_dashed_git() in this\n> codepath) did not do any strip_extension(), so handle_builtin() can\n> take commands with \".exe\" suffix, but now we are feeding the result\n> of strip_extension() to it, so it can deal with both?\n\nYes, it can deal with both. It calls `strip_extension()`, which on Windows\nremoves the `.exe` suffix, if found.\n\n> Sounds convenient and sloppy (not the handle_builtin's\n> implementation, but its callers that sometimes feeds the full\n> executable name, and feeds only the basename some other times) at\n> the same time.\n\nRight, it does not _require_ the extension. I do not necessarily agree\nthat it is sloppy, but I do agree that it is convenient ;-)\n\nCiao,\nDscho\n\n> > \t\targv[0] = cmd;\n> > \t\thandle_builtin(argc, argv);\n> > diff --git a/help.c b/help.c\n> > index c93a76944b00..27b1b26890be 100644\n> > --- a/help.c\n> > +++ b/help.c\n> > @@ -724,9 +724,12 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n> > static struct cmdname_help *find_cmdname_help(const char *name)\n> > {\n> > \tint i;\n> > +\tconst char *p;\n> >\n> > +\tskip_prefix(name, \"git-\", &name);\n> > \tfor (i = 0; i < ARRAY_SIZE(command_list); i++) {\n> > -\t\tif (!strcmp(command_list[i].name, name))\n> > +\t\tif (skip_prefix(command_list[i].name, \"git-\", &p) &&\n> > +\t\t !strcmp(p, name))\n> > \t\t\treturn &command_list[i];\n> > \t}\n> > \treturn NULL;\n> > --\n> > 2.28.0.windows.1\n>\n"},{"id":"404822","messageId":"xmqqmu2ag15t.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"nycvar.QRO.7.76.6.2008311156020.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-31T17:45:02Z","receivedAt":"2020-08-31T17:45:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Fri, 28 Aug 2020, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>> > Signed-off-by: Johannes Schindelin \n>> > ---\n>> > git.c | 3 ++-\n>> > help.c | 5 ++++-\n>> > 2 files changed, 6 insertions(+), 2 deletions(-)\n>> >\n>> > diff --git a/git.c b/git.c\n>> > index 71ef4835b20e..863fd0c58a66 100644\n>> > --- a/git.c\n>> > +++ b/git.c\n>> > @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv)\n>> > \t * that one cannot handle it.\n>> > \t */\n>> > \tif (skip_prefix(cmd, \"git-\", &cmd)) {\n>> > -\t\twarn_on_dashed_git(argv[0]);\n>> > +\t\tstrip_extension(&cmd);\n>> > +\t\twarn_on_dashed_git(cmd);\n>>\n>> The argv[0] may have been NULL from the beginning of cmd_main(), and\n>> cmd would be \"git-help\" in such a case. We used to pass NULL to\n>> warn_on_dashed_git() in such a case because \"git-help\" is not what the\n>> user typed, but what we internally trigger, so we didn't want\n>> warn_on_dashed_git() to do anything based on that internal string.\n>\n> True. The test suite passes with this, though, which means we haven't\n> covered that case.\n\nYup, it would be a good thing to add to our tests, with or without\nthis patch.\n\n>> How does your handle_builtin() work, by the way?\n>>\n>> The original code (even before we added warn_on_dashed_git() in this\n>> codepath) did not do any strip_extension(), so handle_builtin() can\n>> take commands with \".exe\" suffix, but now we are feeding the result\n>> of strip_extension() to it, so it can deal with both?\n>\n> Yes, it can deal with both. It calls `strip_extension()`, which on Windows\n> removes the `.exe` suffix, if found.\n>\n>> Sounds convenient and sloppy (not the handle_builtin's\n>> implementation, but its callers that sometimes feeds the full\n>> executable name, and feeds only the basename some other times) at\n>> the same time.\n>\n> Right, it does not _require_ the extension. I do not necessarily agree\n> that it is sloppy, but I do agree that it is convenient ;-)\n\nBeing lenient to its input is not sloppy. \n\nIt just is by being convenient, it allows its callers to be sloppy,\nwhich may hurt them as not all callees they call may not be as\nlenient as handle_builtin(), which is the only downside I would be\nworried about. Nothing big.\n\nthanks.\n"},{"id":"404833","messageId":"xmqqpn76e873.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"xmqqzh6ht7fg.fsf_-_@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-31T22:56:00Z","receivedAt":"2020-08-31T22:57:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> As child_process structure has an embedded strvec args for\n> formulating the command line, let's use it instead of using\n> an out-of-line argv[] whose length needs to be maintained\n> correctly. \n>\n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/credential-cache.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\n> index d0fafdeb9e..195335a783 100644\n> --- a/builtin/credential-cache.c\n> +++ b/builtin/credential-cache.c\n> @@ -42,13 +42,13 @@ static int send_request(const char *socket, const struct strbuf *out)\n> static void spawn_daemon(const char *socket)\n> {\n> \tstruct child_process daemon = CHILD_PROCESS_INIT;\n> -\tconst char *argv[] = { NULL, NULL, NULL };\n> \tchar buf[128];\n> \tint r;\n> \n> -\targv[0] = \"git-credential-cache--daemon\";\n> -\targv[1] = socket;\n> -\tdaemon.argv = argv;\n> +\tstrvec_pushl(&daemon.args, \n> +\t\t \"credential-cache--daemon\", socket,\n> +\t\t NULL);\n> +\tdaemon.git_cmd = 1;\n> \tdaemon.no_stdin = 1;\n> \tdaemon.out = -1;\n> \n\nBy the way, an interesting fact is that this cannot graduate UNTIL\ncredential-cache becomes a built-in. Having an intermediate level\nprocess seems to break t0301.\n\n"},{"id":"404836","messageId":"20200901044954.GB3956172@coredump.intra.peff.net","threadId":"54128","inReplyTo":"xmqqpn76e873.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-01T04:49:54Z","receivedAt":"2020-09-01T04:49:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 31, 2020 at 03:56:00PM -0700, Junio C Hamano wrote:\n\n> > -\targv[0] = \"git-credential-cache--daemon\";\n> > -\targv[1] = socket;\n> > -\tdaemon.argv = argv;\n> > +\tstrvec_pushl(&daemon.args, \n> > +\t\t \"credential-cache--daemon\", socket,\n> > +\t\t NULL);\n> > +\tdaemon.git_cmd = 1;\n> > \tdaemon.no_stdin = 1;\n> > \tdaemon.out = -1;\n> > \n> \n> By the way, an interesting fact is that this cannot graduate UNTIL\n> credential-cache becomes a built-in. Having an intermediate level\n> process seems to break t0301.\n\nHmm, that is interesting. I thought it would work OK because we don't\nrely on any process-id magic for finding the daemon, etc, and instead\ntalk over pipe descriptors. But that proves to be our undoing.\n\nWhat happens usually is this:\n\n - credential-cache spawns credential-cache--daemon with its\n stdout connected to a pipe\n\n - the client calls read_in_full() waiting for the daemon to tell us\n that it started up\n\n - the daemon opens the socket, then writes \"ok\\n\" to stdout and closes\n the pipe\n\n - the client gets EOF on the pipe, then compares what it read to\n \"ok\\n\", and continues (or relays an error)\n\nBut when we add the extra \"git\" wrapper process into the mix, we never\nsee that EOF. The wrapper's stdout also points to that pipe, so closing\nit in the daemon process isn't enough for the client to get EOF. And the\nwrapper doesn't exit, because it's waiting on the daemon.\n\nSo one hacky fix is:\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex 04df61cf02..9bfddaf050 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -51,7 +51,7 @@ static void spawn_daemon(const char *socket)\n \n \tif (start_command(&daemon))\n \t\tdie_errno(\"unable to start cache daemon\");\n-\tr = read_in_full(daemon.out, buf, sizeof(buf));\n+\tr = read_in_full(daemon.out, buf, 3);\n \tif (r < 0)\n \t\tdie_errno(\"unable to read result code from cache daemon\");\n \tif (r != 3 || memcmp(buf, \"ok\\n\", 3))\n\nwhich makes t0301 pass. A less-hacky solution might be to loosen its\nexpectations not to require EOF at all (and just accept anything\nstarting with \"ok\\n\". But I don't think it's worth doing either, if we\nknow we're going to switch it to a builtin anyway (and that also makes\nme feel slightly better about the plan to do so).\n\nI do wonder if it points to a problem in the git.c wrapper. It's\nduplicating all of the descriptors that the child external commands will\nsee, so callers can't rely on EOF (or EPIPE for its stdin) to know when\nthe external program has closed them. For the most part that's OK,\nbecause we expect them to close when the external program exits, at\nwhich point the wrapper will exit, too. But things get weird around\nhalf-duplex shutdowns, or programs that try to daemonize.\n\nPerhaps git.c should be closing all descriptors after spawning the\nchild. Of course that gets weird if it wants to write an error to stderr\nabout spawning the child. I dunno. It seems as likely to introduce\nproblems as solve them, so if nothing is broken beyond this cache-daemon\nthing, I'd just as soon leave it.\n\nI wouldn't be surprised if older pre-builtin \"upload-pack\" could run\ninto problems. But we always called it as \"git-upload-pack\" back then\nanyway. Possibly modern \"git daemon\" could suffer weirdness, as it's\nstill a separate program (I shied away from including it in my recent\n\"slimming\" series exactly because I was afraid of these kinds of issues;\nbut it sounds like being a builtin probably has less-surprising\nimplications overall).\n\nAll of which is to say I'm happy to do nothing, but that turned out to\nbe a very interesting data point. Thanks for mentioning it. :)\n\n-Peff\n"},{"id":"404874","messageId":"xmqqy2ltcw9t.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"20200901044954.GB3956172@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/2] credential-cache: use child_process.args","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-01T16:11:10Z","receivedAt":"2020-09-01T16:11:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hmm, that is interesting. I thought it would work OK because we don't\n> rely on any process-id magic for finding the daemon, etc, and instead\n> talk over pipe descriptors. But that proves to be our undoing.\n\nYup. I also do suspect that closing all excess fds in the git\nprocess in the middle may not be a bad idea, but we may not know\nwhat the upper bound is.\n\n> Perhaps git.c should be closing all descriptors after spawning the\n> child. Of course that gets weird if it wants to write an error to stderr\n> about spawning the child. I dunno. It seems as likely to introduce\n> problems as solve them, so if nothing is broken beyond this cache-daemon\n> thing, I'd just as soon leave it.\n\nWe do employ the \"open extra pipe that is set to close-on-exec so\nthat child that failed to exec can report back\" trick, but in order\nto report the failure back, the standard error of the process in the\nmiddle may have to be kept open, so let's not disturb this sleeping\ndragon ;-)\n\n"},{"id":"412781","messageId":"nycvar.QRO.7.76.6.2012201623490.56@tvgsbejvaqbjf.bet","threadId":"54128","inReplyTo":"nycvar.QRO.7.76.6.2008280412030.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-20T15:25:15Z","receivedAt":"2020-12-21T21:56:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 28 Aug 2020, Johannes Schindelin wrote:\n\n> On Tue, 25 Aug 2020, Junio C Hamano wrote:\n>\n> > diff --git a/git.c b/git.c\n> > index 8bd1d7551d..927018bda7 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -839,6 +839,8 @@ int cmd_main(int argc, const char **argv)\n> > \t * that one cannot handle it.\n> > \t */\n> > \tif (skip_prefix(cmd, \"git-\", &cmd)) {\n> > +\t\twarn_on_dashed_git(argv[0]);\n> > +\n> > \t\targv[0] = cmd;\n> > \t\thandle_builtin(argc, argv);\n> > \t\tdie(_(\"cannot handle %s as a builtin\"), cmd);\n> > diff --git a/help.c b/help.c\n> > index d478afb2af..490d2bc3ae 100644\n> > --- a/help.c\n> > +++ b/help.c\n> > @@ -720,3 +720,37 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n> > \tstring_list_clear(&suggested_refs, 0);\n> > \texit(1);\n> > }\n> > +\n> > +static struct cmdname_help *find_cmdname_help(const char *name)\n> > +{\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < ARRAY_SIZE(command_list); i++) {\n> > +\t\tif (!strcmp(command_list[i].name, name))\n> > +\t\t\treturn &command_list[i];\n> > +\t}\n> > +\treturn NULL;\n> > +}\n> > +\n> > +void warn_on_dashed_git(const char *cmd)\n> > +{\n> > +\tstruct cmdname_help *cmdname;\n> > +\tstatic const char *still_in_use_var = \"GIT_I_STILL_USE_DASHED_GIT\";\n> > +\tstatic const char *still_in_use_msg =\n> > +\t\tN_(\"Use of '%s' in the dashed-form is nominated for removal.\\n\"\n> > +\t\t \"If you still use it, export '%s=true'\\n\"\n> > +\t\t \"and send an e-mail to \\n\"\n> > +\t\t \"to let us know and stop our removal plan. Thanks.\\n\");\n> > +\n> > +\tif (!cmd)\n> > +\t\treturn; /* git-help is OK */\n> > +\n> > +\tcmdname = find_cmdname_help(cmd);\n> > +\tif (cmdname && (cmdname->category & CAT_onpath))\n> > +\t\treturn; /* git-upload-pack and friends are OK */\n> > +\n> > +\tif (!git_env_bool(still_in_use_var, 0)) {\n> > +\t\tfprintf(stderr, _(still_in_use_msg), cmd, still_in_use_var);\n> > +\t\texit(1);\n> > +\t}\n> > +}\n>\n> I need this on top, to make it work on Windows:\n>\n> -- snipsnap --\n> From: Johannes Schindelin \n> Subject: [PATCH] fixup??? git: catch an attempt to run \"git-foo\"\n>\n> This is needed to handle the case where `argv[0]` contains the full path\n> (which is the case on Windows) and the suffix `.exe` (which is also the\n> case on Windows).\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> git.c | 3 ++-\n> help.c | 5 ++++-\n> 2 files changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 71ef4835b20e..863fd0c58a66 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -851,7 +851,8 @@ int cmd_main(int argc, const char **argv)\n> \t * that one cannot handle it.\n> \t */\n> \tif (skip_prefix(cmd, \"git-\", &cmd)) {\n> -\t\twarn_on_dashed_git(argv[0]);\n> +\t\tstrip_extension(&cmd);\n> +\t\twarn_on_dashed_git(cmd);\n>\n> \t\targv[0] = cmd;\n> \t\thandle_builtin(argc, argv);\n> diff --git a/help.c b/help.c\n> index c93a76944b00..27b1b26890be 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -724,9 +724,12 @@ NORETURN void help_unknown_ref(const char *ref, const char *cmd,\n> static struct cmdname_help *find_cmdname_help(const char *name)\n> {\n> \tint i;\n> +\tconst char *p;\n>\n> +\tskip_prefix(name, \"git-\", &name);\n> \tfor (i = 0; i < ARRAY_SIZE(command_list); i++) {\n> -\t\tif (!strcmp(command_list[i].name, name))\n> +\t\tif (skip_prefix(command_list[i].name, \"git-\", &p) &&\n> +\t\t !strcmp(p, name))\n> \t\t\treturn &command_list[i];\n> \t}\n> \treturn NULL;\n> --\n> 2.28.0.windows.1\n\nHow about this instead (to fix that part of the CI failures of `seen`)?\n\n-- snipsnap --\nFrom e8ce19db04657b6ef1c73989695c97a773a9c001 Mon Sep 17 00:00:00 2001\nFrom: Johannes Schindelin \nDate: Fri, 28 Aug 2020 14:50:25 +0200\nSubject: [PATCH] fixup??? git: catch an attempt to run \"git-foo\"\n\nThis is needed to handle the case where `argv[0]` contains the full path\n(which is the case on Windows) and the suffix `.exe` (which is also the\ncase on Windows).\n\nSigned-off-by: Johannes Schindelin \n---\n git.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 7544d2187306..c924c53ea76f 100644\n--- a/git.c\n+++ b/git.c\n@@ -854,6 +854,7 @@ int cmd_main(int argc, const char **argv)\n \tconst char *cmd;\n \tint done_help = 0;\n\n+\tstrip_extension(argv);\n \tcmd = argv[0];\n \tif (!cmd)\n \t\tcmd = \"git-help\";\n@@ -875,12 +876,11 @@ int cmd_main(int argc, const char **argv)\n \t * So we just directly call the builtin handler, and die if\n \t * that one cannot handle it.\n \t */\n-\tif (skip_prefix(cmd, \"git-\", &cmd)) {\n-\t\twarn_on_dashed_git(argv[0]);\n+\tif (skip_prefix(cmd, \"git-\", &argv[0])) {\n+\t\twarn_on_dashed_git(cmd);\n\n-\t\targv[0] = cmd;\n \t\thandle_builtin(argc, argv);\n-\t\tdie(_(\"cannot handle %s as a builtin\"), cmd);\n+\t\tdie(_(\"cannot handle %s as a builtin\"), argv[0]);\n \t}\n\n \t/* Look for flags.. */\n--\n2.30.0.rc0.windows.1\n\n"},{"id":"412783","messageId":"xmqqblemlrv2.fsf@gitster.c.googlers.com","threadId":"54128","inReplyTo":"nycvar.QRO.7.76.6.2012201623490.56@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-21T22:24:49Z","receivedAt":"2020-12-21T22:25:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> I need this on top, to make it work on Windows:\n>> ...\n> How about this instead (to fix that part of the CI failures of `seen`)?\n\nAh, I didn't knew the backburnered stuff was breaking 'seen'.\nThanks for helping to cleanse 'seen'; I do not actually mind\ndropping the offending topic at this point in the cycle, though.\n\n> -- snipsnap --\n> From e8ce19db04657b6ef1c73989695c97a773a9c001 Mon Sep 17 00:00:00 2001\n> From: Johannes Schindelin \n> Date: Fri, 28 Aug 2020 14:50:25 +0200\n> Subject: [PATCH] fixup??? git: catch an attempt to run \"git-foo\"\n>\n> This is needed to handle the case where `argv[0]` contains the full path\n> (which is the case on Windows) and the suffix `.exe` (which is also the\n> case on Windows).\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> git.c | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/git.c b/git.c\n> index 7544d2187306..c924c53ea76f 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -854,6 +854,7 @@ int cmd_main(int argc, const char **argv)\n> \tconst char *cmd;\n> \tint done_help = 0;\n>\n> +\tstrip_extension(argv);\n> \tcmd = argv[0];\n\nHph, would this make strip_extension() at the beginning of\nhandle_builtin() redundant and unneeded, I wonder?\n\nYes, I know stripping .exe twice would be fine most of the time, so\nI'll queue the patch on top just to make 'seen' pass the tests, but\nit is just as easy to discard jc/war-on-dashed-git topic, so...\n\n> \tif (!cmd)\n> \t\tcmd = \"git-help\";\n> @@ -875,12 +876,11 @@ int cmd_main(int argc, const char **argv)\n> \t * So we just directly call the builtin handler, and die if\n> \t * that one cannot handle it.\n> \t */\n> -\tif (skip_prefix(cmd, \"git-\", &cmd)) {\n> -\t\twarn_on_dashed_git(argv[0]);\n> +\tif (skip_prefix(cmd, \"git-\", &argv[0])) {\n> +\t\twarn_on_dashed_git(cmd);\n>\n> -\t\targv[0] = cmd;\n> \t\thandle_builtin(argc, argv);\n> -\t\tdie(_(\"cannot handle %s as a builtin\"), cmd);\n> +\t\tdie(_(\"cannot handle %s as a builtin\"), argv[0]);\n> \t}\n>\n> \t/* Look for flags.. */\n> --\n> 2.30.0.rc0.windows.1\n"},{"id":"413201","messageId":"nycvar.QRO.7.76.6.2012300627240.56@tvgsbejvaqbjf.bet","threadId":"54128","inReplyTo":"xmqqblemlrv2.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v1 3/3] git: catch an attempt to run \"git-foo\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-12-30T05:30:54Z","receivedAt":"2020-12-30T22:00:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 21 Dec 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > -- snipsnap --\n> > From e8ce19db04657b6ef1c73989695c97a773a9c001 Mon Sep 17 00:00:00 2001\n> > From: Johannes Schindelin \n> > Date: Fri, 28 Aug 2020 14:50:25 +0200\n> > Subject: [PATCH] fixup??? git: catch an attempt to run \"git-foo\"\n> >\n> > This is needed to handle the case where `argv[0]` contains the full path\n> > (which is the case on Windows) and the suffix `.exe` (which is also the\n> > case on Windows).\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > git.c | 8 ++++----\n> > 1 file changed, 4 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/git.c b/git.c\n> > index 7544d2187306..c924c53ea76f 100644\n> > --- a/git.c\n> > +++ b/git.c\n> > @@ -854,6 +854,7 @@ int cmd_main(int argc, const char **argv)\n> > \tconst char *cmd;\n> > \tint done_help = 0;\n> >\n> > +\tstrip_extension(argv);\n> > \tcmd = argv[0];\n>\n> Hph, would this make strip_extension() at the beginning of\n> handle_builtin() redundant and unneeded, I wonder?\n\nI poured over this for a couple minutes, and I think you're right.\n\n> Yes, I know stripping .exe twice would be fine most of the time, so\n> I'll queue the patch on top just to make 'seen' pass the tests, but\n> it is just as easy to discard jc/war-on-dashed-git topic, so...\n\nI dunno. There is probably value in starting the deprecation for realz. I\nmean, we deprecated dashed commands, like, an age ago (2020 alone felt\nlike a century, didn't it?). Maybe it is time to warn about using dashed\ncommands now. We could even consider \"brown-outs\" at some stage, via a\npseudo-random condition that triggers rarely in spring 2021 but\nincrementally frequently over time?\n\nCiao,\nDscho\n"}]}