{"thread":{"id":"61786","subject":"v2.46.0-rc0 test failures on cygwin","startedAt":"2024-07-16T19:49:00Z","lastAt":"2024-07-23T20:52:33Z","messageCount":15,"participants":["Ramsay Jones","Jeff King","Junio C Hamano","Adam Dinwoodie","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"498800","messageId":"aacb4067-a16b-4953-a72f-3c66efd3cf25@ramsayjones.plus.com","threadId":"61786","inReplyTo":null,"subject":"v2.46.0-rc0 test failures on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-07-16T19:45:48Z","receivedAt":"2024-07-16T19:49:00Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe 'Test Summary Report' for the v2.46.0-rc0 test run, on cygwin, includes:\n\n  ...\n  t1460-refs-migrate.sh                            (Wstat: 256 (exited 1) Tests: 30 Failed: 9)\n    Failed tests:  8-15, 29\n    Non-zero exit status: 1\n  ...\n\nLooking at the test output directly, we see:\n \n  $ pwd\n  /home/ramsay/git\n  $ cd t\n  $ make clean\n  rm -f -r 'chainlinttmp'\n  rm -f -r 'trash directory'.*\n  rm -f -r valgrind/bin\n  rm -f -r 'test-results'\n  rm -f .prove\n  $ ./t1460-refs-migrate.sh -v -i\n  ...\n  ok 7 - files -> reftable: migration with worktree fails\n  \n  expecting success of 1460.8 'files -> reftable: unborn HEAD': \n  \t\t\ttest_when_finished \"rm -rf repo\" &&\n  \t\t\tgit init --ref-format=$from_format repo &&\n  \t\t\ttest_migration repo \"$to_format\"\n  \t\t\n  Initialized empty Git repository in /home/ramsay/git/t/trash directory.t1460-refs-migrate/repo/.git/\n  error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied\n  migrated refs can be found at '.git/ref_migration.sr9pEF'\n  not ok 8 - files -> reftable: unborn HEAD\n  #\t\n  #\t\t\t\ttest_when_finished \"rm -rf repo\" &&\n  #\t\t\t\tgit init --ref-format=$from_format repo &&\n  #\t\t\t\ttest_migration repo \"$to_format\"\n  #\t\t\t\n  1..8\n  $ \n\nNote that all of the errors in this test look similar to this one (ie. when\nrename()-ing the 'migrated' reftable directory to .git/reftable we get an\n'Permission denied' error). So the error message is from line 2672 of refs.c\nin the move_files() function.\n\nLet's have a quick look at the test directory:\n\n  $ cd trash\\ directory.t1460-refs-migrate/\n  $ ls\n  err  expect  repo/\n  $ cd repo\n  $ ls -l .git\n  total 7.0K\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 branches/\n  -rw-r--r-- 1 ramsay None 86 Jul 16 19:53 config\n  -rw-r--r-- 1 ramsay None 73 Jul 16 19:53 description\n  -rw-r--r-- 1 ramsay None 25 Jul 16 19:53 HEAD\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 hooks/\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 info/\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 objects/\n  drwx------ 1 ramsay None  0 Jul 16 19:53 ref_migration.sr9pEF/\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 refs/\n\nNow try to finish the migration by hand:\n\n  $ mv .git/ref_migration.sr9pEF/reftable .git/reftable\n\nHmm, note no error; of course, the mv command may well do much more than\nthe rename() library function, so they are not necessarily equivalent.\n\n  $ ls -l .git\n  total 7.0K\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 branches/\n  -rw-r--r-- 1 ramsay None 86 Jul 16 19:53 config\n  -rw-r--r-- 1 ramsay None 73 Jul 16 19:53 description\n  -rw-r--r-- 1 ramsay None 25 Jul 16 19:53 HEAD\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 hooks/\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 info/\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 objects/\n  drwx------ 1 ramsay None  0 Jul 16 19:55 ref_migration.sr9pEF/\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 refs/\n  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 reftable/\n  $ \n  $ rm -rf .git/ref_migration.sr9pEF/\n  $ git branch -v\n  fatal: failed to resolve HEAD as a valid ref\n  $ ls -l .git/reftable\n  total 2.0K\n  -rw-r--r-- 1 ramsay None 124 Jul 16 19:53 0x000000000001-0x000000000001-64e987ec.ref\n  -rw-r--r-- 1 ramsay None  43 Jul 16 19:53 tables.list\n  $ xxd .git/reftable/0x000000000001-0x000000000001-64e987ec.ref \n  00000000: 5245 4654 0100 1000 0000 0000 0000 0001  REFT............\n  00000010: 0000 0000 0000 0001 7200 0038 0023 4845  ........r..8.#HE\n  00000020: 4144 000f 7265 6673 2f68 6561 6473 2f6d  AD..refs/heads/m\n  00000030: 6169 6e00 001c 0001 5245 4654 0100 1000  ain.....REFT....\n  00000040: 0000 0000 0000 0001 0000 0000 0000 0001  ................\n  00000050: 0000 0000 0000 0000 0000 0000 0000 0000  ................\n  00000060: 0000 0000 0000 0000 0000 0000 0000 0000  ................\n  00000070: 0000 0000 0000 0000 b6bf f78a            ............\n  $ \n\nSo, the HEAD ref was referring to refs/heads/main.\n\n  $ pwd\n  /home/ramsay/git/t/trash directory.t1460-refs-migrate/repo\n  $ git init --ref-format=reftable\n  fatal: attempt to reinitialize repository with different reference storage format\n  $ xxd .git/refs/heads \n  00000000: 7468 6973 2072 6570 6f73 6974 6f72 7920  this repository \n  00000010: 7573 6573 2074 6865 2072 6566 7461 626c  uses the reftabl\n  00000020: 6520 666f 726d 6174 0a                   e format.\n  $ \n  $ git branch -v\n  fatal: failed to resolve HEAD as a valid ref\n  $ \n \nHmm, so quite broken :)\n\nMaybe the order of some of the actions in repo_migrate_ref_storage_format() may\nneed to be re-thought! ;)\n\nThe 'man 3 rename' is not very enlightening; 'The conditions for failure depend\non the host operating system.'\n\nUnfortunately, I don't have any (well much) spare time to dig further into\nthis at the moment, so I thought I should at least report it here for others\nto hopefully find a solution.\n\nATB,\nRamsay Jones\n\n"},{"id":"498816","messageId":"20240717064241.GF547635@coredump.intra.peff.net","threadId":"61786","inReplyTo":"aacb4067-a16b-4953-a72f-3c66efd3cf25@ramsayjones.plus.com","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-17T06:42:41Z","receivedAt":"2024-07-17T06:42:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:\n\n>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied\n>   migrated refs can be found at '.git/ref_migration.sr9pEF'\n> [...]\n> Now try to finish the migration by hand:\n> \n>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable\n> \n> Hmm, note no error; of course, the mv command may well do much more than\n> the rename() library function, so they are not necessarily equivalent.\n\nThis is a shot in the dark, but: could the problem be an open file that\ncannot be moved? If I run a \"ref migrate\" on my Linux system in the\ndebugger and stop at move_files(), checking /proc/<pid>/fd shows an open\ndescriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.\n\nDoes the patch below fix things for you? I'm not too familiar with the\ncode, so this is what I cobbled together.  The best response will be\nfrom Patrick, but I think he's offline for another week or so. In the\nmeantime, this at least doesn't crash for me. ;) And I confirmed that\nthe tables.list file is closed during the move_files() call.\n\n-Peff\n\n---\ndiff --git a/refs.c b/refs.c\nindex bb90a18875..06a0fc5099 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2843,6 +2843,12 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \t\tgoto done;\n \t}\n \n+\t/*\n+\t * Close the new ref store to avoid holding on to any open files\n+\t * which could interfere with moving things behind the scenes.\n+\t */\n+\tref_store_release(new_refs);\n+\n \t/*\n \t * Until now we were in the non-destructive phase, where we only\n \t * populated the new ref store. From hereon though we are about\n@@ -2874,8 +2880,13 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \t */\n \tinitialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1);\n \n-\tfree(new_refs->gitdir);\n-\tnew_refs->gitdir = xstrdup(old_refs->gitdir);\n+\t/*\n+\t * Re-open the now-migrated ref store. I'm not sure if this is strictly\n+\t * needed or not. Perhaps it would also be a good time to check that\n+\t * we correctly opened it, it's in the expected format, etc?\n+\t */\n+\tnew_refs = ref_store_init(repo, format, old_refs->gitdir,\n+\t\t\t\t  REF_STORE_ALL_CAPS);\n \trepo->refs_private = new_refs;\n \tref_store_release(old_refs);\n \n"},{"id":"498829","messageId":"xmqq4j8o6pm5.fsf@gitster.g","threadId":"61786","inReplyTo":"20240717064241.GF547635@coredump.intra.peff.net","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-17T14:48:50Z","receivedAt":"2024-07-17T14:48:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:\n>\n>>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied\n>>   migrated refs can be found at '.git/ref_migration.sr9pEF'\n>> [...]\n>> Now try to finish the migration by hand:\n>> \n>>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable\n>> \n>> Hmm, note no error; of course, the mv command may well do much more than\n>> the rename() library function, so they are not necessarily equivalent.\n>\n> This is a shot in the dark, but: could the problem be an open file that\n> cannot be moved? If I run a \"ref migrate\" on my Linux system in the\n> debugger and stop at move_files(), checking /proc/<pid>/fd shows an open\n> descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.\n\nHmph, I wonder ifthat means the same test should be failing on\nWindows, too?\n"},{"id":"498845","messageId":"3e6abe6c-2c15-47f9-89e8-3e8710802562@ramsayjones.plus.com","threadId":"61786","inReplyTo":"20240717064241.GF547635@coredump.intra.peff.net","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-07-17T18:05:43Z","receivedAt":"2024-07-17T18:08:53Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 17/07/2024 07:42, Jeff King wrote:\n> On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:\n> \n>>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied\n>>   migrated refs can be found at '.git/ref_migration.sr9pEF'\n>> [...]\n>> Now try to finish the migration by hand:\n>>\n>>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable\n>>\n>> Hmm, note no error; of course, the mv command may well do much more than\n>> the rename() library function, so they are not necessarily equivalent.\n> \n> This is a shot in the dark, but: could the problem be an open file that\n> cannot be moved? If I run a \"ref migrate\" on my Linux system in the\n> debugger and stop at move_files(), checking /proc/<pid>/fd shows an open\n> descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.\n\nHeh, a very good shot in the dark! ;)\n\n> Does the patch below fix things for you? I'm not too familiar with the\n> code, so this is what I cobbled together.  The best response will be\n> from Patrick, but I think he's offline for another week or so. In the\n> meantime, this at least doesn't crash for me. ;) And I confirmed that\n> the tables.list file is closed during the move_files() call.\n\nThe patch given below fixes the test for me! (I have only run t1460-refs-migrate.sh,\nsince the full test-suite takes 6 hours to run, but now all 30 tests pass!)\n\nI also don't know the code well enough to answer your question regarding\nthe re-opening of the migrated ref-store, but it doesn't look like it would\ncause any problems (famous last words).\n\nThanks!\n\nATB,\nRamsay Jones\n\n> ---\n> diff --git a/refs.c b/refs.c\n> index bb90a18875..06a0fc5099 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2843,6 +2843,12 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n>  \t\tgoto done;\n>  \t}\n>  \n> +\t/*\n> +\t * Close the new ref store to avoid holding on to any open files\n> +\t * which could interfere with moving things behind the scenes.\n> +\t */\n> +\tref_store_release(new_refs);\n> +\n>  \t/*\n>  \t * Until now we were in the non-destructive phase, where we only\n>  \t * populated the new ref store. From hereon though we are about\n> @@ -2874,8 +2880,13 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n>  \t */\n>  \tinitialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1);\n>  \n> -\tfree(new_refs->gitdir);\n> -\tnew_refs->gitdir = xstrdup(old_refs->gitdir);\n> +\t/*\n> +\t * Re-open the now-migrated ref store. I'm not sure if this is strictly\n> +\t * needed or not. Perhaps it would also be a good time to check that\n> +\t * we correctly opened it, it's in the expected format, etc?\n> +\t */\n> +\tnew_refs = ref_store_init(repo, format, old_refs->gitdir,\n> +\t\t\t\t  REF_STORE_ALL_CAPS);\n>  \trepo->refs_private = new_refs;\n>  \tref_store_release(old_refs);\n>  \n> \n"},{"id":"498846","messageId":"07a74e5d-f4f0-4bfe-8334-ad6388c10c5c@ramsayjones.plus.com","threadId":"61786","inReplyTo":"xmqq4j8o6pm5.fsf@gitster.g","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-07-17T18:10:51Z","receivedAt":"2024-07-17T18:10:54Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 17/07/2024 15:48, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:\n>>\n>>>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied\n>>>   migrated refs can be found at '.git/ref_migration.sr9pEF'\n>>> [...]\n>>> Now try to finish the migration by hand:\n>>>\n>>>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable\n>>>\n>>> Hmm, note no error; of course, the mv command may well do much more than\n>>> the rename() library function, so they are not necessarily equivalent.\n>>\n>> This is a shot in the dark, but: could the problem be an open file that\n>> cannot be moved? If I run a \"ref migrate\" on my Linux system in the\n>> debugger and stop at move_files(), checking /proc/<pid>/fd shows an open\n>> descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.\n> \n> Hmph, I wonder ifthat means the same test should be failing on\n> Windows, too?\n> \n\nSince we have not heard anything from Gfw, I had assumed it was cygwin\nspecific (Gfw uses mingw_rename() not rename() so we should not necessarily\nexpect it to behave the same).\n\nHowever, Jeff's patch fixes the problem for me (see other email).\n\nATB,\nRamsay Jones\n\n"},{"id":"498888","messageId":"20240718005723.GA675057@coredump.intra.peff.net","threadId":"61786","inReplyTo":"3e6abe6c-2c15-47f9-89e8-3e8710802562@ramsayjones.plus.com","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-18T00:57:23Z","receivedAt":"2024-07-18T00:57:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:\n\n> > This is a shot in the dark, but: could the problem be an open file that\n> > cannot be moved? If I run a \"ref migrate\" on my Linux system in the\n> > debugger and stop at move_files(), checking /proc/<pid>/fd shows an open\n> > descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.\n> \n> Heh, a very good shot in the dark! ;)\n\nLucky guess. :) When Junio pointed out that we'd expect Windows to fail\nin that case, too, I thought for sure I was just wrong. So I'm glad it\nworked out.\n\n> The patch given below fixes the test for me! (I have only run t1460-refs-migrate.sh,\n> since the full test-suite takes 6 hours to run, but now all 30 tests pass!)\n> \n> I also don't know the code well enough to answer your question regarding\n> the re-opening of the migrated ref-store, but it doesn't look like it would\n> cause any problems (famous last words).\n\nThanks for testing. This is new in the upcoming release, but I think\nit's localized to the \"git ref migrate\" command. So aside from the\nannoyance of the test failure for you, it is not too urgent. I'm tempted\nto put it off until Patrick has had a chance to weigh in, even if it\nmeans missing the v2.46 cutoff.\n\nI'd also be OK with pursuing it in the meantime if folks feel\ndifferently. Having slept on it, I think the answer to one of my\nquestions here...\n\n> > -\tfree(new_refs->gitdir);\n> > -\tnew_refs->gitdir = xstrdup(old_refs->gitdir);\n> > +\t/*\n> > +\t * Re-open the now-migrated ref store. I'm not sure if this is strictly\n> > +\t * needed or not. Perhaps it would also be a good time to check that\n> > +\t * we correctly opened it, it's in the expected format, etc?\n> > +\t */\n> > +\tnew_refs = ref_store_init(repo, format, old_refs->gitdir,\n> > +\t\t\t\t  REF_STORE_ALL_CAPS);\n> >  \trepo->refs_private = new_refs;\n> >  \tref_store_release(old_refs);\n\n...is that we must put _something_ useful into repo->refs_private,\nbecause old_refs is an alias for it that we are freeing. I suspect that\n\"git ref migrate\" does not really look at the repo any more after this\nmigration function returns, but it makes sense for it to leave things in\na consistent state.\n\nSo my biggest question is just whether there is any downside to doing\nthe release/init pair rather than trying to reuse the existing struct.\n\nI do think it probably causes a small memory leak. The \"init\" function\nallocates the actual ref_store struct, but the \"release\" function\ndoesn't seem to free it. So we are probably leaking the store that\npoints to the temp directory. But that is also true of \"old_refs\", or of\n\"new_refs\" if we hit an error. So I think the solution is probably for\ninit/release to be symmetric, and for the latter to clean up everything.\nBut again, I'd prefer to get input from Patrick there.\n\n-Peff\n"},{"id":"498892","messageId":"f97198e1-bfcc-4f3c-ad0a-2dd27d4f20cf@ramsayjones.plus.com","threadId":"61786","inReplyTo":"20240718005723.GA675057@coredump.intra.peff.net","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-07-18T01:22:30Z","receivedAt":"2024-07-18T01:22:33Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 18/07/2024 01:57, Jeff King wrote:\n> On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:\n[snip]\n\n>> I also don't know the code well enough to answer your question regarding\n>> the re-opening of the migrated ref-store, but it doesn't look like it would\n>> cause any problems (famous last words).\n> \n> Thanks for testing. This is new in the upcoming release, but I think\n> it's localized to the \"git ref migrate\" command. So aside from the\n> annoyance of the test failure for you, it is not too urgent. I'm tempted\n> to put it off until Patrick has had a chance to weigh in, even if it\n> means missing the v2.46 cutoff.\n\nYes, I think it would be better for Patrick to take a look. I added Adam to\nthe CC list to keep him in the loop (because he is the cygwin git package\nmaintainer); he may have a view on the timing issues.\n\nPersonally, I would be fine with a post v2.46 fix, but it is not up to me. :)\n\nThanks.\n\nATB,\nRamsay Jones\n\n"},{"id":"498897","messageId":"xmqqh6cn1ev1.fsf@gitster.g","threadId":"61786","inReplyTo":"f97198e1-bfcc-4f3c-ad0a-2dd27d4f20cf@ramsayjones.plus.com","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-18T04:52:18Z","receivedAt":"2024-07-18T04:52:24Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> On 18/07/2024 01:57, Jeff King wrote:\n>> On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:\n> [snip]\n>\n>>> I also don't know the code well enough to answer your question regarding\n>>> the re-opening of the migrated ref-store, but it doesn't look like it would\n>>> cause any problems (famous last words).\n>> \n>> Thanks for testing. This is new in the upcoming release, but I think\n>> it's localized to the \"git ref migrate\" command. So aside from the\n>> annoyance of the test failure for you, it is not too urgent. I'm tempted\n>> to put it off until Patrick has had a chance to weigh in, even if it\n>> means missing the v2.46 cutoff.\n>\n> Yes, I think it would be better for Patrick to take a look. I added Adam to\n> the CC list to keep him in the loop (because he is the cygwin git package\n> maintainer); he may have a view on the timing issues.\n>\n> Personally, I would be fine with a post v2.46 fix, but it is not up to me. :)\n\nThanks.  I am inclined to postpone it after 2.46 final, and not to\ndisturb Patrick during his honeymoon.\n\n"},{"id":"499032","messageId":"CA+kUOakUYURJN0O+jRHVKdN9O0MY01Ck_PYa3ewipqUrpKFaCA@mail.gmail.com","threadId":"61786","inReplyTo":"f97198e1-bfcc-4f3c-ad0a-2dd27d4f20cf@ramsayjones.plus.com","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2024-07-21T13:48:03Z","receivedAt":"2024-07-21T13:48:40Z","isPatch":false,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Thu, 18 Jul 2024 at 02:22, Ramsay Jones wrote:\n> On 18/07/2024 01:57, Jeff King wrote:\n> > On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:\n> [snip]\n>\n> >> I also don't know the code well enough to answer your question regarding\n> >> the re-opening of the migrated ref-store, but it doesn't look like it would\n> >> cause any problems (famous last words).\n> >\n> > Thanks for testing. This is new in the upcoming release, but I think\n> > it's localized to the \"git ref migrate\" command. So aside from the\n> > annoyance of the test failure for you, it is not too urgent. I'm tempted\n> > to put it off until Patrick has had a chance to weigh in, even if it\n> > means missing the v2.46 cutoff.\n>\n> Yes, I think it would be better for Patrick to take a look. I added Adam to\n> the CC list to keep him in the loop (because he is the cygwin git package\n> maintainer); he may have a view on the timing issues.\n>\n> Personally, I would be fine with a post v2.46 fix, but it is not up to me. :)\n\nAbsolutely fine with a post-v2.46 fix for Cygwin's official packaging.\nReal Life™ has meant I'm not on top of the latest releases anyway, so\nneeding to wait a bit longer before taking v2.46 won't realistically\nmake much difference anyway.\n\nAdam\n"},{"id":"499144","messageId":"Zp-GQ0MQmuIyAear@tanuki","threadId":"61786","inReplyTo":"20240718005723.GA675057@coredump.intra.peff.net","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-23T10:30:27Z","receivedAt":"2024-07-23T10:30:36Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 17, 2024 at 08:57:23PM -0400, Jeff King wrote:\n> On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:\n> > The patch given below fixes the test for me! (I have only run t1460-refs-migrate.sh,\n> > since the full test-suite takes 6 hours to run, but now all 30 tests pass!)\n> > \n> > I also don't know the code well enough to answer your question regarding\n> > the re-opening of the migrated ref-store, but it doesn't look like it would\n> > cause any problems (famous last words).\n> \n> Thanks for testing. This is new in the upcoming release, but I think\n> it's localized to the \"git ref migrate\" command. So aside from the\n> annoyance of the test failure for you, it is not too urgent. I'm tempted\n> to put it off until Patrick has had a chance to weigh in, even if it\n> means missing the v2.46 cutoff.\n\nThanks all for digging into this while I was out!\n\n> I'd also be OK with pursuing it in the meantime if folks feel\n> differently. Having slept on it, I think the answer to one of my\n> questions here...\n> \n> > > -\tfree(new_refs->gitdir);\n> > > -\tnew_refs->gitdir = xstrdup(old_refs->gitdir);\n> > > +\t/*\n> > > +\t * Re-open the now-migrated ref store. I'm not sure if this is strictly\n> > > +\t * needed or not. Perhaps it would also be a good time to check that\n> > > +\t * we correctly opened it, it's in the expected format, etc?\n> > > +\t */\n> > > +\tnew_refs = ref_store_init(repo, format, old_refs->gitdir,\n> > > +\t\t\t\t  REF_STORE_ALL_CAPS);\n> > >  \trepo->refs_private = new_refs;\n> > >  \tref_store_release(old_refs);\n> \n> ...is that we must put _something_ useful into repo->refs_private,\n> because old_refs is an alias for it that we are freeing. I suspect that\n> \"git ref migrate\" does not really look at the repo any more after this\n> migration function returns, but it makes sense for it to leave things in\n> a consistent state.\n\nYeah, `repo->refs_private` shouldn't ever be accessed after the\nmigration has finished. Still, as you say, it feels like the correct\nthing to do to keep the `repo` in a consistent state, even though it's\nnot necessary in our codebase right now.\n\n> So my biggest question is just whether there is any downside to doing\n> the release/init pair rather than trying to reuse the existing struct.\n\nThere shouldn't be any downside, but it is wasted effort. The main ref\nstore should always be accessed via `get_main_ref_store()`, and that\nfunction knows to lazily initialize the refdb as required. So instead, I\nthink the preferable fix is to release the new ref store after we have\npopulated it and set the main ref store of the repository to `NULL`\ninstead of re-initializing it.\n\nSo the following should be sufficient:\n\n    diff --git a/refs.c b/refs.c\n    index bb90a18875..915aeb4d1d 100644\n    --- a/refs.c\n    +++ b/refs.c\n    @@ -2843,6 +2843,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n            goto done;\n        }\n     \n    +\t/*\n    +\t * Release the new ref store such that any potentially-open files will\n    +\t * be closed. This is required for platforms like Cygwin, where\n    +\t * renaming an open file results in EPERM.\n    +\t */\n    +\tref_store_release(new_refs);\n    +\tFREE_AND_NULL(new_refs);\n    +\n        /*\n         * Until now we were in the non-destructive phase, where we only\n         * populated the new ref store. From hereon though we are about\n    @@ -2874,10 +2882,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n         */\n        initialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1);\n     \n    -\tfree(new_refs->gitdir);\n    -\tnew_refs->gitdir = xstrdup(old_refs->gitdir);\n    -\trepo->refs_private = new_refs;\n    +\t/*\n    +\t * Unset the old ref store and release it. `get_main_ref_store()` will\n    +\t * make sure to lazily re-initialize the repository's ref store with\n    +\t * the new format.\n    +\t */\n        ref_store_release(old_refs);\n    +\tFREE_AND_NULL(old_refs);\n    +\trepo->refs_private = NULL;\n     \n        ret = 0;\n     \n    @@ -2888,8 +2900,10 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n                    new_gitdir.buf);\n        }\n     \n    -\tif (ret && new_refs)\n    +\tif (new_refs) {\n            ref_store_release(new_refs);\n    +\t\tfree(new_refs);\n    +\t}\n        ref_transaction_free(transaction);\n        strbuf_release(&new_gitdir);\n        return ret;\n\n> I do think it probably causes a small memory leak. The \"init\" function\n> allocates the actual ref_store struct, but the \"release\" function\n> doesn't seem to free it. So we are probably leaking the store that\n> points to the temp directory. But that is also true of \"old_refs\", or of\n> \"new_refs\" if we hit an error. So I think the solution is probably for\n> init/release to be symmetric, and for the latter to clean up everything.\n> But again, I'd prefer to get input from Patrick there.\n\nYeah, we'd have to free the new ref store struct, as well. I wouldn't\nmake `release()` free the structure as that would be uncustomarily\nnamed.\n\nPatrick\n"},{"id":"499147","messageId":"d031aef5552d894784650a8c6580925e877df794.1721731179.git.ps@pks.im","threadId":"61786","inReplyTo":"aacb4067-a16b-4953-a72f-3c66efd3cf25@ramsayjones.plus.com","subject":"[PATCH] refs: fix format migration on Cygwin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-23T12:31:28Z","receivedAt":"2024-07-23T12:31:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was reported that t1460-refs-migrate.sh fails when using Cygwin with\nerrors like the following:\n\n    error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied\n\nAs some debugging surfaced, the root cause of this is that some files of\nthe newly-initialized ref store are still open when the target format is\nthe \"reftable\" format, and Cygwin refuses to rename open files.\n\nFix this issue by closing the new ref store before renaming its files\ninto place. This is a slight change in behaviour compared to before,\nwhere we kept the new ref store open and then updated the repository's\nref store to point to it.\n\nWhile we could re-open the new ref store after we have moved files\naround, this is ultimately unnecessary. We know that the only user of\n`repo_migrate_ref_storage_format()` is the git-refs(1) command, and it\nwon't access the ref store after it has been migrated anyway. So\nreinitializing the ref store would be a waste of time. Regardless of\nthat it is still sensible to leave the repository in a consistent state.\nBut instead of reinitializing the ref store, we can simply unset the\nrepo's ref store altogether and let `get_main_ref_store()` lazily\ninitialize the new ref store as required.\n\nReported-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs.c | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex bb90a18875..915aeb4d1d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2843,6 +2843,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \t\tgoto done;\n \t}\n \n+\t/*\n+\t * Release the new ref store such that any potentially-open files will\n+\t * be closed. This is required for platforms like Cygwin, where\n+\t * renaming an open file results in EPERM.\n+\t */\n+\tref_store_release(new_refs);\n+\tFREE_AND_NULL(new_refs);\n+\n \t/*\n \t * Until now we were in the non-destructive phase, where we only\n \t * populated the new ref store. From hereon though we are about\n@@ -2874,10 +2882,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \t */\n \tinitialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1);\n \n-\tfree(new_refs->gitdir);\n-\tnew_refs->gitdir = xstrdup(old_refs->gitdir);\n-\trepo->refs_private = new_refs;\n+\t/*\n+\t * Unset the old ref store and release it. `get_main_ref_store()` will\n+\t * make sure to lazily re-initialize the repository's ref store with\n+\t * the new format.\n+\t */\n \tref_store_release(old_refs);\n+\tFREE_AND_NULL(old_refs);\n+\trepo->refs_private = NULL;\n \n \tret = 0;\n \n@@ -2888,8 +2900,10 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n \t\t\t    new_gitdir.buf);\n \t}\n \n-\tif (ret && new_refs)\n+\tif (new_refs) {\n \t\tref_store_release(new_refs);\n+\t\tfree(new_refs);\n+\t}\n \tref_transaction_free(transaction);\n \tstrbuf_release(&new_gitdir);\n \treturn ret;\n-- \n2.46.0.rc1.dirty\n\n"},{"id":"499193","messageId":"6a4737df-16a0-4337-87ce-7ec11b40255d@ramsayjones.plus.com","threadId":"61786","inReplyTo":"d031aef5552d894784650a8c6580925e877df794.1721731179.git.ps@pks.im","subject":"Re: [PATCH] refs: fix format migration on Cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2024-07-23T19:29:11Z","receivedAt":"2024-07-23T19:29:21Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 23/07/2024 13:31, Patrick Steinhardt wrote:\n> It was reported that t1460-refs-migrate.sh fails when using Cygwin with\n> errors like the following:\n> \n>     error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied\n> \n> As some debugging surfaced, the root cause of this is that some files of\n> the newly-initialized ref store are still open when the target format is\n> the \"reftable\" format, and Cygwin refuses to rename open files.\n> \n> Fix this issue by closing the new ref store before renaming its files\n> into place. This is a slight change in behaviour compared to before,\n> where we kept the new ref store open and then updated the repository's\n> ref store to point to it.\n> \n> While we could re-open the new ref store after we have moved files\n> around, this is ultimately unnecessary. We know that the only user of\n> `repo_migrate_ref_storage_format()` is the git-refs(1) command, and it\n> won't access the ref store after it has been migrated anyway. So\n> reinitializing the ref store would be a waste of time. Regardless of\n> that it is still sensible to leave the repository in a consistent state.\n> But instead of reinitializing the ref store, we can simply unset the\n> repo's ref store altogether and let `get_main_ref_store()` lazily\n> initialize the new ref store as required.\n> \n> Reported-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n\nI applied this patch on top of v2.46.0-rc1, on both Linux and cygwin, and\nran the tests (just t1460-*.sh on cygwin, complete test-suite in Linux).\n\nI can confirm all 30 tests pass on cygwin! :)\n\nThanks all.\n\nATB,\nRamsay Jones\n\n\n>  refs.c | 22 ++++++++++++++++++----\n>  1 file changed, 18 insertions(+), 4 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index bb90a18875..915aeb4d1d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2843,6 +2843,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n>  \t\tgoto done;\n>  \t}\n>  \n> +\t/*\n> +\t * Release the new ref store such that any potentially-open files will\n> +\t * be closed. This is required for platforms like Cygwin, where\n> +\t * renaming an open file results in EPERM.\n> +\t */\n> +\tref_store_release(new_refs);\n> +\tFREE_AND_NULL(new_refs);\n> +\n>  \t/*\n>  \t * Until now we were in the non-destructive phase, where we only\n>  \t * populated the new ref store. From hereon though we are about\n> @@ -2874,10 +2882,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n>  \t */\n>  \tinitialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1);\n>  \n> -\tfree(new_refs->gitdir);\n> -\tnew_refs->gitdir = xstrdup(old_refs->gitdir);\n> -\trepo->refs_private = new_refs;\n> +\t/*\n> +\t * Unset the old ref store and release it. `get_main_ref_store()` will\n> +\t * make sure to lazily re-initialize the repository's ref store with\n> +\t * the new format.\n> +\t */\n>  \tref_store_release(old_refs);\n> +\tFREE_AND_NULL(old_refs);\n> +\trepo->refs_private = NULL;\n>  \n>  \tret = 0;\n>  \n> @@ -2888,8 +2900,10 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n>  \t\t\t    new_gitdir.buf);\n>  \t}\n>  \n> -\tif (ret && new_refs)\n> +\tif (new_refs) {\n>  \t\tref_store_release(new_refs);\n> +\t\tfree(new_refs);\n> +\t}\n>  \tref_transaction_free(transaction);\n>  \tstrbuf_release(&new_gitdir);\n>  \treturn ret;\n"},{"id":"499195","messageId":"xmqqcyn327fi.fsf@gitster.g","threadId":"61786","inReplyTo":"6a4737df-16a0-4337-87ce-7ec11b40255d@ramsayjones.plus.com","subject":"Re: [PATCH] refs: fix format migration on Cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-23T20:13:21Z","receivedAt":"2024-07-23T20:13:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n>> Reported-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n>> Helped-by: Jeff King <peff@peff.net>\n>> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n>> ---\n>\n> I applied this patch on top of v2.46.0-rc1, on both Linux and cygwin, and\n> ran the tests (just t1460-*.sh on cygwin, complete test-suite in Linux).\n>\n> I can confirm all 30 tests pass on cygwin! :)\n\nLet's merge it down through 'next' to 'master' and have it in the\nupcoming release.\n\nThanks, all.\n"},{"id":"499197","messageId":"20240723204953.GA6779@coredump.intra.peff.net","threadId":"61786","inReplyTo":"Zp-GQ0MQmuIyAear@tanuki","subject":"Re: v2.46.0-rc0 test failures on cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-23T20:49:53Z","receivedAt":"2024-07-23T20:49:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 23, 2024 at 12:30:27PM +0200, Patrick Steinhardt wrote:\n\n> > So my biggest question is just whether there is any downside to doing\n> > the release/init pair rather than trying to reuse the existing struct.\n> \n> There shouldn't be any downside, but it is wasted effort. The main ref\n> store should always be accessed via `get_main_ref_store()`, and that\n> function knows to lazily initialize the refdb as required. So instead, I\n> think the preferable fix is to release the new ref store after we have\n> populated it and set the main ref store of the repository to `NULL`\n> instead of re-initializing it.\n\nAh, that is even better. I'm not too worried about wasted effort (this\nis a one-time thing during the ref backend migration), but I think we\nare better off leaving as much to the \"regular\" ref code as possible.\n\n> So the following should be sufficient:\n> [...]\n\nYeah, that looks nice.\n\n> > I do think it probably causes a small memory leak. The \"init\" function\n> > allocates the actual ref_store struct, but the \"release\" function\n> > doesn't seem to free it. So we are probably leaking the store that\n> > points to the temp directory. But that is also true of \"old_refs\", or of\n> > \"new_refs\" if we hit an error. So I think the solution is probably for\n> > init/release to be symmetric, and for the latter to clean up everything.\n> > But again, I'd prefer to get input from Patrick there.\n> \n> Yeah, we'd have to free the new ref store struct, as well. I wouldn't\n> make `release()` free the structure as that would be uncustomarily\n> named.\n\nWe usually don't free in the release() function because it was not\nallocated on the init() side in the first place. The real sin here is\nthat the init/release pair is not symmetric, regardless of what they are\nnamed.\n\nIn the patch above you worked around it by just doing a manual free() of\nthe struct. That's crossing the abstraction barrier a little bit, but I\nthink is OK in this instance. We don't generally expect a ref stores to\nbe created and released a lot. If this were a general purpose data\nstructure like a strbuf that gets used everywhere, I'd be more\nconcerned.\n\n-Peff\n"},{"id":"499198","messageId":"20240723205232.GB6779@coredump.intra.peff.net","threadId":"61786","inReplyTo":"d031aef5552d894784650a8c6580925e877df794.1721731179.git.ps@pks.im","subject":"Re: [PATCH] refs: fix format migration on Cygwin","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-23T20:52:32Z","receivedAt":"2024-07-23T20:52:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 23, 2024 at 02:31:28PM +0200, Patrick Steinhardt wrote:\n\n> @@ -2874,10 +2882,14 @@ int repo_migrate_ref_storage_format(struct repository *repo,\n>  \t */\n>  \tinitialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1);\n>  \n> -\tfree(new_refs->gitdir);\n> -\tnew_refs->gitdir = xstrdup(old_refs->gitdir);\n> -\trepo->refs_private = new_refs;\n> +\t/*\n> +\t * Unset the old ref store and release it. `get_main_ref_store()` will\n> +\t * make sure to lazily re-initialize the repository's ref store with\n> +\t * the new format.\n> +\t */\n>  \tref_store_release(old_refs);\n> +\tFREE_AND_NULL(old_refs);\n> +\trepo->refs_private = NULL;\n\nI think this FREE_AND_NULL() is not technically part of the fix that the\ncommit message describes. It is fixing an existing leak that happens\nwhen we overwrite repo->refs_private (whether with new_refs or with\nNULL).\n\nThat said, I don't know that it's worth going back to split it out now.\n\nThe rest of the patch looks good to me, and the commit message nicely\ndescribes the problem and solution.\n\n-Peff\n"}]}