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

[Victoria Dye <vdye@github.com>]

Derrick Stolee wrote:
> On 3/24/2022 9:56 PM, Victoria Dye via GitGitGadget wrote:
>> From: Victoria Dye <vdye@github.com>
>>
>> Add 'refresh_cache_entry()' after moving the index entry in
>> 'rename_index_entry_at()'. Internally, 'git mv' uses
>> 'rename_index_entry_at()' to move the source index entry to the destination,
>> overwriting the old entry if '-f' is specified. However, it does not refresh
>> the stat information on destination index entry, making its 'CE_UPTODATE'
>> flag out-of-date until the index is refreshed (e.g., by 'git status').
>>
>> Some commands, such as 'git reset', assume the 'CE_UPTODATE' information
>> they read from the index is accurate, and use that information to determine
>> whether the operation can be done successfully or not. In order to ensure
>> the index is correct for commands such as these, explicitly refresh the
>> destination index entry in 'git mv' before exiting.
> 
> Good find. Thanks for the fix!
> 
>> Reported-by: Maximilian Reichel <reichemn@icloud.com>
> 
> Thanks for the report, Maximilian!
> 
>> @@ -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);
> 
> It certainly seems reasonable to add this line. I was unfamiliar
> with this method, and it is used only twice: when creating a new
> cache entry in make_cache_entry() and in merge-recursive.c's
> add_cache_info(). So, it is currently acting in the case of a
> newly-inserted cache entry in its existing cases, and here in
> 'git mv' that's essentially what we are doing (deleting the old
> and adding a new would be more appropriate than just moving the
> old one).
> 
> I took a brief detour thinking about performance, but this is
> run only once per command-line argument, so any new overhead
> is minimal.
>   
>> +test_expect_success 'mv -f refreshes updated index entry' '
>> +	echo test >bar &&
>> +	git add bar &&
>> +	git commit -m test &&
>> +
>> +	echo foo >foo &&
>> +	git add foo &&
>> +	git mv -f foo bar &&
>> +	git reset --merge HEAD
> 
> Is there any post-condition on the index that we want to check here?
> 
> That is, is there anything that we could notice via 'git status' or
> similar that would break before this patch (assuming we put a
> test_might_fail in front of the 'git reset --merge HEAD' line)?
> 

While looking into this, I realized 1) that I wasn't actually refreshing the
cache entry because 'refresh_cache_entry' doesn't work in-place (will fix in
the next version) and 2) the test was only passing because of a race
condition that (as of right now) I can't quite figure out. 

If I add a 'sleep 1' after the 'git add', the test behaves as I expect:
fails when 'mv' doesn't refresh the entry, passes when it does. However,
when the sleep *isn't* there and I'm testing 'git mv' without the refresh,
most of the time the test *doesn't* fail (sometimes it does, but not
reliably). I'm going to try finding the root cause of that before sending
V2, in case I'm missing something else that should be fixed.

But to answer your question, yes - 'git diff-files' will produce an empty
result if the cache is reset properly, and will be non-empty if it is not.
I'll include that in the re-roll as well. 

> Thanks,
> -Stolee