Re: [PATCH 1/1] Test case for checkout of added/deleted submodules in clones

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Sep 22, 2020, 09:35 UTC
Message-ID: <d1b6df73-e945-2ccf-129c-62add58e5747@gmail.com>
In-Reply-To: <20200921081537.15300-2-luke@diamand.org>

On 21-09-2020 13:45, Luke Diamand wrote:

Show 16 quoted lines

> +
> +# Checkout the OLD tag inside the original repo. This works fine since all of
> +# the submodules are present in .git/modules.
> +test_expect_success 'checkout old inside original repo' '
> +	(cd super &&
> +		git config advice.detachedHead false &&
> +		git tag LATEST &&
> +		git checkout --recurse-submodules OLD &&
> +		git submodule update --checkout --remote --force &&
> +		test_path_is_file old/commit_old.t &&
> +		test_path_is_missing new/commit_new.t &&
> +		git checkout --recurse-submodules LATEST &&
> +		test_path_is_file new/commit_new.t
> +	)
> +'
> +

Philippe pointed out the reason behind this behavriour [1] for this in another thread.

Show 54 quoted lines

> +# Clone the repo, and then checkout the OLD tag inside the clone.
> +# The `old` submodule does not get updated. Instead we get:
> +#
> +# fatal: not a git repository: ../.git/modules/old
> +# fatal: could not reset submodule index
> +#
> +# That's because `old` is missing from .git/modules since it
> +# was not cloned originally and `checkout` does not know how to
> +# fetch the remote submodules, whereas `submodule update --remote` does.
> +
> +test_expect_failure 'checkout old with --recurse-submodules' '
> +	test_when_finished "rm -fr super-clone" &&
> +	git clone --recurse-submodules super super-clone &&
> +	(cd super-clone &&
> +		git config advice.detachedHead false &&
> +		test_path_is_file commit3.t &&
> +		test_path_is_file commit2.t &&
> +		test_path_is_missing old &&
> +		test_path_is_file new/commit_new.t &&
> +		git checkout --recurse-submodules OLD &&
> +		git submodule update --checkout --remote --force &&
> +		test_path_is_file commit2.t &&
> +		test_path_is_missing commit3.t &&
> +		test_path_is_dir old &&
> +		test_path_is_file old/commit_old.t
> +	)
> +'
> +
> +# As above, but this time, instead of using "checkout --recurse-submodules" we just
> +# use "checkout" to avoid the missing submodule error.
> +#
> +# The checkout of `old` now works fine, but instead `new` is left lying
> +# around with seemingly no way to clean it up. Even a later invocation of
> +# `git checkout --recurse-submodules` does not get rid of it.
> +
> +test_expect_failure 'checkout old without --recurse-submodules' '
> +	test_when_finished "rm -fr super-clone" &&
> +	git clone --recurse-submodules super super-clone &&
> +	(cd super-clone &&
> +		git config advice.detachedHead false &&
> +		test_path_is_file new/commit_new.t &&
> +		git checkout OLD &&
> +		git submodule update --checkout --remote --force &&
> +		git checkout --recurse-submodules OLD &&
> +		test_path_is_file commit2.t &&
> +		test_path_is_missing commit3.t &&
> +		test_path_is_dir old &&
> +		test_path_is_file old/commit_old.t &&
> +		test_path_is_missing new/commit_new.t
> +	)
> +'
> +
> +test_done
>

I think this isn't the complete story. The submodule removal is done properly if `git checkout --recurse-submodules` is called when the revision is not checked out (modulo the checkout failing issue detailed in the previous test case). For some reason, it doesn't work when the revisions is already checked out. i.e., the following passes

  test_expect_success 'checkout old without --recurse-submodules' '
        test_when_finished "rm -fr super-clone" &&
        git clone --recurse-submodules super super-clone &&
        (cd super-clone &&
                git config advice.detachedHead false &&
                test_path_is_file new/commit_new.t &&
                git checkout OLD &&
                git submodule update --checkout --remote --force &&
                git checkout --recurse-submodules LATEST &&
                test_path_is_file commit2.t &&
                test_path_is_file commit3.t &&
                test_path_is_missing old &&
                test_path_is_missing old/commit_old.t &&
                test_path_is_file new/commit_new.t
        )
  '

Also, this is the exact case that seems to be explained in commit bbad9f9314 (rm: better document side effects when removing a submodule, 2014-01-07) which adds the following BUGS part to the documentation.

    BUGS
    ----
    Each time a superproject update removes a populated submodule
    e.g. when switching between commits before and after the removal) a
    stale submodule checkout will remain in the old location. Removing
    the old directory is only safe when it uses a gitfile, as otherwise
    the history of the submodule will be deleted too. This step will be
    obsolete when recursive submodule update has been implemented.

As Phillipe points out in [2], I do wonder if this part has now become stale.

[ References ]

[1]: https://lore.kernel.org/git/20200501005432.h62dnpkx7feb7rto@glandium.org/T/#u

[2]: https://public-inbox.org/git/0B191753-C1AD-499C-B8B2-122F49CF6F14@gmail.com/

-- 
Sivaraam