Issue #176: handling missing revisions in pull requests
Reported by: | domruf |
State: | resolved |
Created on: | 2015-11-25 18:45 |
Updated on: | 2018-05-19 15:18 |
Description
git allows to modify the history and mercurial with evolve extension and non publishing repositories does as well.
This means that revisions can be 'deleted'. Which currently causes errors when trying to open a pull request that has deleted revisions.
I think I remember a discussion about automatically updating the pull request and it was decided that pull request changes should not happen automatically. But in a world with history modifications I think this must be reconsidered.
Attachments
Comments
Comment by domruf, on 2015-11-25 18:45
Comment by domruf, on 2015-11-26 16:51
I created a quickfix here b148b5c97eb842a201420bedcc4e5843bd043c6a
But this doesn't solve the 'real' problem which is that git pull request currently don't support updates.
@kiilerix I would work on this but...
I strongly believe ALL the vcs specific code belongs in lib.vcs.backends.
This means moving quite some code from pullrequests.py to lib.vcs.backends.hg/git so I since it is your code I wanted to ask for your input on this.
And the questions remains: What to do with pull request that have missing revisions. I think the only sensible thinkg to do in that case is to auto update the pull request.
Comment by Long Vu, on 2015-12-18 19:50
FYI, I have tried to use Evolve (rev b338fe4e0657) with Kallithea 0.3 turning on the non publishing feature.
When evolving a rev that was part of a PR, that rev is obsoleted but not deleted so the PR will works perfectly fine (PR open without error, see all previous comments attached to the now obsoleted rev). We can even view the obsoleted rev (/changeset/<obsoleted rev>). Awsome.
I actually like this behavior "no PR auto-update but require user to manually push a button to update which create a new PR" because it means we do not lose old comments on previous versions of the rev(s) because the old rev(s) are still referenced by the old PR.
I have also tried to strip a rev that was part of a PR and the PR do not open anymore. I think this is bad. We probably should have ignored the missing rev and display a warning to user that a rev is missing instead of completely blowing off in front of the user (I think I got a 500 return code page).
Comment by Mads Kiilerich, on 2015-12-22 13:59
Yes, all vcs specific code should be in the vcs backend ... especially if it can be done with a clean API that can be used for both VCSs. A patch refactoring just that would be great!
Both Mercurial and Git should have the old revisions around until they are stripped/garbage collected. For the same reason, that shouldn't happen on a server.
I don't think it would be a good idea to auto update if revisions are missing. One reason is that it can't know what to update to. Doing so would remove the context for all reviews and thus lose data. It would also make it impossible (harder) to fix the situation by finding the missing revisions somewhere (backup or user's clone).
It would be nice to show the PR anyway. The diff will of course be empty and all comments will be "out of context" but it might still be somewhat useful - at least to get an idea what was lost.
Comment by domruf, on 2016-02-17 12:06
I created a new task for the factoring stuff #195
I couldn't assign it to myself @kiilerix could you give me the permissions to do that?
Comment by domruf, on 2016-02-17 14:05
fixed in PR #206.
Comment by Thomas De Schampheleire, on 2018-05-19 15:18
Fixed with 3f646f7bac39