Re: [GSoC][PATCH v2 3/3] t3600: use helpers to replace test -d/f/e/s <path>

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Mar 5, 2019 at 9:22 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:
> I was asking if this patch is good enough to be added to the
> existing code? Does this patch look good?

I didn't review the patch with a critical-enough eye to be able to say
that every change maintains fidelity with the original code. As
mentioned in [1]:

    [...] an important reason for limiting the scope of this change
    [...] is to ease the burden on people who review your submission.
    Large patch series tend to tax reviewers heavily, even (and often)
    when repetitive and simple, like replacing `test -d` with
    `test_path_is_dir()`. The shorter and more concise a patch series
    is, the more likely that it will receive quality reviews.

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".

The meaning of that expression can vary depending upon the context. Is
it checking that that path is not a directory (but it is okay if a
plain file exists there)? Or does it merely care about existence
(neither directory nor any other type of entry)? If the latter, then
the transformation is probably correct, however, if the former, then
it likely isn't correct. So, understanding the overall context of the
test is important for judging if a particular change is correct, and
many (volunteer) reviewers simply don't have the time to delve that
deeply to make a proper judgment.

[1]: https://public-inbox.org/git/CAPig+cSZZaCT0G3hysmjn_tNvZmYGp=5cXpZHkdphbWXnONSVQ@mail.gmail.com/