Mark Mentovai <mark@xxxxxxxxxxxx> writes: > Commit 01aff0a (apply: correctly reverse patch's pre- and post-image > mode bits; 2023-12-26) revised reverse_patches() to maintain the desired > property that when only one of patch::old_mode and patch::new_mode is > set, the mode will be carried in old_mode. That property is generally > correct, with one notable notable exception: when creating a file, only > new_mode will be set. Since reversing a deletion results in a creation, > new_mode must be set in that case. Very well reasoned and clearly explained. > SWAP(p->new_name, p->old_name); > - if (p->new_mode) > + if (p->new_mode || p->is_delete) > SWAP(p->new_mode, p->old_mode); > SWAP(p->is_new, p->is_delete); > SWAP(p->lines_added, p->lines_deleted); > diff --git a/t/t4129-apply-samemode.sh b/t/t4129-apply-samemode.sh > index 2149ad5da44c..036613ad8fed 100755 > --- a/t/t4129-apply-samemode.sh > +++ b/t/t4129-apply-samemode.sh > @@ -124,9 +124,124 @@ test_expect_success 'git apply respects core.fileMode' ' > > git apply patch 2>err && > test_grep ! "has type 100644, expected 100755" err && > + git reset --hard && Interesting. This is taking advantage of the fact that the "apply" above only mucks with the working tree files without disturbing the index entries, so the next "apply --cached" can do its thing cleanly. By adding "reset --hard" here, we lose a bit of test coverage in that "apply --cached" should not care how dirty the working tree files are. Because the main focus of this particular test is, as its title states, what the filemode setting does, it is tempting to add this "reset --hard", but unless we know that we test the same in another test so this loss of test coverage is OK, I'd rather not to see this change. > git apply --cached patch 2>err && > - test_grep ! "has type 100644, expected 100755" err > + test_grep ! "has type 100644, expected 100755" err && > + git reset --hard > +' On the other hand, this "reset --hard" is necessary, as you want to start the next test with a clean slate. > +test_expect_success 'git apply restores file modes' ' > + test_config core.fileMode false && > + echo "This is data, do not execute!" >data.txt && > + git add --chmod=+x data.txt && > + git ls-files -s data.txt >ls-files-output && > + test_grep "^100755" ls-files-output && OK, the file is executable. > + test_tick && git commit -m "Add data" && > + git ls-tree -r HEAD data.txt >ls-tree-output && > + test_grep "^100755" ls-tree-output && Again, the file is executable. > + git checkout -- data.txt && This should be a no-op, right? What are we testing here? > + git add --chmod=-x data.txt && Now we drop the executable bit. > + git ls-files -s data.txt >ls-files-output && > + test_grep "^100644" ls-files-output && > + test_tick && git commit -m "Make data non-executable" && > + git ls-tree -r HEAD data.txt >ls-tree-output && > + test_grep "^100644" ls-tree-output && And do the same. > + git checkout -- data.txt && Ditto. This does not seem to be "we make sure checkout would not barf when asked to checkout data.txt", and it also does not seem to be "now we have made data.txt dirty, we need to check it out of the index". > + git rm data.txt && > + git ls-files -s data.txt >ls-files-output && > + test_must_be_empty ls-files-output && OK. We removed, so it is gone. > + test_tick && git commit -m "Remove data" && > + git ls-tree -r HEAD data.txt >ls-tree-output && > + test_must_be_empty ls-tree-output && Same. At this point, HEAD~0 is "remove data.txt" HEAD~1 is "chmod -x data.txt" HEAD~2 is "create data.txt that is executable" Hence > + git format-patch HEAD~3..HEAD~2 --stdout >patch && Always follow the "command --dashed-options... revisions... other args" ordering, i.e. git format-patch --stdout HEAD~3..HEAD~2 >patch && Anyway, we now have a patch to create an executable data.txt. > + test_grep "^new file mode 100755$" patch && > + git apply --index patch && > + git ls-files -s data.txt >ls-files-output && > + test_grep "^100755" ls-files-output && > + test_tick && git commit -m "Re-add data" && > + git ls-tree -r HEAD data.txt >ls-tree-output && > + test_grep "^100755" ls-tree-output && OK, a patch to create an executable file does create an executable file. > + git format-patch HEAD~3..HEAD~2 --stdout >patch && Now the HEAD~<n> has shifted, this gives us "chmod -x" patch. One suggestion. Perhaps you'd want to grab the necessary set of patches all much earlier, around the point where I said "At ths point" above? With something like git format-patch -o patches -3 HEAD && mv patches/0001-* 0001-create-executable.patch && mv patches/0002-* 0002-chmod-x.patch && mv patches/0003-* 0003-remove.patch && and then consume them using the name we explicitly gave, e.g. git apply --index 0001-create-executable.patch && > + test_grep "^old mode 100755$" patch && > + test_grep "^new mode 100644$" patch && > + git apply --index patch 2>err && > + test_grep ! "has type 100644, expected 100755" err && > + git ls-files -s data.txt >ls-files-output && > + test_grep "^100644" ls-files-output && > + test_tick && git commit -m "Redo data mode change" && > + git ls-tree -r HEAD data.txt >ls-tree-output && > + test_grep "^100644" ls-tree-output && OK. > + git format-patch HEAD~3..HEAD~2 --stdout >patch && This one to remove. > + test_grep "^deleted file mode 100644$" patch && > + git apply --index patch 2>err && > + test_grep ! "has type 100755, expected 100644" err && > + git ls-files -s data.txt >ls-files-output && > + test_must_be_empty ls-files-output && > + test_tick && git commit -m "Redo data removal" && > + git ls-tree -r HEAD tool.sh >ls-tree-output && > + test_must_be_empty ls-tree-output All expected. > +' > + > +test_expect_success 'git apply --reverse restores file modes' ' > + test_config core.fileMode false && > + echo true >tool.sh && I we took the above approach to prepare patches in separate files, we do not have to set up a different scenario completely anew. Instead, we can start from a state where data.txt is missing, and then reverse-apply the remove patch we used in the previous test first (and make sure the mode is without executable bit), then reverse-apply the chmod-x patch (and make sure the file is now executable), and then reverse-apply the creatoin patch (to ensure it is gone). Thanks. > + git add --chmod=-x tool.sh && > + git ls-files -s tool.sh >ls-files-output && > + test_grep "^100644" ls-files-output && > + test_tick && git commit -m "Add tool" && > + git ls-tree -r HEAD tool.sh >ls-tree-output && > + test_grep "^100644" ls-tree-output && > + > + git add --chmod=+x tool.sh && > + git ls-files -s tool.sh >ls-files-output && > + test_grep "^100755" ls-files-output && > + test_tick && git commit -m "Make tool executable" && > + git ls-tree -r HEAD tool.sh >ls-tree-output && > + test_grep "^100755" ls-tree-output && > + git checkout -- tool.sh && > + > + git rm tool.sh && > + git ls-files -s tool.sh >ls-files-output && > + test_must_be_empty ls-files-output && > + test_tick && git commit -m "Remove tool" && > + git ls-tree -r HEAD tool.sh >ls-tree-output && > + test_must_be_empty ls-tree-output && > + > + git format-patch -1 --stdout >patch && > + test_grep "^deleted file mode 100755$" patch && > + git apply --index --reverse patch && > + git ls-files -s tool.sh >ls-files-output && > + test_grep "^100755" ls-files-output && > + test_tick && git commit -m "Undo tool removal" && > + git ls-tree -r HEAD tool.sh >ls-tree-output && > + test_grep "^100755" ls-tree-output && > + > + git format-patch HEAD~3..HEAD~2 --stdout >patch && > + test_grep "^old mode 100644$" patch && > + test_grep "^new mode 100755$" patch && > + git apply --index --reverse patch 2>err && > + test_grep ! "has type 100644, expected 100755" err && > + git ls-files -s tool.sh >ls-files-output && > + test_grep "^100644" ls-files-output && > + test_tick && git commit -m "Undo tool mode change" && > + git ls-tree -r HEAD tool.sh >ls-tree-output && > + test_grep "^100644" ls-tree-output && > + > + git format-patch HEAD~5..HEAD~4 --stdout >patch && > + test_grep "^new file mode 100644$" patch && > + git apply --index --reverse patch 2>err && > + test_grep ! "has type 100755, expected 100644" err && > + git ls-files -s tool.sh >ls-files-output && > + test_must_be_empty ls-files-output && > + test_tick && git commit -m "Undo tool addition" && > + git ls-tree -r HEAD tool.sh >ls-tree-output && > + test_must_be_empty ls-tree-output > ' > > test_expect_success POSIXPERM 'patch mode for new file is canonicalized' '
