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

fix(import): Handle filepaths with spaces more robustly #1655

Merged
merged 4 commits into from
Sep 8, 2018

Conversation

chrisdothtml
Copy link
Contributor

@chrisdothtml chrisdothtml commented Sep 5, 2018

CC @evocateur

Fixes #432

Description

Update replace pattern when rewriting commit history to support files with spaces in them. Still has some edge cases where this issue will happen (see this comment)

How Has This Been Tested?

Ran my new test before and after the pattern update. Confirmed it failed before, and passes after. Also have confirmed this fix works on a repo of my own

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@chrisdothtml
Copy link
Contributor Author

chrisdothtml commented Sep 5, 2018

@evocateur I am interested in trying something with custom prefix options, but figured I'd get this fix pushed at the very least.

What pattern would you suggest instead of a/ and b/? Maybe something crazy explicit like COMPARE_FILE_A/ and COMPARE_FILE_B/?

Also, side note: is it normal to be getting a ton of linting errors? Not even related to anything I did, getting them on the latest master branch. Also, it probably should be mentioned in the CONTRIBUTING doc that you need lerna globally installed for tests to run (or I needed to at least)

EDIT: Any idea what's going on with this test?

@chrisdothtml
Copy link
Contributor Author

I've added the custom prefixes, and improved my test for this a bit (so now all cases are resolved for filepaths with spaces). The tests are all passing for me locally and I'm unable to tell what's causing them to fail in the CI. May just need to be restarted.

Let me know if there's anything else you need from me in this PR

Copy link
Member

@evocateur evocateur left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@evocateur
Copy link
Member

The AppVeyor build is failing in master because apparently quoted shell arguments are hard for Windows, not your fault. I restarted the Travis builds, looks like they hit a cascade of timeouts for environmental reasons.

@lock
Copy link

lock bot commented Dec 27, 2018

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import failure: "git diff header lacks filename information when removing 1 leading pathname component"
2 participants