Hi Peter, I'm facing the issue that changes after the initial commit

<a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/us

v2 has been released. <div class="snippet-clipboa

<a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/us

diff with existing branch not detected,about peter-evans/create-pull-request

Comments (11)

peter-evans commented on August 20, 2024 1

@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.

matfax commented on August 20, 2024 1

@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.

peter-evans commented on August 20, 2024 1

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.

peter-evans commented on August 20, 2024 1

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.

matfax commented on August 20, 2024 1

Works perfectly now. Good work!

from create-pull-request.

matfax commented on August 20, 2024

@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.

matfax commented on August 20, 2024

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.

matfax commented on August 20, 2024

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.

matfax commented on August 20, 2024

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.

peter-evans commented on August 20, 2024

@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.

peter-evans commented on August 20, 2024

@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.

diff with existing branch not detected about create-pull-request HOT 11 CLOSED

Comments (11)

Related Issues (20)

Recommend Projects

React

Vue.js

Typescript

TensorFlow

Django

Laravel

D3

Recommend Topics

javascript

web

server

Machine learning

Visualization

Game

Recommend Org

Facebook

Microsoft

Google

Alibaba

D3

Tencent