Re: [PATCH 05/11] tests: use test_ln_s_add to remove SYMLINKS prerequisite (trivial cases)
Am 04.06.2013 23:06, schrieb Junio C Hamano: Johannes Sixt j...@kdbg.org writes: There are many instances where the treatment of symbolic links in the object model and the algorithms are tested, but where it is not necessary to actually have a symbolic link in the worktree. Make adjustments to the tests and remove the SYMLINKS prerequisite when appropriate in trivial cases, where trivial means: - merely a replacement of 'ln -s a b' to test_ln_s or of 'ln -s a b git add b' to test_ln_s_add is needed; - a test for symbolic link on the file system can be split off (and remains protected by SYMLINKS); - existing code is equivalent to test_ln_s[_add]. This is too big to review in one go, so I may have separate messages later on this same patch. diff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh index ff163cf..bd17ba2 100755 --- a/t/t2003-checkout-cache-mkdir.sh +++ b/t/t2003-checkout-cache-mkdir.sh @@ -19,10 +19,10 @@ test_expect_success 'setup' ' git update-index --add path0 path1/file1 ' -test_expect_success SYMLINKS 'have symlink in place where dir is expected.' ' +test_expect_success 'have symlink in place where dir is expected.' ' rm -fr path0 path1 mkdir path2 -ln -s path2 path1 +test_ln_s path2 path1 git checkout-index -f -a test ! -h path1 test -d path1 test -f path1/file1 test ! -f path2/file1 I do not think this hunk is correct. [...] Under the precondition checkout-index runs in this test, a casual echo rezrov path1/file1 would leave path1 as a symlink without turning it into a real directory, and we will end up creating path2/file1. We are making sure that checkout-index does not behave that way, and it is essential to have symlink support in the working tree for the bug to trigger. @@ -79,10 +79,10 @@ test_expect_success SYMLINKS 'use --prefix=tmp/orary- where tmp is a symlink' ' test -h tmp ' -test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' ' +test_expect_success 'use --prefix=tmp- where tmp-path1 is a symlink' ' rm -fr path0 path1 path2 tmp* mkdir tmp1 -ln -s tmp1 tmp-path1 +test_ln_s tmp1 tmp-path1 git checkout-index --prefix=tmp- -f -a test -f tmp-path0 test ! -h tmp-path1 This change has the same issue, I think. Yes, agreed. The converted tests -- when SYMLINKS are not available -- just repeat what is tested elsewhere. Nice catch. After I've found a hammer (test_ln_s) I was mindlessly looking for nails and found two of them in these two instances. ;-) -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/11] tests: use test_ln_s_add to remove SYMLINKS prerequisite (trivial cases)
Am 04.06.2013 23:55, schrieb Junio C Hamano: Johannes Sixt j...@kdbg.org writes: diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh index 88be904..563ac7f 100755 --- a/t/t3000-ls-files-others.sh +++ b/t/t3000-ls-files-others.sh @@ -19,12 +19,7 @@ filesystem. test_expect_success 'setup ' ' date path0 -if test_have_prereq SYMLINKS -then -ln -s xyzzy path1 -else -date path1 -fi +test_ln_s xyzzy path1 mkdir path2 path3 path4 date path2/file2 date path2-junk This also is not appropriate, but it is not as bad as the one in t2003 I earlier commented on. This test wants an untracked symbolic link in the working tree as path1 and wants to see ls-files -o report it as other. On a filesystem that lack symbolic link, we obviously cannot have one, so as a substitute we just create another regular file to make the expected output and comparison simpler. Exactly. This is just a convenience. The issue is not introduced by the conversion, but dates back 4 years when I added the SYMLINKS check. We now only use a less random string on !SYMLINKS filesystems. test_expect_success 'git ls-files -k to show killed files.' ' date path2 -if test_have_prereq SYMLINKS -then -ln -s frotz path3 -ln -s nitfol path5 -else -date path3 -date path5 -fi +test_ln_s frotz path3 +test_ln_s nitfol path5 This falls into the same category as the one in t3000 above. The only thing that matters in this test is path3 and path5 are non directories so that the former is killed when path3/file3 needs to be checked out, while path5 will not appear in ls-files -k output. Ideally we would want to test regular files and symlinks as two different kinds of non directory filesystem entities, but on platforms that lack symbolic links we cannot do so, hence we punt and create a regular file. Indeed. Same answer. diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh -test_expect_success SYMLINKS 'diff identical, but newly created symlink and file' ' +test_expect_success 'diff identical, but newly created symlink and file' ' expected rm -f frotz nitfol echo xyzzy nitfol test-chmtime +10 nitfol -ln -s xyzzy frotz +test_ln_s xyzzy frotz git diff-index -M -p $tree current compare_diff_patch expected current As the point of test is to compare $tree that has symlink with another symlink that is identical but newly created one, I think this _does_ want the filesystem entity to be a symbolic link, but the index has frotz as a symlink and the mode propagates to what we read from the filesystem on !has_symlinks systems, so this conversion may be a correct one, though it is a bit tricky. Yes, this test depends on the mode propagation. I'll add a comment along these lines and keep the change in this patch with a title marked trivial cases. @@ -100,7 +103,7 @@ test_expect_success SYMLINKS 'diff different symlink and file' ' +yxyyz EOF rm -f frotz -ln -s yxyyz frotz +test_ln_s yxyyz frotz echo yxyyz nitfol git diff-index -M -p $tree current compare_diff_patch expected current Likewise. -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/11] tests: use test_ln_s_add to remove SYMLINKS prerequisite (trivial cases)
Johannes Sixt j...@kdbg.org writes: There are many instances where the treatment of symbolic links in the object model and the algorithms are tested, but where it is not necessary to actually have a symbolic link in the worktree. Make adjustments to the tests and remove the SYMLINKS prerequisite when appropriate in trivial cases, where trivial means: - merely a replacement of 'ln -s a b' to test_ln_s or of 'ln -s a b git add b' to test_ln_s_add is needed; - a test for symbolic link on the file system can be split off (and remains protected by SYMLINKS); - existing code is equivalent to test_ln_s[_add]. This is too big to review in one go, so I may have separate messages later on this same patch. diff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh index ff163cf..bd17ba2 100755 --- a/t/t2003-checkout-cache-mkdir.sh +++ b/t/t2003-checkout-cache-mkdir.sh @@ -19,10 +19,10 @@ test_expect_success 'setup' ' git update-index --add path0 path1/file1 ' -test_expect_success SYMLINKS 'have symlink in place where dir is expected.' ' +test_expect_success 'have symlink in place where dir is expected.' ' rm -fr path0 path1 mkdir path2 - ln -s path2 path1 + test_ln_s path2 path1 git checkout-index -f -a test ! -h path1 test -d path1 test -f path1/file1 test ! -f path2/file1 I do not think this hunk is correct. We have two regular files in the index: path0, path1/file1, and we add a symbolic link path1 that happens to point at directory path2/ in the working tree. The test is about making sure that checkout-index is not confused by the symbolic link in the working tree, by attempting to checkout path1/file1. Under the precondition checkout-index runs in this test, a casual echo rezrov path1/file1 would leave path1 as a symlink without turning it into a real directory, and we will end up creating path2/file1. We are making sure that checkout-index does not behave that way, and it is essential to have symlink support in the working tree for the bug to trigger. On a filesystem without symbolic links, the patched test would pass just fine, but there can be no aliasing between path1 and path2 in the first place. @@ -79,10 +79,10 @@ test_expect_success SYMLINKS 'use --prefix=tmp/orary- where tmp is a symlink' ' test -h tmp ' -test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' ' +test_expect_success 'use --prefix=tmp- where tmp-path1 is a symlink' ' rm -fr path0 path1 path2 tmp* mkdir tmp1 - ln -s tmp1 tmp-path1 + test_ln_s tmp1 tmp-path1 git checkout-index --prefix=tmp- -f -a test -f tmp-path0 test ! -h tmp-path1 This change has the same issue, I think. We prepare tmp-path1 symbolic link to trap a casual echo rezrov tmp-path1/file1 to be redirected to tmp1/file1 while leaving tmp-path1 as a symlink, making sure we do the equivalent of rm tmp-path1; mkdir tmp-path1 before echo rezrov tmp-path1/file1. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/11] tests: use test_ln_s_add to remove SYMLINKS prerequisite (trivial cases)
Johannes Sixt j...@kdbg.org writes: There are many instances where the treatment of symbolic links in the object model and the algorithms are tested, but where it is not necessary to actually have a symbolic link in the worktree. diff --git a/t/t3000-ls-files-others.sh b/t/t3000-ls-files-others.sh index 88be904..563ac7f 100755 --- a/t/t3000-ls-files-others.sh +++ b/t/t3000-ls-files-others.sh @@ -19,12 +19,7 @@ filesystem. test_expect_success 'setup ' ' date path0 - if test_have_prereq SYMLINKS - then - ln -s xyzzy path1 - else - date path1 - fi + test_ln_s xyzzy path1 mkdir path2 path3 path4 date path2/file2 date path2-junk This also is not appropriate, but it is not as bad as the one in t2003 I earlier commented on. This test wants an untracked symbolic link in the working tree as path1 and wants to see ls-files -o report it as other. On a filesystem that lack symbolic link, we obviously cannot have one, so as a substitute we just create another regular file to make the expected output and comparison simpler. diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh index 2d0ff2d..310e0a2 100755 --- a/t/t3010-ls-files-killed-modified.sh +++ b/t/t3010-ls-files-killed-modified.sh @@ -39,12 +39,7 @@ modified without reporting path9 and path10. ... + test_ln_s_add xyzzy path1 ... date path2/file2 date path3/file3 This one is correct; the test wants to make sure that path1 would be checked out as a non-directory, killing any files in path1/ that is a directory on the filesystem. @@ -52,20 +47,14 @@ test_expect_success 'git update-index --add to add various paths.' ' date path8 : path9 date path10 - git update-index --add -- path0 path1 path?/file? path7 path8 path9 path10 + git update-index --add -- path0 path?/file? path7 path8 path9 path10 And exclusion of path1 here is a logical consequence of the previous change, so it is good, too. test_expect_success 'git ls-files -k to show killed files.' ' date path2 - if test_have_prereq SYMLINKS - then - ln -s frotz path3 - ln -s nitfol path5 - else - date path3 - date path5 - fi + test_ln_s frotz path3 + test_ln_s nitfol path5 This falls into the same category as the one in t3000 above. The only thing that matters in this test is path3 and path5 are non directories so that the former is killed when path3/file3 needs to be checked out, while path5 will not appear in ls-files -k output. Ideally we would want to test regular files and symlinks as two different kinds of non directory filesystem entities, but on platforms that lack symbolic links we cannot do so, hence we punt and create a regular file. diff --git a/t/t4011-diff-symlink.sh b/t/t4011-diff-symlink.sh index f0d5041..3888519 100755 --- a/t/t4011-diff-symlink.sh +++ b/t/t4011-diff-symlink.sh @@ -66,12 +69,12 @@ test_expect_success SYMLINKS 'diff removed symlink and file' ' compare_diff_patch expected current ' -test_expect_success SYMLINKS 'diff identical, but newly created symlink and file' ' +test_expect_success 'diff identical, but newly created symlink and file' ' expected rm -f frotz nitfol echo xyzzy nitfol test-chmtime +10 nitfol - ln -s xyzzy frotz + test_ln_s xyzzy frotz git diff-index -M -p $tree current compare_diff_patch expected current As the point of test is to compare $tree that has symlink with another symlink that is identical but newly created one, I think this _does_ want the filesystem entity to be a symbolic link, but the index has frotz as a symlink and the mode propagates to what we read from the filesystem on !has_symlinks systems, so this conversion may be a correct one, though it is a bit tricky. @@ -80,7 +83,7 @@ test_expect_success SYMLINKS 'diff identical, but newly created symlink and file compare_diff_patch expected current ' -test_expect_success SYMLINKS 'diff different symlink and file' ' +test_expect_success 'diff different symlink and file' ' cat expected -\EOF diff --git a/frotz b/frotz index 7c465af..df1db54 12 @@ -100,7 +103,7 @@ test_expect_success SYMLINKS 'diff different symlink and file' ' +yxyyz EOF rm -f frotz - ln -s yxyyz frotz + test_ln_s yxyyz frotz echo yxyyz nitfol git diff-index -M -p $tree current compare_diff_patch expected current Likewise. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/11] tests: use test_ln_s_add to remove SYMLINKS prerequisite (trivial cases)
There are many instances where the treatment of symbolic links in the object model and the algorithms are tested, but where it is not necessary to actually have a symbolic link in the worktree. Make adjustments to the tests and remove the SYMLINKS prerequisite when appropriate in trivial cases, where trivial means: - merely a replacement of 'ln -s a b' to test_ln_s or of 'ln -s a b git add b' to test_ln_s_add is needed; - a test for symbolic link on the file system can be split off (and remains protected by SYMLINKS); - existing code is equivalent to test_ln_s[_add]. Signed-off-by: Johannes Sixt j...@kdbg.org --- The changes in t9500-gitweb-* were not tested on a system that does not have SYMLINKS. t/t1004-read-tree-m-u-wf.sh| 7 +++--- t/t2001-checkout-cache-clash.sh| 7 +++--- t/t2003-checkout-cache-mkdir.sh| 8 +++ t/t2004-checkout-cache-temp.sh | 5 ++--- t/t2007-checkout-symlink.sh| 12 +-- t/t2021-checkout-overwrite.sh | 12 +++ t/t2200-add-update.sh | 5 ++--- t/t3000-ls-files-others.sh | 7 +- t/t3010-ls-files-killed-modified.sh| 19 - t/t3700-add.sh | 15 ++--- t/t3903-stash.sh | 39 -- t/t4008-diff-break-rewrite.sh | 12 +-- t/t4011-diff-symlink.sh| 23 +++- t/t4030-diff-textconv.sh | 8 +++ t/t4115-apply-symlink.sh | 10 - t/t4122-apply-symlink-inside.sh| 8 +++ t/t7001-mv.sh | 18 ++-- t/t7607-merge-overwrite.sh | 5 ++--- t/t8006-blame-textconv.sh | 14 +--- t/t8007-cat-file-textconv.sh | 10 - t/t9350-fast-export.sh | 5 ++--- t/t9500-gitweb-standalone-no-errors.sh | 15 + 22 files changed, 126 insertions(+), 138 deletions(-) diff --git a/t/t1004-read-tree-m-u-wf.sh b/t/t1004-read-tree-m-u-wf.sh index b3ae7d5..3e72aff 100755 --- a/t/t1004-read-tree-m-u-wf.sh +++ b/t/t1004-read-tree-m-u-wf.sh @@ -158,7 +158,7 @@ test_expect_success '3-way not overwriting local changes (their side)' ' ' -test_expect_success SYMLINKS 'funny symlink in work tree' ' +test_expect_success 'funny symlink in work tree' ' git reset --hard git checkout -b sym-b side-b @@ -170,15 +170,14 @@ test_expect_success SYMLINKS 'funny symlink in work tree' ' rm -fr a git checkout -b sym-a side-a mkdir -p a - ln -s ../b a/b - git add a/b + test_ln_s_add ../b a/b git commit -m we add a/b read_tree_u_must_succeed -m -u sym-a sym-a sym-b ' -test_expect_success SYMLINKS,SANITY 'funny symlink in work tree, un-unlink-able' ' +test_expect_success SANITY 'funny symlink in work tree, un-unlink-able' ' rm -fr a b git reset --hard diff --git a/t/t2001-checkout-cache-clash.sh b/t/t2001-checkout-cache-clash.sh index 98aa73e..1fc8e63 100755 --- a/t/t2001-checkout-cache-clash.sh +++ b/t/t2001-checkout-cache-clash.sh @@ -59,10 +59,9 @@ test_expect_success \ 'git read-tree -m $tree1 git checkout-index -f -a' test_debug 'show_files $tree1' -test_expect_success SYMLINKS \ -'git update-index --add a symlink.' \ -'ln -s path0 path1 - git update-index --add path1' +test_expect_success \ +'add a symlink' \ +'test_ln_s_add path0 path1' test_expect_success \ 'writing tree out with git write-tree' \ 'tree3=$(git write-tree)' diff --git a/t/t2003-checkout-cache-mkdir.sh b/t/t2003-checkout-cache-mkdir.sh index ff163cf..bd17ba2 100755 --- a/t/t2003-checkout-cache-mkdir.sh +++ b/t/t2003-checkout-cache-mkdir.sh @@ -19,10 +19,10 @@ test_expect_success 'setup' ' git update-index --add path0 path1/file1 ' -test_expect_success SYMLINKS 'have symlink in place where dir is expected.' ' +test_expect_success 'have symlink in place where dir is expected.' ' rm -fr path0 path1 mkdir path2 - ln -s path2 path1 + test_ln_s path2 path1 git checkout-index -f -a test ! -h path1 test -d path1 test -f path1/file1 test ! -f path2/file1 @@ -79,10 +79,10 @@ test_expect_success SYMLINKS 'use --prefix=tmp/orary- where tmp is a symlink' ' test -h tmp ' -test_expect_success SYMLINKS 'use --prefix=tmp- where tmp-path1 is a symlink' ' +test_expect_success 'use --prefix=tmp- where tmp-path1 is a symlink' ' rm -fr path0 path1 path2 tmp* mkdir tmp1 - ln -s tmp1 tmp-path1 + test_ln_s tmp1 tmp-path1 git checkout-index --prefix=tmp- -f -a test -f tmp-path0 test ! -h tmp-path1 diff --git a/t/t2004-checkout-cache-temp.sh b/t/t2004-checkout-cache-temp.sh index 0f4b289..f171a55 100755 --- a/t/t2004-checkout-cache-temp.sh +++
