Learn more about these different git repos.
Other Git URLs
I often rebase or amend my commits in git and force push them to my development branches. Today, I rebased https://pagure.io/fegistry/pull-request/1 on master before merging, merged using git, and then pushed. I was surprised not to get a notification on IRC that I closed the PR and when I went to look at the PR it was still open, but oddly shows that it changes 0 files and contains 0 commits.
To reproduce, here's what I did in git on my dev box (I use "origin" to mean my bowlofeggs fork, and "upstream" is the upstream fegistry repo):100:
$ git checkout license $ git rebase master $ git push -f origin license $ git checkout master $ git merge license $ git push # My master branch's remote is set to upstream $ git push origin :license # Maybe this is how the PR got to 0 commits?
I'd expect Pagure to have noticed that my PR was rebased when I did the git push -f operation, and subsequently to have noticed that the PR was closed when I did the git push operation.
git push -f
git push
Due to this issue, it is difficult to see the history of the pull request now. It is merged, but the information about what was in the pull request is gone. Of course, the commit history remains, but the connection between which commit was reviewed and the commit log is lost.
So basically the step that was missing was: git commit --amendand add to the commit message Merges https://pagure.io/fegistry/pull-request/1
git commit --amend
Merges https://pagure.io/fegistry/pull-request/1
Without this pagure has basically no way to know if you just removed all your commits in purpose (or by accident) or if the branch got merged (I can't think of a third option right now ^_^).
I am a little bit wondering if we should go the road of: if we find no commit in that PR, consider it merged. Will think about it :)
@pingou When I did the git push -f origin license, I would expect Pagure to see that I had updated my branch (and thus update the PR). Then when the commits from that branch landed in master, I would expect Pagure to notice that the commits from that PR are now merged, and close the PR. I do think all the information Pagure needs to accomplish this is present in the workflow I described.
git push -f origin license
@pingou When I did the git push -f origin license, I would expect Pagure to see that I had updated my branch (and thus update the PR).
Pagure normally already detects the rebase and adds a notification on the PR that a rebase was done.
Then when the commits from that branch landed in master, I would expect Pagure to notice that the commits from that PR are now merged, and close the PR.
How does pagure know that commit Foo to Bar have landed in master vs Foo to Bar were in master from the start and the user just removed all the commits on the top of them?
If you want to look at this I can point you to the place in the code. I do think we could come up with something but I fear it would be fragile and potentially report false-positive.
After giving this more thoughts, I figured we could come up with something.
While I was trying to think of the correct approach, I then pinged @puiterwijk on IRC this morning to discuss this with him, here is our conversation:
08:10:46 pingou | Good Morning 08:59:57 pingou | puiterwijk|cld: what do you think of https://pagure.io/pagure/issue/1683 09:00:55 pingou | should we assume that if the PR exists and when we refresh there are no more commits it was merged? 09:01:15 --> | puiterwijk (~puiterwij@redhat/puiterwijk) has joined #pagure 09:05:21 puiterwijk | pingou: hah. This is a particular thing I've been telling you for a long time now :P 09:05:53 pingou | ^^ 09:07:16 * | pingou trying to think of the right algorithm 09:07:29 puiterwijk | Well, I'm not 100% sure that this needs to be fixed. 09:07:45 puiterwijk | It would be relatively simple to do, but I'm not sure it's the right thing to do 09:08:07 pingou | mind sharing your thoughts on the ticket? 09:08:23 puiterwijk | You *could* just check on push to $main_branch whether any of the commits that were just pushed has is the commit ID that's on top of a current PR 09:08:50 puiterwijk | The problem here is: which branch does it need to be pushed to to be considered merged? 09:09:13 pingou | the main one 09:09:27 pingou | well technically the branch the PR was against 09:09:35 puiterwijk | any branch? But you pushed it to your PR branch. The main branch? What is your main branch, develop, master, topic branch? 09:09:57 pingou | you open a PR against develop, if develop has the commits, the PR is merged, no? 09:10:14 puiterwijk | The branch the PR was against? It happens sometimes that someone opens a PR against master, and then you decide that it should go in a topic branch, so you merge it to "acl" instead. 09:10:31 puiterwijk | "you" as in "the project owner" 09:10:32 pingou | then the PR still stands imgo 09:10:35 pingou | imho* 09:10:56 puiterwijk | In that case it would never be marked as merged, since it might very well be that in the topic branch you change it slightly 09:11:10 puiterwijk | Also, this only deals with the case that you push the *exact* commit at the top of the PR... 09:11:17 puiterwijk | Which is literally what "Merge PR" on the UI does 09:11:21 pingou | yup 09:11:26 puiterwijk | In which case... you could've just pressed the button 09:11:55 puiterwijk | Since that would've done exactly the same, since seemingly the commit ID on top is exactly the same.. 09:12:05 pingou | yup 09:12:30 puiterwijk | I think that when you add this, people might get even more confused if the commit you end up pushing is slightly different (because of rebasing etc) 09:12:38 puiterwijk | Because it wouldn't trigger 09:12:52 puiterwijk | That's why I think it's a good idea to be explicit and use Merges: #xxx 09:13:20 puiterwijk | So either you use the button in the UI to mark as merged (works), or you add Merges: to the commit you push (also works) 09:14:09 puiterwijk | Now, do I think Pagure should have a "Rebase & merge" button like Github has nowadays? Yes, absolutely. 09:15:02 pingou | same here for this :) 09:17:25 puiterwijk | pingou: you can copy&paste my IRC conversation into the ticket, or I can in an hour or so :) 09:19:10 pingou | puiterwijk: I can :)
Here is a workflow I use commonly on Github:
If pagure supported this, and also included the Git command in the PR notification e-mail, I could have a command-line-only workflow for cases that don't require discussion.
The OP is rather complicated in comparison to my recent use case I'd like to see handled well with Pagure in context of https://src.fedoraproject.org:
I think the risk of confusion was next to none in this very case.
By default pagure won't merge a PR if not instructed in a commit message (and that currently doesn't work in dist-git, cf: #2874)
The last update was 5 years ago, no further requests, updates or actionable tasks since then, I'm going to close this issue for now to reduce our backlog.
Metadata Update from @wombelix: - Issue close_status updated to: Won't Fix - Issue status updated to: Closed (was: Open)
Login to comment on this ticket.