From: Elijah Newren <newren@gmail.com>
To: Kevin Willford via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Derrick Stolee <stolee@gmail.com>,
Junio C Hamano <gitster@pobox.com>, Taylor Blau <me@ttaylorr.com>,
Bagas Sanjaya <bagasdotme@gmail.com>,
Victoria Dye <vdye@github.com>,
Kevin Willford <kewillf@microsoft.com>
Subject: Re: [PATCH v2 1/7] reset: behave correctly with sparse-checkout
Date: Tue, 5 Oct 2021 18:46:43 -0700 [thread overview]
Message-ID: <CABPp-BFFG=puwpB4Gz8nWU=CNMUrLbH7XPTkpSGqWedTK2NWLg@mail.gmail.com> (raw)
In-Reply-To: <22c69bc60308fef13acd7c3aab4e11e175c89440.1633440057.git.gitgitgadget@gmail.com>
Hi!
It appears Junio has already commented on this patch and in more
detail, but since I already typed up some comments I'll send them
along in case they are useful.
On Tue, Oct 5, 2021 at 6:20 AM Kevin Willford via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Kevin Willford <kewillf@microsoft.com>
>
> When using the sparse checkout feature, 'git reset' will add entries to
> the index that will have the skip-worktree bit off but will leave the
> working directory empty.
Yes, that seems like a problem.
> File data is lost because the index version of
> the files has been changed but there is nothing that is in the working
> directory. This will cause the next 'git status' call to show either
> deleted for files modified or deleting or nothing for files added. The
> added files should be shown as untracked and modified files should be
> shown as modified.
Why is the solution to add the files to the working tree rather than
to make sure the files have the skip-worktree bit set? That's not at
all what I would have expected.
> To fix this when the reset is running if there is not a file in the
> working directory and if it will be missing with the new index entry or
> was not missing in the previous version, we create the previous index
> version of the file in the working directory so that status will report
> correctly and the files will be availble for the user to deal with.
s/availble/available/
>
> This fixes a documented failure from t1092 that was created in 19a0acc
> (t1092: test interesting sparse-checkout scenarios, 2021-01-23).
>
> Signed-off-by: Kevin Willford <kewillf@microsoft.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> builtin/reset.c | 24 ++++++++--
> t/t1092-sparse-checkout-compatibility.sh | 4 +-
> t/t7114-reset-sparse-checkout.sh | 61 ++++++++++++++++++++++++
> 3 files changed, 83 insertions(+), 6 deletions(-)
> create mode 100755 t/t7114-reset-sparse-checkout.sh
>
> diff --git a/builtin/reset.c b/builtin/reset.c
> index 51c9e2f43ff..3b75d3b2f20 100644
> --- a/builtin/reset.c
> +++ b/builtin/reset.c
> @@ -25,6 +25,8 @@
> #include "cache-tree.h"
> #include "submodule.h"
> #include "submodule-config.h"
> +#include "dir.h"
> +#include "entry.h"
>
> #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000)
>
> @@ -130,11 +132,27 @@ static void update_index_from_diff(struct diff_queue_struct *q,
> int intent_to_add = *(int *)data;
>
> for (i = 0; i < q->nr; i++) {
> + int pos;
> struct diff_filespec *one = q->queue[i]->one;
> - int is_missing = !(one->mode && !is_null_oid(&one->oid));
> + struct diff_filespec *two = q->queue[i]->two;
> + int is_in_reset_tree = one->mode && !is_null_oid(&one->oid);
Isn't !is_null_oid(&one->oid) redundant to checking one->mode? When
does the diff machinery ever give you a non-zero mode with a null oid?
Also, is_in_reset_tree == !is_missing; I'll note that below.
> struct cache_entry *ce;
>
> + /*
> + * If the file being reset has `skip-worktree` enabled, we need
> + * to check it out to prevent the file from being hard reset.
I don't understand this comment. If the file wasn't originally in the
index (is_missing), and is being added to it, and is correctly marked
as skip_worktree, and the file isn't in the working tree, then it
sounds like everything is already in a good state. Files outside the
sparse checkout are meant to have the skip_worktree bit set and be
missing from the working tree.
Also, I don't know what you mean by 'hard reset' here.
> + */
> + pos = cache_name_pos(two->path, strlen(two->path));
> + if (pos >= 0 && ce_skip_worktree(active_cache[pos])) {
> + struct checkout state = CHECKOUT_INIT;
> + state.force = 1;
> + state.refresh_cache = 1;
> + state.istate = &the_index;
> +
> + checkout_entry(active_cache[pos], &state, NULL, NULL);
Does this introduce an error in the opposite direction from the one
stated in the commit message? Namely we have two things that should
be in sync: the skip_worktree flag stating whether the file should be
present in the working directory (skip_worktree), and the question of
whether the file is actually in the working directory. In the commit
message, you pointed out a case where the y were out of sync one way:
the skip_worktree flag was not set but the file was missing. Here you
say the skip_worktree flag is set, but you add it to the working tree
anyway.
Or am I misunderstanding the code?
> + }
> +
[I did some slight editing to the diff to make the next two parts
appear next to each other]
> - if (is_missing && !intent_to_add) {
> + if (!is_in_reset_tree && !intent_to_add) {
I thought this was some subtle bugfix or something, and spent a while
trying to figure it out, before realizing that is_in_reset_tree was
simply defined as !is_missing (for some reason I was assuming it was
dealing with two->mode while is_missing was looking at one->mode). So
this is a simple variable renaming, which I think is probably good,
but I'd prefer if this was separated into a different patch to make it
easier to review.
> remove_file_from_cache(one->path);
> continue;
> }
> @@ -144,7 +162,7 @@ static void update_index_from_diff(struct diff_queue_struct *q,
> if (!ce)
> die(_("make_cache_entry failed for path '%s'"),
> one->path);
> - if (is_missing) {
> + if (!is_in_reset_tree) {
same note as above; the variable rename is good, but should be a separate patch.
> ce->ce_flags |= CE_INTENT_TO_ADD;
> set_object_name_for_intent_to_add_entry(ce);
> }
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 886e78715fe..c5977152661 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -459,9 +459,7 @@ test_expect_failure 'blame with pathspec outside sparse definition' '
> test_all_match git blame deep/deeper2/deepest/a
> '
>
> -# NEEDSWORK: a sparse-checkout behaves differently from a full checkout
> -# in this scenario, but it shouldn't.
> -test_expect_failure 'checkout and reset (mixed)' '
> +test_expect_success 'checkout and reset (mixed)' '
> init_repos &&
>
> test_all_match git checkout -b reset-test update-deep &&
> diff --git a/t/t7114-reset-sparse-checkout.sh b/t/t7114-reset-sparse-checkout.sh
> new file mode 100755
> index 00000000000..a8029707fb1
> --- /dev/null
> +++ b/t/t7114-reset-sparse-checkout.sh
> @@ -0,0 +1,61 @@
> +#!/bin/sh
> +
> +test_description='reset when using a sparse-checkout'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + test_tick &&
> + echo "checkout file" >c &&
> + echo "modify file" >m &&
> + echo "delete file" >d &&
> + git add . &&
> + git commit -m "initial commit" &&
> + echo "added file" >a &&
> + echo "modification of a file" >m &&
> + git rm d &&
> + git add . &&
> + git commit -m "second commit" &&
> + git checkout -b endCommit
> +'
> +
> +test_expect_success 'reset when there is a sparse-checkout' '
> + echo "/c" >.git/info/sparse-checkout &&
> + test_config core.sparsecheckout true &&
> + git checkout -B resetBranch &&
> + test_path_is_missing m &&
> + test_path_is_missing a &&
> + test_path_is_missing d &&
> + git reset HEAD~1 &&
> + echo "checkout file" >expect &&
> + test_cmp expect c &&
> + echo "added file" >expect &&
> + test_cmp expect a &&
> + echo "modification of a file" >expect &&
> + test_cmp expect m &&
> + test_path_is_missing d
> +'
> +
> +test_expect_success 'reset after deleting file without skip-worktree bit' '
> + git checkout -f endCommit &&
> + git clean -xdf &&
> + cat >.git/info/sparse-checkout <<-\EOF &&
> + /c
> + /m
> + EOF
> + test_config core.sparsecheckout true &&
> + git checkout -B resetAfterDelete &&
> + test_path_is_file m &&
> + test_path_is_missing a &&
> + test_path_is_missing d &&
> + rm -f m &&
> + git reset HEAD~1 &&
> + echo "checkout file" >expect &&
> + test_cmp expect c &&
> + echo "added file" >expect &&
> + test_cmp expect a &&
> + test_path_is_missing m &&
> + test_path_is_missing d
> +'
> +
> +test_done
> --
> gitgitgadget
>
next prev parent reply other threads:[~2021-10-06 1:46 UTC|newest]
Thread overview: 114+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-30 14:50 [PATCH 0/7] Sparse Index: integrate with reset Victoria Dye via GitGitGadget
2021-09-30 14:50 ` [PATCH 1/7] reset: behave correctly with sparse-checkout Kevin Willford via GitGitGadget
2021-09-30 18:34 ` Junio C Hamano
2021-10-01 14:55 ` Victoria Dye
2021-09-30 14:50 ` [PATCH 2/7] sparse-index: update command for expand/collapse test Victoria Dye via GitGitGadget
2021-09-30 19:17 ` Taylor Blau
2021-09-30 20:11 ` Victoria Dye
2021-09-30 21:32 ` Junio C Hamano
2021-09-30 22:59 ` Victoria Dye
2021-10-01 0:04 ` Junio C Hamano
2021-10-04 13:47 ` Victoria Dye
2021-10-01 9:14 ` Bagas Sanjaya
2021-09-30 14:50 ` [PATCH 3/7] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-09-30 14:50 ` [PATCH 4/7] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-09-30 14:50 ` [PATCH 5/7] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-09-30 14:51 ` [PATCH 6/7] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-10-01 15:03 ` Victoria Dye
2021-09-30 14:51 ` [PATCH 7/7] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-10-05 13:20 ` [PATCH v2 0/7] Sparse Index: integrate with reset Victoria Dye via GitGitGadget
2021-10-05 13:20 ` [PATCH v2 1/7] reset: behave correctly with sparse-checkout Kevin Willford via GitGitGadget
2021-10-05 19:30 ` Junio C Hamano
2021-10-05 21:59 ` Victoria Dye
2021-10-06 12:44 ` Junio C Hamano
2021-10-06 1:46 ` Elijah Newren [this message]
2021-10-06 20:09 ` Victoria Dye
2021-10-06 10:31 ` Bagas Sanjaya
2021-10-05 13:20 ` [PATCH v2 2/7] update-index: add --force-full-index option for expand/collapse test Victoria Dye via GitGitGadget
2021-10-06 2:00 ` Elijah Newren
2021-10-06 20:40 ` Victoria Dye
2021-10-08 3:42 ` Elijah Newren
2021-10-08 17:11 ` Junio C Hamano
2021-10-06 10:33 ` Bagas Sanjaya
2021-10-05 13:20 ` [PATCH v2 3/7] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-10-06 2:04 ` Elijah Newren
2021-10-05 13:20 ` [PATCH v2 4/7] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-10-06 2:15 ` Elijah Newren
2021-10-06 17:48 ` Junio C Hamano
2021-10-05 13:20 ` [PATCH v2 5/7] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-10-06 3:43 ` Elijah Newren
2021-10-06 20:56 ` Victoria Dye
2021-10-06 10:34 ` Bagas Sanjaya
2021-10-05 13:20 ` [PATCH v2 6/7] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-10-06 4:43 ` Elijah Newren
2021-10-07 14:34 ` Victoria Dye
2021-10-05 13:20 ` [PATCH v2 7/7] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-10-06 10:37 ` Bagas Sanjaya
2021-10-05 15:34 ` [PATCH v2 0/7] Sparse Index: integrate with reset Ævar Arnfjörð Bjarmason
2021-10-05 20:44 ` Victoria Dye
2021-10-06 5:46 ` Elijah Newren
2021-10-07 21:15 ` [PATCH v3 0/8] " Victoria Dye via GitGitGadget
2021-10-07 21:15 ` [PATCH v3 1/8] reset: rename is_missing to !is_in_reset_tree Victoria Dye via GitGitGadget
2021-10-07 21:15 ` [PATCH v3 2/8] reset: preserve skip-worktree bit in mixed reset Kevin Willford via GitGitGadget
2021-10-08 9:04 ` Junio C Hamano
2021-10-07 21:15 ` [PATCH v3 3/8] update-index: add --force-full-index option for expand/collapse test Victoria Dye via GitGitGadget
2021-10-08 2:50 ` Bagas Sanjaya
2021-10-08 5:24 ` Junio C Hamano
2021-10-08 15:47 ` Victoria Dye
2021-10-08 17:19 ` Junio C Hamano
2021-10-11 14:12 ` Derrick Stolee
2021-10-11 15:05 ` Victoria Dye
2021-10-11 15:24 ` Junio C Hamano
2021-10-07 21:15 ` [PATCH v3 4/8] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-10-07 21:15 ` [PATCH v3 5/8] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-10-07 21:15 ` [PATCH v3 6/8] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-10-08 11:09 ` Phillip Wood
2021-10-08 17:14 ` Victoria Dye
2021-10-08 18:31 ` Junio C Hamano
2021-10-09 11:18 ` Phillip Wood
2021-10-10 22:03 ` Junio C Hamano
2021-10-11 15:55 ` Victoria Dye
2021-10-11 16:16 ` Junio C Hamano
2021-10-12 10:16 ` Phillip Wood
2021-10-12 19:15 ` Junio C Hamano
2021-10-12 10:17 ` Phillip Wood
2021-10-07 21:15 ` [PATCH v3 7/8] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-11-20 22:02 ` Elijah Newren
2021-11-22 16:46 ` Victoria Dye
2021-10-07 21:15 ` [PATCH v3 8/8] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-10-11 20:30 ` [PATCH v4 0/8] Sparse Index: integrate with reset Victoria Dye via GitGitGadget
2021-10-11 20:30 ` [PATCH v4 1/8] reset: rename is_missing to !is_in_reset_tree Victoria Dye via GitGitGadget
2021-10-11 20:30 ` [PATCH v4 2/8] reset: preserve skip-worktree bit in mixed reset Victoria Dye via GitGitGadget
2021-10-22 4:19 ` Emily Shaffer
2021-10-25 15:59 ` Victoria Dye
2021-10-11 20:30 ` [PATCH v4 3/8] sparse-index: update command for expand/collapse test Victoria Dye via GitGitGadget
2021-10-11 20:30 ` [PATCH v4 4/8] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-10-11 20:30 ` [PATCH v4 5/8] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-10-11 20:30 ` [PATCH v4 6/8] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-10-11 20:30 ` [PATCH v4 7/8] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-10-11 20:30 ` [PATCH v4 8/8] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-10-27 14:39 ` [PATCH v5 0/8] Sparse Index: integrate with reset Victoria Dye via GitGitGadget
2021-10-27 14:39 ` [PATCH v5 1/8] reset: rename is_missing to !is_in_reset_tree Victoria Dye via GitGitGadget
2021-10-27 14:39 ` [PATCH v5 2/8] reset: preserve skip-worktree bit in mixed reset Victoria Dye via GitGitGadget
2021-10-27 14:39 ` [PATCH v5 3/8] sparse-index: update command for expand/collapse test Victoria Dye via GitGitGadget
2021-10-27 14:39 ` [PATCH v5 4/8] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-10-27 14:39 ` [PATCH v5 5/8] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-10-27 14:39 ` [PATCH v5 6/8] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-10-27 14:39 ` [PATCH v5 7/8] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-11-20 22:05 ` Elijah Newren
2021-11-22 16:54 ` Victoria Dye
2021-10-27 14:39 ` [PATCH v5 8/8] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-11-20 22:13 ` [PATCH v5 0/8] Sparse Index: integrate with reset Elijah Newren
2021-11-29 15:52 ` [PATCH v6 " Victoria Dye via GitGitGadget
2021-11-29 15:52 ` [PATCH v6 1/8] reset: rename is_missing to !is_in_reset_tree Victoria Dye via GitGitGadget
2021-11-29 15:52 ` [PATCH v6 2/8] reset: preserve skip-worktree bit in mixed reset Victoria Dye via GitGitGadget
2021-11-29 15:52 ` [PATCH v6 3/8] sparse-index: update command for expand/collapse test Victoria Dye via GitGitGadget
2021-11-29 15:52 ` [PATCH v6 4/8] reset: expand test coverage for sparse checkouts Victoria Dye via GitGitGadget
2021-11-29 15:52 ` [PATCH v6 5/8] reset: integrate with sparse index Victoria Dye via GitGitGadget
2021-11-29 15:52 ` [PATCH v6 6/8] reset: make sparse-aware (except --mixed) Victoria Dye via GitGitGadget
2021-11-29 15:52 ` [PATCH v6 7/8] reset: make --mixed sparse-aware Victoria Dye via GitGitGadget
2021-11-29 15:52 ` [PATCH v6 8/8] unpack-trees: improve performance of next_cache_entry Victoria Dye via GitGitGadget
2021-11-29 18:37 ` [PATCH v6 0/8] Sparse Index: integrate with reset Elijah Newren
2021-11-29 19:44 ` Victoria Dye
2021-11-30 3:59 ` Elijah Newren
2021-12-01 20:26 ` Ævar Arnfjörð Bjarmason
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABPp-BFFG=puwpB4Gz8nWU=CNMUrLbH7XPTkpSGqWedTK2NWLg@mail.gmail.com' \
--to=newren@gmail.com \
--cc=bagasdotme@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=kewillf@microsoft.com \
--cc=me@ttaylorr.com \
--cc=stolee@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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).