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

Convert path separators in patch file to unix explicitly #35

Closed
wants to merge 2 commits into from

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Aug 1, 2019

I ran into trouble when trying to apply a patch file generated with Windows Git to a mercurial repository through cygwin. It wasn't recognizing the files since the paths in the patch file were using Windows path separators.

The spec doesn't seem to mention path separators: https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html#Detailed-Unified

But, both the Windows Git and Mercurial clients I have installed are handling unix path separators just fine. So, in this PR I've changed the patch file generators to use unix path separators explicitly, which solves the problem.

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Approvers

  • Erik Helin (ehelin - Reviewer)

@openjdk openjdk bot added the rfr label Aug 1, 2019
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 1, 2019

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 1, 2019

Webrevs

@edvbld
Copy link
Member

@edvbld edvbld commented Aug 5, 2019

Hi Jorn,

thanks for testing out the tools using Cygwin!

The patch itself looks good, but maybe this patch should take the opportunity to fix https://bugs.openjdk.java.net/browse/SKARA-7 as well? I would prefer moving away from Webrev having its own logic for creating .patch files, it should use Diff::write and Diff::toString instead. I suspect we have the same exact issue in Diff::write, so please fix the bug there as well, even if you don't refactor Webrev to use Diff::write 🙇‍♂️

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Aug 5, 2019

@edvbld I'll take a look :)

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Aug 5, 2019

I've just fixed the problem for Patch::write as well, since switching Webrev to use Diff::write is not a simple switch. The latter does not have support for printing hunk context lines it seems. Doesn't seem like a quick fix, and I don't really have time to look into it right now :/

I ran the vcs module tests, but this is the one that had a lot of failures (27). I can say that at least there were no additional failures from the Patch::write change, but you might want to test on your side as well.

@edvbld
Copy link
Member

@edvbld edvbld commented Aug 5, 2019

Ok, thanks, I will take the patch for a spin.

edvbld
edvbld approved these changes Aug 22, 2019
Copy link
Member

@edvbld edvbld left a comment

Sorry for taking so long to review this @JornVernee, the patch looks good! I executed sh gradlew test on Linux x86_64 and all tests passed!

@openjdk openjdk bot removed the rfr label Aug 22, 2019
@openjdk
Copy link

@openjdk openjdk bot commented Aug 22, 2019

@JornVernee This change can now be integrated. The commit message will be:

Convert path separators in patch file to unix explicitly

Reviewed-by: ehelin
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 37 commits pushed to the master branch:

  • 0f1e47d: Parse list of repositories to mirror as a list
  • ac33f2f: Add support for ready markers to the PR bot as well
  • 8467aae: 61: Remove sponsor label if PR is updated after flagged for integration
  • 3e24d9c: 63: Remove the census cache
  • 2e94eb0: 60: Add integrated to closed pull requests that have been integrated
  • f57fa46: Add Repository::revert
  • 7623f82: Implement HgRepository::amend
  • db3e4c0: Fix another index out of bounds in webrev generation
  • 0a354f4: 62: Proper parsing of GitLab discussions outside of a diff context
  • ef52825: Remove stray newline in HgRepository
  • c19224a: GitCommitIterator should handle empty commits
  • ef42f82: git combined diffs suffer from underflow
  • 7a45536: Improve the previous empty hunks fix
  • 6f88442: Fix webrev generation issue when a source hunk is empty
  • 8f81e32: Cache the value of root() in GitRepository
  • f9979b0: Proper parsing of git diff --raw lines
  • d2e6beb: Use ZonedDateTime for date in Commit
  • b3d4d7c: Fix implementation of addremove
  • 9d206dd: 59: Implement summary command
  • 9325c74: 58: Add mirror and forward bots to skara-bots CLI image
  • 71b944d: 57: The hgbridge should push marks to a configurable repository
  • 39f574e: Improve testing of GitHub PositionMapper
  • d4ab5fd: 55: Reduce disk usage for notify bot
  • fbddc6f: 54: Add a mirror bot
  • 3c5c3d8: 49: Add forwarder bot
  • 8b41fd7: 50: Fix failing tests after e8d32c7
  • ab9a473: Use the web url in the displayed fetch command
  • 2698709: 22: Wrong lines for comments
  • e8d32c7: Fix start of hunk range off-by-one error when parsing range string
  • 1c8b488: Update README.md
  • 83ad6c8: Get resource path from URI to avoid getting spurious leading path separator on Windows
  • 53a6c51: 45: Fix remaining integration test issues
  • 6746b03: 44: GitHubApplication should not use a JSONParser instance
  • 90edbab: 43: Repository.get should return Optional.empty on non-existing directory
  • e83e473: 42: Notifier tests can fail depending on execution order
  • 965be9c: 41: Bot launcher should detect ev. http proxy in environment
  • 7c68b3b: 40: Fix failing tests after remote locking

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

  • To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Aug 22, 2019
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Aug 22, 2019

/integrate

@openjdk openjdk bot closed this Aug 22, 2019
@openjdk
Copy link

@openjdk openjdk bot commented Aug 22, 2019

@JornVernee The following commits have been pushed to master since your change was applied:

Your commit was automatically rebased without conflicts.
Pushed as commit 1b97234.

@openjdk openjdk bot added the integrated label Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants