Re: [PATCH] mv: refresh stat info for moved entry

From: Victoria Dye <vdye@github.com>
Date: Mar 26, 2022, 01:23 UTC
Message-ID: <703439d9-bf4e-4e71-754b-60dabc07301a@github.com>
In-Reply-To: <xmqqk0ch8vc3.fsf@gitster.g>

Junio C Hamano wrote:

Show 25 quoted lines

> "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> diff --git a/read-cache.c b/read-cache.c
>> index 1ad56d02e1d..2c5ccc48d6c 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -148,6 +148,7 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n
>>  	untracked_cache_remove_from_index(istate, old_entry->name);
>>  	remove_index_entry_at(istate, nr);
>>  	add_index_entry(istate, new_entry, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
>> +	refresh_cache_entry(istate, new_entry, CE_MATCH_REFRESH);
>>  }
> 
> This is a bit unforunate.
> 
> If we are renaming "foo" to "bar", I wonder if we can grab the
> cached stat info before calling remove_index_entry_at() for "foo"
> and copy it to the new entry we create for "bar".  After all, we
> would be making a corresponding change to rename from "foo" to "bar"
> at the filesystem level, too, no?
> 
> Well, we are already doing that by calling copy_cache_entry().  So
> why do we further need to refresh the cache entry in the first
> place?  There is something fishy going on, isn't it?
>

After some more debugging, I found that the exact trigger for the "not uptodate" error is a mismatch between the 'ctime' logged in the index entry and the 'ctime' found on disk. Because we're copying stat information over directly from the original index entry in 'git mv', the 'ctime' does *not* reflect the time of file renaming, leading to the error later.

I think this explains all of the behavior we've seen so far:

1. Any index-refreshing operation run after 'git mv' would prevent the 'git
   reset --merge' error, since the ctime would be updated.
2. The error doesn't happen when the file is created within ~1 second of the
   'git mv' (noticed in the test earlier today [1]), since the 'ctime' would
   be seen as "matching" between the index and on-disk.
3. Adding 'refresh_cache_entry()' (and assigning the return value properly,
   unlike in V1 of this patch) avoids the error for the same reason as #1.

So, based on that, I think a "refresh" at the end of 'rename_index_entry_at()' is still needed, but only the stat info needs to be updated. If that sounds reasonable, I'll send V2 with that change and update the test to more appropriately test this scenario.

Thanks!

[1] https://lore.kernel.org/git/d03a34e6-d6a7-6ddb-5784-57078e32ab89@github.com/

Show 8 quoted lines

> Puzzling...
> 
> In any case, refresh_cache_entry() either returns ce or create a
> newly allocated entry, so you'd want to first refresh and then the
> add the cache entry returned from the function to the index, I
> think.
> 
> Thanks.