| Message ID | 20180921235833.99045-2-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bring some tests to newer style. | expand |
On 9/21/2018 7:58 PM, Stefan Beller wrote: > The old formatting style is a real hindrance of getting people up to speed > contributing as they use existing code as an example and follow that style. > So let's get rid of the old style and reformat it in our current style. I was suspicious of your automated approach catching everything, so I looked carefully at this patch. There are still a lot of things happening that we would not recommend doing in new tests. > > Reported-by: Derrick Stolee <stolee@gmail.com> > Reported-by: Jeff Hostetler <git@jeffhostetler.com> > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > t/t7001-mv.sh | 268 +++++++++++++++++++++++++------------------------- > 1 file changed, 134 insertions(+), 134 deletions(-) > > diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh > index 36b50d0b4c1..2251d24735c 100755 > --- a/t/t7001-mv.sh > +++ b/t/t7001-mv.sh > @@ -3,74 +3,74 @@ > test_description='git mv in subdirs' > . ./test-lib.sh > > -test_expect_success \ > - 'prepare reference tree' \ > - 'mkdir path0 path1 && > - cp "$TEST_DIRECTORY"/../COPYING path0/COPYING && > - git add path0/COPYING && > - git commit -m add -a' > +test_expect_success 'prepare reference tree' ' > + mkdir path0 path1 && > + cp "$TEST_DIRECTORY"/../COPYING path0/COPYING && > + git add path0/COPYING && > + git commit -m add -a > +' > > -test_expect_success \ > - 'moving the file out of subdirectory' \ > - 'cd path0 && git mv COPYING ../path1/COPYING' > +test_expect_success 'moving the file out of subdirectory' ' > + cd path0 && git mv COPYING ../path1/COPYING > +' Perhaps split this line on the &&? > > # in path0 currently > -test_expect_success \ > - 'commiting the change' \ > - 'cd .. && git commit -m move-out -a' > +test_expect_success 'commiting the change' ' > + cd .. && git commit -m move-out -a > +' This "cd .." should probably be removed and use a subshell in the test above where we "cd path0". > > -test_expect_success \ > - 'checking the commit' \ > - 'git diff-tree -r -M --name-status HEAD^ HEAD >actual && > - grep "^R100..*path0/COPYING..*path1/COPYING" actual' > +test_expect_success 'checking the commit' ' > + git diff-tree -r -M --name-status HEAD^ HEAD >actual && > + grep "^R100..*path0/COPYING..*path1/COPYING" actual > +' > > -test_expect_success \ > - 'moving the file back into subdirectory' \ > - 'cd path0 && git mv ../path1/COPYING COPYING' > +test_expect_success 'moving the file back into subdirectory' ' > + cd path0 && git mv ../path1/COPYING COPYING > +' Split at &&, use subshell? > +test_expect_success 'commiting the change' ' > + cd .. && git commit -m move-in -a > +' Drop "cd .." (and the comments about being in path0) [big snip] > +test_expect_success 'moving to existing tracked target with trailing slash' ' > + mkdir path2 && > + >path2/file && git add path2/file && This line in particular looks a bit strange. What is this doing? At least we should split the &&. > + git mv path1/path0/ path2/ && > + test_path_is_dir path2/path0/ > +' > + > +test_expect_success 'clean up' ' > + git reset --hard > +' > + > +test_expect_success 'adding another file' ' > + cp "$TEST_DIRECTORY"/../README.md path0/README && > + git add path0/README && > + git commit -m add2 -a > +' > + > +test_expect_success 'moving whole subdirectory' ' > + git mv path0 path2 > +' > + > +test_expect_success 'commiting the change' ' > + git commit -m dir-move -a > +' > + > +test_expect_success 'checking the commit' ' > + git diff-tree -r -M --name-status HEAD^ HEAD >actual && > + grep "^R100..*path0/COPYING..*path2/COPYING" actual && > + grep "^R100..*path0/README..*path2/README" actual > +' > + > +test_expect_success 'succeed when source is a prefix of destination' ' > + git mv path2/COPYING path2/COPYING-renamed > +' > + > +test_expect_success 'moving whole subdirectory into subdirectory' ' > + git mv path2 path1 > +' > + > +test_expect_success 'commiting the change' ' > + git commit -m dir-move -a > +' > + > +test_expect_success 'checking the commit' ' > + git diff-tree -r -M --name-status HEAD^ HEAD >actual && > + grep "^R100..*path2/COPYING..*path1/path2/COPYING" actual && > + grep "^R100..*path2/README..*path1/path2/README" actual > +' > + > +test_expect_success 'do not move directory over existing directory' ' > + mkdir path0 && mkdir path0/path2 && test_must_fail git mv path2 path0 > +' Split this line. Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: >> +test_expect_success 'moving the file back into subdirectory' ' >> + cd path0 && git mv ../path1/COPYING COPYING >> +' > > Split at &&, use subshell? Yes, I was almost going to point out the same, saying "'reformat to newer style' is much larger than only changing how the test body is formatted", but was debating myself, as a good "modernization patch" needs both mechanical changes and manual/semantic clean-ups, and it is very clear that these three patches deliberately limit themselves to the former for easier verification. It is relatively rare that files are not touched by any in-flight topic in the codebase, which is a good opportunity to apply this kind of wholesale clean-up, so I tend to agree that it is a shame not to do the non-mechanical clean up in the same series. Perhaps the best way would be to keep these three mechanical steps as they are, and then follow-up with non-mechanical clean-up like you suggested. >> +test_expect_success 'commiting the change' ' >> + cd .. && git commit -m move-in -a >> +' > > Drop "cd .." (and the comments about being in path0) ... when the previous step moves to "git -C path0 mv ..." or preferrably "(cd path0 && git mv ...)". > [big snip] > >> +test_expect_success 'moving to existing tracked target with trailing slash' ' >> + mkdir path2 && >> + >path2/file && git add path2/file && > This line in particular looks a bit strange. What is this doing? At > least we should split the &&. Create an empty file by redirecting the output from a no-op into it, and then adding the result. I agree with you that this would be easier to read on two lines. >> + git mv path1/path0/ path2/ && >> + test_path_is_dir path2/path0/ >> +'
On Mon, Sep 24, 2018 at 6:31 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 9/21/2018 7:58 PM, Stefan Beller wrote: > > The old formatting style is a real hindrance of getting people up to speed > > contributing as they use existing code as an example and follow that style. > > So let's get rid of the old style and reformat it in our current style. > I was suspicious of your automated approach catching everything, so I > looked carefully at this patch. There are still a lot of things > happening that we would not recommend doing in new tests. Heh, thanks for calling that out. So we're looking at a full formatter instead of a partial formatter that helps moving in the right direction now. :-/ I would prefer to use automation as much as possible for these tasks to keep it easy to apply it at scale once a file is not touched by master..pu any more. When applying styles manually, there is sometimes a judgement call, which would like to the inevitable bikeshedding that I'd prefer to avoid. > > +test_expect_success 'moving the file out of subdirectory' ' > > + cd path0 && git mv COPYING ../path1/COPYING > > +' > Perhaps split this line on the &&? In real modern testing, this could also be git -C path0 mv ... which would also fix the cd.. below and not needing a subshell there either (using -C again). Looking at this from a higher level, nowadays I would write tests that have more lines in them, instead of having one git command per test. > > +test_expect_success 'moving to existing tracked target with trailing slash' ' > > + mkdir path2 && > > + >path2/file && git add path2/file && > This line in particular looks a bit strange. What is this doing? At > least we should split the &&. Yes. > > +test_expect_success 'do not move directory over existing directory' ' > > + mkdir path0 && mkdir path0/path2 && test_must_fail git mv path2 path0 > > +' > > Split this line. Okay, I'll go manually over these tests to adapt to new style. Thanks for looking over the patch! Stefan
Stefan Beller <sbeller@google.com> writes: > Heh, thanks for calling that out. So we're looking at a full formatter > instead of a partial formatter that helps moving in the right direction now. :-/ The parts left out of these patches (e.g. use subshell when working in a subdirectory) need human decision and a bit of good taste, and I think it was a good design decision to keep these three patches all mechanical. There do need a follow-up [PATCH {4,5,6}/3] that cannot be done mechanically, though, before the result can be called "this script has been cleaned up and new people are encouraged to mimick the way it does things". That way, we can keep the mechanically good bits that are early in the series while polishing the later bits that need human judgement.
diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 36b50d0b4c1..2251d24735c 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -3,74 +3,74 @@ test_description='git mv in subdirs' . ./test-lib.sh -test_expect_success \ - 'prepare reference tree' \ - 'mkdir path0 path1 && - cp "$TEST_DIRECTORY"/../COPYING path0/COPYING && - git add path0/COPYING && - git commit -m add -a' +test_expect_success 'prepare reference tree' ' + mkdir path0 path1 && + cp "$TEST_DIRECTORY"/../COPYING path0/COPYING && + git add path0/COPYING && + git commit -m add -a +' -test_expect_success \ - 'moving the file out of subdirectory' \ - 'cd path0 && git mv COPYING ../path1/COPYING' +test_expect_success 'moving the file out of subdirectory' ' + cd path0 && git mv COPYING ../path1/COPYING +' # in path0 currently -test_expect_success \ - 'commiting the change' \ - 'cd .. && git commit -m move-out -a' +test_expect_success 'commiting the change' ' + cd .. && git commit -m move-out -a +' -test_expect_success \ - 'checking the commit' \ - 'git diff-tree -r -M --name-status HEAD^ HEAD >actual && - grep "^R100..*path0/COPYING..*path1/COPYING" actual' +test_expect_success 'checking the commit' ' + git diff-tree -r -M --name-status HEAD^ HEAD >actual && + grep "^R100..*path0/COPYING..*path1/COPYING" actual +' -test_expect_success \ - 'moving the file back into subdirectory' \ - 'cd path0 && git mv ../path1/COPYING COPYING' +test_expect_success 'moving the file back into subdirectory' ' + cd path0 && git mv ../path1/COPYING COPYING +' # in path0 currently -test_expect_success \ - 'commiting the change' \ - 'cd .. && git commit -m move-in -a' - -test_expect_success \ - 'checking the commit' \ - 'git diff-tree -r -M --name-status HEAD^ HEAD >actual && - grep "^R100..*path1/COPYING..*path0/COPYING" actual' - -test_expect_success \ - 'mv --dry-run does not move file' \ - 'git mv -n path0/COPYING MOVED && - test -f path0/COPYING && - test ! -f MOVED' - -test_expect_success \ - 'checking -k on non-existing file' \ - 'git mv -k idontexist path0' - -test_expect_success \ - 'checking -k on untracked file' \ - 'touch untracked1 && - git mv -k untracked1 path0 && - test -f untracked1 && - test ! -f path0/untracked1' - -test_expect_success \ - 'checking -k on multiple untracked files' \ - 'touch untracked2 && - git mv -k untracked1 untracked2 path0 && - test -f untracked1 && - test -f untracked2 && - test ! -f path0/untracked1 && - test ! -f path0/untracked2' - -test_expect_success \ - 'checking -f on untracked file with existing target' \ - 'touch path0/untracked1 && - test_must_fail git mv -f untracked1 path0 && - test ! -f .git/index.lock && - test -f untracked1 && - test -f path0/untracked1' +test_expect_success 'commiting the change' ' + cd .. && git commit -m move-in -a +' + +test_expect_success 'checking the commit' ' + git diff-tree -r -M --name-status HEAD^ HEAD >actual && + grep "^R100..*path1/COPYING..*path0/COPYING" actual +' + +test_expect_success 'mv --dry-run does not move file' ' + git mv -n path0/COPYING MOVED && + test -f path0/COPYING && + test ! -f MOVED +' + +test_expect_success 'checking -k on non-existing file' ' + git mv -k idontexist path0 +' + +test_expect_success 'checking -k on untracked file' ' + touch untracked1 && + git mv -k untracked1 path0 && + test -f untracked1 && + test ! -f path0/untracked1 +' + +test_expect_success 'checking -k on multiple untracked files' ' + touch untracked2 && + git mv -k untracked1 untracked2 path0 && + test -f untracked1 && + test -f untracked2 && + test ! -f path0/untracked1 && + test ! -f path0/untracked2 +' + +test_expect_success 'checking -f on untracked file with existing target' ' + touch path0/untracked1 && + test_must_fail git mv -f untracked1 path0 && + test ! -f .git/index.lock && + test -f untracked1 && + test -f path0/untracked1 +' # clean up the mess in case bad things happen rm -f idontexist untracked1 untracked2 \ @@ -78,79 +78,79 @@ rm -f idontexist untracked1 untracked2 \ .git/index.lock rmdir path1 -test_expect_success \ - 'moving to absent target with trailing slash' \ - 'test_must_fail git mv path0/COPYING no-such-dir/ && - test_must_fail git mv path0/COPYING no-such-dir// && - git mv path0/ no-such-dir/ && - test_path_is_dir no-such-dir' - -test_expect_success \ - 'clean up' \ - 'git reset --hard' - -test_expect_success \ - 'moving to existing untracked target with trailing slash' \ - 'mkdir path1 && - git mv path0/ path1/ && - test_path_is_dir path1/path0/' - -test_expect_success \ - 'moving to existing tracked target with trailing slash' \ - 'mkdir path2 && - >path2/file && git add path2/file && - git mv path1/path0/ path2/ && - test_path_is_dir path2/path0/' - -test_expect_success \ - 'clean up' \ - 'git reset --hard' - -test_expect_success \ - 'adding another file' \ - 'cp "$TEST_DIRECTORY"/../README.md path0/README && - git add path0/README && - git commit -m add2 -a' - -test_expect_success \ - 'moving whole subdirectory' \ - 'git mv path0 path2' - -test_expect_success \ - 'commiting the change' \ - 'git commit -m dir-move -a' - -test_expect_success \ - 'checking the commit' \ - 'git diff-tree -r -M --name-status HEAD^ HEAD >actual && - grep "^R100..*path0/COPYING..*path2/COPYING" actual && - grep "^R100..*path0/README..*path2/README" actual' - -test_expect_success \ - 'succeed when source is a prefix of destination' \ - 'git mv path2/COPYING path2/COPYING-renamed' - -test_expect_success \ - 'moving whole subdirectory into subdirectory' \ - 'git mv path2 path1' - -test_expect_success \ - 'commiting the change' \ - 'git commit -m dir-move -a' - -test_expect_success \ - 'checking the commit' \ - 'git diff-tree -r -M --name-status HEAD^ HEAD >actual && - grep "^R100..*path2/COPYING..*path1/path2/COPYING" actual && - grep "^R100..*path2/README..*path1/path2/README" actual' - -test_expect_success \ - 'do not move directory over existing directory' \ - 'mkdir path0 && mkdir path0/path2 && test_must_fail git mv path2 path0' - -test_expect_success \ - 'move into "."' \ - 'git mv path1/path2/ .' +test_expect_success 'moving to absent target with trailing slash' ' + test_must_fail git mv path0/COPYING no-such-dir/ && + test_must_fail git mv path0/COPYING no-such-dir// && + git mv path0/ no-such-dir/ && + test_path_is_dir no-such-dir +' + +test_expect_success 'clean up' ' + git reset --hard +' + +test_expect_success 'moving to existing untracked target with trailing slash' ' + mkdir path1 && + git mv path0/ path1/ && + test_path_is_dir path1/path0/ +' + +test_expect_success 'moving to existing tracked target with trailing slash' ' + mkdir path2 && + >path2/file && git add path2/file && + git mv path1/path0/ path2/ && + test_path_is_dir path2/path0/ +' + +test_expect_success 'clean up' ' + git reset --hard +' + +test_expect_success 'adding another file' ' + cp "$TEST_DIRECTORY"/../README.md path0/README && + git add path0/README && + git commit -m add2 -a +' + +test_expect_success 'moving whole subdirectory' ' + git mv path0 path2 +' + +test_expect_success 'commiting the change' ' + git commit -m dir-move -a +' + +test_expect_success 'checking the commit' ' + git diff-tree -r -M --name-status HEAD^ HEAD >actual && + grep "^R100..*path0/COPYING..*path2/COPYING" actual && + grep "^R100..*path0/README..*path2/README" actual +' + +test_expect_success 'succeed when source is a prefix of destination' ' + git mv path2/COPYING path2/COPYING-renamed +' + +test_expect_success 'moving whole subdirectory into subdirectory' ' + git mv path2 path1 +' + +test_expect_success 'commiting the change' ' + git commit -m dir-move -a +' + +test_expect_success 'checking the commit' ' + git diff-tree -r -M --name-status HEAD^ HEAD >actual && + grep "^R100..*path2/COPYING..*path1/path2/COPYING" actual && + grep "^R100..*path2/README..*path1/path2/README" actual +' + +test_expect_success 'do not move directory over existing directory' ' + mkdir path0 && mkdir path0/path2 && test_must_fail git mv path2 path0 +' + +test_expect_success 'move into "."' ' + git mv path1/path2/ . +' test_expect_success "Michael Cassar's test case" ' rm -fr .git papers partA &&
The old formatting style is a real hindrance of getting people up to speed contributing as they use existing code as an example and follow that style. So let's get rid of the old style and reformat it in our current style. Reported-by: Derrick Stolee <stolee@gmail.com> Reported-by: Jeff Hostetler <git@jeffhostetler.com> Signed-off-by: Stefan Beller <sbeller@google.com> --- t/t7001-mv.sh | 268 +++++++++++++++++++++++++------------------------- 1 file changed, 134 insertions(+), 134 deletions(-)