diff mbox series

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

Message ID 20200925170256.11490-7-shubhunic@gmail.com (mailing list archive)
State New, archived
Headers show
Series Modernizing the t7001 test script | expand

Commit Message

Shubham Verma Sept. 25, 2020, 5:02 p.m. UTC 
From: Shubham Verma <shubhunic@gmail.com>

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

Signed-off-by: shubham verma <shubhunic@gmail.com>
---
 t/t7001-mv.sh | 45 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 34 deletions(-)

Comments

Eric Sunshine Sept. 25, 2020, 6:53 p.m. UTC | #1 
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`.
diff mbox series

Patch

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 7581e4b407..3a3ace6d73 100755
--- 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
 '
 
 test_expect_success 'checking the commit' '
@@ -25,12 +24,11 @@  test_expect_success 'checking the commit' '
 '
 
 test_expect_success 'moving the file back into subdirectory' '
-	cd path0 && git mv ../path1/COPYING COPYING
+	git -C path0 mv ../path1/COPYING COPYING
 '
 
-# in path0 currently
 test_expect_success 'commiting the change' '
-	cd .. && git commit -m move-in -a
+	git commit -m move-in -a
 '
 
 test_expect_success 'checking the commit' '
@@ -324,10 +322,7 @@  test_expect_success 'git mv moves a submodule with a .git directory and no .gitm
 	git mv sub mod/sub &&
 	! test -e sub &&
 	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
-	(
-		cd mod/sub &&
-		git status
-	) &&
+	git -C mod/sub status &&
 	git update-index --refresh &&
 	git diff-files --quiet
 '
@@ -347,10 +342,7 @@  test_expect_success 'git mv moves a submodule with a .git directory and .gitmodu
 	git mv sub mod/sub &&
 	! test -e sub &&
 	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
-	(
-		cd mod/sub &&
-		git status
-	) &&
+	git -C mod/sub status &&
 	echo mod/sub >expected &&
 	git config -f .gitmodules submodule.sub.path >actual &&
 	test_cmp expected actual &&
@@ -364,16 +356,10 @@  test_expect_success 'git mv moves a submodule with gitfile' '
 	git submodule update &&
 	entry="$(git ls-files --stage sub | cut -f 1)" &&
 	mkdir mod &&
-	(
-		cd mod &&
-		git mv ../sub/ .
-	) &&
+	git -C mod mv ../sub/ . &&
 	! test -e sub &&
 	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
-	(
-		cd mod/sub &&
-		git status
-	) &&
+	git -C mod/sub status &&
 	echo mod/sub >expected &&
 	git config -f .gitmodules submodule.sub.path >actual &&
 	test_cmp expected actual &&
@@ -392,10 +378,7 @@  test_expect_success 'mv does not complain when no .gitmodules file is found' '
 	test_must_be_empty actual.err &&
 	! test -e sub &&
 	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
-	(
-		cd mod/sub &&
-		git status
-	) &&
+	git -C mod/sub status &&
 	git update-index --refresh &&
 	git diff-files --quiet
 '
@@ -416,10 +399,7 @@  test_expect_success 'mv will error out on a modified .gitmodules file unless sta
 	test_must_be_empty actual.err &&
 	! test -e sub &&
 	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
-	(
-		cd mod/sub &&
-		git status
-	) &&
+	git -C mod/sub status &&
 	git update-index --refresh &&
 	git diff-files --quiet
 '
@@ -437,10 +417,7 @@  test_expect_success 'mv issues a warning when section is not found in .gitmodule
 	test_i18ncmp expect.err actual.err &&
 	! test -e sub &&
 	[ "$entry" = "$(git ls-files --stage mod/sub | cut -f 1)" ] &&
-	(
-		cd mod/sub &&
-		git status
-	) &&
+	git -C mod/sub status &&
 	git update-index --refresh &&
 	git diff-files --quiet
 '