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

git: Fix submodule fetch, rewrite logic #104

Merged
merged 1 commit into from
Jun 16, 2024
Merged

Conversation

ReillyBrogan
Copy link
Contributor

The logic for this was quite broken and had several failure cases due to that. This commit re-writes that to be saner.

Changes:

  • Individual exec commands are all split out to separate functions so that they can be more easily composed. No function does more than a single command
  • Control flow now happens entirely in the Fetch() function
  • Before there were several failure states around submodules due to the git switch command happening immediately after the git fetch if the repo was already cloned. This is fixed by moving to a unified control flow.
  • Fetch() will now run either the initial clone OR update refs before moving onto further steps
  • git clone no longer attempts to initialize submodules. Several features from git submodules aren't yet implemented by git clone and so submodule initialization is moved entirely into a git submodule command
  • git switch now happens after the clone/fetch but before git submodule
  • git switch now no longer runs with any submodule-related flags. Like git clone the features we need are just not implemented yet and as far as I can tell there are subtle behavioral differences that were causing issues.
  • git submodule will now use --filter=blob:none to specify that submodule checkouts should themselves be blobless when possible. As far as I can tell this works correctly with --recursive and recursive submodules.
  • Because we are now checking out submodules with blobless clones when possible the fixPermissions() logic has been updated to account for promisor files that could be in submodule .git directories

@ReillyBrogan ReillyBrogan added the backport-me Backport to stable release label Jun 14, 2024
@ReillyBrogan
Copy link
Contributor Author

Comment should end in a period

I don't tell you how to live your life CI

@ReillyBrogan ReillyBrogan force-pushed the remove-submodules-command branch 2 times, most recently from 80093d4 to d5ec53d Compare June 14, 2024 00:35
builder/source/git.go Outdated Show resolved Hide resolved
GZGavinZhao
GZGavinZhao previously approved these changes Jun 14, 2024
Copy link
Member

@GZGavinZhao GZGavinZhao left a comment

Choose a reason for hiding this comment

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

Logic is sane to me. If this has been tested, then feel free to go ahead and merge!

The logic for this was quite broken and had several failure cases due to that. This commit re-writes that to be saner.

Changes:
- Individual exec commands are all split out to separate functions so that they can be more easily composed. No function does more than a single command
- Control flow now happens entirely in the Fetch() function
- Before there were several failure states around submodules due to the `git switch` command happening immediately after the `git fetch` if the repo was already cloned. This is fixed by moving to a unified control flow.
- `Fetch()` will now run either the initial clone OR update refs before moving onto further steps
- `git clone` no longer attempts to initialize submodules. Several features from `git submodules` aren't yet implemented by `git clone` and so submodule initialization is moved entirely into a `git submodule` command
- `git switch` now happens after the clone/fetch but before `git submodule`
- `git switch` now no longer runs with any submodule-related flags. Like `git clone` the features we need are just not implemented yet and as far as I can tell there are subtle behavioral differences that were causing issues.
- `git submodule` will now use `--filter=blob:none` to specify that submodule checkouts should themselves be blobless when possible. As far as I can tell this works correctly with `--recursive` and recursive submodules.
- Because we are now checking out submodules with blobless clones when possible the `fixPermissions()` logic has been updated to account for promisor files that could be in submodule `.git` directories
Copy link
Member

@silkeh silkeh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@silkeh silkeh merged commit 1b87395 into master Jun 16, 2024
1 check passed
@silkeh silkeh deleted the remove-submodules-command branch June 16, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-me Backport to stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants