Re: [PATCH (Apple Git) 03/13] t0500: New regression test for git add of a path that contains a .git directory

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 30, 2019, 11:47 UTC
Message-ID: <CAPig+cR_p9ybUjfSqY_Kod39Ztxt9Y7Js=QvFU-WDJh58sb5Yg@mail.gmail.com>
In-Reply-To: <20190129193818.8645-4-jeremyhu@apple.com>

On Tue, Jan 29, 2019 at 4:19 PM Jeremy Huddleston Sequoia <jeremyhu@apple.com> wrote:

> Subject: t0500: New regression test for git add of a path that contains a .git directory

Please describe the actual problem here in the commit message so readers of this change can understand what this is all about.

Show 10 quoted lines

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---
> diff --git a/t/t0500-apple.sh b/t/t0500-apple.sh
> @@ -0,0 +1,40 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2012-2016 Apple Inc.
> +#
> +# Tests for regressions found by Apple Inc. for issues that upstream does not
> +# want to fix or accept tests for.

This is an odd comment for a patch which is intended to be upstreamed.

> +test_description='Apple Inc. specific tests'

Is this script actually specific to Apple? If not, a better description is likely warranted. Alternatively, place this new test in an appropriate existing test script.

> +# <rdar://problem/10238070>

Inaccessible private bug report. Please describe the actual regression here.

Show 5 quoted lines

> +# This test case addresses a regression introduced between v1.7.3 and v1.7.5
> +# git bisect good v1.7.3
> +# git bisect bad v1.7.5
> +# ...
> +# found 18e051a3981f38db08521bb61ccf7e4571335353

This commentary isn't very useful going forward, thus not worth having in the script itself, although it may make useful information for the commit message (though more likely not). Usually, such commentary would be placed below the "---" line just under your sign-off.

> +test_expect_success '<rdar://problem/10238070> -- git add of a path that contains a .git directory' '

As above, a better title would be welcome, one which actually means something to people without access to the private bug report.

> +       rm -rf .git &&
> +       mkdir -p orig/sub/dir/otherdir &&
> +       cd orig/sub &&

We don't 'cd' around inside tests without ensuring that the 'cd' is undone automatically even if the test fails. (See below.)

> +       echo "1" > dir/file &&
> +       echo "2" > dir/otherdir/file &&
> +       git init --quiet &&

Why --quiet? Output generated by commands is already suppressed by default when the test is run normally, but it is useful to have when something goes wrong, so we don't usually want to suppress it manually. Same comment applies to >/dev/null redirects.

> +       git add -A &&
> +       git commit -m "Initial Commit" --quiet &&
> +       cd - > /dev/null &&

If something fails above this point, then this "cd -" will never execute, so any tests which get added below this one in the script will operate in the wrong directory. The normal way to 'cd' within a test is within a subshell so the 'cd' is undone automatically whether the test fails or not:

    (
        cd orig/sub
        ...
    )

Show 6 quoted lines

> +       git init --bare --quiet "${TESTROOT}/git_dir.git" &&
> +       git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/sub/" &&
> +       git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ add -f -- "${TESTROOT}/orig/" &&
> +       git --git-dir="${TESTROOT}/git_dir.git" --work-tree=/ commit -m "Commit." |
> +               grep -q "2 files changed, 2 insertions"
> +'

We don't normally place a Git command upstream of a pipe since its exit status will get swallowed by the pipe, thus potentially losing important information. Instead, redirect the command output to a file and 'grep' on the file.

Also, the string you're grepping is likely to be localized, so use test_i18ngrep() instead.