Have init script clone submodules unconditionally

[EliahKagan]

Fixes #1713

This changes the init-tests-after-clone.sh script to always clone submodules, even when it has detected that it is running on CI.

This does not have to be done for the CI in this repository, nor typically in GitHub forks, reuploads, etc. So in 7110bf8 (#1693) I had cloning submodules be one of the things the script avoids doing if it detects it is running on CI, based on the idea that they would already have been cloned. I believe that, in doing this, I inadvertently introduced the bug described in #1713. What I did not think about was how this upstream GitPython repository, its forks, and other repositories that use its CI workflows are not the only repositories in which CI must run. When the tests are run on CI, it is sometimes due to other workflows in other projects.

Downstream projects that package GitPython also usually run the tests on their own CI, using their own workflows that do not necessarily clone submodules. This includes operating systems such as Arch Linux. Some operating systems and other downstream projects that package GitPython probably do clone submodules in their CI workflows. But they should not have to do so--this is a bug in GitPython, because init-tests-after-clone.sh is documented as sufficient to prepare the project for testing after an ordinary clone.

To test that this really addresses #1713, I first modified the workflows to stop automatically pre-cloning the submodules when they clone the repository initially. Keeping it that way would serve as a kind of regression test for #1713, which might justify continuing not to have actions/checkout clone the submodules, so at least for now I have continued to omit that. Because the init script always clones them now, it is no longer necessary for CI workflows--in this repository or in other repositories that run them--to clone them at the time they clone GitPython (or some repository providing it).

(This also brings back the comment from fc96980 that says more about how the tests rely on submodules being present: that they need a submodule with a submodule. But that is not specifically related to the bug being fixed.)

Detailed analysis

See #1713 (comment) for a tour of the relevant parts of the code and a description of the bug in terms of them.

To give some more information about why I am pretty sure I introduced bug #1713 in 7110bf8 (#1693) and it is not due to any of the other changes between 3.1.37 and 3.1.38, consider that all the tests shown as failing in #1713 were tests that relied on submodules being present in the repository being used for testing, and the detailed output showed, in each case, error messages reporting the absence of a file or repository at a path inside ext. In some of the tests it was the direct gitdb submodule whose files were needed, and in others it was the indirect smmap submodule whose files were needed, but all the failing tests showed such errors, including the minority of them whose names did not directly reference submodule functionality.

Furthermore, the tests that failed for building GitPython on Arch Linux were almost the same, and a strict subset of, the tests that (prior to the fix in this PR) fail on GitPython's CI when its test workflows are modified not to automatically pre-clone submodules. The output format shown in #1713 is not exactly the same as we see in this upstream GitPython repository, but comparing which tests fail is simple. The following is a "fantasy" diff, where all of the lines are actually from the summary of failures when submodules: recursive is removed from actions/checkout in this GitPython repository, but the two tests that failed here and not in the Arch Linux build are shown as "added" lines:

 FAILED test/test_docs.py::Tutorials::test_references_and_objects - git.exc.GitCommandError: Cmd('git') failed due to: exit code(128)
 FAILED test/test_docs.py::Tutorials::test_submodules - IndexError: list index out of range
 FAILED test/test_repo.py::TestRepo::test_clone_from_with_path_contains_unicode - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_repo.py::TestRepo::test_submodule_update - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_repo.py::TestRepo::test_submodules - AssertionError: 1 not greater than or equal to 2
 FAILED test/test_submodule.py::TestSubmodule::test_add_clone_multi_options_argument - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_submodule.py::TestSubmodule::test_add_no_clone_multi_options_argument - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_submodule.py::TestSubmodule::test_base_rw - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_submodule.py::TestSubmodule::test_branch_renames - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_submodule.py::TestSubmodule::test_depth - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_submodule.py::TestSubmodule::test_git_submodule_compatibility - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
+FAILED test/test_submodule.py::TestSubmodule::test_git_submodules_and_add_sm_with_new_commit - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
+FAILED test/test_submodule.py::TestSubmodule::test_list_only_valid_submodules - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_submodule.py::TestSubmodule::test_remove_norefs - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_submodule.py::TestSubmodule::test_rename - git.exc.GitCommandError: Cmd('/usr/bin/git') failed due to: exit code(128)
 FAILED test/test_submodule.py::TestSubmodule::test_root_module - AssertionError: assert 1 >= 2
 FAILED test/test_submodule.py::TestSubmodule::test_update_clone_multi_options_argument - git.exc.NoSuchPathError: /home/runner/work/GitPython/GitPython/git/ext/gitdb/gitdb/ext/smmap
 FAILED test/test_submodule.py::TestSubmodule::test_update_no_clone_multi_options_argument - git.exc.NoSuchPathError: /home/runner/work/GitPython/GitPython/git/ext/gitdb/gitdb/ext/smmap

I don't know why those two tests are not shown as failing in the output from the Arch Linux build. However, they are not shown as passing, either; rather, they are not listed. I don't know if this is just a matter of what part of the output was shown, or if the test job was cancelled before they ran, or if they are already disabled in tests for the Arch Linux build for an unrelated reason. Some lines of output are truncated, and it is also possible that with a more careful inspection I might notice the cause. However, I think there is good reason to believe, in spite of this discrepancy, that #1713 is caused by submodules not being cloned in the tests run for that Arch Linux build, and since this began recently, it is probably due to the init script not cloning them on CI. We should expect that this may affect some other downstream projects in a similar way.

It might seem like other submodule-related changes since 3.1.37 could be contributing, but I strongly believe that is not the case. Examining the list of pull requests included in the 3.1.38 release my make it seem like #1659, #1702, #1704, or #1705 could have contributed. But none of these cause the files of either the direct or indirect submodules to be absent as the test output reports. What is more interesting is why #1693, which I claim is the cause, is not listed there at all. #1693 is actually one of the PRs that came between the 3.1.37 and 3.1.38 releases; the reason it is not listed in the auto-generated 3.1.38 changelog (or any other) is that the merge commit that closed it, 4345faa, was not done on GitHub.

Releasing the fix

Assuming this pull request is approved, the problem will still not be fully fixed until a release is made. Even if Arch Linux would tolerate a tag-only "release" or even if it could use a commit later than the latest release--I am not sure--I would expect that some other downstream projects that package GitPython are likely to suffer similar problems unless there is a GitPython release tagged and uploaded to PyPI (matching the tag).

However, the changes here do not affect anything packaged on PyPI, not even in principle, since they affect only init-tests-after-clone.sh and CI workflows. So releasing on PyPI is strange. However, I think this is within the ambit of "post" releases. Unless there is a reason to do otherwise, I suggest making a fully fledged release with this fix, but--if it includes only the changes from this pull request--then having it be 3.1.38.post1 instead of 3.1.39. I think that is reasonable.

This may be a moot point, as there may very well be a reason to do otherwise. If any change is made to any files in the git module, an ordinary 3.1.39 release should be done instead. This PR doesn't include any such change, but the unrelated PR #1714 does. So if that is also merged before the release is made, then it should not be a post release.

[Byron]

Thanks a lot for your help and … ultra detailed analysis. It's definitely one of these "super-human" contributions that I can't explain.

Here is the new release: https://github.com/gitpython-developers/GitPython/releases/tag/3.1.40 - it skipped a version as I forgot to fetch the first time around so 3.1.39 was basically a no-op. Lesson learned: Pypi lets you delete releases, and it's gone from the GUI, but the version can't be reused nonetheless. That's fair, and I would argue that versions should be immutable anyway and be yanked at best.

CC @dvzrv in case the two missing tests from the Detailed analysis portion of the PR are something to look into.