Matheus Tavares [off-list ref] writes:
We already have a couple tests for `add` with SKIP_WORKTREE entries in
t7012, but these only cover the most basic scenarios. As we will be
changing how `add` deals with sparse paths in the subsequent commits,
let's move these two tests to their own file and add more test cases
for different `add` options and situations. This also demonstrates two
options that don't currently respect SKIP_WORKTREE entries: `--chmod`
and `--renormalize`.
Nice. It makes sense to describe what we want first, like this step..
quoted hunk
Signed-off-by: Matheus Tavares <redacted>
---
t/t3705-add-sparse-checkout.sh | 92 ++++++++++++++++++++++++++++++++
t/t7012-skip-worktree-writing.sh | 19 -------
2 files changed, 92 insertions(+), 19 deletions(-)
create mode 100755 t/t3705-add-sparse-checkout.sh
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
new file mode 100755
index 0000000000..5530e796b5
--- /dev/null
+++ b/t/t3705-add-sparse-checkout.sh
@@ -0,0 +1,92 @@
+#!/bin/sh
+
+test_description='git add in sparse checked out working trees'
+
+. ./test-lib.sh
+
+SPARSE_ENTRY_BLOB=""
+
+# Optionally take a string for the entry's contents
+setup_sparse_entry()
+{
Style.
setup_sparse_entry () {
on a single line.
+ if test -f sparse_entry
+ then
+ rm sparse_entry
+ fi &&
+ git update-index --force-remove sparse_entry &&
Why not an unconditional removal on the working tree side?
rm -f sparse_entry &&
git update-index --force-remove sparse_entry &&
Are there cases where we may have sparse_entry directory here?
+
+ if test "$#" -eq 1
No need to quote $# (we know it is a number).
+ then
+ printf "$1" >sparse_entry
Make sure the test writers know that they are passing a string that
will be interpreted as a printf format. Review the comment before
the function and adjust it appropriately ("a string" is not what you
want to tell them).
+ else
+ printf "" >sparse_entry
Just
>sparse_entry
is sufficient, no?
+ fi &&
+ git add sparse_entry &&
+ git update-index --skip-worktree sparse_entry &&
+ SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry)
+}
+
+test_sparse_entry_unchanged() {
Style.
test_sparse_entry_unchanged () {
+ echo "100644 $SPARSE_ENTRY_BLOB 0 sparse_entry" >expected &&
+ git ls-files --stage sparse_entry >actual &&
+ test_cmp expected actual
OK. So the expected pattern is to first "setup", do stuff that
shouldn't affect the sparse entry in the index, and then call this
to make sure?
+}
+test_expect_success "git add does not remove SKIP_WORKTREE entries" '
We use the term SKIP_WORKTREE and SPARSE interchangeably here. I
wonder if it is easier to understand if we stick to one e.g. by
saying "... does not remove 'sparse' entries" instead? I dunno.
+ setup_sparse_entry &&
+ rm sparse_entry &&
+ git add sparse_entry &&
+ test_sparse_entry_unchanged
Wow. OK. Makes a reader wonder what should happen when the two
operations are replaced with "git rm sparse_entry"; let's read on.
+'
+
+test_expect_success "git add -A does not remove SKIP_WORKTREE entries" '
+ setup_sparse_entry &&
+ rm sparse_entry &&
+ git add -A &&
+ test_sparse_entry_unchanged
+'
OK. As there is nothing other than sparse_entry in the working tree
or in the index, the above two should be equivalent.
I wonder what should happen if the "add -A" gets replaced with "add .";
it should behave the same way, I think. Is it worth testing that
case as well, or is it redundant?
+for opt in "" -f -u --ignore-removal
+do
+ if test -n "$opt"
+ then
+ opt=" $opt"
+ fi
The above is cumulative, and as a consequence, "git add -u <path>"
is not tested, but "git add -f -u <path>" is. Intended? How was
the order of the options listed in "for opt in ..." chosen?
+ test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
+ setup_sparse_entry &&
+ echo modified >sparse_entry &&
+ git add $opt sparse_entry &&
+ test_sparse_entry_unchanged
+ '
+done
+
+test_expect_success 'git add --refresh does not update SKIP_WORKTREE entries' '
+ setup_sparse_entry &&
+ test-tool chmtime -60 sparse_entry &&
+ git add --refresh sparse_entry &&
+
+ # We must unset the SKIP_WORKTREE bit, otherwise
+ # git diff-files would skip examining the file
+ git update-index --no-skip-worktree sparse_entry &&
+
+ echo sparse_entry >expected &&
+ git diff-files --name-only sparse_entry >actual &&
+ test_cmp actual expected
Hmph, I am not sure what we are testing at this point. One way to
make the final diff-files step show sparse_entry would be for "git
add --refresh" to be a no-op, in which case, the cached stat
information in the index would be different in mtime from the path
in the working tree. But "update-index --no-skip-worktree" may be
buggy and further change or invalidate the cached stat information
to cause diff-files to report that the path may be different.
+'
+
+test_expect_failure 'git add --chmod does not update SKIP_WORKTREE entries' '
+ setup_sparse_entry &&
+ git add --chmod=+x sparse_entry &&
+ test_sparse_entry_unchanged
Hmph. Should we also check if sparse_entry in the filesystem also
is not made executable, not just the entry in the index?
+'
+
+test_expect_failure 'git add --renormalize does not update SKIP_WORKTREE entries' '
+ test_config core.autocrlf false &&
+ setup_sparse_entry "LINEONE\r\nLINETWO\r\n" &&
+ echo "sparse_entry text=auto" >.gitattributes &&
+ git add --renormalize sparse_entry &&
+ test_sparse_entry_unchanged
Makes sense.
What should "git diff sparse_entry" say at this point, I have to
wonder?
+'
+
+test_done
Thanks.
On Wed, Feb 17, 2021 at 6:04 PM Junio C Hamano [off-list ref] wrote:
Matheus Tavares [off-list ref] writes:
quoted
+for opt in "" -f -u --ignore-removal
+do
+ if test -n "$opt"
+ then
+ opt=" $opt"
+ fi
+ test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
The above is cumulative, and as a consequence, "git add -u <path>"
is not tested, but "git add -f -u <path>" is. Intended? How was
the order of the options listed in "for opt in ..." chosen?
I may be misreading, but I don't think this is cumulative (though it's
easy to mistake it as such due to the way it inserts a space before
$opt). My interpretation is that `opt` gets overwritten with a new
value on each iteration, and it is inserting the space merely to make
the test title print nicely. A more idiomatic way to do this would
have been:
for opt in "" -f -u --ignore-removal
do
test_expect_success " git add${opt:+ $opt} does ..." '
On Wed, Feb 17, 2021 at 8:01 PM Junio C Hamano [off-list ref] wrote:
Matheus Tavares [off-list ref] writes:
quoted
+test_expect_success "git add does not remove SKIP_WORKTREE entries" '
We use the term SKIP_WORKTREE and SPARSE interchangeably here. I
wonder if it is easier to understand if we stick to one e.g. by
saying "... does not remove 'sparse' entries" instead? I dunno.
Good idea, thanks.
quoted
+ setup_sparse_entry &&
+ rm sparse_entry &&
+ git add sparse_entry &&
+ test_sparse_entry_unchanged
Wow. OK. Makes a reader wonder what should happen when the two
operations are replaced with "git rm sparse_entry"; let's read on.
quoted
+'
+
+test_expect_success "git add -A does not remove SKIP_WORKTREE entries" '
+ setup_sparse_entry &&
+ rm sparse_entry &&
+ git add -A &&
+ test_sparse_entry_unchanged
+'
OK. As there is nothing other than sparse_entry in the working tree
or in the index, the above two should be equivalent.
I just realized that the "actual" file created by the previous
test_sparse_entry_unchanged would also be added to the index here.
This doesn't affect the current test or the next ones, but I guess we
could use `git add -A sparse_entry` to avoid any future problems.
I wonder what should happen if the "add -A" gets replaced with "add .";
it should behave the same way, I think. Is it worth testing that
case as well, or is it redundant?
Hmm, I think it might be better to test only `add -A sparse_entry`, to
avoid adding the "actual" file or others that might be introduced in
future changes.
quoted
+test_expect_success 'git add --refresh does not update SKIP_WORKTREE entries' '
+ setup_sparse_entry &&
+ test-tool chmtime -60 sparse_entry &&
+ git add --refresh sparse_entry &&
+
+ # We must unset the SKIP_WORKTREE bit, otherwise
+ # git diff-files would skip examining the file
+ git update-index --no-skip-worktree sparse_entry &&
+
+ echo sparse_entry >expected &&
+ git diff-files --name-only sparse_entry >actual &&
+ test_cmp actual expected
Hmph, I am not sure what we are testing at this point. One way to
make the final diff-files step show sparse_entry would be for "git
add --refresh" to be a no-op, in which case, the cached stat
information in the index would be different in mtime from the path
in the working tree. But "update-index --no-skip-worktree" may be
buggy and further change or invalidate the cached stat information
to cause diff-files to report that the path may be different.
Oh, that is a problem... We could use `git ls-files --debug` and
directly compare the mtime field. But the ls-files doc says that
--debug format may change at any time... Any other idea?
quoted
+'
+
+test_expect_failure 'git add --chmod does not update SKIP_WORKTREE entries' '
+ setup_sparse_entry &&
+ git add --chmod=+x sparse_entry &&
+ test_sparse_entry_unchanged
Hmph. Should we also check if sparse_entry in the filesystem also
is not made executable, not just the entry in the index?
I think so. This is already tested at the "core" --chmod tests in
t3700, but it certainly wouldn't hurt to test here too.
Thanks for all the great feedback
On Thu, Feb 18, 2021 at 12:07 AM Matheus Tavares Bernardino [off-list ref] wrote:
On Wed, Feb 17, 2021 at 8:01 PM Junio C Hamano [off-list ref] wrote:
quoted
Matheus Tavares [off-list ref] writes:
quoted
+test_expect_success 'git add --refresh does not update SKIP_WORKTREE entries' '
+ setup_sparse_entry &&
+ test-tool chmtime -60 sparse_entry &&
+ git add --refresh sparse_entry &&
+
+ # We must unset the SKIP_WORKTREE bit, otherwise
+ # git diff-files would skip examining the file
+ git update-index --no-skip-worktree sparse_entry &&
+
+ echo sparse_entry >expected &&
+ git diff-files --name-only sparse_entry >actual &&
+ test_cmp actual expected
Hmph, I am not sure what we are testing at this point. One way to
make the final diff-files step show sparse_entry would be for "git
add --refresh" to be a no-op, in which case, the cached stat
information in the index would be different in mtime from the path
in the working tree. But "update-index --no-skip-worktree" may be
buggy and further change or invalidate the cached stat information
to cause diff-files to report that the path may be different.
Oh, that is a problem... We could use `git ls-files --debug` and
directly compare the mtime field. But the ls-files doc says that
--debug format may change at any time... Any other idea?
Or maybe we could use a test helper for this? Something like:
-- >8 --
#include "test-tool.h"
#include "cache.h"
int cmd__cached_mtime(int argc, const char **argv)
{
int i;
setup_git_directory();
if (read_cache() < 0)
die("could not read the index");
for (i = 1; i < argc; i++) {
const struct stat_data *sd;
int pos = cache_name_pos(argv[i], strlen(argv[i]));
if (pos < 0) {
pos = -pos-1;
if (pos < active_nr && !strcmp(active_cache[pos]->name, argv[i]))
die("'%s' is unmerged", argv[i]);
else
die("'%s' is not in the index", argv[i]);
}
sd = &active_cache[pos]->ce_stat_data;
printf("%s %u:%u\n", argv[i], sd->sd_mtime.sec, sd->sd_mtime.nsec);
}
discard_cache();
return 0;
}
-- >8 --
Then, the test would become:
setup_sparse_entry &&
test-tool cached_mtime sparse_entry >before &&
test-tool chmtime -60 sparse_entry &&
git add --refresh sparse_entry &&
test-tool cached_mtime sparse_entry >after &&
test_cmp before after
What do you think?
On Wed, Feb 17, 2021 at 3:01 PM Junio C Hamano [off-list ref] wrote:
Matheus Tavares [off-list ref] writes:
quoted
We already have a couple tests for `add` with SKIP_WORKTREE entries in
t7012, but these only cover the most basic scenarios. As we will be
changing how `add` deals with sparse paths in the subsequent commits,
let's move these two tests to their own file and add more test cases
for different `add` options and situations. This also demonstrates two
options that don't currently respect SKIP_WORKTREE entries: `--chmod`
and `--renormalize`.
Nice. It makes sense to describe what we want first, like this step..
quoted
Signed-off-by: Matheus Tavares <redacted>
---
t/t3705-add-sparse-checkout.sh | 92 ++++++++++++++++++++++++++++++++
t/t7012-skip-worktree-writing.sh | 19 -------
2 files changed, 92 insertions(+), 19 deletions(-)
create mode 100755 t/t3705-add-sparse-checkout.sh
diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
new file mode 100755
index 0000000000..5530e796b5
--- /dev/null
+++ b/t/t3705-add-sparse-checkout.sh
@@ -0,0 +1,92 @@
+#!/bin/sh
+
+test_description='git add in sparse checked out working trees'
+
+. ./test-lib.sh
+
+SPARSE_ENTRY_BLOB=""
+
+# Optionally take a string for the entry's contents
+setup_sparse_entry()
+{
Style.
setup_sparse_entry () {
on a single line.
quoted
+ if test -f sparse_entry
+ then
+ rm sparse_entry
+ fi &&
+ git update-index --force-remove sparse_entry &&
Why not an unconditional removal on the working tree side?
rm -f sparse_entry &&
git update-index --force-remove sparse_entry &&
Are there cases where we may have sparse_entry directory here?
quoted
+
+ if test "$#" -eq 1
No need to quote $# (we know it is a number).
quoted
+ then
+ printf "$1" >sparse_entry
Make sure the test writers know that they are passing a string that
will be interpreted as a printf format. Review the comment before
the function and adjust it appropriately ("a string" is not what you
want to tell them).
quoted
+ else
+ printf "" >sparse_entry
Just
>sparse_entry
is sufficient, no?
quoted
+ fi &&
+ git add sparse_entry &&
+ git update-index --skip-worktree sparse_entry &&
+ SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry)
+}
+
+test_sparse_entry_unchanged() {
Style.
test_sparse_entry_unchanged () {
quoted
+ echo "100644 $SPARSE_ENTRY_BLOB 0 sparse_entry" >expected &&
+ git ls-files --stage sparse_entry >actual &&
+ test_cmp expected actual
OK. So the expected pattern is to first "setup", do stuff that
shouldn't affect the sparse entry in the index, and then call this
to make sure?
quoted
+}
quoted
+test_expect_success "git add does not remove SKIP_WORKTREE entries" '
We use the term SKIP_WORKTREE and SPARSE interchangeably here. I
wonder if it is easier to understand if we stick to one e.g. by
saying "... does not remove 'sparse' entries" instead? I dunno.
quoted
+ setup_sparse_entry &&
+ rm sparse_entry &&
+ git add sparse_entry &&
+ test_sparse_entry_unchanged
Wow. OK. Makes a reader wonder what should happen when the two
operations are replaced with "git rm sparse_entry"; let's read on.
quoted
+'
+
+test_expect_success "git add -A does not remove SKIP_WORKTREE entries" '
+ setup_sparse_entry &&
+ rm sparse_entry &&
+ git add -A &&
+ test_sparse_entry_unchanged
+'
OK. As there is nothing other than sparse_entry in the working tree
or in the index, the above two should be equivalent.
I wonder what should happen if the "add -A" gets replaced with "add .";
it should behave the same way, I think. Is it worth testing that
case as well, or is it redundant?
quoted
+for opt in "" -f -u --ignore-removal
+do
+ if test -n "$opt"
+ then
+ opt=" $opt"
+ fi
The above is cumulative, and as a consequence, "git add -u <path>"
is not tested, but "git add -f -u <path>" is. Intended? How was
the order of the options listed in "for opt in ..." chosen?
quoted
+ test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
+ setup_sparse_entry &&
+ echo modified >sparse_entry &&
+ git add $opt sparse_entry &&
+ test_sparse_entry_unchanged
+ '
+done
+
+test_expect_success 'git add --refresh does not update SKIP_WORKTREE entries' '
+ setup_sparse_entry &&
+ test-tool chmtime -60 sparse_entry &&
+ git add --refresh sparse_entry &&
+
+ # We must unset the SKIP_WORKTREE bit, otherwise
+ # git diff-files would skip examining the file
+ git update-index --no-skip-worktree sparse_entry &&
+
+ echo sparse_entry >expected &&
+ git diff-files --name-only sparse_entry >actual &&
+ test_cmp actual expected
Hmph, I am not sure what we are testing at this point. One way to
make the final diff-files step show sparse_entry would be for "git
add --refresh" to be a no-op, in which case, the cached stat
information in the index would be different in mtime from the path
in the working tree. But "update-index --no-skip-worktree" may be
buggy and further change or invalidate the cached stat information
to cause diff-files to report that the path may be different.
quoted
+'
+
+test_expect_failure 'git add --chmod does not update SKIP_WORKTREE entries' '
+ setup_sparse_entry &&
+ git add --chmod=+x sparse_entry &&
+ test_sparse_entry_unchanged
Hmph. Should we also check if sparse_entry in the filesystem also
is not made executable, not just the entry in the index?
quoted
+'
+
+test_expect_failure 'git add --renormalize does not update SKIP_WORKTREE entries' '
+ test_config core.autocrlf false &&
+ setup_sparse_entry "LINEONE\r\nLINETWO\r\n" &&
+ echo "sparse_entry text=auto" >.gitattributes &&
+ git add --renormalize sparse_entry &&
+ test_sparse_entry_unchanged
Makes sense.
What should "git diff sparse_entry" say at this point, I have to
wonder?
It'll show nothing:
$ git show :0:sparse_entry
LINEONE
LINETWO
$ echo foobar >sparse_entry
$ cat sparse_entry
foobar
$ git diff sparse_entry
$
Likewise, `git status` will ignore SKIP_WORKTREE entries, and `reset
--hard` will fail to correct these files. Several of these were
discussed at https://lore.kernel.org/git/CABPp-BGJ_Nvi5TmgriD9Bh6eNXE2EDq2f8e8QKXAeYG3BxZafA@mail.gmail.com/;
Matheus is just cleaning up the first few candidates.
I thought that present-despite-SKIP_WORKTREE-setting files would be
extremely rare. While they are somewhat rare, they show up more than
I thought. The reason I've seen them appear for users is that they
hit Ctrl+C in the middle of progress updates for a `git
sparse-checkout {disable,add} ...` command; they decide mid-operation
that they specified the wrong set of sparsity paths or didn't really
want to unsparsify or whatever, and wrongly assume that operations are
atomic (individual files are checked out to the working copy first,
followed by an update of the index and .git/info/sparse-checkout file
at the end).
The same kind of issue exists without SKIP_WORKTREE when people use
checkout or switch to change branches and hit Ctrl+C in the middle,
but `git status` will warn users about those and `git reset --hard`
will clean them up. Neither is true in the
present-despite-SKIP_WORKTREE files case. That should also be fixed
up, but add & rm were easier low-hanging fruit.