Issue #86: Pull requests also show changes from other pull requests.
| Reported by: | Wouter Vermeiren |
| State: | new |
| Created on: | 2015-01-26 10:18 |
| Updated on: | 2016-02-17 12:05 |
Description
If I create a commit A and B in a repository and I open a pull request in Kallithea for these commits. Then (assuming the review repository and its fork are in sync) I will only see these commits in my pull request and reviewing it is a piece of cake. Now while the review is ongoing, if I continue to work on top of my previous two commits and create a commit C. Then when I push this commit to Kallithea and start a review, also commit A & B are included. Although these are already under review in a different pull request.
It would be nice that the changesets that are already in another pull request could be ignored (for example a checkbox to exclude them or ...)
Attachments
Comments
Comment by Mads Kiilerich, on 2015-01-26 16:09
Correct. There could be 20 other ongoing pull requests and some of them might be dropped again. Filtering changesets that are in use on another PR would not be feasible ... and could easily make it a moving target.
After the first PR has been merged to your mainline, you can merge that to the branch and you can 'update' the PR - then it will exclude the parts that already has been merged.
Comment by Wouter Vermeiren, on 2015-01-27 10:13
I don't really understand the issue you are bringing up. As Kallithea is not doing any merges, it is sitting next to our normal workflow and not in between. This means that for merging PR you are responsible yourself. Consecutive development in the same repo is not so unlikely then. Especially since I don't want to clone a local repo again to work on another change while another is still under review. In times were workload peaks and code review take longer to finish, this could mean that you might have x clones for x features (or x heads or ...). If these changes then have dependencies, it can get very complicated.
Comment by Thomas De Schampheleire, on 2015-02-09 20:33
@kiilerix: if I understand what you mean, then the updated PR would have an extra commit (the merge) and the overall delta would exclude the already merged parts.
However, if you are not working on separate branches this model does not work. I made three commits on top of each other, and launched 3 separate pull requests for the first one, the first+second one, and for all three together. When the first commit is pushed to the mainline (no merge was necessary), then the first pull request is automatically marked as 'this PR has been merged'. The changes remain visible though. The other two pull requests do not 'see' that the first commit is already mainlined, and do not change.
I'm not sure if there is anything that can/should be done for this issue, except adding the possibility to launch separate 'review requests' that include a subrange of changes that may not necessarily be all changes in the anonymous branch. This is actually what I proposed in http://lists.sfconservancy.org/pipermail/kallithea-general/2015q1/000145.html
Is this something you'd be open to? You commented on the thread, but it's not 100% clear to me if you'd accept such a change.
Comment by Mads Kiilerich, on 2015-02-11 01:33
I agree that this might fit in if we get a more generic model where this 'review specified changesets' (not pulling, not merging, but perhaps grafting) is a not-so-special case. Right now we only support "review preview of merge" and this does not directly fit in. A first step could be to get better control over per-changeset review/approve status.
Comment by Alexander Mollberg, on 2016-01-08 08:10
What is the current status on support for including a subrange of changes in a review request? The UI support is evidently already there since you can mark two commits in the changelog and then select "Open New Pull Request for XXX → YYY" but then it generates an internal server error. What is the holdup on this feature?
Comment by Mads Kiilerich, on 2016-01-08 15:39
I am not aware of any internal server error. If using the latest version, please file a new issue and provide the stacktrace and other relevant info.
Comment by Alexander Mollberg, on 2016-02-17 12:05
A new issue no longer appears to be needed since user eivindt has opened issue #190 which appears to be directly related to my problem; the use case he describes causes the exact same stack trace for me.