From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 06 Oct 2026 14:27:27 GMT
Subject: Re: [PATCH 2/2] stash push: remove duplicate changes detection
Message-ID: <xmqqpkxmgb00.fsf@gitster.g>
In-Reply-To: <95b7d582a2f86a3db4a9e182e482e9eb904ddeee.1791218125.git.phillip.wood@dunelm.org.uk>

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