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

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

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