Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
> 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"
