Re: [RFC PATCH v2 10/12] t7416: add new test about HEAD:.gitmodules and not existing .gitmodules

From: Stefan Beller <sbeller@google.com>
Date: Aug 2, 2018, 20:43 UTC
Message-ID: <CAGZ79kbQ0DsAXZrvpp3_2CrMU6Jburf6UdjTxNSd72JqQCczWQ@mail.gmail.com>
In-Reply-To: <20180802134634.10300-11-ao2@ao2.it>

On Thu, Aug 2, 2018 at 6:47 AM Antonio Ospite <ao2@ao2.it> wrote:

Show 11 quoted lines

>
> git submodule commands can now access .gitmodules from the current
> branch even when it's not in the working tree, add some tests for that
> scenario.
>
> Signed-off-by: Antonio Ospite <ao2@ao2.it>
> ---
>
> For the test files I used the most used style in other tests, Stefan suggested
> to avoid subshells and use "git -C" but subshells make the test look cleaner
> IMHO.

Oh well. Let's not dive into a style argument, so let me focus on the tests.

IMHO it would be nice if (at least partially) these tests are in the same patch that added the reading from HEAD:.gitmodules (although I like short patches, too).

Show 26 quoted lines

>
>  t/t7416-submodule-sparse-gitmodules.sh | 112 +++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
>  create mode 100755 t/t7416-submodule-sparse-gitmodules.sh
>
> diff --git a/t/t7416-submodule-sparse-gitmodules.sh b/t/t7416-submodule-sparse-gitmodules.sh
> new file mode 100755
> index 0000000000..3c7a53316b
> --- /dev/null
> +++ b/t/t7416-submodule-sparse-gitmodules.sh
> @@ -0,0 +1,112 @@
> +#!/bin/sh
> +#
> +# Copyright (C) 2018  Antonio Ospite <ao2@ao2.it>
> +#
> +
> +test_description=' Test reading/writing .gitmodules is not in the working tree
> +
> +This test verifies that, when .gitmodules is in the current branch but is not
> +in the working tree reading from it still works but writing to it does not.
> +
> +The test setup uses a sparse checkout, but the same scenario can be set up
> +also by committing .gitmodules and then just removing it from the filesystem.
> +
> +NOTE: "git mv" and "git rm" are still supposed to work even without
> +a .gitmodules file, as stated in the t3600-rm.sh and t7001-mv.sh tests.

"supposed to work" != "tested that it works" ? I am not sure what the NOTE wants to tell me? (Should I review those tests to double check them now? or do we just want to tell future readers of this test there are other tangent tests to this?)

Show 39 quoted lines

> +test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
> +       (cd super &&
> +               git submodule init
> +       )
> +'
> +
> +test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
> +       (cd super &&
> +               git submodule summary
> +       )
> +'
> +
> +test_expect_success 'updating submodule when the gitmodules config is not checked out' '
> +       (cd submodule &&
> +               echo file2 >file2 &&
> +               git add file2 &&
> +               git commit -m "add file2 to submodule"
> +       ) &&
> +       (cd super &&
> +               git submodule update
> +       )
> +'
> +
> +test_expect_success 'not adding submodules when the gitmodules config is not checked out' '
> +       (cd super &&
> +               test_must_fail git submodule add ../new_submodule
> +       )
> +'
> +
> +# "git add" in the test above fails as expected, however it still leaves the
> +# cloned tree in there and adds a config entry to .git/config. This is because
> +# no cleanup is done by cmd_add in git-submodule.sh when "git
> +# submodule--helper config" fails to add a new config setting.
> +#
> +# If we added the following commands to the test above:
> +#
> +#   rm -rf .git/modules/new_submodule &&
> +#   git reset HEAD new_submodule &&
> +#   rm -rf new_submodule

Alternatively we could check for the existence of .gitmodules before starting all these things?

I think it is okay to not clean up if we check all "regular" or rather expected things such as a non-writable .gitmodules file before actually doing it. (This is similar to 'checkout' that walks the whole tree and checks if the checkout is possible given the dirtyness of the tree, to either abort early or pull through completely. In catastrophic problems such as a full disk we'd still die in the middle of work)

Show 6 quoted lines

> +#
> +# then the repository would be in a clean state and the test below would pass.
> +#
> +# Maybe cmd_add should do the cleanup from above itself when failing to add
> +# a submodule.
> +test_expect_failure 'init submodule after adding failed when the gitmodules config is not checked out' '

So this comment and test is about explaining why we can fail mid way through, which we could not before unless we had the catastrophic event.

I think we should check for a "writable" .gitmodules file at the beginning, which is if (G || (!G && !H)) [using the notation from the cover letter]?

Show 9 quoted lines

> +       (cd super &&
> +               git submodule init
> +       )
> +'
> +
> +test_done
> --
> 2.18.0
>