Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path>
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 8, 2019, 05:38 UTC
- Message-ID
- <xmqq7edancws.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <CAPig+cT_YT-1=ymAYiTpjgRQEe8906Y6yyBU=XuP_wbw+ixxiQ@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
Show 20 quoted lines
> This patch, due to its length and repetitive nature, falls under the > category of being tedious to review, which makes it all the more > likely that a reviewer will overlook a problem. > > And, it's not always obvious at a glance that a change is correct. For > instance, taking a look at the final patch band: > > - ! test -d submod && > - ! test -d submod/subsubmod/.git && > + test_path_is_missing submod && > + test_path_is_missing submod/subsubmod/.git && > > Superficially, the transformation seems straightforward. However, that > doesn't mean it maintains fidelity with the original or even means the > same thing. To review this change properly requires understanding the > original intent of "! test -d". > > ... , and > many (volunteer) reviewers simply don't have the time to delve that > deeply to make a proper judgment.
True. The microproject was supposed to be a gentle introduction to and a practice session of the process of modifying, committing, submitting, and responding to reviews. Learning the usual Git contributor workflow, without spending too much community resources like reviewers' time (as opposed to the real "here is my itch; let's improve the system, and please help me doing so").
In any case, I have spent some time with the patch and I think the changes are generaly OK; some (like using "test_path_is_missing foo" instead of "test ! -f foo" after "git rm foo" to ensure the path no longer exists) are improvements.
An unrelated tangent, but what do you think of this patch? In the context of testing "git rm", if foo is a dangling symbolic link, "git rm foo && test_path_is_missing foo" would need something like this to work correctly, I would think.
t/test-lib-functions.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 681c41ba32..dfe0d4aff4 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -603,7 +603,7 @@ test_file_not_empty () { } test_path_is_missing () { - if test -e "$1" + if test -e "$1" || test -L "$1" then echo "Path exists:" ls -ld "$1"