Re: [PATCH 2/2] stash push: remove duplicate changes detection

[Phillip Wood <phillip.wood123@gmail.com>]

On 07/10/2026 18:15, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> Before the precontext of this hunk, repo_refresh_and_write_index()
>>> is called to refresh the index.  We used to leave early when
>>> check_changes() saw no need to save.  We no longer do so, and
>>> instead keep going.
>>>
>>>> @@ -1743,8 +1737,15 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>>>>    
>>>>    	if (stash_msg)
>>>>    		strbuf_addstr(&stash_msg_buf, stash_msg);
>>>> -	if (do_create_stash(ps, &stash_msg_buf, include_untracked, patch_mode,
>>>> -			    interactive_opts, only_staged, &info, &patch, quiet)) {
>>>> +	ret =  do_create_stash(ps, &stash_msg_buf, include_untracked,
>>>> +			       patch_mode, interactive_opts, only_staged, &info,
>>>> +			       &patch, quiet);
>>>
>>> And we call do_create_stash().  The first thing it does is to call
>>> repo_read_index_preload() and repo_refresh_and_write_index().
>>>
>>> Are we refreshing the index twice now, even though we know nothing
>>> has changed in between, when we run "git stash push"?
>>
>> We've always been doing that when there is something to stash (which is
>> the normal case).
> 
> So when there is nothing to stash, we only refreshed once but now
> refreshing twice is not a regression?

If there are no changes then the first refresh will mark all the index 
entries as up-to-date, which means the second refresh wont stat anything 
so I'm not sure there is a noticeable change. To me the more important 
problem is that we're lstat()ing each changed file half a dozen times 
before we call check_changes() when we should be lstat()ing them twice.

>> To avoid that we need to make the callers of do_create_stash()
>> responsible for refreshing the index.  At the moment
>> create_stash() calls do_create_stash() without evening reading the
>> index, but do_push_stash() needs to read the index to check if it
>> the pathspec contains paths that don't match the index. That would
>> also avoid calling preload_index() multiple times.
> 
> Yes, that exactly is the right direction.  Then we can reduce the
> number of check_changes() without increasing the number of
> refreshes, right?

Yes and improve things for the case where we actually have something to 
stash.

Thanks

Phillip