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@xxxxxxxxx> wrote:
>
> From: Kevin Willford <kewillf@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Signed-off-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> Signed-off-by: Victoria Dye <vdye@xxxxxxxxxx>
> ---
> 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
>
