Comments (11)
@matfax Thank you for creating a new issue to discuss this. I think my first impression about this was wrong and I believe the problem is simpler.
new changes from the master branch are not correctly detected
The action does not rebase the PR branch. It used to rebase, but I removed it.
So what happens is that local changes that apply to new commits on master
are lost when branch black
is checked out and the stash popped. You can try to see if it works correctly by manually rebasing the PR branch and running the workflow again. The diff should then be correct.
I will think about how to add rebase back in to the action safely.
I hope I've understood this correctly. Does that make sense to you, too?
from create-pull-request.
@peter-evans I'm not sure if rebasing would solve this issue. Rebasing per se would not work because there still were the conflicts, so I chose to merge with a --strategy=ours
. That did not work either. Then, I had a more thorough look at the code and tried to reproduce the issue by manually performing the git commands.
The checkout causes the problem, in the first hand. The stash pop fails as you suspected:
Auto-merging tests/helper.py
CONFLICT (content): Merge conflict in tests/helper.py
...
The subsequent checkout and reset confuse things for me. The action does not seem to detect the branch to be dirty. However, my local version is dirty. I pushed my local version now and even though there are still two conflicts, they seem to be related to inconsistent formatting.
I wondered why you chose to checkout first and commit afterward, unlike in a manual use case scenario in that you would typically commit first and then try to push. If that fails, you would rebase or merge respectively so that the subsequent push would succeed. That seems to be more straight-forward and should most likely solve issues like mine.
- Create git config
- Commit
- Push to the remote branch with the given name. If that branch is new, the push will succeed. If the branch already exists and has a conflict, it will fail.
- If the push failed, use rebase or merge from the current remote of the local branch to solve the conflicts, respecting the user preference (i.e., action inputs) and the chosen the strategy (i.e., ours, theirs) as well.
- Push again.
from create-pull-request.
The reason it tries to checkout first is to pop the stash on top of the PR branch. It seemed like a good approach at the time. I now realise there are better ways and I'm in the process of changing it for a v2
major version release.
You might be right that rebase alone wouldn't solve it. There may even be conflict cases where it would be impossible for the action to decide what to do and it would just need to fail and request manual intervention.
What would help me tremendously is being able to reproduce this scenario in a test case. Do you know the steps in simple git commands that can put the repository in the state you experienced? I'm trying to put together a set of test cases to make sure I cover conflict scenarios and edge cases for v2
.
from create-pull-request.
v2
has been released.
- name: Create Pull Request
uses: peter-evans/create-pull-request@v2
with:
token: ${{ secrets.GITHUB_TOKEN }}
@matfax I'm going to close this issue for now. Please reopen or create a new issue if you find that v2
doesn't behave well for your use case.
from create-pull-request.
Works perfectly now. Good work!
from create-pull-request.
@peter-evans Here is a simple test scenario but one that would be mergeable without a strategy.
Setup repository:
git init
echo "Test" > file1
git add file1
git commit -m "file1"
Create clone branch (representing local/master):
git branch clone
git checkout clone
echo "Addition" >> file1
(deterministic modification)git add file1
git commit -m "Addition1"
(representing the state of the new PR branch)
Modify master:
git checkout master
echo "Test" > file2
git add file2
git commit -m "file2"
Create 2nd clone branch (representing local/master at a later time):
git branch later
git checkout later
echo "Addition" >> file1 && echo "Addition" >> file2
(deterministic modification)
Branch is dirty now, but mergeable. Let's test what create-pull-request
currently does:
- Check if the remote exists (assuming True)
git stash --include-untracked
git checkout clone
git stash pop
(=> conflict in file2)git checkout --theirs .
(assuming BaseException is raised due to the conflict)git reset
(assuming BaseException is raised due to the conflict)git status
(commit and push if it is dirty)- file2 is untracked
This would still work because it would merge without conflicts.
from create-pull-request.
Now, let's try a scenario that creates a dirty branch:
Setup repository:
git init
echo "Test" > file1
git add file1
git commit -m "file1"
Create clone branch (representing local/master):
git branch clone
git checkout clone
echo "Addition" >> file1
(deterministic modification)git add file1
git commit -m "Addition1"
(representing the state of the new PR branch)
Modify master:
git checkout master
echo "Test2" >> file1
git add file1
git commit -m "modified file1"
Create 2nd clone branch (representing local/master at a later time):
git branch later
git checkout later
echo "Addition" >> file1
(deterministic modification)
Branch is dirty now, but mergeable. Let's test what create-pull-request
currently does:
- Check if the remote exists (assuming True)
git stash --include-untracked
git checkout clone
git stash pop
(=> conflict in file1)git checkout --theirs .
(assuming BaseException is raised due to the conflict)git reset
(assuming BaseException is raised due to the conflict)git status
(commit and push if it is dirty)- file1 is dirty, should be detected as such
from create-pull-request.
Finally, let's test a scenario that creates a branch that can not be merged without a strategy:
Setup repository:
git init
echo "Test" > file1
git add file1
git commit -m "file1"
Create clone branch (representing local/master):
git branch clone
git checkout clone
echo "Other" > file1
(simulating deterministic modification that replaces "Test" with "Other")git commit -a -m "replaced test with other"
(representing the state of the new PR branch)
Modify master:
git checkout master
echo "Test2" >> file1
git commit -a -m "modified file1"
Create 2nd clone branch (representing local/master at a later time):
git branch later
git checkout later
echo "Later" > file1 && echo "Later2" >> file1
(simulating deterministic modification with a new version that replaces "Test" with "Later")
Branch is dirty now and not mergeable. Let's test what create-pull-request
currently does:
- Check if the remote exists (assuming True)
git stash --include-untracked
git checkout clone
git stash pop
(=> conflict in file1)git checkout --theirs .
(assuming BaseException is raised due to the conflict)git reset
(assuming BaseException is raised due to the conflict)git status
(commit and push if it is dirty)- file1 is dirty, should be detected as such (bug? or is the BaseException not raised?)
New try, this time with a previous merge as discussed earlier:
git merge clone
(fails because of conflict)git merge --strategy=ours clone
(succeeds)- Check if the remote exists (assuming True)
git stash --include-untracked
git checkout clone
git stash pop
(=> conflict in file1)git checkout --theirs .
(assuming BaseException is raised due to the conflict)git reset
(assuming BaseException is raised due to the conflict)git status
(commit and push if it is dirty)- file1 is dirty, merge was useless
New try, this time with a previous commit & merge:
git commit -a -m "replaced test with later"
git merge clone
(fails because of conflict)git merge --strategy-option=ours clone
(succeeds)
Even though only a push would be left to do:- Check if the remote exists (assuming True)
git stash --include-untracked
git checkout clone
git stash pop
(=> no changes)git status
(nothing to commit, will not be pushed even though there are commits to be pushed)
from create-pull-request.
After all, I could not reproduce a test state in which the branch was neither dirty nor had untracked-files. However, the branch is not detected to be dirty by the deployed version of create-pull-request
. Preceding merging as a workaround does not change anything.
from create-pull-request.
@matfax Thank you for these scenarios. In the middle of working on v2
at the moment so I'll work through these and try to make sure the next major version behaves sensibly.
from create-pull-request.
@matfax I've worked through many different test scenarios and completely refactored the way the action handles creating/updating pull request branches. It's beta right now, but feel free to test it out and let me know how it behaves. The big change is that each update will now factor in all new changes on the base. This is effectively a rebase of the pull request branch. This should solve your original problem that changes on master were not being reflected in updates to the PR.
See this documentation for details of breaking changes and new features.
- name: Create Pull Request
uses: peter-evans/create-pull-request@v2-beta
with:
token: ${{ secrets.GITHUB_TOKEN }}
from create-pull-request.
Related Issues (20)
- Error: Input 'token' not supplied. Unable to continue. HOT 1
- Changes from test folder doesn't added to the PR HOT 10
- Node version - Warning HOT 1
- Permission denied (publickey) HOT 1
- Merge conflict error for different files with different names
- Add branch-label as an output so it can be passed to other actions and easily commited to HOT 2
- PR still created with 0 files (no diffs) HOT 4
- [FEATURE] Add ability to enable auto-merge HOT 1
- Can't figure out how to include commits made during workflow HOT 2
- Action failing to update existing PRs on differently named fork HOT 3
- pr not generated HOT 1
- this works great!
- This works great
- This works great
- When the workflow is triggered with `base` and `branch`, the PR is not created HOT 1
- Syntax error line 17 HOT 2
- remote missing HOT 2
- How to always have open PR from dev to prod HOT 1
- Autosync PR out of date HOT 2
- Automatically delete body-path HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from create-pull-request.