[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

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

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

[PATCH v2 0/7] stash creation: avoid unecessary index loads and refreshes

[Phillip Wood]

Thanks to Junio for his comments on V1, this series has expanded in
scope to avoid unecessary index refreshes.

"git stash push" and "git stash create" load and refresh the index
multiple times when they only need to do it once. This series removes
a number of calls to repo_read_preload_index() and refresh_index()
which reduces the number of lstat() calls on each changed path by
four for "git stash create" and eight for "git stash push". It also
removes a duplicate call to check_changes() so that when stashing
untracked files we only walk them once.

Changes since V1:
 - Added an enum for the various return values from do_create_stash()
 - Fixed typos in the commit message of patch 2 (was patch 1)
 - Removed an unnecessary index refresh and several unnecessary index
   preloads

Cover letter for V1:

"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%2Fv2
View-Changes-At: https://github.com/phillipwood/git/compare/c46c1e377...db53bf3b5
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/stash-optimize-check_changes-calls/v2


Phillip Wood (7):
  stash: use an enum for create status
  stash create: remove duplicate changes detection
  stash push: remove duplicate changes detection
  stash push: avoid duplicate calls to preload_index()
  stash create: rework temporary index creation
  stash create: remove unnecessary index load and refresh
  stash: simplify reset_tree() function signature

 builtin/stash.c | 137 ++++++++++++++++++++++--------------------------
 1 file changed, 62 insertions(+), 75 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  9e27bcd676 stash: use an enum for create status
1:  1617d92942 ! 2:  d56b7c6f67 stash create: remove duplicate changes detection
    @@ Commit message
         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
    +    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 store", called "git
    +    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 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.
    +    check_changes_tracked_files() from create_stash(). In principle we
    +    could remove the call to check_changes() from do_create_stash()
    +    instead, but it is more convenient to keep it there, rather than
    +    require each caller to call it instead.
    +
    +    The remaining callers of check_changes_tracked_files() sensibly read
    +    and refresh the index before calling it, so it no-longer needs to
    +    call to repo_read_index().
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## builtin/stash.c ##
    +@@ builtin/stash.c: static int check_changes_tracked_files(const struct pathspec *ps)
    + 	if (repo_get_oid(the_repository, "HEAD", &dummy))
    + 		return -1;
    + 
    +-	if (repo_read_index(the_repository) < 0)
    +-		return -1;
    +-
    + 	repo_init_revisions(the_repository, &rev, NULL);
    + 	copy_pathspec(&rev.prune_data, ps);
    + 
     @@ builtin/stash.c: static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
      	strbuf_join_argv(&stash_msg_buf, argc - 1, ++argv, ' ');
      
    @@ builtin/stash.c: static int create_stash(int argc, const char **argv, const char
      
      	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, NULL, 0, &info,
      			      NULL, 0);
    -@@ builtin/stash.c: 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:  95b7d582a2 ! 3:  36fc0eb913 stash push: remove duplicate changes detection
    @@ Metadata
      ## Commit message ##
         stash push: 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. "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.
    +    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(). While duplicating the checks for staged and
    +    unstaged charges is fairly cheap because they stop after finding the
    +    first changed path, if we're stashing untracked files then we visit
    +    them all twice. 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 CREATE_NO_CHANGES
    +    rather than CREATE_EMPTY. This enables us to distinguish between there
    +    being no changes and there being nothing stashed, so that "git stash
    +    push --patch" when nothing is selected, and "git stash push --staged"
    +    when the index matches HEAD, still exit 1 rather than 0.
    +
    +    As we're not calling check_changes() in do_push_stash() anymore, we do
    +    not need to refresh the index there either because do_create_stash()
    +    refreshes it again before calling check_changes(). Avoiding the extra
    +    index refresh saves us two lstat() calls per changed path.
     
         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
    +    to stash, we'll now try 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 ##
    -@@ builtin/stash.c: static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
    +@@ builtin/stash.c: enum create_result {
    + 	CREATE_SUCCESS,
    + 	/* No changes were stashed */
    + 	CREATE_EMPTY,
    ++	/* There were no changes to stash */
    ++	CREATE_NO_CHANGES,
    + };
    + 
    + static enum create_result stash_staged(struct stash_info *info,
    +@@ builtin/stash.c: static enum create_result do_create_stash(const struct pathspec *ps,
      	}
      
      	if (!check_changes(ps, include_untracked, &untracked_files)) {
    --		ret = 1;
    -+		ret = 2;
    +-		ret = CREATE_EMPTY;
    ++		ret = CREATE_NO_CHANGES;
      		goto done;
      	}
      
    -@@ builtin/stash.c: 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;
    - }
     @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
    - 		goto done;
    + 		free(ps_matched);
      	}
      
    +-	if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
    +-					 NULL, NULL, NULL)) {
    +-		ret = error(_("could not write index"));
    +-		goto done;
    +-	}
    +-
     -	if (!check_changes(ps, include_untracked, &untracked_files)) {
     -		if (!quiet)
     -			printf_ln(_("No local changes to save"));
    @@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char
      		ret = -1;
      		if (!quiet)
     @@ builtin/stash.c: 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) {
    + 	create_res =  do_create_stash(ps, &stash_msg_buf, include_untracked,
    + 				      patch_mode, interactive_opts, only_staged,
    + 				      &info, &patch, quiet);
    +-	if (create_res != CREATE_SUCCESS) {
    ++	if (create_res == CREATE_NO_CHANGES) {
     +		if (!quiet)
     +			printf_ln(_("No local changes to save"));
     +		ret = 0;
     +		goto done;
    -+	} else if (ret) {
    ++	} else if (create_res != CREATE_SUCCESS) {
      		ret = -1;
      		goto done;
      	}
-:  ---------- > 4:  6ca7e1c567 stash push: avoid duplicate calls to preload_index()
-:  ---------- > 5:  f799d86f52 stash create: rework temporary index creation
-:  ---------- > 6:  768d6bc7e5 stash create: remove unnecessary index load and refresh
-:  ---------- > 7:  db53bf3b57 stash: simplify reset_tree() function signature
-- 
2.56.0.134.g299a3c16181

[PATCH v2 1/7] stash: use an enum for create status

[Phillip Wood]

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

Change the return type of do_create_stash() to an enum in preparation
for adding a separate value for "there are no changes to stash" in a
future commit. There is a small change to behavior here which fixes
an anomaly with the exit status of create_stash(). That function calls
check_changes() without refreshing the index and exits 0 if there are
no changes. However, because it does not refresh the index, that check
is unreliable which means do_create_stash() can return 1 when there
aren't any changes, and then create_stash() exits 1 rather than 0.
This commit fixes that anomaly.

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

diff --git a/builtin/stash.c b/builtin/stash.c
index 7a9843413b..3e50ccfb40 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1336,16 +1336,23 @@ static int save_untracked_files(struct stash_info *info, struct strbuf *msg,
 	return ret;
 }
 
-static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
-			int quiet)
+enum create_result {
+	CREATE_ERROR = -1,
+	CREATE_SUCCESS,
+	/* No changes were stashed */
+	CREATE_EMPTY,
+};
+
+static enum create_result stash_staged(struct stash_info *info,
+				  struct strbuf *out_patch, int quiet)
 {
-	int ret = 0;
+	enum create_result ret = 0;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
 	struct index_state istate = INDEX_STATE_INIT(the_repository);
 
 	if (write_index_as_tree(&info->w_tree, &istate, the_repository->index_file,
 				0, NULL)) {
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 
@@ -1354,26 +1361,26 @@ static int stash_staged(struct stash_info *info, struct strbuf *out_patch,
 		     "--no-color",
 		     "-U1", "HEAD", oid_to_hex(&info->w_tree), "--", NULL);
 	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 
 	if (!out_patch->len) {
 		if (!quiet)
 			fprintf_ln(stderr, _("No staged changes"));
-		ret = 1;
+		ret = CREATE_EMPTY;
 	}
 
 done:
 	release_index(&istate);
 	return ret;
 }
 
-static int stash_patch(struct stash_info *info, const struct pathspec *ps,
-		       struct strbuf *out_patch, int quiet,
-		       struct interactive_options *interactive_opts)
+static enum create_result stash_patch(struct stash_info *info,
+		const struct pathspec *ps, struct strbuf *out_patch, int quiet,
+		struct interactive_options *interactive_opts)
 {
-	int ret = 0;
+	enum create_result ret = 0;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
 	struct commit *head_commit;
 	const struct object_id *head_tree;
@@ -1384,17 +1391,17 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 
 	head_commit = lookup_commit(the_repository, &info->b_commit);
 	if (!head_commit || repo_parse_commit(the_repository, head_commit)) {
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 	head_tree = get_commit_tree_oid(head_commit);
 	if (!head_tree) {
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 
 	if (create_index_from_tree(head_tree, stash_index_path.buf)) {
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 
@@ -1416,7 +1423,7 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 	/* State of the working tree. */
 	if (write_index_as_tree(&info->w_tree, &istate, stash_index_path.buf, 0,
 				NULL)) {
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 
@@ -1425,14 +1432,14 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 		     "--no-color",
 		     oid_to_hex(&info->w_tree), "--", NULL);
 	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 
 	if (!out_patch->len) {
 		if (!quiet)
 			fprintf_ln(stderr, _("No changes selected"));
-		ret = 1;
+		ret = CREATE_EMPTY;
 	}
 
 done:
@@ -1499,13 +1506,13 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	return ret;
 }
 
-static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_buf,
-			   int include_untracked, int patch_mode,
-			   struct interactive_options *interactive_opts,
-			   int only_staged, struct stash_info *info, struct strbuf *patch,
-			   int quiet)
+static enum create_result do_create_stash(const struct pathspec *ps,
+		struct strbuf *stash_msg_buf, int include_untracked,
+		int patch_mode, struct interactive_options *interactive_opts,
+		int only_staged, struct stash_info *info, struct strbuf *patch,
+		int quiet)
 {
-	int ret = 0;
+	enum create_result ret = 0;
 	int flags = 0;
 	int untracked_commit_option = 0;
 	const char *head_short_sha1 = NULL;
@@ -1531,14 +1538,14 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 		if (!quiet)
 			fprintf_ln(stderr, _("You do not have "
 					     "the initial commit yet"));
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	} else {
 		head_commit = lookup_commit(the_repository, &info->b_commit);
 	}
 
 	if (!check_changes(ps, include_untracked, &untracked_files)) {
-		ret = 1;
+		ret = CREATE_EMPTY;
 		goto done;
 	}
 
@@ -1565,7 +1572,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 		if (!quiet)
 			fprintf_ln(stderr, _("Cannot save the current "
 					     "index state"));
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 
@@ -1577,37 +1584,37 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 			if (!quiet)
 				fprintf_ln(stderr, _("Cannot save "
 						     "the untracked files"));
-			ret = -1;
+			ret = CREATE_ERROR;
 			goto done;
 		}
 		untracked_commit_option = 1;
 	}
 	if (patch_mode) {
 		ret = stash_patch(info, ps, patch, quiet, interactive_opts);
-		if (ret < 0) {
+		if (ret == CREATE_ERROR) {
 			if (!quiet)
 				fprintf_ln(stderr, _("Cannot save the current "
 						     "worktree state"));
 			goto done;
-		} else if (ret > 0) {
+		} else if (ret != CREATE_SUCCESS) {
 			goto done;
 		}
 	} else if (only_staged) {
 		ret = stash_staged(info, patch, quiet);
-		if (ret < 0) {
+		if (ret == CREATE_ERROR) {
 			if (!quiet)
 				fprintf_ln(stderr, _("Cannot save the current "
 						     "staged state"));
 			goto done;
-		} else if (ret > 0) {
+		} else if (ret != CREATE_SUCCESS) {
 			goto done;
 		}
 	} else {
 		if (stash_working_tree(info, ps)) {
 			if (!quiet)
 				fprintf_ln(stderr, _("Cannot save the current "
 						     "worktree state"));
-			ret = -1;
+			ret = CREATE_ERROR;
 			goto done;
 		}
 	}
@@ -1630,7 +1637,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
 		if (!quiet)
 			fprintf_ln(stderr, _("Cannot record "
 					     "working tree state"));
-		ret = -1;
+		ret = CREATE_ERROR;
 		goto done;
 	}
 
@@ -1660,12 +1667,13 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
 
 	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, NULL, 0, &info,
 			      NULL, 0);
-	if (!ret)
+	if (ret == CREATE_SUCCESS)
 		printf_ln("%s", oid_to_hex(&info.w_commit));
 
 	free_stash_info(&info);
 	strbuf_release(&stash_msg_buf);
-	return ret;
+	/* Do not return an error if there are no changes */
+	return ret == CREATE_ERROR ? -1 : 0;
 }
 
 static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet,
@@ -1674,6 +1682,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 			 int include_untracked, int only_staged)
 {
 	int ret = 0;
+	enum create_result create_res;
 	struct stash_info info = STASH_INFO_INIT;
 	struct strbuf patch = STRBUF_INIT;
 	struct strbuf stash_msg_buf = STRBUF_INIT;
@@ -1741,8 +1750,10 @@ 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)) {
+	create_res =  do_create_stash(ps, &stash_msg_buf, include_untracked,
+				      patch_mode, interactive_opts, only_staged,
+				      &info, &patch, quiet);
+	if (create_res != CREATE_SUCCESS) {
 		ret = -1;
 		goto done;
 	}
-- 
2.56.0.134.g299a3c16181

[PATCH v2 2/7] 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 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(). In principle we
could remove the call to check_changes() from do_create_stash()
instead, but it is more convenient to keep it there, rather than
require each caller to call it instead.

The remaining callers of check_changes_tracked_files() sensibly read
and refresh the index before calling it, so it no-longer needs to
call to repo_read_index().

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

diff --git a/builtin/stash.c b/builtin/stash.c
index 3e50ccfb40..f461f002e5 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1248,9 +1248,6 @@ static int check_changes_tracked_files(const struct pathspec *ps)
 	if (repo_get_oid(the_repository, "HEAD", &dummy))
 		return -1;
 
-	if (repo_read_index(the_repository) < 0)
-		return -1;
-
 	repo_init_revisions(the_repository, &rev, NULL);
 	copy_pathspec(&rev.prune_data, ps);
 
@@ -1662,8 +1659,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);
-- 
2.56.0.134.g299a3c16181

[PATCH v2 3/7] 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(). While duplicating the checks for staged and
unstaged charges is fairly cheap because they stop after finding the
first changed path, if we're stashing untracked files then we visit
them all twice. 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 CREATE_NO_CHANGES
rather than CREATE_EMPTY. This enables us to distinguish between there
being no changes and there being nothing stashed, so that "git stash
push --patch" when nothing is selected, and "git stash push --staged"
when the index matches HEAD, still exit 1 rather than 0.

As we're not calling check_changes() in do_push_stash() anymore, we do
not need to refresh the index there either because do_create_stash()
refreshes it again before calling check_changes(). Avoiding the extra
index refresh saves us two lstat() calls per changed path.

There is still one small change in behavior as, if there is nothing
to stash, we'll now try 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, 9 insertions(+), 14 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index f461f002e5..aec79ed473 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1338,6 +1338,8 @@ enum create_result {
 	CREATE_SUCCESS,
 	/* No changes were stashed */
 	CREATE_EMPTY,
+	/* There were no changes to stash */
+	CREATE_NO_CHANGES,
 };
 
 static enum create_result stash_staged(struct stash_info *info,
@@ -1542,7 +1544,7 @@ static enum create_result do_create_stash(const struct pathspec *ps,
 	}
 
 	if (!check_changes(ps, include_untracked, &untracked_files)) {
-		ret = CREATE_EMPTY;
+		ret = CREATE_NO_CHANGES;
 		goto done;
 	}
 
@@ -1724,18 +1726,6 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		free(ps_matched);
 	}
 
-	if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
-					 NULL, NULL, NULL)) {
-		ret = error(_("could not write index"));
-		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)
@@ -1748,7 +1738,12 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 	create_res =  do_create_stash(ps, &stash_msg_buf, include_untracked,
 				      patch_mode, interactive_opts, only_staged,
 				      &info, &patch, quiet);
-	if (create_res != CREATE_SUCCESS) {
+	if (create_res == CREATE_NO_CHANGES) {
+		if (!quiet)
+			printf_ln(_("No local changes to save"));
+		ret = 0;
+		goto done;
+	} else if (create_res != CREATE_SUCCESS) {
 		ret = -1;
 		goto done;
 	}
-- 
2.56.0.134.g299a3c16181

[PATCH v2 4/7] stash push: avoid duplicate calls to preload_index()

[Phillip Wood]

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

do_stash_push() needs to load the index so it can check that any
pathspec argument refers to tracked files. To do that it calls
repo_read_index_preload() which does not just load the index, it also
calls preload_index() which lstat()s all the files to see which ones
are up-to-date. do_create_stash() then calls repo_read_index_preload()
for a second time which lstat()s all the changed paths again and then
calls repo_refresh_and_write_index() which invokes preload_index() for a
third time. So we're preloading the index three times when we only need
to do it once. Avoid this by making the callers of do_create_stash()
responsible for reading the index and use repo_read_index() rather
than repo_read_index_preload(). This saves two lstat() calls per
changed path.

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

diff --git a/builtin/stash.c b/builtin/stash.c
index aec79ed473..b4fe7465e4 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1526,7 +1526,6 @@ static enum create_result do_create_stash(const struct pathspec *ps,
 
 	prepare_fallback_ident("git stash", "git@stash");
 
-	repo_read_index_preload(the_repository, NULL, 0);
 	if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
 					 NULL, NULL, NULL) < 0) {
 		ret = error(_("could not write index"));
@@ -1662,6 +1661,7 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
 
 	memset(&ps, 0, sizeof(ps));
 
+	repo_read_index(the_repository);
 	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, NULL, 0, &info,
 			      NULL, 0);
 	if (ret == CREATE_SUCCESS)
@@ -1707,7 +1707,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
 		goto done;
 	}
 
-	repo_read_index_preload(the_repository, NULL, 0);
+	repo_read_index(the_repository);
 	if (!include_untracked && ps->nr) {
 		char *ps_matched = xcalloc(ps->nr, 1);
 
-- 
2.56.0.134.g299a3c16181

Re: [PATCH v2 4/7] stash push: avoid duplicate calls to preload_index()

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> do_stash_push() needs to load the index so it can check that any

Perhaps you meant do_push_stash()?

> pathspec argument refers to tracked files. To do that it calls
> repo_read_index_preload() which does not just load the index, it also
> calls preload_index() which lstat()s all the files to see which ones
> are up-to-date. do_create_stash() then calls repo_read_index_preload()
> for a second time which lstat()s all the changed paths again and then
> calls repo_refresh_and_write_index() which invokes preload_index() for a
> third time. So we're preloading the index three times when we only need
> to do it once. Avoid this by making the callers of do_create_stash()
> responsible for reading the index and use repo_read_index() rather
> than repo_read_index_preload(). This saves two lstat() calls per
> changed path.

The other caller of do_create_stash() is create_stash().  And it
does not need to call the preload version for the same reason.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/stash.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index aec79ed473..b4fe7465e4 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1526,7 +1526,6 @@ static enum create_result do_create_stash(const struct pathspec *ps,
>  
>  	prepare_fallback_ident("git stash", "git@stash");
>  
> -	repo_read_index_preload(the_repository, NULL, 0);
>  	if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
>  					 NULL, NULL, NULL) < 0) {
>  		ret = error(_("could not write index"));
> @@ -1662,6 +1661,7 @@ static int create_stash(int argc, const char **argv, const char *prefix UNUSED,
>  
>  	memset(&ps, 0, sizeof(ps));
>  
> +	repo_read_index(the_repository);
>  	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, NULL, 0, &info,
>  			      NULL, 0);
>  	if (ret == CREATE_SUCCESS)
> @@ -1707,7 +1707,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>  		goto done;
>  	}
>  
> -	repo_read_index_preload(the_repository, NULL, 0);
> +	repo_read_index(the_repository);
>  	if (!include_untracked && ps->nr) {
>  		char *ps_matched = xcalloc(ps->nr, 1);

[PATCH v2 5/7] stash create: rework temporary index creation

[Phillip Wood]

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

Use create_index_from_tree() instead of reset_tree() when we create a
temporary index file to stash the working tree changes. This matches
what we do for "git stash push -p" and means we can avoid messing with
set_alternate_index_output(). Add a new parameter to control whether
the index is reset, as we need to merge the index rather than reset
it so that we still detect intent-to-add entries. We do not alter the
in-memory index so we can remove the call to repo_read_index_preload()
which saves one lstat() call per changed path.

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

diff --git a/builtin/stash.c b/builtin/stash.c
index b4fe7465e4..ddeb5a5d2a 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -374,7 +374,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 }
 
 static int create_index_from_tree(const struct object_id *tree_id,
-				  const char *index_path)
+				  const char *index_path, bool reset)
 {
 	int nr_trees = 1;
 	int ret = 0;
@@ -403,7 +403,7 @@ static int create_index_from_tree(const struct object_id *tree_id,
 	opts.src_index = the_repository->index;
 	opts.dst_index = &dst_istate;
 	opts.merge = 1;
-	opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
 	opts.fn = oneway_merge;
 
 	if (unpack_trees(nr_trees, t, &opts)) {
@@ -1399,7 +1399,7 @@ static enum create_result stash_patch(struct stash_info *info,
 		goto done;
 	}
 
-	if (create_index_from_tree(head_tree, stash_index_path.buf)) {
+	if (create_index_from_tree(head_tree, stash_index_path.buf, true)) {
 		ret = CREATE_ERROR;
 		goto done;
 	}
@@ -1458,22 +1458,15 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
 	repo_init_revisions(the_repository, &rev, NULL);
 	copy_pathspec(&rev.prune_data, ps);
 
-	set_alternate_index_output(stash_index_path.buf);
-	if (reset_tree(&info->i_tree, 0, 0)) {
+	if (create_index_from_tree(&info->i_tree, stash_index_path.buf, false)) {
 		ret = -1;
 		goto done;
 	}
-	set_alternate_index_output(NULL);
 
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = add_diff_to_buf;
 	rev.diffopt.format_callback_data = &diff_output;
 
-	if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0) {
-		ret = -1;
-		goto done;
-	}
-
 	add_pending_object(&rev, parse_object(the_repository, &info->b_commit),
 			   "");
 	run_diff_index(&rev, 0);
-- 
2.56.0.134.g299a3c16181

Re: [PATCH v2 5/7] stash create: rework temporary index creation

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Use create_index_from_tree() instead of reset_tree() when we create a
> temporary index file to stash the working tree changes. This matches
> what we do for "git stash push -p" and means we can avoid messing with
> set_alternate_index_output(). Add a new parameter to control whether
> the index is reset, as we need to merge the index rather than reset
> it so that we still detect intent-to-add entries. We do not alter the
> in-memory index so we can remove the call to repo_read_index_preload()
> which saves one lstat() call per changed path.

It is unclear what problem we are trying to solve.  Obviously we can
enhance create_index_from_tree() and replace reset_tree() just fine,
but the proposed log message should make it clear why we are doing so.

Please follow the usual pattern to state "observation" (e.g., "we
use reset_tree() when we do X"), "problem" (e.g., "use of
reset_tree() is undesirable because ..."), and "solution" (e.g, "we
can improve by replacing reset_tree() with create_index_from_tree()
because the latter has these desirable characteristics") in this
order, perhaps in separate paragraphs for extra readability.



> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/stash.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index b4fe7465e4..ddeb5a5d2a 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -374,7 +374,7 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>  }
>  
>  static int create_index_from_tree(const struct object_id *tree_id,
> -				  const char *index_path)
> +				  const char *index_path, bool reset)
>  {
>  	int nr_trees = 1;
>  	int ret = 0;
> @@ -403,7 +403,7 @@ static int create_index_from_tree(const struct object_id *tree_id,
>  	opts.src_index = the_repository->index;
>  	opts.dst_index = &dst_istate;
>  	opts.merge = 1;
> -	opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
> +	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
>  	opts.fn = oneway_merge;
>  
>  	if (unpack_trees(nr_trees, t, &opts)) {
> @@ -1399,7 +1399,7 @@ static enum create_result stash_patch(struct stash_info *info,
>  		goto done;
>  	}
>  
> -	if (create_index_from_tree(head_tree, stash_index_path.buf)) {
> +	if (create_index_from_tree(head_tree, stash_index_path.buf, true)) {
>  		ret = CREATE_ERROR;
>  		goto done;
>  	}
> @@ -1458,22 +1458,15 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
>  	repo_init_revisions(the_repository, &rev, NULL);
>  	copy_pathspec(&rev.prune_data, ps);
>  
> -	set_alternate_index_output(stash_index_path.buf);
> -	if (reset_tree(&info->i_tree, 0, 0)) {
> +	if (create_index_from_tree(&info->i_tree, stash_index_path.buf, false)) {
>  		ret = -1;
>  		goto done;
>  	}
> -	set_alternate_index_output(NULL);
>  
>  	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
>  	rev.diffopt.format_callback = add_diff_to_buf;
>  	rev.diffopt.format_callback_data = &diff_output;
>  
> -	if (repo_read_index_preload(the_repository, &rev.diffopt.pathspec, 0) < 0) {
> -		ret = -1;
> -		goto done;
> -	}
> -
>  	add_pending_object(&rev, parse_object(the_repository, &info->b_commit),
>  			   "");
>  	run_diff_index(&rev, 0);

[PATCH v2 6/7] stash create: remove unnecessary index load and refresh

[Phillip Wood]

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

create_index_from_tree() is called either directly or indirectly
from do_create_stash() after it has loaded and refreshed the index,
so we do not need to refresh it again here. This saves three lstat()
calls per changed path.

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

diff --git a/builtin/stash.c b/builtin/stash.c
index ddeb5a5d2a..55916259c3 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -384,9 +384,6 @@ static int create_index_from_tree(const struct object_id *tree_id,
 	struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
 	struct lock_file lock_file = LOCK_INIT;
 
-	repo_read_index_preload(the_repository, NULL, 0);
-	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
-
 	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
 
 	memset(&opts, 0, sizeof(opts));
-- 
2.56.0.134.g299a3c16181

Re: [PATCH v2 6/7] stash create: remove unnecessary index load and refresh

[Junio C Hamano]

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

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> create_index_from_tree() is called either directly or indirectly
> from do_create_stash() after it has loaded and refreshed the index,
> so we do not need to refresh it again here. This saves three lstat()
> calls per changed path.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/stash.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index ddeb5a5d2a..55916259c3 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -384,9 +384,6 @@ static int create_index_from_tree(const struct object_id *tree_id,
>  	struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
>  	struct lock_file lock_file = LOCK_INIT;
>  
> -	repo_read_index_preload(the_repository, NULL, 0);
> -	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
> -

Oh, it is emrarrasingly stupid follow read_index_preload() with
refresh_index(), since the reason former is not read_index() is
because we want it refreshed.  The original is doubly bad.

>  	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
>  
>  	memset(&opts, 0, sizeof(opts));

[PATCH v2 7/7] stash: simplify reset_tree() function signature

[Phillip Wood]

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

There is only one caller and it does not want to update the worktree or
reset the index so remove those unused parameters.

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

diff --git a/builtin/stash.c b/builtin/stash.c
index 55916259c3..bc1252790b 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -332,7 +332,7 @@ static int clear_stash(int argc, const char **argv, const char *prefix,
 	return do_clear_stash();
 }
 
-static int reset_tree(struct object_id *i_tree, int update, int reset)
+static int reset_tree(struct object_id *i_tree)
 {
 	int nr_trees = 1;
 	struct unpack_trees_options opts;
@@ -358,10 +358,6 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	opts.src_index = the_repository->index;
 	opts.dst_index = the_repository->index;
 	opts.merge = 1;
-	opts.reset = reset ? UNPACK_RESET_PROTECT_UNTRACKED : 0;
-	opts.update = update;
-	if (update)
-		opts.preserve_ignored = 0; /* FIXME: !overwrite_ignore */
 	opts.fn = oneway_merge;
 
 	if (unpack_trees(nr_trees, t, &opts))
@@ -738,7 +734,7 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 	}
 
 	if (has_index) {
-		if (reset_tree(&index_tree, 0, 0))
+		if (reset_tree(&index_tree))
 			ret = -1;
 	} else {
 		unstage_changes_unless_new(&c_tree);
-- 
2.56.0.134.g299a3c16181