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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Sep 25, 2020 at 1:03 PM shubham verma <shubhunic@gmail.com> wrote:
> t7001: change (cd <path> && git foo) to (git -C <path> foo)

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

> Let's avoid the use of `cd` outside subshells by encapsulating them
> inside subshells or by using `git -C <dir> ...`.

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

> Signed-off-by: shubham verma <shubhunic@gmail.com>
> ---
> diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
> @@ -11,12 +11,11 @@ test_expect_success 'prepare reference tree' '
>  test_expect_success 'moving the file out of subdirectory' '
> -       cd path0 && git mv COPYING ../path1/COPYING
> +       git -C path0 mv COPYING ../path1/COPYING
>  '
>
> -# in path0 currently
>  test_expect_success 'commiting the change' '
> -       cd .. && git commit -m move-out -a
> +       git commit -m move-out -a
>  '

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.

> @@ -364,16 +356,10 @@ test_expect_success 'git mv moves a submodule with gitfile' '
> -       (
> -               cd mod &&
> -               git mv ../sub/ .
> -       ) &&
> +       git -C mod mv ../sub/ . &&

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