From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 07 Oct 2026 13:46:08 GMT
Subject: Re: [PATCH 2/2] stash push: remove duplicate changes detection
Message-ID: <725487d3-5a07-40c6-a603-990a661e0193@gmail.com>
In-Reply-To: <xmqqpkxmgb00.fsf@gitster.g>

On 06/10/2026 15:27, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> >> diff --git a/builtin/stash.c b/builtin/stash.c
>> index 9a5006e3d92..79fdfff09a2 100644
>> --- a/builtin/stash.c
>> +++ b/builtin/stash.c
>> @@ -1538,7 +1538,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>>   	}
>>   >>   	if (!check_changes(ps, include_untracked, &untracked_files)) {
>> -		ret = 1;
>> +		ret = 2;
>>   		goto done;
>>   	}
> > It may be time for us to introduce symbolic constants once we have
> three choices instead of two.

That makes sense

>> @@ -1728,12 +1728,6 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>>   		goto done;
>>   	}
>>   >> -	if (!check_changes(ps, include_untracked, &untracked_files)) {
>> -		if (!quiet)
>> -			printf_ln(_("No local changes to save"));
>> -		goto done;
>> -	}
>> -
>>   	if (!refs_reflog_exists(get_main_ref_store(the_repository), ref_stash) && do_clear_stash()) {
>>   		ret = -1;
>>   		if (!quiet)
> > 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). 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.

> do_create_stash() does call check_changes() to return early without
> creating stash, so we did save the cost of check_changes() with this
> patch, though.

Yes, lets add another patch to avoid unnecessarily refreshing the index as well.

Thanks

Phillip

>> +	if (ret == 2) {
>> +		if (!quiet)
>> +			printf_ln(_("No local changes to save"));
>> +		ret = 0;
>> +		goto done;
>> +	} else if (ret) {
>>   		ret = -1;
>>   		goto done;
>>   	}