Skip to content

Conversation

leander-dsouza
Copy link
Collaborator

Basic Info

Info Please fill out this column
Ticket(s) this addresses #272
Primary OS tested on Ubuntu
Is this a breaking change? No
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

  • Since submodules have .git as a file instead of a folder, this change accounts for detecting it as a repository to clone into.

Description of how this change was tested

  • Initialise a submodule locally within the repository under a temp folder.

    mkdir temp && cd temp
    git submodule add https://github.com/dirk-thomas/vcstool.git
  • Compare the contents of repo.yaml before and after this change:

    cd vcs2l
    vcs export --nested > repos.yaml
  • Final result of the YAML file:

    repositories:
      vcs2l:
        type: git
        url: [email protected]:ros-infrastructure/vcs2l.git
        version: leander-dsouza/add-submodule-support
      vcs2l/temp/vcstool:
        type: git
        url: https://github.com/dirk-thomas/vcstool.git
        version: daf389377310f7f31b2171f51a79af82c31bf3e2

    The submodule would not have been detected if this change were not present.

Signed-off-by: Leander Stephen Desouza [email protected]

@leander-dsouza leander-dsouza marked this pull request as ready for review August 25, 2025 10:35
Copy link
Contributor

@claraberendsen claraberendsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for porting this over.
I think we should approach submodules by recognizing them by .gitsubmodules instead of checking if the .git is a file or a dir.
Additionally I have questions on how this behaves when doing a vcs pull and vcs import since there is no specific recognition on the output that these are submodules directories.

return cls._git_version

@staticmethod
def is_repository(path):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should conform to recognizing submodules as how they are defined in the parent repository. On git this is by using the .gitmodules file in the parent repository. See https://git-scm.com/docs/gitmodules.

If you check the contents of the .git file that you are recognizing it is a link to the git dir on ../../.git/modules.

With the example from the test of this PR:


cat .git
gitdir: ../../.git/modules/temp/vcstool

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the --nested flag for vcs export, recursively searches all the subdirectories of the parent directory and determines if the directory is a repository by using the is_repository function.

I guess when the --nested flag is active, instead of using recursion, this would be cleaner to determine all the submodules from the .gitmodules file.

@christophebedard
Copy link
Member

christophebedard commented Aug 27, 2025

@leander-dsouza just a thought, but you might want to ping the original author of PRs that you port over. They may have been using a given feature internally and could have interesting feedback to share.

@leander-dsouza
Copy link
Collaborator Author

Additionally I have questions on how this behaves when doing a vcs pull and vcs import since there is no specific recognition on the output that these are submodules directories.

  • vcs pull also uses the same --nested flag to search for submodules to pull from. Therefore, this would rectify the behaviour of this command as well.

  • vcs import, however, uses a different --recursive flag to import submodules, and is unaffected by this change.

@leander-dsouza
Copy link
Collaborator Author

Hello @varunsairam, just a gentle ping on this PR :)

I was wondering if you could take a look at this PR and probably describe an internal use case for the change, as this is a direct port of your contribution over at vcstools.

@leander-dsouza
Copy link
Collaborator Author

@leander-dsouza just a thought, but you might want to ping the original author of PRs that you port over. They may have been using a given feature internally and could have interesting feedback to share.

Thank you for the advice, Christophe :)

@leander-dsouza leander-dsouza marked this pull request as draft August 27, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants