Re: [PATCH 06/11] t7001: change (cd <path> && git foo) to (git -C <path> foo)

On Fri, Sep 25, 2020 at 1:03 PM shubham verma <shubhunic@gmail.com> wrote:

This is misleading. We don't want the `git -C` form in a subshell, so it shouldn't be enclosed in parentheses. Perhaps write it like this:

    t7001 use `git -C` to avoid `cd` outside of subshells

This is misleading in two ways. First, none of the changes made by this patch add subshell encapsulation. Second, many of the changes drop the subhsell in favor of `git -C`, so describing them as "`cd` outside of subshells" is wrong.

It's also important for the commit message to explain _why_ this change is important when `cd` is used outside of a subshell. A possible rewrite might be:

    t7001: avoid using `cd` outside of subshells

    Avoid using `cd` outside of subshells since, if the test fails,
    there is no guarantee that the current working directory is the
    expected one, which may cause subsequent tests to run in the wrong
    directory.

    While at it, make some other tests more concise by replacing
    simple subshells with `git -C`.

In fact, fixing the cases in which `cd` is used outside of a subshell is much more important than the mere mechanical conversion made to the other tests by replacing a subshell with `git -C`. As such, I'm tempted to suggest splitting this patch into two: one which fixes the cases of `cd` outside of subshell, and another which converts the simple subshell cases to use `git -C`.

This transformation looks fine, as do the following two tests which get the same transformation.

I do have a very slight hesitation, though, that these changes go against the grain of the tests. In particular, at the top of this script, we see:

    test_description='git mv in subdirs'

which suggests that the tests really want to test the bare `git mv` command while actually running in a subdirectory. This would imply that these test should be rewritten as:

    test_expect_success 'title' '
        (
            cd path0 &&
            ...
        )
    '

However, it's such a minor misgiving that it's probably not worth considering.

Okay. At first glance one might expect you to strip the `../` from the argument, but indeed `../sub/` is correct since `-C mod` really does change to the new directory, so the argument is interpreted relative to `mod`.