Pull requests gets into a bad state after stripping a source changeset (BB-10087)

Issue #8283 resolved
Martin Geisler
created an issue

We had a pull request with two commits, one of which turned out to be unnecessary. After stripping the second commit in the source repository, the pull request broke.

It broke in the sense that trying to update it gave us a page where the reviewers field was unstyled so one could see that it's a select field. The preview below (showing the commits in the pull request) never finished loading.

Going back to the overview page, we could decline the request. When we did so, the overview diff showed the expected diff once again — it had been showing the "Nothing to merge, all changesets already exist in the destination repo" message after we stripped the second changeset from the source repo.

Let me know if the above makes sense, if not I'll try to reproduce it and post some screenshots.

Comments (13)

  1. Brian Nguyen

    Hi Martin,

    I've tried reproducing this locally and I have so far been unsuccessful. I suspect that the problem is specific to the repository/commit.

    Would you be able to point me to the repository and pull request that is breaking for you? It sounds like the pull request diff request is failing.

    Cheers, Brian

  2. Martin Geisler reporter

    I finally sat down and reproduced this. Here are the steps and my observations:

    1. I created https://bitbucket.org/mg/pr-strip-test-main/ and pushed one changeset (x).

    2. I forked the main repo to https://bitbucket.org/mg/pr-strip-test-fork/ and pushed two changesets (y and z).

    3. I opened a pull request to bring y and z back to main: https://bitbucket.org/mg/pr-strip-test-main/pull-request/1/

    4. I stripped z from the fork.

    5. Refreshing the pull request now shows:

      Nothing to merge. The source repository was deleted or these changes already exist in the destination repository.

      This is not completely correct: the source repository was not deleted and there is still something to merge. It can be debated exactly what should happen in this case, but I guess I would have expected the pull request to be updated to only include the y changeset.

    6. Going to the Commits tab shows

      There are no changes.

      The margin looks off to me, it would have been better if the text had been styled like the text on the Overview tab with the small circled exclamation mark. I'll make a screenshot of this.

    7. The activity tab is empty, even though something has happened to the pull request, i.e., it went from being a valid pull request to a non-mergeable pull request.

    8. Clicking Edit brings me to a screen where the normal changeset list preview at the bottom doesn't load and where the reviewers section isn't styled like normal, i.e., I can see that it's a multi select field. I'll attach a screenshot to show what I mean.

    9. Clicking the "Update" button in that view shows me JSON in my browser:

      {"errors": {"source": ["This field is required."]}, "error": "An error occurred saving this pull request. Please check the required fields and try again"}

      My guess is that there is a JavaScript error on the previous page which means that the right click handler isn't added.

    10. Declining the pull request should work, but I haven't done it here to avoid disturbing the state. My observation from the real repo is that the commit preview starts working again after the request is declined.

    This scenario has played itself out a couple of times now: someone accidentally clicked the "Update pull request" link and a pull request ended up covering too many changesets.

    Since we cannot make a pull request with an arbitrary commit (only heads), the only way to fix the pull request seems to be to decline it, strip the extra commits, and then create a new pull request. This means losing the old reviewer comments, which is unfortunate.

  3. Brodie Rao

    Hey Martin,

    Thanks for the detailed steps to reproduce the issue.

    Your hunch about the expected behavior is right: when you strip a changeset from the source branch of a pull request, we're supposed to update the pull request to one of the heads on the pull request. (The way it's implemented right now, it's an arbitrary head on the branch.)

    It sounds like there was either an error in updating the PR's internal state after stripping, or for some reason our strip-related hook didn't run properly.

    We'll look into the issue more and let you know when we have more information.

  4. Log in to comment