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

Update git package submodules after clone and reset #4314

Merged
merged 3 commits into from
Sep 25, 2018

Conversation

tclem
Copy link
Contributor

@tclem tclem commented Sep 21, 2018

Related to #1764 and a subtle fix for what was implemented in #1852, this changes the logic for packages identified by git locations so that submodules are properly updated. The process now looks like this:

  1. clone the repo
  2. reset --hard to a specific commit
  3. update submodules (recursively)

The problem with the existing implementation is that a hard reset (git reset --hard) doesn't update submodules so the state of submodules in a package will always be whatever's on the default branch.

Please also shortly describe how you tested your change. Bonus points for added tests!

I tested a similar version of this patch against the v1.9.0.1 tag, but I was unable to get stack on master to properly read the stack.yaml of my project. To test requires a little bit of setup. You need:

  • A stack project with a package identified using a git location
  • A package that uses submodules
  • A commit SHA of that package where the submodule SHAs are different than what's on the default branch.

TODO:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary. I don't believe there's anything to update here, but please let me know!

For packages identified by git locations, this changes the logic to:
1. clone the repo
2. reset --hard to a specific commit
3. update submodules (recursively)
@dbaynard
Copy link
Contributor

Hello @tclem,

Git behaviour is changing for the next stack version (after 1.9.*) — @snoyberg will know more; I've requested a review.

, ["update", "-C", T.unpack commit]
, []
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this mean that, for Mercurial, we'll be calling hg here with no arguments? I think using a Nothing value here and checking for that below would be appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Good suggestion. There's definitely no reason to have an extra shell out to hg with no arguments, I pushed another commit to fix that.

Also happy to squash things up back to a single commit, but I might wait for further feedback and do that once at the end.

Copy link
Contributor

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Nice!

@snoyberg snoyberg merged commit a299ac6 into commercialhaskell:master Sep 25, 2018
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.

3 participants