Re: [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2022, 19:27 UTC
Message-ID: <xmqq7d8ws5bo.fsf@gitster.g>
In-Reply-To: <7f0226bc3e646167808fbd2413dc54e87417230c.1647274230.git.gitgitgadget@gmail.com>

"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 29 quoted lines

> From: Victoria Dye <vdye@github.com>
>
> Add a new --[no-]refresh option that is intended to explicitly determine
> whether a mixed reset should end in an index refresh.
>
> Starting at 9ac8125d1a (reset: don't compute unstaged changes after reset
> when --quiet, 2018-10-23), using the '--quiet' option results in skipping
> the call to 'refresh_index(...)' at the end of a mixed reset with the goal
> of improving performance. However, by coupling behavior that modifies the
> index with the option that silences logs, there is no way for users to have
> one without the other (i.e., silenced logs with a refreshed index) without
> incurring the overhead of a separate call to 'git update-index --refresh'.
> Furthermore, there is minimal user-facing documentation indicating that
> --quiet skips the index refresh, potentially leading to unexpected issues
> executing commands after 'git reset --quiet' that do not themselves refresh
> the index (e.g., internals of 'git stash', 'git read-tree').
>
> To mitigate these issues, '--[no-]refresh' and 'reset.refresh' are
> introduced to provide a dedicated mechanism for refreshing the index. When
> either is set, '--quiet' and 'reset.quiet' revert to controlling only
> whether logs are silenced and do not affect index refresh.
>
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
>  Documentation/git-reset.txt |  9 +++++
>  builtin/reset.c             | 13 ++++++-
>  t/t7102-reset.sh            | 77 +++++++++++++++++++++++++++++++++----
>  3 files changed, 91 insertions(+), 8 deletions(-)

No complaints, but it is somewhat unsatisfying that we need these two steps that keep --quiet tied to the decision to or not to refresh. In the longer term, it may be cleaner to completely dissociate them, but it probably is not a huge deal.

Show 6 quoted lines

> +	/*
> +	 * If refresh is completely unspecified (either by config or by command
> +	 * line option), decide based on 'quiet'.
> +	 */
> +	if (refresh < 0)
> +		refresh = !quiet;

OK.

Show 9 quoted lines

> @@ -517,7 +528,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
>  			if (read_from_tree(&pathspec, &oid, intent_to_add))
>  				return 1;
>  			the_index.updated_skipworktree = 1;
> -			if (!quiet && get_git_work_tree()) {
> +			if (refresh && get_git_work_tree()) {
>  				uint64_t t_begin, t_delta_in_ms;
>  
>  				t_begin = getnanotime();

Quite sensible.

Show 11 quoted lines

> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index d05426062ec..005940778b7 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -462,14 +462,77 @@ test_expect_success 'resetting an unmodified path is a no-op' '
>  	git diff-index --cached --exit-code HEAD
>  '
>  
> +test_index_refreshed () {
> +
> +	# To test whether the index is refresh, create a scenario where a

Doesn't the verb "refresh" refer to the act of making it "fresh" (again)? i.e. update the cached stat info to up-to-date?

"To test whether the index has been refreshed" or "To test whether the cached stat info is up-to-date", perhaps?

> +	# command will fail if the index is *not* refreshed:
> +	#   1. update the worktree to match HEAD & remove file2 in the index

In other words, file2 tentatively becomes untracked.

> +	#   2. reset --mixed to unstage the change from step 1

But then, file2 is "added" to the index again, but added from the HEAD. If this did not refresh, then we do not know if the contents of the file in the working tree is the same, and "diff-files" may say "file2 may be modified". If "reset" refreshes, this will take us back to the same state as "reset --hard HEAD", and "diff-files" will not report that "file2" is different.

> +	#   3. read-tree HEAD~1 (which differs from HEAD in file2)

With "-m" option, I presume?  Do we want "-u" here, too?

Show 5 quoted lines

> +	# If the index is refreshed in step 2, then file2 in the index will be
> +	# up-to-date with HEAD and read-tree will succeed (thus failing the
> +	# test). If the index is *not* refreshed, however, the staged deletion
> +	# of file2 from step 1 will conflict with the changes from the tree read
> +	# in step 3, resulting in a failure.

This feels a bit brittle. The implementation of "read-tree -m" may choose to refresh beforehand to avoid such a failure.

In any case, the name of the helper alone wasn't of any help to realize that this is about checking if "reset" refreshes the index or not. Perhaps call it more like

	reset_refreshes_index

or something?

In any case, instead of the big comment block, comments interspersed in the steps may be easier to follow.

> +	# Step 0: start with a clean index
> +	git reset --hard HEAD &&
> +
> +	# Step 1

	# remove file2 from the index

> +	git rm --cached file2 &&
> +
> +	# Step 2

	# resurrect file2 to the index from HEAD; if the cached stat
	# info gets refreshed, this brings us back to the state
        # after Step 0.  If not, "diff-files" would report file2 is
	# different.

> +	git $1 reset $2 --mixed HEAD &&
> +
> +	# Step 3
> +	git read-tree -m HEAD~1

And use "diff-files file2" here? Then you do not even have to rely on HEAD and HEAD~1 being different at file2.