Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail when a git submodule fails to update instead of ignoring the error #6132

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

kit-ty-kate
Copy link
Member

Fixes #6131

@kit-ty-kate kit-ty-kate added this to the 2.3.0~alpha milestone Jul 31, 2024
@kit-ty-kate kit-ty-kate force-pushed the git-submodule-fail branch 2 times, most recently from cab43a1 to ed71ca0 Compare July 31, 2024 09:22
src/repository/opamGit.ml Outdated Show resolved Hide resolved
Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

On the code itself, lgtm!
Some remarks:

  • The title of the PR doesn't fit the content, the second commit it more submodules handling than the failure itself. It needs to be updated by something more general, like "git submodules enhancements".
  • Add a test to highlight the modifications, and follow them

src/repository/opamGit.ml Outdated Show resolved Hide resolved
src/repository/opamGit.ml Outdated Show resolved Hide resolved
@kit-ty-kate
Copy link
Member Author

I've split off the second commit in #6153 and added a test

Copy link
Collaborator

@rjbou rjbou left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines +12 to +14
### ### This test may fail locally depending on your git version
### ### File protocol is removed locally since git 2.38,
### ### to fix a CVE: https://www.cve.org/CVERecord?id=CVE-2022-39253
Copy link
Collaborator

Choose a reason for hiding this comment

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

I added that note

Copy link
Member Author

Choose a reason for hiding this comment

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

That note isn't quite true. Only a couple of versions have file protocol disabled. More recent git versions work just fine

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah nevermind, what i'm saying is not true. My memory failed me.

@kit-ty-kate kit-ty-kate merged commit 6a212c3 into ocaml:master Aug 20, 2024
29 checks passed
@kit-ty-kate kit-ty-kate deleted the git-submodule-fail branch August 20, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

failure to fetch git submodules are not errors
3 participants