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

Don't change directories when cloning a repo #4862

Merged
merged 2 commits into from
Jun 19, 2019

Conversation

amalloy
Copy link
Contributor

@amalloy amalloy commented Jun 13, 2019

This was making it impossible to pin to a specific commit when using relative file paths as repo URLs.

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

I tested this change by running stack test three times:

  1. Before I made any changes, tests pass
  2. After intentionally implementing my change incorrectly, tests fail
  3. After a correct implementation of my change, tests pass again

@amalloy
Copy link
Contributor Author

amalloy commented Jun 13, 2019

I've included a Changelog update in what I think was the right place, and I believe no documentation update is necessary (already-documented behavior now works better).

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.

In addition to the inline comments:

  1. I see that there's a CI failure from the style checker
  2. It may make sense to PR this against the stable branch instead of master

ChangeLog.md Outdated
@@ -277,6 +277,8 @@ Bug fixes:
[#4292](https://github.com/commercialhaskell/stack/issues/4292)
* Fix for git packages to update submodules to the correct state. See
[#4314](https://github.com/commercialhaskell/stack/pull/4314)
* Fix to allow dependencies on specific versions of local git repositories. See
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in "unreleased changes" instead. It should also likely be mentioned in the pantry changelog.

-- ENABLE_VIRTUAL_TERMINAL_PROCESSING flag for native terminals. The
-- folowing hack re-enables the lost ANSI-capability.
when osIsWindows $ void $ liftIO $ hSupportsANSIWithoutEmulation stdout
runCommand ["clone", T.unpack url, dir]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a comment that this is intentionally not run within the withWorkingDir below in order to allow relative paths to work.

@amalloy amalloy changed the base branch from master to stable June 15, 2019 00:27
@amalloy amalloy force-pushed the master branch 2 times, most recently from b90cf52 to 356c662 Compare June 15, 2019 00:36
@amalloy
Copy link
Contributor Author

amalloy commented Jun 15, 2019

The hlint failure was on unmodified code in a different file; I'm not inclined to touch that as part of this PR.

I've adjusted the target branch, and made the changes you requested, except: the pantry changelog appears to currently be empty, after a year's worth of commits to it. Do you want this to be the first change documented therein?

@snoyberg
Copy link
Contributor

the pantry changelog appears to currently be empty, after a year's worth of commits to it

Right, that's intended: pantry has been in development for a year, but only last week had its first release. This is the first actual change versus a released version.

This was making it impossible to use relative file paths as repo URLs.
@amalloy
Copy link
Contributor Author

amalloy commented Jun 17, 2019

Done.

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.

LGTM. I've merged in the changes on stable so CI will pass. This is good to merge once CI goes green.

@snoyberg snoyberg merged commit 22fe3b9 into commercialhaskell:stable Jun 19, 2019
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.

2 participants