Re: [PATCH v6 09/10] submodule: support reading .gitmodules when it's not in the working tree

On Wed, Oct 10, 2018 at 11:56 AM Antonio Ospite [off-list ref] wrote:

On Mon, 8 Oct 2018 15:19:00 -0700
Stefan Beller [off-list ref] wrote:

quoted

quoted

+test_expect_success 'not writing gitmodules config file when it is not checked out' '
+        test_must_fail git -C super submodule--helper config submodule.submodule.url newurl

This only checks the exit code, do we also want to check for

    test_path_is_missing .gitmodules ?

OK, I agree, let's re-check also *after* we tried and failed to set
a config value, just to be sure that the code does not get accidentally
changed in the future to create the file. I'll add the check.

quoted

quoted

+test_expect_success 'initialising submodule when the gitmodules config is not checked out' '
+       git -C super submodule init
+'
+
+test_expect_success 'showing submodule summary when the gitmodules config is not checked out' '
+       git -C super submodule summary
+'

Same for these, is the exit code enough, or do we want to look at
specific things?

Except for the "summary" test which was not even exercising the
config_from_gitmodule path,  checking exist status should be sufficient
to verify that "submodule--helper config" does not fail, but we can
surely do better.

I will add checks to confirm that not only the commands exited without
errors but they also achieved the desired effect, to validate the actual
high-level use case advertised by the test file. This should be more
future-proof.

And I think I'll merge the summary and the update tests.

quoted

quoted

+
+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"
+       ) &&
+       git -C super submodule update

git status would want to be clean afterwards?

Mmh, this should have been "submodule update --remote" in the first
place to have any effect, I'll take the chance and rewrite this test in
a different way and also check the effect of the update operation, and
the repository status.

I'll be something like this:

ORIG_SUBMODULE=$(git -C submodule rev-parse HEAD)
ORIG_UPSTREAM=$(git -C upstream rev-parse HEAD)
ORIG_SUPER=$(git -C super rev-parse HEAD)

test_expect_success 're-updating submodule when the gitmodules config is not checked out' '
        test_when_finished "git -C submodule reset --hard $ORIG_SUBMODULE;
                            git -C upstream reset --hard $ORIG_UPSTREAM;
                            git -C super reset --hard $ORIG_SUPER;
                            git -C upstream submodule update --remote;
                            git -C super pull;
                            git -C super submodule update --remote" &&
        (cd submodule &&
                echo file2 >file2 &&
                git add file2 &&
                test_tick &&
                git commit -m "add file2 to submodule"
        ) &&
        (cd upstream &&
                git submodule update --remote &&
                git add submodule &&
                test_tick &&
                git commit -m "Update submodule"
        ) &&
        git -C super pull &&
        # The --for-status options reads the gitmdoules config

gitmodules

        git -C super submodule summary --for-status >actual &&
        cat >expect <<-\EOF &&
        * submodule 951c301...a939200 (1):

hardcoding hash values burdens the plan to migrate to another
hash function,

    rev1=$(git -C submodule rev-parse --short HEAD^)
    rev2=$(git -C submodule rev-parse --short HEAD)

and then use ${rev1}..${rev2} ?

          < add file2 to submodule

        EOF
        test_cmp expect actual &&
        # Test that the update actually succeeds
        test_path_is_missing super/submodule/file2 &&
        git -C super submodule update &&
        test_cmp submodule/file2 super/submodule/file2 &&
        git -C super status --short >output &&
        test_must_be_empty output
'

Maybe a little overkill?

Wow, very thorough! You might call it overkill, but now that you have it...

The "upstream" repo will be added in test 1 to better clarify the roles
of the involved repositories.

The commit ids should be stable because of test_tick, shouldn't they?

Yes, but see
Documentation/technical/hash-function-transition.txt
that a couple people are working on. Let's be nice to them. :-)

Stefan