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

[Eric Sunshine <sunshine@sunshineco.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.

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

> +# 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
        ...
    )

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