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

Coalesce hunks that have overlapping context #113

Closed
wants to merge 2 commits into from

Conversation

@JornVernee
Copy link

JornVernee commented Aug 30, 2019

The webrev command can generate patch files with adjacent hunks that have overlapping context. git apply rejects these patches.

This PR coalesces adjacent hunks with overlapping context, which is also what's done by git diff.

Also added a test for HunkCoalescer (since it's a pretty complex class I think it deserved it's own tests).

Resulting webrev applies with both git and hg.

Progress

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

Approvers

@bridgekeeper

This comment has been minimized.

Copy link

bridgekeeper bot commented Aug 30, 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).

@openjdk openjdk bot added the rfr label Aug 30, 2019
@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Aug 30, 2019

Webrevs

@edvbld
edvbld approved these changes Sep 2, 2019
Copy link
Member

edvbld left a comment

This looks fine, but please run some additional manual testing to check that it works. Sorry for the lack of unit tests for this code :(

@openjdk openjdk bot removed the rfr label Sep 2, 2019
@openjdk

This comment has been minimized.

Copy link

openjdk bot commented Sep 2, 2019

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

Coalesce hunks that have overlapping context

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 6 commits pushed to the master branch:

  • 4c6b8ae: Coalesce hunks that have overlapping context
  • ac12500: Allow reverse mapping of census namespace users
  • 2aa4482: Extend the repository configuration syntax
  • a0e7eda: Topological bot tests needs to read org.junit.jupiter.params
  • a2d8107: 8: Use a file based approach for GitToHgConverter.java
  • bb34b1d: Add merge and topological bots to cli image

Your changes cannot be rebased automatically without conflicts, so you will need to merge master into your branch before integrating.

@openjdk openjdk bot added the ready label Sep 2, 2019
@JornVernee

This comment has been minimized.

Copy link
Author

JornVernee commented Sep 2, 2019

@edvbld I tested with the following script, on jdk/jdk:

for ($i = 1; $i -le 20; $i++) {
	git webrev -r "HEAD~$i" -N
	git checkout "HEAD~$i"
	git apply .\webrev\git-jdk2.patch
	git checkout master --force
}

This went alright until about HEAD~16, which threw a 'corrupt patch' error. Looking into it, the offending line is:

diff a/test/hotspot/jtreg/ProblemList-aot.txt b/test/hotspot/jtreg/ProblemList-aot.txt

I thought it might be the hyphen in the name, but git diff generates the same, and that patch does apply cleanly. Even when copying over the line from the git diff to the patch file generated by webrev it still complains. Not sure what's going on...

However, running that script without this PR fails on the first webrev, so I'd say that is a net improvement :), and since the issue seems unrelated, I think we could still move ahead with this PR?

@edvbld

This comment has been minimized.

Copy link
Member

edvbld commented Sep 3, 2019

@JornVernee yeah lets integrate this one since it is an improvement at least

@JornVernee

This comment has been minimized.

Copy link
Author

JornVernee commented Sep 3, 2019

/integrate

@openjdk openjdk bot closed this Sep 3, 2019
@openjdk openjdk bot added the integrated label Sep 3, 2019
@openjdk

This comment has been minimized.

Copy link

openjdk bot commented Sep 3, 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 4c6b8ae.

@openjdk openjdk bot added the outdated label Sep 3, 2019
@JornVernee JornVernee deleted the JornVernee:overlapping_context branch Sep 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.