| Message ID | d031aef5552d894784650a8c6580925e877df794.1721731179.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 09c817383f56d4bbcfb8089e7c4ed5dd9db0f547 |
| Headers | show |
| Series | refs: fix format migration on Cygwin | expand |
On 23/07/2024 13:31, Patrick Steinhardt wrote: > It was reported that t1460-refs-migrate.sh fails when using Cygwin with > errors like the following: > > error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied > > As some debugging surfaced, the root cause of this is that some files of > the newly-initialized ref store are still open when the target format is > the "reftable" format, and Cygwin refuses to rename open files. > > Fix this issue by closing the new ref store before renaming its files > into place. This is a slight change in behaviour compared to before, > where we kept the new ref store open and then updated the repository's > ref store to point to it. > > While we could re-open the new ref store after we have moved files > around, this is ultimately unnecessary. We know that the only user of > `repo_migrate_ref_storage_format()` is the git-refs(1) command, and it > won't access the ref store after it has been migrated anyway. So > reinitializing the ref store would be a waste of time. Regardless of > that it is still sensible to leave the repository in a consistent state. > But instead of reinitializing the ref store, we can simply unset the > repo's ref store altogether and let `get_main_ref_store()` lazily > initialize the new ref store as required. > > Reported-by: Ramsay Jones <ramsay@ramsayjones.plus.com> > Helped-by: Jeff King <peff@peff.net> > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- I applied this patch on top of v2.46.0-rc1, on both Linux and cygwin, and ran the tests (just t1460-*.sh on cygwin, complete test-suite in Linux). I can confirm all 30 tests pass on cygwin! :) Thanks all. ATB, Ramsay Jones > refs.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/refs.c b/refs.c > index bb90a18875..915aeb4d1d 100644 > --- a/refs.c > +++ b/refs.c > @@ -2843,6 +2843,14 @@ int repo_migrate_ref_storage_format(struct repository *repo, > goto done; > } > > + /* > + * Release the new ref store such that any potentially-open files will > + * be closed. This is required for platforms like Cygwin, where > + * renaming an open file results in EPERM. > + */ > + ref_store_release(new_refs); > + FREE_AND_NULL(new_refs); > + > /* > * Until now we were in the non-destructive phase, where we only > * populated the new ref store. From hereon though we are about > @@ -2874,10 +2882,14 @@ int repo_migrate_ref_storage_format(struct repository *repo, > */ > initialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1); > > - free(new_refs->gitdir); > - new_refs->gitdir = xstrdup(old_refs->gitdir); > - repo->refs_private = new_refs; > + /* > + * Unset the old ref store and release it. `get_main_ref_store()` will > + * make sure to lazily re-initialize the repository's ref store with > + * the new format. > + */ > ref_store_release(old_refs); > + FREE_AND_NULL(old_refs); > + repo->refs_private = NULL; > > ret = 0; > > @@ -2888,8 +2900,10 @@ int repo_migrate_ref_storage_format(struct repository *repo, > new_gitdir.buf); > } > > - if (ret && new_refs) > + if (new_refs) { > ref_store_release(new_refs); > + free(new_refs); > + } > ref_transaction_free(transaction); > strbuf_release(&new_gitdir); > return ret;
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: >> Reported-by: Ramsay Jones <ramsay@ramsayjones.plus.com> >> Helped-by: Jeff King <peff@peff.net> >> Signed-off-by: Patrick Steinhardt <ps@pks.im> >> --- > > I applied this patch on top of v2.46.0-rc1, on both Linux and cygwin, and > ran the tests (just t1460-*.sh on cygwin, complete test-suite in Linux). > > I can confirm all 30 tests pass on cygwin! :) Let's merge it down through 'next' to 'master' and have it in the upcoming release. Thanks, all.
On Tue, Jul 23, 2024 at 02:31:28PM +0200, Patrick Steinhardt wrote: > @@ -2874,10 +2882,14 @@ int repo_migrate_ref_storage_format(struct repository *repo, > */ > initialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1); > > - free(new_refs->gitdir); > - new_refs->gitdir = xstrdup(old_refs->gitdir); > - repo->refs_private = new_refs; > + /* > + * Unset the old ref store and release it. `get_main_ref_store()` will > + * make sure to lazily re-initialize the repository's ref store with > + * the new format. > + */ > ref_store_release(old_refs); > + FREE_AND_NULL(old_refs); > + repo->refs_private = NULL; I think this FREE_AND_NULL() is not technically part of the fix that the commit message describes. It is fixing an existing leak that happens when we overwrite repo->refs_private (whether with new_refs or with NULL). That said, I don't know that it's worth going back to split it out now. The rest of the patch looks good to me, and the commit message nicely describes the problem and solution. -Peff
diff --git a/refs.c b/refs.c index bb90a18875..915aeb4d1d 100644 --- a/refs.c +++ b/refs.c @@ -2843,6 +2843,14 @@ int repo_migrate_ref_storage_format(struct repository *repo, goto done; } + /* + * Release the new ref store such that any potentially-open files will + * be closed. This is required for platforms like Cygwin, where + * renaming an open file results in EPERM. + */ + ref_store_release(new_refs); + FREE_AND_NULL(new_refs); + /* * Until now we were in the non-destructive phase, where we only * populated the new ref store. From hereon though we are about @@ -2874,10 +2882,14 @@ int repo_migrate_ref_storage_format(struct repository *repo, */ initialize_repository_version(hash_algo_by_ptr(repo->hash_algo), format, 1); - free(new_refs->gitdir); - new_refs->gitdir = xstrdup(old_refs->gitdir); - repo->refs_private = new_refs; + /* + * Unset the old ref store and release it. `get_main_ref_store()` will + * make sure to lazily re-initialize the repository's ref store with + * the new format. + */ ref_store_release(old_refs); + FREE_AND_NULL(old_refs); + repo->refs_private = NULL; ret = 0; @@ -2888,8 +2900,10 @@ int repo_migrate_ref_storage_format(struct repository *repo, new_gitdir.buf); } - if (ret && new_refs) + if (new_refs) { ref_store_release(new_refs); + free(new_refs); + } ref_transaction_free(transaction); strbuf_release(&new_gitdir); return ret;
It was reported that t1460-refs-migrate.sh fails when using Cygwin with errors like the following: error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied As some debugging surfaced, the root cause of this is that some files of the newly-initialized ref store are still open when the target format is the "reftable" format, and Cygwin refuses to rename open files. Fix this issue by closing the new ref store before renaming its files into place. This is a slight change in behaviour compared to before, where we kept the new ref store open and then updated the repository's ref store to point to it. While we could re-open the new ref store after we have moved files around, this is ultimately unnecessary. We know that the only user of `repo_migrate_ref_storage_format()` is the git-refs(1) command, and it won't access the ref store after it has been migrated anyway. So reinitializing the ref store would be a waste of time. Regardless of that it is still sensible to leave the repository in a consistent state. But instead of reinitializing the ref store, we can simply unset the repo's ref store altogether and let `get_main_ref_store()` lazily initialize the new ref store as required. Reported-by: Ramsay Jones <ramsay@ramsayjones.plus.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)