Re: [PATCH 11/30] subtree: t7900: add porcelain tests for 'pull' and 'push'

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 23, 2021, 20:19 UTC
Message-ID: <CAPig+cRoxCUcVkLch70x+nUzJbTiyT4KtLDsJ0vZNJD1NboyYQ@mail.gmail.com>
In-Reply-To: <20210423194230.1388945-12-lukeshu@lukeshu.com>

On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <lukeshu@lukeshu.com> wrote:

Show 14 quoted lines

> The 'pull' and 'push' subcommands deserve their own sections in the tests.
> Add some basic tests for them.
>
> Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
> ---
> diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
> @@ -202,8 +202,8 @@ test_expect_success 'merge the added subproj again, should do nothing' '
>  test_expect_success 'merge new subproj history into subdir/ with a slash appended to the argument of --prefix' '
> -       test_create_repo "$test_count" &&
> -       test_create_repo "$test_count/subproj" &&
> +       subtree_test_create_repo "$test_count" &&
> +       subtree_test_create_repo "$test_count/subproj" &&
>         test_create_commit "$test_count" main1 &&
>         test_create_commit "$test_count/subproj" sub1 &&

This change doesn't seem to be related to the stated purpose of this patch. Was it included by accident or is it just a drive-by "while at it" fix that seems somewhat related since you're using subtree_test_create_repo() in the newly-added tests? It might deserve mention in the commit message.

Show 11 quoted lines

> @@ -427,6 +427,133 @@ test_expect_success 'split "sub dir"/ with --branch for an incompatible branch'
> +test_expect_success 'pull requires path given by option --prefix must exist' '
> +       test_create_commit "$test_count/sub proj" sub1 &&
> +       (
> +               test_must_fail git subtree pull --prefix="sub dir" ./"sub proj" HEAD >out 2>err &&
> +
> +               echo "'\''sub dir'\'' does not exist; use '\''git subtree add'\''" > expected &&
> +               test_must_be_empty out &&
> +               test_cmp expected err
> +       )
> +'

The use of single-quotes and escaped single-quotes within the single-quoted test body is breaking my brain. Perhaps take advantage of SQ from test-lib.sh and interoplate it into the string rather than dealing with raw single-quotes?

    echo "this $SQ is a single-quote"

(After writing the above, I now see that you are just mirroring existing practice in this test script. The single-quotes are confusing, but following existing style may be important -- or not.)

Show 8 quoted lines

> +test_expect_success 'pull basic operation' '
> +       subtree_test_create_repo "$test_count" &&
> +       subtree_test_create_repo "$test_count/sub proj" &&
> +       test_create_commit "$test_count" main1 &&
> +       test_create_commit "$test_count/sub proj" sub1 &&
> +       (
> +               cd "$test_count" &&
> +               git fetch ./"sub proj" HEAD &&

I was going to comment on the unusual:

    ./"sub proj"

rather than the more typical:

    "./sub proj"

but I see that that also is mirroring existing practice in this script, so... [intentionally left blank]

Show 11 quoted lines

> +test_expect_success 'push requires option --prefix' '
> +       subtree_test_create_repo "$test_count" &&
> +       subtree_test_create_repo "$test_count/sub proj" &&
> +       test_create_commit "$test_count" main1 &&
> +       test_create_commit "$test_count/sub proj" sub1 &&
> +       (
> +               cd "$test_count" &&
> +               git fetch ./"sub proj" HEAD &&
> +               git subtree add --prefix="sub dir" FETCH_HEAD &&
> +               echo "You must provide the --prefix option." > expected &&
> +               test_must_fail git subtree push "./sub proj" from-mainline > actual 2>&1 &&

Style: There is an inconsistent mix of "> foo" and ">foo" formatting
in the newly-added tests. These days, we prefer ">foo".