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

[Derrick Stolee <derrickstolee@github.com>]

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

Thanks,
-Stolee