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

On Tue, Mar 5, 2019 at 9:22 AM Rohit Ashiwal <rohit.ashiwal265@gmail.com> wrote:

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/