v2.46.0-rc0 test failures on cygwin

v2.46.0-rc0 test failures on cygwin

[Ramsay Jones]

The 'Test Summary Report' for the v2.46.0-rc0 test run, on cygwin, includes:

  ...
  t1460-refs-migrate.sh                            (Wstat: 256 (exited 1) Tests: 30 Failed: 9)
    Failed tests:  8-15, 29
    Non-zero exit status: 1
  ...

Looking at the test output directly, we see:
 
  $ pwd
  /home/ramsay/git
  $ cd t
  $ make clean
  rm -f -r 'chainlinttmp'
  rm -f -r 'trash directory'.*
  rm -f -r valgrind/bin
  rm -f -r 'test-results'
  rm -f .prove
  $ ./t1460-refs-migrate.sh -v -i
  ...
  ok 7 - files -> reftable: migration with worktree fails
  
  expecting success of 1460.8 'files -> reftable: unborn HEAD': 
  			test_when_finished "rm -rf repo" &&
  			git init --ref-format=$from_format repo &&
  			test_migration repo "$to_format"
  		
  Initialized empty Git repository in /home/ramsay/git/t/trash directory.t1460-refs-migrate/repo/.git/
  error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied
  migrated refs can be found at '.git/ref_migration.sr9pEF'
  not ok 8 - files -> reftable: unborn HEAD
  #	
  #				test_when_finished "rm -rf repo" &&
  #				git init --ref-format=$from_format repo &&
  #				test_migration repo "$to_format"
  #			
  1..8
  $ 

Note that all of the errors in this test look similar to this one (ie. when
rename()-ing the 'migrated' reftable directory to .git/reftable we get an
'Permission denied' error). So the error message is from line 2672 of refs.c
in the move_files() function.

Let's have a quick look at the test directory:

  $ cd trash\ directory.t1460-refs-migrate/
  $ ls
  err  expect  repo/
  $ cd repo
  $ ls -l .git
  total 7.0K
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 branches/
  -rw-r--r-- 1 ramsay None 86 Jul 16 19:53 config
  -rw-r--r-- 1 ramsay None 73 Jul 16 19:53 description
  -rw-r--r-- 1 ramsay None 25 Jul 16 19:53 HEAD
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 hooks/
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 info/
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 objects/
  drwx------ 1 ramsay None  0 Jul 16 19:53 ref_migration.sr9pEF/
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 refs/

Now try to finish the migration by hand:

  $ mv .git/ref_migration.sr9pEF/reftable .git/reftable

Hmm, note no error; of course, the mv command may well do much more than
the rename() library function, so they are not necessarily equivalent.

  $ ls -l .git
  total 7.0K
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 branches/
  -rw-r--r-- 1 ramsay None 86 Jul 16 19:53 config
  -rw-r--r-- 1 ramsay None 73 Jul 16 19:53 description
  -rw-r--r-- 1 ramsay None 25 Jul 16 19:53 HEAD
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 hooks/
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 info/
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 objects/
  drwx------ 1 ramsay None  0 Jul 16 19:55 ref_migration.sr9pEF/
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 refs/
  drwxr-xr-x 1 ramsay None  0 Jul 16 19:53 reftable/
  $ 
  $ rm -rf .git/ref_migration.sr9pEF/
  $ git branch -v
  fatal: failed to resolve HEAD as a valid ref
  $ ls -l .git/reftable
  total 2.0K
  -rw-r--r-- 1 ramsay None 124 Jul 16 19:53 0x000000000001-0x000000000001-64e987ec.ref
  -rw-r--r-- 1 ramsay None  43 Jul 16 19:53 tables.list
  $ xxd .git/reftable/0x000000000001-0x000000000001-64e987ec.ref 
  00000000: 5245 4654 0100 1000 0000 0000 0000 0001  REFT............
  00000010: 0000 0000 0000 0001 7200 0038 0023 4845  ........r..8.#HE
  00000020: 4144 000f 7265 6673 2f68 6561 6473 2f6d  AD..refs/heads/m
  00000030: 6169 6e00 001c 0001 5245 4654 0100 1000  ain.....REFT....
  00000040: 0000 0000 0000 0001 0000 0000 0000 0001  ................
  00000050: 0000 0000 0000 0000 0000 0000 0000 0000  ................
  00000060: 0000 0000 0000 0000 0000 0000 0000 0000  ................
  00000070: 0000 0000 0000 0000 b6bf f78a            ............
  $ 

So, the HEAD ref was referring to refs/heads/main.

  $ pwd
  /home/ramsay/git/t/trash directory.t1460-refs-migrate/repo
  $ git init --ref-format=reftable
  fatal: attempt to reinitialize repository with different reference storage format
  $ xxd .git/refs/heads 
  00000000: 7468 6973 2072 6570 6f73 6974 6f72 7920  this repository 
  00000010: 7573 6573 2074 6865 2072 6566 7461 626c  uses the reftabl
  00000020: 6520 666f 726d 6174 0a                   e format.
  $ 
  $ git branch -v
  fatal: failed to resolve HEAD as a valid ref
  $ 
 
Hmm, so quite broken :)

Maybe the order of some of the actions in repo_migrate_ref_storage_format() may
need to be re-thought! ;)

The 'man 3 rename' is not very enlightening; 'The conditions for failure depend
on the host operating system.'

Unfortunately, I don't have any (well much) spare time to dig further into
this at the moment, so I thought I should at least report it here for others
to hopefully find a solution.

ATB,
Ramsay Jones

Re: v2.46.0-rc0 test failures on cygwin

[Jeff King]

On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:

>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied
>   migrated refs can be found at '.git/ref_migration.sr9pEF'
> [...]
> Now try to finish the migration by hand:
> 
>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable
> 
> Hmm, note no error; of course, the mv command may well do much more than
> the rename() library function, so they are not necessarily equivalent.

This is a shot in the dark, but: could the problem be an open file that
cannot be moved? If I run a "ref migrate" on my Linux system in the
debugger and stop at move_files(), checking /proc/<pid>/fd shows an open
descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.

Does the patch below fix things for you? I'm not too familiar with the
code, so this is what I cobbled together.  The best response will be
from Patrick, but I think he's offline for another week or so. In the
meantime, this at least doesn't crash for me. ;) And I confirmed that
the tables.list file is closed during the move_files() call.

-Peff

---
diff --git a/refs.c b/refs.c
index bb90a18875..06a0fc5099 100644
--- a/refs.c
+++ b/refs.c
@@ -2843,6 +2843,12 @@ int repo_migrate_ref_storage_format(struct repository *repo,
 		goto done;
 	}
 
+	/*
+	 * Close the new ref store to avoid holding on to any open files
+	 * which could interfere with moving things behind the scenes.
+	 */
+	ref_store_release(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,8 +2880,13 @@ 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);
+	/*
+	 * Re-open the now-migrated ref store. I'm not sure if this is strictly
+	 * needed or not. Perhaps it would also be a good time to check that
+	 * we correctly opened it, it's in the expected format, etc?
+	 */
+	new_refs = ref_store_init(repo, format, old_refs->gitdir,
+				  REF_STORE_ALL_CAPS);
 	repo->refs_private = new_refs;
 	ref_store_release(old_refs);
 

Re: v2.46.0-rc0 test failures on cygwin

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:
>
>>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied
>>   migrated refs can be found at '.git/ref_migration.sr9pEF'
>> [...]
>> Now try to finish the migration by hand:
>> 
>>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable
>> 
>> Hmm, note no error; of course, the mv command may well do much more than
>> the rename() library function, so they are not necessarily equivalent.
>
> This is a shot in the dark, but: could the problem be an open file that
> cannot be moved? If I run a "ref migrate" on my Linux system in the
> debugger and stop at move_files(), checking /proc/<pid>/fd shows an open
> descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.

Hmph, I wonder ifthat means the same test should be failing on
Windows, too?

Re: v2.46.0-rc0 test failures on cygwin

[Ramsay Jones]

On 17/07/2024 15:48, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:
>>
>>>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied
>>>   migrated refs can be found at '.git/ref_migration.sr9pEF'
>>> [...]
>>> Now try to finish the migration by hand:
>>>
>>>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable
>>>
>>> Hmm, note no error; of course, the mv command may well do much more than
>>> the rename() library function, so they are not necessarily equivalent.
>>
>> This is a shot in the dark, but: could the problem be an open file that
>> cannot be moved? If I run a "ref migrate" on my Linux system in the
>> debugger and stop at move_files(), checking /proc/<pid>/fd shows an open
>> descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.
> 
> Hmph, I wonder ifthat means the same test should be failing on
> Windows, too?
> 

Since we have not heard anything from Gfw, I had assumed it was cygwin
specific (Gfw uses mingw_rename() not rename() so we should not necessarily
expect it to behave the same).

However, Jeff's patch fixes the problem for me (see other email).

ATB,
Ramsay Jones

Re: v2.46.0-rc0 test failures on cygwin

[Ramsay Jones]

On 17/07/2024 07:42, Jeff King wrote:
> On Tue, Jul 16, 2024 at 08:45:48PM +0100, Ramsay Jones wrote:
> 
>>   error: could not link file '.git/ref_migration.sr9pEF/reftable' to '.git/reftable': Permission denied
>>   migrated refs can be found at '.git/ref_migration.sr9pEF'
>> [...]
>> Now try to finish the migration by hand:
>>
>>   $ mv .git/ref_migration.sr9pEF/reftable .git/reftable
>>
>> Hmm, note no error; of course, the mv command may well do much more than
>> the rename() library function, so they are not necessarily equivalent.
> 
> This is a shot in the dark, but: could the problem be an open file that
> cannot be moved? If I run a "ref migrate" on my Linux system in the
> debugger and stop at move_files(), checking /proc/<pid>/fd shows an open
> descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.

Heh, a very good shot in the dark! ;)

> Does the patch below fix things for you? I'm not too familiar with the
> code, so this is what I cobbled together.  The best response will be
> from Patrick, but I think he's offline for another week or so. In the
> meantime, this at least doesn't crash for me. ;) And I confirmed that
> the tables.list file is closed during the move_files() call.

The patch given below fixes the test for me! (I have only run t1460-refs-migrate.sh,
since the full test-suite takes 6 hours to run, but now all 30 tests pass!)

I also don't know the code well enough to answer your question regarding
the re-opening of the migrated ref-store, but it doesn't look like it would
cause any problems (famous last words).

Thanks!

ATB,
Ramsay Jones

> ---
> diff --git a/refs.c b/refs.c
> index bb90a18875..06a0fc5099 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2843,6 +2843,12 @@ int repo_migrate_ref_storage_format(struct repository *repo,
>  		goto done;
>  	}
>  
> +	/*
> +	 * Close the new ref store to avoid holding on to any open files
> +	 * which could interfere with moving things behind the scenes.
> +	 */
> +	ref_store_release(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,8 +2880,13 @@ 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);
> +	/*
> +	 * Re-open the now-migrated ref store. I'm not sure if this is strictly
> +	 * needed or not. Perhaps it would also be a good time to check that
> +	 * we correctly opened it, it's in the expected format, etc?
> +	 */
> +	new_refs = ref_store_init(repo, format, old_refs->gitdir,
> +				  REF_STORE_ALL_CAPS);
>  	repo->refs_private = new_refs;
>  	ref_store_release(old_refs);
>  
> 

Re: v2.46.0-rc0 test failures on cygwin

[Jeff King]

On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:

> > This is a shot in the dark, but: could the problem be an open file that
> > cannot be moved? If I run a "ref migrate" on my Linux system in the
> > debugger and stop at move_files(), checking /proc/<pid>/fd shows an open
> > descriptor for .git/ref_migration.WnJ8TS/reftable/tables.list.
> 
> Heh, a very good shot in the dark! ;)

Lucky guess. :) When Junio pointed out that we'd expect Windows to fail
in that case, too, I thought for sure I was just wrong. So I'm glad it
worked out.

> The patch given below fixes the test for me! (I have only run t1460-refs-migrate.sh,
> since the full test-suite takes 6 hours to run, but now all 30 tests pass!)
> 
> I also don't know the code well enough to answer your question regarding
> the re-opening of the migrated ref-store, but it doesn't look like it would
> cause any problems (famous last words).

Thanks for testing. This is new in the upcoming release, but I think
it's localized to the "git ref migrate" command. So aside from the
annoyance of the test failure for you, it is not too urgent. I'm tempted
to put it off until Patrick has had a chance to weigh in, even if it
means missing the v2.46 cutoff.

I'd also be OK with pursuing it in the meantime if folks feel
differently. Having slept on it, I think the answer to one of my
questions here...

> > -	free(new_refs->gitdir);
> > -	new_refs->gitdir = xstrdup(old_refs->gitdir);
> > +	/*
> > +	 * Re-open the now-migrated ref store. I'm not sure if this is strictly
> > +	 * needed or not. Perhaps it would also be a good time to check that
> > +	 * we correctly opened it, it's in the expected format, etc?
> > +	 */
> > +	new_refs = ref_store_init(repo, format, old_refs->gitdir,
> > +				  REF_STORE_ALL_CAPS);
> >  	repo->refs_private = new_refs;
> >  	ref_store_release(old_refs);

...is that we must put _something_ useful into repo->refs_private,
because old_refs is an alias for it that we are freeing. I suspect that
"git ref migrate" does not really look at the repo any more after this
migration function returns, but it makes sense for it to leave things in
a consistent state.

So my biggest question is just whether there is any downside to doing
the release/init pair rather than trying to reuse the existing struct.

I do think it probably causes a small memory leak. The "init" function
allocates the actual ref_store struct, but the "release" function
doesn't seem to free it. So we are probably leaking the store that
points to the temp directory. But that is also true of "old_refs", or of
"new_refs" if we hit an error. So I think the solution is probably for
init/release to be symmetric, and for the latter to clean up everything.
But again, I'd prefer to get input from Patrick there.

-Peff

Re: v2.46.0-rc0 test failures on cygwin

[Ramsay Jones]

On 18/07/2024 01:57, Jeff King wrote:
> On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:
[snip]

>> I also don't know the code well enough to answer your question regarding
>> the re-opening of the migrated ref-store, but it doesn't look like it would
>> cause any problems (famous last words).
> 
> Thanks for testing. This is new in the upcoming release, but I think
> it's localized to the "git ref migrate" command. So aside from the
> annoyance of the test failure for you, it is not too urgent. I'm tempted
> to put it off until Patrick has had a chance to weigh in, even if it
> means missing the v2.46 cutoff.

Yes, I think it would be better for Patrick to take a look. I added Adam to
the CC list to keep him in the loop (because he is the cygwin git package
maintainer); he may have a view on the timing issues.

Personally, I would be fine with a post v2.46 fix, but it is not up to me. :)

Thanks.

ATB,
Ramsay Jones

Re: v2.46.0-rc0 test failures on cygwin

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 18/07/2024 01:57, Jeff King wrote:
>> On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:
> [snip]
>
>>> I also don't know the code well enough to answer your question regarding
>>> the re-opening of the migrated ref-store, but it doesn't look like it would
>>> cause any problems (famous last words).
>> 
>> Thanks for testing. This is new in the upcoming release, but I think
>> it's localized to the "git ref migrate" command. So aside from the
>> annoyance of the test failure for you, it is not too urgent. I'm tempted
>> to put it off until Patrick has had a chance to weigh in, even if it
>> means missing the v2.46 cutoff.
>
> Yes, I think it would be better for Patrick to take a look. I added Adam to
> the CC list to keep him in the loop (because he is the cygwin git package
> maintainer); he may have a view on the timing issues.
>
> Personally, I would be fine with a post v2.46 fix, but it is not up to me. :)

Thanks.  I am inclined to postpone it after 2.46 final, and not to
disturb Patrick during his honeymoon.

Re: v2.46.0-rc0 test failures on cygwin

[Adam Dinwoodie]

On Thu, 18 Jul 2024 at 02:22, Ramsay Jones wrote:
> On 18/07/2024 01:57, Jeff King wrote:
> > On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:
> [snip]
>
> >> I also don't know the code well enough to answer your question regarding
> >> the re-opening of the migrated ref-store, but it doesn't look like it would
> >> cause any problems (famous last words).
> >
> > Thanks for testing. This is new in the upcoming release, but I think
> > it's localized to the "git ref migrate" command. So aside from the
> > annoyance of the test failure for you, it is not too urgent. I'm tempted
> > to put it off until Patrick has had a chance to weigh in, even if it
> > means missing the v2.46 cutoff.
>
> Yes, I think it would be better for Patrick to take a look. I added Adam to
> the CC list to keep him in the loop (because he is the cygwin git package
> maintainer); he may have a view on the timing issues.
>
> Personally, I would be fine with a post v2.46 fix, but it is not up to me. :)

Absolutely fine with a post-v2.46 fix for Cygwin's official packaging.
Real Life™ has meant I'm not on top of the latest releases anyway, so
needing to wait a bit longer before taking v2.46 won't realistically
make much difference anyway.

Adam

Re: v2.46.0-rc0 test failures on cygwin

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5111 bytes --]

On Wed, Jul 17, 2024 at 08:57:23PM -0400, Jeff King wrote:
> On Wed, Jul 17, 2024 at 07:05:43PM +0100, Ramsay Jones wrote:
> > The patch given below fixes the test for me! (I have only run t1460-refs-migrate.sh,
> > since the full test-suite takes 6 hours to run, but now all 30 tests pass!)
> > 
> > I also don't know the code well enough to answer your question regarding
> > the re-opening of the migrated ref-store, but it doesn't look like it would
> > cause any problems (famous last words).
> 
> Thanks for testing. This is new in the upcoming release, but I think
> it's localized to the "git ref migrate" command. So aside from the
> annoyance of the test failure for you, it is not too urgent. I'm tempted
> to put it off until Patrick has had a chance to weigh in, even if it
> means missing the v2.46 cutoff.

Thanks all for digging into this while I was out!

> I'd also be OK with pursuing it in the meantime if folks feel
> differently. Having slept on it, I think the answer to one of my
> questions here...
> 
> > > -	free(new_refs->gitdir);
> > > -	new_refs->gitdir = xstrdup(old_refs->gitdir);
> > > +	/*
> > > +	 * Re-open the now-migrated ref store. I'm not sure if this is strictly
> > > +	 * needed or not. Perhaps it would also be a good time to check that
> > > +	 * we correctly opened it, it's in the expected format, etc?
> > > +	 */
> > > +	new_refs = ref_store_init(repo, format, old_refs->gitdir,
> > > +				  REF_STORE_ALL_CAPS);
> > >  	repo->refs_private = new_refs;
> > >  	ref_store_release(old_refs);
> 
> ...is that we must put _something_ useful into repo->refs_private,
> because old_refs is an alias for it that we are freeing. I suspect that
> "git ref migrate" does not really look at the repo any more after this
> migration function returns, but it makes sense for it to leave things in
> a consistent state.

Yeah, `repo->refs_private` shouldn't ever be accessed after the
migration has finished. Still, as you say, it feels like the correct
thing to do to keep the `repo` in a consistent state, even though it's
not necessary in our codebase right now.

> So my biggest question is just whether there is any downside to doing
> the release/init pair rather than trying to reuse the existing struct.

There shouldn't be any downside, but it is wasted effort. The main ref
store should always be accessed via `get_main_ref_store()`, and that
function knows to lazily initialize the refdb as required. So instead, I
think the preferable fix is to release the new ref store after we have
populated it and set the main ref store of the repository to `NULL`
instead of re-initializing it.

So the following should be sufficient:

    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;

> I do think it probably causes a small memory leak. The "init" function
> allocates the actual ref_store struct, but the "release" function
> doesn't seem to free it. So we are probably leaking the store that
> points to the temp directory. But that is also true of "old_refs", or of
> "new_refs" if we hit an error. So I think the solution is probably for
> init/release to be symmetric, and for the latter to clean up everything.
> But again, I'd prefer to get input from Patrick there.

Yeah, we'd have to free the new ref store struct, as well. I wouldn't
make `release()` free the structure as that would be uncustomarily
named.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: v2.46.0-rc0 test failures on cygwin

[Jeff King]

On Tue, Jul 23, 2024 at 12:30:27PM +0200, Patrick Steinhardt wrote:

> > So my biggest question is just whether there is any downside to doing
> > the release/init pair rather than trying to reuse the existing struct.
> 
> There shouldn't be any downside, but it is wasted effort. The main ref
> store should always be accessed via `get_main_ref_store()`, and that
> function knows to lazily initialize the refdb as required. So instead, I
> think the preferable fix is to release the new ref store after we have
> populated it and set the main ref store of the repository to `NULL`
> instead of re-initializing it.

Ah, that is even better. I'm not too worried about wasted effort (this
is a one-time thing during the ref backend migration), but I think we
are better off leaving as much to the "regular" ref code as possible.

> So the following should be sufficient:
> [...]

Yeah, that looks nice.

> > I do think it probably causes a small memory leak. The "init" function
> > allocates the actual ref_store struct, but the "release" function
> > doesn't seem to free it. So we are probably leaking the store that
> > points to the temp directory. But that is also true of "old_refs", or of
> > "new_refs" if we hit an error. So I think the solution is probably for
> > init/release to be symmetric, and for the latter to clean up everything.
> > But again, I'd prefer to get input from Patrick there.
> 
> Yeah, we'd have to free the new ref store struct, as well. I wouldn't
> make `release()` free the structure as that would be uncustomarily
> named.

We usually don't free in the release() function because it was not
allocated on the init() side in the first place. The real sin here is
that the init/release pair is not symmetric, regardless of what they are
named.

In the patch above you worked around it by just doing a manual free() of
the struct. That's crossing the abstraction barrier a little bit, but I
think is OK in this instance. We don't generally expect a ref stores to
be created and released a lot. If this were a general purpose data
structure like a strbuf that gets used everywhere, I'd be more
concerned.

-Peff

[PATCH] refs: fix format migration on Cygwin

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]

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(-)

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;
-- 
2.46.0.rc1.dirty


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] refs: fix format migration on Cygwin

[Ramsay Jones]

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;

Re: [PATCH] refs: fix format migration on Cygwin

[Junio C Hamano]

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.

Re: [PATCH] refs: fix format migration on Cygwin

[Jeff King]

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