Skip to content

Conversation

@phillipj
Copy link
Member

We've seen a lot of patches not applied cleanly by the bot, which in fact
should have landed without trouble, at least with 3-way merges enabled.

This trivial change enables fallback to 3-way merge when a patch
doesn't apply cleanly when running git am.

Should we give this a try before possibly disabling the attempt-backport labels?

Refs #116

/cc @mscdex @MylesBorins @Fishrock123

We've seen a lot of patches not applied cleanly by the bot, which in fact
should have landed without trouble, at least with 3-way merges enabled.

This trivial change enables fallback to 3-way merge when a patch
doesn't apply cleanly when running `git am`.
@mscdex
Copy link
Contributor

mscdex commented Jan 25, 2017

It should be safe to add this, but I'm not still 100% sure that this is all that is needed for safe, automatic conflict resolution.

@sam-github
Copy link

I'm pretty sure its not all that's needed.

If master has A, B, C on it, and C depends on B, then C won't cherry-pick clean, and will get marked as not applying, but when the staging branch is built from A, B, and C, it will pick clean.

This is how the check is made, right, a single commit cherry-pick/git am?

If it was made by cherry-picking every commit, in order, from master that hasn't been labelled by a person as don't land, that might work because its closer to how the tools are used when backporting, and supports dependencies between commits.

@phillipj
Copy link
Member Author

Okey, so it's probably not going to be rock solid after these changes either. But we agree that falling back to 3-way merges won't do any harm or be even more incorrect than the bot is currently, right?

@sam-github
Copy link

Yes

Fishrock123

This comment was marked as off-topic.

@phillipj phillipj merged commit e2c3895 into nodejs:master Jan 28, 2017
@phillipj phillipj deleted the git-am-3way branch January 28, 2017 11:33
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.

4 participants