From: Phillip Wood <phillip.wood123@gmail.com>
Date: Thu, 08 Oct 2026 13:35:40 GMT
Subject: Re: [PATCH 2/2] stash push: remove duplicate changes detection
Message-ID: <82bf1054-8455-489d-b832-302909711b2a@gmail.com>
In-Reply-To: <xmqqa4opa0up.fsf@gitster.g>

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