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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

OK.

> @@ -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.

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

> +	# 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.