git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, derrickstolee@github.com,
	Victoria Dye <vdye@github.com>
Subject: Re: [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed
Date: Mon, 14 Mar 2022 19:27:07 +0000	[thread overview]
Message-ID: <xmqq7d8ws5bo.fsf@gitster.g> (raw)
In-Reply-To: <7f0226bc3e646167808fbd2413dc54e87417230c.1647274230.git.gitgitgadget@gmail.com> (Victoria Dye via GitGitGadget's message of "Mon, 14 Mar 2022 16:10:27 +0000")

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


  reply	other threads:[~2022-03-14 19:27 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-12  0:08 [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash' Victoria Dye via GitGitGadget
2022-03-12  0:08 ` [PATCH 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-12  0:08 ` [PATCH 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-14 15:05   ` Derrick Stolee
2022-03-14 15:13     ` Derrick Stolee
2022-03-14 15:55     ` Victoria Dye
2022-03-12  0:08 ` [PATCH 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-12  0:08 ` [PATCH 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-12  0:08 ` [PATCH 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-14 15:10   ` Derrick Stolee
2022-03-14 15:56     ` Victoria Dye
2022-03-12 17:12 ` [PATCH 0/5] Separate '--skip-refresh' from '--quiet' in 'reset', use '--quiet' internally in 'stash' Victoria Dye
2022-03-14  6:22 ` Junio C Hamano
2022-03-14 15:13 ` Derrick Stolee
2022-03-14 16:10 ` [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' " Victoria Dye via GitGitGadget
2022-03-14 16:10   ` [PATCH v2 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-14 16:10   ` [PATCH v2 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-14 19:27     ` Junio C Hamano [this message]
2022-03-14 23:48       ` Victoria Dye
2022-03-14 16:10   ` [PATCH v2 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-14 16:10   ` [PATCH v2 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-14 19:38     ` Junio C Hamano
2022-03-14 16:10   ` [PATCH v2 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-14 19:42     ` Junio C Hamano
2022-03-14 23:54       ` Victoria Dye
2022-03-14 16:30   ` [PATCH v2 0/5] Allow 'reset --quiet' to refresh the index, use 'reset --quiet' in 'stash' Derrick Stolee
2022-03-14 23:17   ` Junio C Hamano
2022-03-15  1:49   ` [PATCH v3 " Victoria Dye via GitGitGadget
2022-03-15  1:49     ` [PATCH v3 1/5] reset: revise index refresh advice Victoria Dye via GitGitGadget
2022-03-15  1:49     ` [PATCH v3 2/5] reset: introduce --[no-]refresh option to --mixed Victoria Dye via GitGitGadget
2022-03-18 11:08       ` Phillip Wood
2022-03-18 17:17         ` Junio C Hamano
2022-03-18 19:19           ` Victoria Dye
2022-03-15  1:49     ` [PATCH v3 3/5] reset: replace '--quiet' with '--no-refresh' in performance advice Victoria Dye via GitGitGadget
2022-03-15  1:49     ` [PATCH v3 4/5] reset: suppress '--no-refresh' advice if logging is silenced Victoria Dye via GitGitGadget
2022-03-15  1:49     ` [PATCH v3 5/5] stash: make internal resets quiet and refresh index Victoria Dye via GitGitGadget
2022-03-15 10:23       ` Junio C Hamano
2022-03-16 20:07         ` Victoria Dye
2022-03-16 20:55           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq7d8ws5bo.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).