On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
>
> From: Victoria Dye <vdye@xxxxxxxxxx>
>
> Introduce tests for a variety of `git update-index` use cases, including
> performance scenarios.
Makes sense.
> Tests for `update-index add/remove` are specifically
> focused on how `git stash` uses `git update-index` as a subcommand to
> prepare for sparse index integration with `stash` in a future series.
This is possibly a tangent, but I'd rather that if we were trying to
fix `git stash`, that we instead would do so by making it stop forking
subprocesses and having it call internal API instead. See for
example, a4031f6dc0 ("Merge branch 'en/stash-apply-sparse-checkout'
into maint", 2021-02-05) which did this. The fact that it forks so
many subprocesses is a bug, and comes from the fact that stash is a
partial conversion from shell to C. I think the subprocess forking is
part of the problem that causes issues for sparse-checkouts, as I
spelled out in patches 2 & 3 of the series I mentioned above.
However, that doesn't affect this series.
> Co-authored-by: Derrick Stolee <dstolee@xxxxxxxxxxxxx>
> Signed-off-by: Victoria Dye <vdye@xxxxxxxxxx>
> ---
> t/perf/p2000-sparse-operations.sh | 1 +
> t/t1092-sparse-checkout-compatibility.sh | 125 +++++++++++++++++++++++
> 2 files changed, 126 insertions(+)
>
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 54f8602f3c1..7dbed330160 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -118,5 +118,6 @@ test_perf_on_all git diff --cached
> test_perf_on_all git blame $SPARSE_CONE/a
> test_perf_on_all git blame $SPARSE_CONE/f3/a
> test_perf_on_all git checkout-index -f --all
> +test_perf_on_all git update-index --add --remove
>
> test_done
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 6ecf1f2bf8e..6804ab23a27 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -630,6 +630,131 @@ test_expect_success 'reset with wildcard pathspec' '
> test_all_match git ls-files -s -- folder1
> '
>
> +test_expect_success 'update-index modify outside sparse definition' '
> + init_repos &&
> +
> + write_script edit-contents <<-\EOF &&
> + echo text >>$1
> + EOF
> +
> + # Create & modify folder1/a
> + run_on_sparse mkdir -p folder1 &&
> + run_on_sparse cp ../initial-repo/folder1/a folder1/a &&
> + run_on_all ../edit-contents folder1/a &&
As I've mentioned to Stolee, I'd rather these were explicitly marked
as intentionally setting up an erroneous condition.
However, that might not matter if my other series gets accepted (the
one I promised to send out yesterday, but then spent all day
responding to emails instead). Hopefully I'll send it soon.
> +
> + # If file has skip-worktree enabled, update-index does not modify the
> + # index entry
> + test_sparse_match git update-index folder1/a &&
> + test_sparse_match git status --porcelain=v2 &&
> + test_must_be_empty sparse-checkout-out &&
> +
> + # When skip-worktree is disabled (even on files outside sparse cone), file
> + # is updated in the index
> + test_sparse_match git update-index --no-skip-worktree folder1/a &&
> + test_all_match git status --porcelain=v2 &&
> + test_all_match git update-index folder1/a &&
> + test_all_match git status --porcelain=v2
These make sense.
> +'
> +
> +test_expect_success 'update-index --add outside sparse definition' '
> + init_repos &&
> +
> + write_script edit-contents <<-\EOF &&
> + echo text >>$1
> + EOF
> +
> + # Create folder1, add new file
> + run_on_sparse mkdir -p folder1 &&
> + run_on_all ../edit-contents folder1/b &&
> +
> + # Similar to `git add`, the untracked out-of-cone file is added to the index
> + # identically across sparse and non-sparse checkouts
> + test_all_match git update-index --add folder1/b &&
> + test_all_match git status --porcelain=v2
The comment is not correct:
$ git add out-of-cone/file
The following paths and/or pathspecs matched paths that exist
outside of your sparse-checkout definition, so will not be
updated in the index:
out-of-cone/file
hint: If you intend to update such entries, try one of the following:
hint: * Use the --sparse option.
hint: * Disable or modify the sparsity rules.
hint: Disable this message with "git config advice.updateSparsePath false"
I might buy that `git update-index` is lower level and should be
considered the same as `git add --sparse`, but the comment should
mention that and try to sell why update-index should be equivalent to
that instead of to `git add`.
> +'
> +
> +test_expect_success 'update-index --remove outside sparse definition' '
> + init_repos &&
> +
> + # When `--ignore-skip-worktree-entries` is specified, out-of-cone files are
> + # not removed from the index if they do not exist on disk
> + test_sparse_match git update-index --remove --ignore-skip-worktree-entries folder1/a &&
> + test_all_match git status --porcelain=v2 &&
The file is present despite being marked to be missing, we're ignoring
the intention of the marking, and we ask for it to be removed, so we
don't remove it?
The levels of negation are _very_ confusing. It took me a while to
unravel this. I think the logic is something like this
* folder1/a is marked as SKIP_WORKTREE, meaning it's not supposed to
be in the worktree.
* and it's not.
* We are stating that we're ignoring SKIP_WORKTREE, though, so this
looks like a regular file that has been deleted.
So, what would `git update-index --remove $FILE` do for a normal $FILE
deleted from the working copy? According to the docs:
--remove
If a specified file is in the index but is missing then it’s
removed. Default behavior is to ignore removed file.
So, the docs say it would remove it. But you don't.
After digging around and looking at the testcase below, if I had to
guess what happened, I would say that you figured out what the
SKIP_WORKTREE behavior was, and assumed that was correct, and added a
flag that allowed you to request the opposite behavior.
Unfortunately, I think the pre-existing behavior is buggy.
Of course, there's lots of negation here. Did I get something
backwards by chance?
> +
> + # When the flag is _not_ specified ...
In my head I'm translating this as:
SKIP_WORKTREE = !worktree
--ignore-skip-worktree-entries = !!worktree
"flag is _not_ specified" = !!!worktree ?
I'm not sure I would do anything to change it, but just pointing out
it can be a little hard for others to come up to speed.
> ... , out-of-cone, not-on-disk files are
> + # removed from the index
> + rm full-checkout/folder1/a &&
> + test_all_match git update-index --remove folder1/a &&
> + test_all_match git status --porcelain=v2 &&
Documentation/git-update-index.txt defines SKIP_WORKTREE as follows:
"Skip-worktree bit can be defined in one (long) sentence: When reading
an entry, if it is marked as skip-worktree, then Git pretends its
working directory version is up to date and read the index version
instead."
If Git is pretending the file is up-to-date, and `git update-index
--remove $UP_TO_DATE_FILE` is typically a no-op because the --remove
flag doesn't do anything when a file is present in the working copy,
then why is this the expected behavior?
I know it's the traditional behavior of update-index, but
SKIP_WORKTREE support in Git has traditionally been filled with holes.
So, was this behavior by design (despite contradicting the
documentation), or by accident?
(To be fair, I think the definition given in the manual for
SKIP_WORKTREE is horrible for other reasons, so I don't like leaning
on it. But I used different logic above in the
--ignore-skip-worktree-entries case to arrive at the same conclusion
that the --remove behavior of update-index seems to be backwards.
Unless I missed a negation in both cases somewhere? There are so many
floating around...)
> + # NOTE: --force-remove supercedes --ignore-skip-worktree-entries, removing
> + # a skip-worktree file from the index (and disk) when both are specified
> + test_all_match git update-index --force-remove --ignore-skip-worktree-entries folder1/a &&
> + test_all_match git status --porcelain=v2
Makes sense.
> +'
> +
> +test_expect_success 'update-index with directories' '
> + init_repos &&
> +
> + # update-index will exit silently when provided with a directory name
> + # containing a trailing slash
> + test_all_match git update-index deep/ folder1/ &&
> + grep "Ignoring path deep/" sparse-checkout-err &&
> + grep "Ignoring path folder1/" sparse-checkout-err &&
Is this desired behavior or just current behavior?
> +
> + # When update-index is given a directory name WITHOUT a trailing slash, it will
> + # behave in different ways depending on the status of the directory on disk:
> + # * if it exists, the command exits with an error ("add individual files instead")
> + # * if it does NOT exist (e.g., in a sparse-checkout), it is assumed to be a
> + # file and either triggers an error ("does not exist and --remove not passed")
> + # or is ignored completely (when using --remove)
> + test_all_match test_must_fail git update-index deep &&
> + run_on_all test_must_fail git update-indexe folder1 &&
This one will fail for the wrong reason, though -- `update-indexe` is
not a valid subcommand. (extra 'e' at the end)
> + test_must_fail git -C full-checkout update-index --remove folder1 &&
> + test_sparse_match git update-index --remove folder1 &&
> + test_all_match git status --porcelain=v2
Otherwise these seem reasonable.
> +'
> +
> +test_expect_success 'update-index --again file outside sparse definition' '
> + init_repos &&
> +
> + write_script edit-contents <<-\EOF &&
> + echo text >>$1
> + EOF
Copy and paste and forget to remove? edit-contents doesn't seem to be
used in this test.
> +
> + test_all_match git checkout -b test-reupdate &&
> +
> + # Update HEAD without modifying the index to introduce a difference in
> + # folder1/a
> + test_sparse_match git reset --soft update-folder1 &&
> +
> + # Because folder1/a differs in the index vs HEAD,
> + # `git update-index --remove --again` will effectively perform
> + # `git update-index --remove folder1/a` and remove the folder1/a
> + test_sparse_match git update-index --remove --again &&
> + test_sparse_match git status --porcelain=v2
This might need a --ignore-skip-worktree-entries, as per the
discussion above. Otherwise, this test makes sense.
> +'
> +
> +test_expect_success 'update-index --cacheinfo' '
> + init_repos &&
> +
> + deep_a_oid=$(git -C full-checkout rev-parse update-deep:deep/a) &&
> + folder2_oid=$(git -C full-checkout rev-parse update-folder2:folder2) &&
> + folder1_a_oid=$(git -C full-checkout rev-parse update-folder1:folder1/a) &&
> +
> + test_all_match git update-index --cacheinfo 100644 $deep_a_oid deep/a &&
> + test_all_match git status --porcelain=v2 &&
> +
> + # Cannot add sparse directory, even in sparse index case
> + test_all_match test_must_fail git update-index --add --cacheinfo 040000 $folder2_oid folder2/ &&
> +
> + # Sparse match only - because folder1/a is outside the sparse checkout
> + # definition (and thus not on-disk), it will appear "deleted" in
> + # unstaged changes.
> + test_all_match git update-index --add --cacheinfo 100644 $folder1_a_oid folder1/a &&
> + test_sparse_match git status --porcelain=v2
Makes sense, because the update-index command removes the existing
cache entry and adds a new one without the SKIP_WORKTREE bit. But it
might be worth mentioning that in the commit message. Also, you could
follow this up with `git update-index --skip-worktree folder1/a`, and
then do a test_all_match git status --porcelain=v2, to show that when
the SKIP_WORKTREE bit is restored back to the file, then it again does
as expected despite not being on-disk.
> +'
> +
> test_expect_success 'merge, cherry-pick, and rebase' '
> init_repos &&
>
> --
> gitgitgadget
