Re: [PATCH] submodules: fix of regression on fetching of non-init subsub-repo

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 4, 2020, 18:06 UTC
Message-ID: <CAPig+cR69HJefRMfH_5-dHOMVY-VmVgbqQuWV90ednDEjrnExw@mail.gmail.com>
In-Reply-To: <1607095412-40109-1-git-send-email-peter.kaestle@nokia.com>

On Fri, Dec 4, 2020 at 10:25 AM Peter Kaestle <peter.kaestle@nokia.com> wrote:

> [...]
> Furthermore a regression test case is added, which tests for recursive
> fetches on a superproject with uninitialized sub repositories.  This
> issue was leading to an infinite loop when doing a revert of a62387b.

Just a few small comments (nothing comprehensive) from a quick scan of the patch...

Mostly they are just minor style issues, not necessarily worth a re-roll, but there is one actionable item.

Show 8 quoted lines

> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>
> ---
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> @@ -719,4 +719,98 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup
> +add_commit_push () {
> +       dir="$1"
> +       msg="$2"
> +       shift 2

We typically recommend including these assignments in the &&-chain to future-proof against someone later inserting code above them and not realizing that that code is not part of the &&-chain, in which case if the new code fails, the failure might go unnoticed.

Show 16 quoted lines

> +       git -C "$dir" add "$@" &&
> +       git -C "$dir" commit -a -m "$msg" &&
> +       git -C "$dir" push
> +}
> +
> +compare_refs_in_dir () {
> +       fail= &&
> +       if test "x$1" = 'x!'
> +       then
> +               fail='!' &&
> +               shift
> +       fi &&
> +       git -C "$1" rev-parse --verify "$2" >expect &&
> +       git -C "$3" rev-parse --verify "$4" >actual &&
> +       eval $fail test_cmp expect actual
> +}

We have a test_cmp_rev() similar to this but it doesn't support -C as some of our other test functions do. I briefly wondered if it would make sense to extend it to understand -C, but even that wouldn't help this case since compare_refs_in_dir() introduced here involves two distinct directories. The need here is so special-purpose that it likely would not make sense to upgrade test_cmp_rev() to accommodate it. Okay.

Show 12 quoted lines

> +test_expect_success 'setup nested submodule fetch test' '
> +       # does not depend on any previous test setups
> +
> +       for repo in outer middle inner
> +       do
> +               (
> +                       git init --bare $repo &&
> +                       git clone $repo ${repo}_content &&
> +                       echo "$repo" >"${repo}_content/file" &&
> +                       add_commit_push ${repo}_content "initial" file
> +               ) || return 1
> +       done &&

What is the purpose of the subshell here? Is it to ensure that commits in each repo have identical timestamps? Or is it just for making the && and || expression more clear? If the latter, we normally don't bother with the parentheses.

Show 14 quoted lines

> +       git clone outer A &&
> +       git -C A submodule add "$pwd/middle" &&
> +       git -C A/middle/ submodule add "$pwd/inner" &&
> +       add_commit_push A/middle/ "adding inner sub" .gitmodules inner &&
> +       add_commit_push A/ "adding middle sub" .gitmodules middle &&
> +
> +       git clone outer B &&
> +       git -C B/ submodule update --init middle &&
> +
> +       compare_refs_in_dir A HEAD B HEAD &&
> +       compare_refs_in_dir A/middle HEAD B/middle HEAD &&
> +       test -f B/file &&
> +       test -f B/middle/file &&
> +       ! test -f B/middle/inner/file &&

These days we typically use test_path_exists() (or test_path_is_file()) and test_path_is_missing() rather than bare `test`.

Show 7 quoted lines

> +test_expect_success 'setup recursive fetch with uninit submodule' '
> +       # does not depend on any previous test setups
> +
> +       git init main &&
> +       git init sub &&
> +
> +       touch sub/file &&

Unless the timestamp of the file is significant to the test, in which case `touch` is used, we normally create empty files like this:

    >sub/file &&

> +test_expect_success 'recursive fetch with uninit submodule' '
> +       git -C main submodule deinit -f sub &&
> +       ! git -C main fetch --recurse-submodules |&
> +               grep -v -m1 "Fetching submodule sub$" &&

We want the test scripts to be portable, thus avoid Bashisms such as `|&`.

We also avoid placing a Git command upstream in a pipe since doing so causes the exit code of the Git command to be lost. Instead, we would normally send the Git output to a file and then send that file to whatever would be downstream of the Git command in the pipe. So, a mechanical rewrite of the above (without thinking too hard about it) might be:

    git -C main fetch --recurse-submodules >out 2>&1 &&
    ! grep -v -m1 "Fetching submodule sub$" &&

> +       git -C main submodule status |
> +               sed -e "s/^-//" -e "s/ sub$//" >actual &&

Same comment about avoiding Git upstream in a pipe, so perhaps:

    git -C main submodule status >out &&
    sed -e "s/^-//" -e "s/ sub$//" out >actual &&

> +       test_cmp expect actual
> +'