| Message ID | 9a07c6932f4c7ef844df1fc4f5b6b9feb1810135.1665973401.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | archive: Add --recurse-submodules to git-archive command | expand |
Hi Heather On 17/10/2022 03:23, Heather Lapointe via GitGitGadget wrote: > From: Heather Lapointe <alpha@alphaservcomputing.solutions> > > This highlights some buggy behavior from read_tree for submodules that > was not being executed. The commit message should explain the reason behind the change being made. In this case it would be helpful to give an overview of what the bug is you're testing for. Given the description I was expecting to see some failing tests that are fixed by a later patch but that doesn't seem to be the case, so I'm wondering what these tests do. > This introduces a test-tool tree-read-tree-at command > (the complex name is because it is not related to the read-tree command). It would also be helpful to explain why we cannot reproduce the bug with "git read-tree --recurse-submodules" Best Wishes Phillip > Signed-off-by: Heather Lapointe <alpha@alphaservcomputing.solutions> > --- > Makefile | 1 + > t/helper/test-tool.c | 1 + > t/helper/test-tool.h | 1 + > t/helper/test-tree-read-tree-at.c | 40 +++++++++++++++++++ > t/t1023-tree-read-tree-at.sh | 65 +++++++++++++++++++++++++++++++ > 5 files changed, 108 insertions(+) > create mode 100644 t/helper/test-tree-read-tree-at.c > create mode 100755 t/t1023-tree-read-tree-at.sh > > diff --git a/Makefile b/Makefile > index 6bfb62cbe94..52d17ca7276 100644 > --- a/Makefile > +++ b/Makefile > @@ -788,6 +788,7 @@ TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o > TEST_BUILTINS_OBJS += test-submodule.o > TEST_BUILTINS_OBJS += test-subprocess.o > TEST_BUILTINS_OBJS += test-trace2.o > +TEST_BUILTINS_OBJS += test-tree-read-tree-at.o > TEST_BUILTINS_OBJS += test-urlmatch-normalization.o > TEST_BUILTINS_OBJS += test-userdiff.o > TEST_BUILTINS_OBJS += test-wildmatch.o > diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c > index d1d013bcd92..a8a9bedec5f 100644 > --- a/t/helper/test-tool.c > +++ b/t/helper/test-tool.c > @@ -82,6 +82,7 @@ static struct test_cmd cmds[] = { > { "submodule-nested-repo-config", cmd__submodule_nested_repo_config }, > { "subprocess", cmd__subprocess }, > { "trace2", cmd__trace2 }, > + { "tree-read-tree-at", cmd__tree_read_tree_at }, > { "userdiff", cmd__userdiff }, > { "urlmatch-normalization", cmd__urlmatch_normalization }, > { "xml-encode", cmd__xml_encode }, > diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h > index 6b46b6444b6..409fddfaeb8 100644 > --- a/t/helper/test-tool.h > +++ b/t/helper/test-tool.h > @@ -76,6 +76,7 @@ int cmd__submodule_config(int argc, const char **argv); > int cmd__submodule_nested_repo_config(int argc, const char **argv); > int cmd__subprocess(int argc, const char **argv); > int cmd__trace2(int argc, const char **argv); > +int cmd__tree_read_tree_at(int argc, const char **argv); > int cmd__userdiff(int argc, const char **argv); > int cmd__urlmatch_normalization(int argc, const char **argv); > int cmd__xml_encode(int argc, const char **argv); > diff --git a/t/helper/test-tree-read-tree-at.c b/t/helper/test-tree-read-tree-at.c > new file mode 100644 > index 00000000000..bba759bb264 > --- /dev/null > +++ b/t/helper/test-tree-read-tree-at.c > @@ -0,0 +1,40 @@ > +/* This tests tree.c's read_tree / read_tree_at. > +We call it tree-read-tree-at to disambiguate with the read-tree tool. > +*/ > +#include "cache.h" > +#include "pathspec.h" > +#include "test-tool.h" > +#include "tree.h" > + > +static int test_handle_entry(const struct object_id *oid, > + struct strbuf *base, const char *filename, > + unsigned mode, void *context UNUSED) { > + printf("%i %s %s%s\n", mode, oid_to_hex(oid), base->buf, filename); > + if (S_ISDIR(mode) || S_ISGITLINK(mode)) { > + return READ_TREE_RECURSIVE; > + } > + return 0; > +} > + > +int cmd__tree_read_tree_at(int argc UNUSED, const char **argv) > +{ > + struct pathspec pathspec; > + struct tree *tree; > + struct repository *repo; > + struct object_id oid; > + > + setup_git_directory(); > + repo = the_repository; > + assert(repo); > + > + parse_pathspec(&pathspec, 0, > + PATHSPEC_PREFER_FULL, > + "", argv); > + > + assert(repo_get_oid(repo, "HEAD", &oid) == 0); > + tree = repo_parse_tree_indirect(repo, &oid); > + assert(tree); > + pathspec.recurse_submodules = 1; > + read_tree(repo, tree, &pathspec, test_handle_entry, NULL); > + return 0; > +} > diff --git a/t/t1023-tree-read-tree-at.sh b/t/t1023-tree-read-tree-at.sh > new file mode 100755 > index 00000000000..9e5ce3abb4b > --- /dev/null > +++ b/t/t1023-tree-read-tree-at.sh > @@ -0,0 +1,65 @@ > +#!/bin/sh > + > +# tests for tree.c (not read-tree.c) > +test_description='Test read_tree / read_tree_at' > +. ./test-lib.sh > + > +test_expect_success 'read_tree basic' ' > + rm -rf walk_tree_basic && > + git init walk_tree_basic && > + ( > + cd walk_tree_basic && > + set -x && > + > + mkdir -p dir1/dirA && > + mkdir -p dir1/dirB && > + mkdir -p dir2 && > + echo "file1" > file1.txt && > + echo "file2" > file2.txt && > + # uncommitted > + echo "file3" > file3.txt && > + > + echo "file1A1" > dir1/dirA/file1.txt && > + git add file1.txt file2.txt dir1/dirA/file1.txt && > + git commit -m "initial commit" && > + > + test-tool tree-read-tree-at . > walk1.txt && > + grep " file1.txt" walk1.txt && > + ! grep " file3.txt" walk1.txt && > + ! grep " dir1/dirB" walk1.txt && > + grep " dir1/dirA/file1.txt" walk1.txt > + ) > +' > + > +test_expect_success 'read_tree submodules' ' > + rm -rf walk_tree_submodules && > + git init submodule1 && > + ( > + cd submodule1 && > + mkdir -p dir1/dirA && > + echo "dir2/sub1/file1.txt" > file1.txt && > + echo "dir2/sub1/file1A1.txt" > dir1/dirA/file1.txt && > + git add file1.txt dir1/dirA/file1.txt && > + git commit -m "initial commit" > + ) && > + git init walk_tree_submodules && > + ( > + cd walk_tree_submodules && > + > + mkdir -p dir2 && > + echo "file1" > file1.txt && > + echo "dir2/file2" > dir2/file2.txt && > + git add file1.txt dir2/file2.txt && > + git commit -m "initial commit" && > + > + git submodule add ../submodule1 dir2/sub1 && > + git commit -m "add submodule1" && > + > + test-tool tree-read-tree-at . > walk2.txt && > + grep " file1.txt" walk2.txt && > + grep " dir2/sub1/file1.txt" walk2.txt && > + grep " dir2/sub1/dir1/dirA/file1.txt" walk2.txt > + ) > +' > + > +test_done
"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/t/t1023-tree-read-tree-at.sh b/t/t1023-tree-read-tree-at.sh > new file mode 100755 > index 00000000000..9e5ce3abb4b > --- /dev/null > +++ b/t/t1023-tree-read-tree-at.sh > @@ -0,0 +1,65 @@ > +#!/bin/sh > + > +# tests for tree.c (not read-tree.c) > +test_description='Test read_tree / read_tree_at' > +. ./test-lib.sh > + > +test_expect_success 'read_tree basic' ' > + rm -rf walk_tree_basic && > + git init walk_tree_basic && > + ( > + cd walk_tree_basic && > + set -x && Do we need this, when we have '-x' option alongside with '-v', '-i', and '-d' available in the test harness already? > + mkdir -p dir1/dirA && > + mkdir -p dir1/dirB && > + mkdir -p dir2 && Can't we have these three done by the same single "mkdir -p" process? > + echo "file1" > file1.txt && > + echo "file2" > file2.txt && Lose the SP between redirection operator ">" and its target, i.e. echo file1 >file1.txt cf. Documentation/CodingGuidelines Also you do not necessarily have to have dq around a single token. > + # uncommitted > + echo "file3" > file3.txt && > + > + echo "file1A1" > dir1/dirA/file1.txt && > + git add file1.txt file2.txt dir1/dirA/file1.txt && > + git commit -m "initial commit" && > + > + test-tool tree-read-tree-at . > walk1.txt && > + grep " file1.txt" walk1.txt && > + ! grep " file3.txt" walk1.txt && > + ! grep " dir1/dirB" walk1.txt && > + grep " dir1/dirA/file1.txt" walk1.txt > + ) > +' > + > +test_expect_success 'read_tree submodules' ' > + rm -rf walk_tree_submodules && Curious why the above does not clean "submodule1", too. After all, all the "rm -rf" we saw in this script above are removing what its earlier steps would never have created (but will create), just in case. Why not do the same? If the pattern is to "remove what we will need to create immediately before we actually try to create, just in case", then shouldn't the above be removing "submodule1", and we should have another "rm -rf" for "walk_tree_submodules" immediately before we do "git init" on it several lines below? > + git init submodule1 && > + ( > + cd submodule1 && > + mkdir -p dir1/dirA && > + echo "dir2/sub1/file1.txt" > file1.txt && > + echo "dir2/sub1/file1A1.txt" > dir1/dirA/file1.txt && > + git add file1.txt dir1/dirA/file1.txt && > + git commit -m "initial commit" > + ) && > + git init walk_tree_submodules && > + ( > + cd walk_tree_submodules && > + > + mkdir -p dir2 && > + echo "file1" > file1.txt && > + echo "dir2/file2" > dir2/file2.txt && > + git add file1.txt dir2/file2.txt && > + git commit -m "initial commit" && > + > + git submodule add ../submodule1 dir2/sub1 && > + git commit -m "add submodule1" && > + > + test-tool tree-read-tree-at . > walk2.txt && > + grep " file1.txt" walk2.txt && > + grep " dir2/sub1/file1.txt" walk2.txt && > + grep " dir2/sub1/dir1/dirA/file1.txt" walk2.txt > + ) > +' > + > +test_done
"Heather Lapointe via GitGitGadget" <gitgitgadget@gmail.com> writes: > + setup_git_directory(); > + repo = the_repository; > + assert(repo); > + > + parse_pathspec(&pathspec, 0, > + PATHSPEC_PREFER_FULL, > + "", argv); Here, the repository is hardcoded to be "the_repository", and the C code allows the pathspec to be varied but the shell test code always specifies "." as the pathspec. Given that one of the main points of this series is the repo varying, could the repo be taken in as a CLI argument? The pathspec can be left variable, but if it's not going to change, you might as well hardcode it in the C code. The existing test cases of a basic one and a recursing one is good, but it would be good also to have one where the repo being passed into the function is not the repo whose directory we're currently executing in (that is, different to the_repository). That way we can test that the function works for arbitrary repositories.
diff --git a/Makefile b/Makefile index 6bfb62cbe94..52d17ca7276 100644 --- a/Makefile +++ b/Makefile @@ -788,6 +788,7 @@ TEST_BUILTINS_OBJS += test-submodule-nested-repo-config.o TEST_BUILTINS_OBJS += test-submodule.o TEST_BUILTINS_OBJS += test-subprocess.o TEST_BUILTINS_OBJS += test-trace2.o +TEST_BUILTINS_OBJS += test-tree-read-tree-at.o TEST_BUILTINS_OBJS += test-urlmatch-normalization.o TEST_BUILTINS_OBJS += test-userdiff.o TEST_BUILTINS_OBJS += test-wildmatch.o diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index d1d013bcd92..a8a9bedec5f 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -82,6 +82,7 @@ static struct test_cmd cmds[] = { { "submodule-nested-repo-config", cmd__submodule_nested_repo_config }, { "subprocess", cmd__subprocess }, { "trace2", cmd__trace2 }, + { "tree-read-tree-at", cmd__tree_read_tree_at }, { "userdiff", cmd__userdiff }, { "urlmatch-normalization", cmd__urlmatch_normalization }, { "xml-encode", cmd__xml_encode }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 6b46b6444b6..409fddfaeb8 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -76,6 +76,7 @@ int cmd__submodule_config(int argc, const char **argv); int cmd__submodule_nested_repo_config(int argc, const char **argv); int cmd__subprocess(int argc, const char **argv); int cmd__trace2(int argc, const char **argv); +int cmd__tree_read_tree_at(int argc, const char **argv); int cmd__userdiff(int argc, const char **argv); int cmd__urlmatch_normalization(int argc, const char **argv); int cmd__xml_encode(int argc, const char **argv); diff --git a/t/helper/test-tree-read-tree-at.c b/t/helper/test-tree-read-tree-at.c new file mode 100644 index 00000000000..bba759bb264 --- /dev/null +++ b/t/helper/test-tree-read-tree-at.c @@ -0,0 +1,40 @@ +/* This tests tree.c's read_tree / read_tree_at. +We call it tree-read-tree-at to disambiguate with the read-tree tool. +*/ +#include "cache.h" +#include "pathspec.h" +#include "test-tool.h" +#include "tree.h" + +static int test_handle_entry(const struct object_id *oid, + struct strbuf *base, const char *filename, + unsigned mode, void *context UNUSED) { + printf("%i %s %s%s\n", mode, oid_to_hex(oid), base->buf, filename); + if (S_ISDIR(mode) || S_ISGITLINK(mode)) { + return READ_TREE_RECURSIVE; + } + return 0; +} + +int cmd__tree_read_tree_at(int argc UNUSED, const char **argv) +{ + struct pathspec pathspec; + struct tree *tree; + struct repository *repo; + struct object_id oid; + + setup_git_directory(); + repo = the_repository; + assert(repo); + + parse_pathspec(&pathspec, 0, + PATHSPEC_PREFER_FULL, + "", argv); + + assert(repo_get_oid(repo, "HEAD", &oid) == 0); + tree = repo_parse_tree_indirect(repo, &oid); + assert(tree); + pathspec.recurse_submodules = 1; + read_tree(repo, tree, &pathspec, test_handle_entry, NULL); + return 0; +} diff --git a/t/t1023-tree-read-tree-at.sh b/t/t1023-tree-read-tree-at.sh new file mode 100755 index 00000000000..9e5ce3abb4b --- /dev/null +++ b/t/t1023-tree-read-tree-at.sh @@ -0,0 +1,65 @@ +#!/bin/sh + +# tests for tree.c (not read-tree.c) +test_description='Test read_tree / read_tree_at' +. ./test-lib.sh + +test_expect_success 'read_tree basic' ' + rm -rf walk_tree_basic && + git init walk_tree_basic && + ( + cd walk_tree_basic && + set -x && + + mkdir -p dir1/dirA && + mkdir -p dir1/dirB && + mkdir -p dir2 && + echo "file1" > file1.txt && + echo "file2" > file2.txt && + # uncommitted + echo "file3" > file3.txt && + + echo "file1A1" > dir1/dirA/file1.txt && + git add file1.txt file2.txt dir1/dirA/file1.txt && + git commit -m "initial commit" && + + test-tool tree-read-tree-at . > walk1.txt && + grep " file1.txt" walk1.txt && + ! grep " file3.txt" walk1.txt && + ! grep " dir1/dirB" walk1.txt && + grep " dir1/dirA/file1.txt" walk1.txt + ) +' + +test_expect_success 'read_tree submodules' ' + rm -rf walk_tree_submodules && + git init submodule1 && + ( + cd submodule1 && + mkdir -p dir1/dirA && + echo "dir2/sub1/file1.txt" > file1.txt && + echo "dir2/sub1/file1A1.txt" > dir1/dirA/file1.txt && + git add file1.txt dir1/dirA/file1.txt && + git commit -m "initial commit" + ) && + git init walk_tree_submodules && + ( + cd walk_tree_submodules && + + mkdir -p dir2 && + echo "file1" > file1.txt && + echo "dir2/file2" > dir2/file2.txt && + git add file1.txt dir2/file2.txt && + git commit -m "initial commit" && + + git submodule add ../submodule1 dir2/sub1 && + git commit -m "add submodule1" && + + test-tool tree-read-tree-at . > walk2.txt && + grep " file1.txt" walk2.txt && + grep " dir2/sub1/file1.txt" walk2.txt && + grep " dir2/sub1/dir1/dirA/file1.txt" walk2.txt + ) +' + +test_done