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 PR merging algorithm #26

Merged
merged 3 commits into from
Mar 23, 2019
Merged

Conversation

kitsuyui
Copy link
Contributor

@kitsuyui kitsuyui commented Feb 21, 2019

The problem is that

  • The new_body doesn't know the current status.
  • Diff & merge algorithms can't treat checking status well.

So my solution is:

  1. Scan every pr check status in old_body.
  2. Update it to unchecked.
  3. Then diff and merge as always.
  4. Update check status by results of 1.

Close: #20

The problem is that

- The new_body doesn't know the current status.
- Diff & merge algorithms can't treat checking status well.

So my solution is:

1. scan every pr check status in old_body.
2. Update it to unchecked.
3. Then diff and merge as always.
4. Update check status by results of 1.

Close: x-motemen#20
@kitsuyui
Copy link
Contributor Author

A = <<ERB
- [x] #1001 aaaaaaaaaa
- [x] #1002 cccccccccc
ERB

B = <<ERB
- [ ] #1000 bbbbbbbbbb
- [ ] #1001 aaaaaaaaaa
- [ ] #1002 cccccccccc
ERB
print merge_pr_body A, B

Before

diff: ["!", [0, "- [x] #1001 aaaaaaaaaa"], [0, "- [ ] #1000 bbbbbbbbbb"]]
Found checklist diff; use old one: - [x] #1001 aaaaaaaaaa
diff: ["!", [1, "- [x] #1002 cccccccccc"], [1, "- [ ] #1001 aaaaaaaaaa"]]
Found checklist diff; use old one: - [x] #1002 cccccccccc
diff: ["+", [2, nil], [2, "- [ ] #1002 cccccccccc"]]
Use line as is: - [ ] #1002 cccccccccc
- [x] #1001 aaaaaaaaaa
- [x] #1002 cccccccccc
- [ ] #1002 cccccccccc

After

Found pull-request checkbox #1001 is x.
Found pull-request checkbox #1002 is x.
diff: ["+", [0, "- [ ] #1001 aaaaaaaaaa"], [0, "- [ ] #1000 bbbbbbbbbb"]]
Use line as is: - [ ] #1000 bbbbbbbbbb
diff: ["=", [0, "- [ ] #1001 aaaaaaaaaa"], [1, "- [ ] #1001 aaaaaaaaaa"]]
Use line as is: - [ ] #1001 aaaaaaaaaa
diff: ["=", [1, "- [ ] #1002 cccccccccc"], [2, "- [ ] #1002 cccccccccc"]]
Use line as is: - [ ] #1002 cccccccccc
Update pull-request checkbox #1001 to x.
Update pull-request checkbox #1002 to x.
- [ ] #1000 bbbbbbbbbb
- [x] #1001 aaaaaaaaaa
- [x] #1002 cccccccccc

@onk
Copy link
Collaborator

onk commented Mar 9, 2019

I think that this solution can be acceptable.

Current problem is that checkbox list items are often lost after after merge. (#20, #21)
This is very critical. and there is a method to convert MR to checkbox, so it is reasonable to support this.
https://github.com/motemen/git-pr-release/blob/v0.6.0/bin/git-pr-release#L22-L24

Pros:

  • Checkbox items are not missing

Cons:

  • Checkbox items with no Issue/PR Number will not be merged
    • these will be duplicates

This is a trivial problem compared to losing items.

What do you think? @motemen

@kitsuyui
Copy link
Contributor Author

kitsuyui commented Mar 10, 2019

I think this problem is the limit of "2-way merge" algorithm.
It would be better to implement "3-way merge" if it must be strictly.

  1. Last pull-request (past to be)
  2. Last pull-request + edit by human. (as is)
  3. New pull-request (to be)

Apply diff between 1 and 2 into 3. This makes the result correctly.

Screen Shot 2019-03-10 at 13 15 01

And the cons of this case is that pull-request templete is not renewed by new pull-request template if the pull-request templete has changed between the commits. Because it will be treated as diff of 1 and 2, it will be reverted.

@motemen
Copy link
Member

motemen commented Mar 19, 2019

Overall looks good. Can you pull origin/matter and add some tests?

@kitsuyui
Copy link
Contributor Author

Okay, I will add some tests basing on #25

@kitsuyui
Copy link
Contributor Author

@motemen I added tests. Can you review this?

Copy link
Collaborator

@onk onk left a comment

Choose a reason for hiding this comment

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

🙆‍♀️

@motemen
Copy link
Member

motemen commented Mar 23, 2019

Thanks!

@motemen motemen merged commit 001c77a into x-motemen:master Mar 23, 2019
@kitsuyui kitsuyui deleted the fix-pr-merging-bug branch March 23, 2019 03:11
@kitsuyui kitsuyui restored the fix-pr-merging-bug branch March 23, 2019 03:12
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.

3 participants