Re: [PATCH 1/3] t7001: reformat to newer style
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 24, 2018, 16:09 UTC
- Message-ID
- <xmqq4leeubah.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <7f77be2b-a084-5b8d-df65-1bb4b9c5da82@gmail.com>
Derrick Stolee <stolee@gmail.com> writes:
Show 5 quoted lines
>> +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.
Show 5 quoted lines
>> +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 ...)".
Show 7 quoted lines
> [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/ >> +'