[PATCH 0/2] stash: stop checking for changes twice

[PATCH 0/2] stash: stop checking for changes twice

[Phillip Wood]

"git stash push" and "git stash create" check if there are
any unstaged or uncommitted changes at startup, and then again
when they try to create the stash. This short series removes
that duplication of effort which I spotted while looking at
<20260929074222.11942-1-kazumasa.shigeta@kanamei.com>.

base-commit: c46c1e37724f0478939de636ab8ea5a89086d532
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fstash-optimize-check_changes-calls%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/c46c1e377...95b7d582a
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-optimize-check_changes-calls/v1


Phillip Wood (2):
  stash create: remove duplicate changes detection
  stash push: remove duplicate changes detection

 builtin/stash.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

-- 
2.56.0.134.g299a3c16181

[PATCH 1/2] stash create: remove duplicate changes detection

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Before it creates a stash, git checks if there are any unstaged,
or uncommitted changes. If there isn't anything to stash it bails
out. Since ef0f0b4509 (stash: optimize `get_untracked_files()`
and `check_changes()`, 2019-02-25) "git stash store" has checked
for changes twice, once in create_stash() before we refresh the
index and then again in do_create_stash() after the index has been
refreshed. That commit claims it is an optimization but it is not
clear what it is trying to optimize by checking for changes twice,
especially as checking for changes before refreshing the index is
unreliable (the scripted version of "git stash store", called "git
update-index -q --refresh" before looking for any changes).

Avoid checking for changes twice by removing the call to
check_changes_tracked_files() from store_stash() and restore the return
code handling in store_stash() that was removed by ef0f0b4509 so that
we continue to exit 0 when there are no changes to stash. In principle
we could remove the call to check_changes() from do_store_stash()
instead, but then we'd need to pass in the list of untracked files.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/stash.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 7a9843413b1..9a5006e3d92 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1655,8 +1655,6 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
 	strbuf_join_argv(&stash_msg_buf, argc - 1, ++argv, ' ');
 
 	memset(&ps, 0, sizeof(ps));
-	if (!check_changes_tracked_files(&ps))
-		return 0;
 
 	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, NULL, 0, &info,
 			      NULL, 0);
@@ -1665,7 +1663,11 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
 
 	free_stash_info(&info);
 	strbuf_release(&stash_msg_buf);
-	return ret;
+	/*
+	 * ret is 1 if there were no changes. In this case, we should
+	 * not error out.
+	 */
+	return ret < 0;
 }
 
 static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet,
-- 
2.56.0.134.g299a3c16181

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

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Before it creates a stash, git checks if there are any unstaged,
or uncommitted changes. If there isn't anything to stash it bails
out. "git stash push" checks for changes twice, once in do_push_stash()
and then again in do_create_stash(). Avoid that by removing the call
to check_changes() from do_push_stash() and checking the return value
of do_create_stash() to see if there were any changes to stash. If
check_changes() finds there are no changes do_create_stash() now
returns 2 rather than 1. This enables us to distinguish between there
being no changes and there being nothing stashed so that "git stash
push --patch" when nothing is select, and "git stash push --staged"
when the index matches HEAD still exit 1 rather than 0.

There is still one small change in behavior as, if there is nothing
to stash, we'll try now to create the reflog for stashes before we
realize that there is nothing to stash. I don't think that should
matter in practice.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/stash.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

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;
 	}
 
@@ -1664,8 +1664,8 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
 	free_stash_info(&info);
 	strbuf_release(&stash_msg_buf);
 	/*
-	 * ret is 1 if there were no changes. In this case, we should
-	 * not error out.
+	 * ret is greater than zero if there were no changes. In this case,
+	 * we should not error out.
 	 */
 	return ret < 0;
 }
@@ -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)
@@ -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);
+	if (ret == 2) {
+		if (!quiet)
+			printf_ln(_("No local changes to save"));
+		ret = 0;
+		goto done;
+	} else if (ret) {
 		ret = -1;
 		goto done;
 	}
-- 
2.56.0.134.g299a3c16181

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Before it creates a stash, git checks if there are any unstaged,
> or uncommitted changes. If there isn't anything to stash it bails
> out. Since ef0f0b4509 (stash: optimize `get_untracked_files()`
> and `check_changes()`, 2019-02-25) "git stash store" has checked

"store"?  Aren't we talking about "create"?

> unreliable (the scripted version of "git stash store", called "git
> update-index -q --refresh" before looking for any changes).

Ditto.

> Avoid checking for changes twice by removing the call to
> check_changes_tracked_files() from store_stash() and restore the return
> code handling in store_stash() that was removed by ef0f0b4509 so that
> we continue to exit 0 when there are no changes to stash. In principle
> we could remove the call to check_changes() from do_store_stash()
> instead, but then we'd need to pass in the list of untracked files.

Again "(do_)?store" -> "\1create"?

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

[Junio C Hamano]

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.

> @@ -1664,8 +1664,8 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
>  	free_stash_info(&info);
>  	strbuf_release(&stash_msg_buf);
>  	/*
> -	 * ret is 1 if there were no changes. In this case, we should
> -	 * not error out.
> +	 * ret is greater than zero if there were no changes. In this case,
> +	 * we should not error out.
>  	 */
>  	return ret < 0;
>  }
> @@ -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"?

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.

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

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

[Phillip Wood]

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;
>>   	}

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

[Phillip Wood]

On 06/10/2026 13:44, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Before it creates a stash, git checks if there are any unstaged,
>> or uncommitted changes. If there isn't anything to stash it bails
>> out. Since ef0f0b4509 (stash: optimize `get_untracked_files()`
>> and `check_changes()`, 2019-02-25) "git stash store" has checked
> 
> "store"?  Aren't we talking about "create"?

Sorry, it looks like I managed to confuse "create" with "store" when I 
wrote the message. It should be

stash create: remove duplicate changes detection

Before it creates a stash, git checks if there are any unstaged,
or uncommitted changes. If there isn't anything to stash it bails
out. Since ef0f0b4509 (stash: optimize `get_untracked_files()`
and `check_changes()`, 2019-02-25) "git stash create" has checked
for changes twice, once in create_stash() before we refresh the
index and then again in do_create_stash() after the index has been
refreshed. That commit claims it is an optimization but it is not
clear what it is trying to optimize by checking for changes twice,
especially as checking for changes before refreshing the index is
unreliable (the scripted version of "git stash create", called "git
update-index -q --refresh" before looking for any changes).

Avoid checking for changes twice by removing the call to
check_changes_tracked_files() from create_stash() and restore the return
code handling in create_stash() that was removed by ef0f0b4509 so that
we continue to exit 0 when there are no changes to stash. In principle
we could remove the call to check_changes() from do_create_stash()
instead, but then we'd need to pass in the list of untracked files.

Thanks

Phillip

> 
>> unreliable (the scripted version of "git stash store", called "git
>> update-index -q --refresh" before looking for any changes).
> 
> Ditto.
> 
>> Avoid checking for changes twice by removing the call to
>> check_changes_tracked_files() from store_stash() and restore the return
>> code handling in store_stash() that was removed by ef0f0b4509 so that
>> we continue to exit 0 when there are no changes to stash. In principle
>> we could remove the call to check_changes() from do_store_stash()
>> instead, but then we'd need to pass in the list of untracked files.
> 
> Again "(do_)?store" -> "\1create"?

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

[Junio C Hamano]

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?

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

Thanks.

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

[Phillip Wood]

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