Uploaded image for project: 'Bitbucket Data Center'
  1. Bitbucket Data Center
  2. BSERV-3469

Pull request references in the Git repository are never removed after merge or decline

XMLWordPrintable

    • We collect Bitbucket feedback from various sources, and we evaluate what we've collected when planning our product roadmap. To understand how this piece of feedback will be reviewed, see our Implementation of New Features Policy.

      When a user opens a pull request, some references are created inside the Git repository hosted in the Stash server. These references live in files stored in <STASH_HOME>\data\repositories\<REPO_ID>\refs\pull-requests\<PULL_REQUEST_ID>

      This is great and allows one to, for example, configure the build server to build pull requests and the result of the merge.

      However, these references are never deleted from the Git repository, so there's no way for a build server to know when to stop monitoring these references. Because of that, if you merged 1000 pull requests in the past, the build server will show you 1000 build results, one for each pull request, even though you already merged (or declined) these 1000 pull requests and are not interested in these build results anymore.

      I would expect the reference to be removed after the pull-request has been merged or declined. If a user reopens it, then the reference should be recreated.

      Thanks,
      Caio Proiete

        1. pull-requests.png
          pull-requests.png
          34 kB

            [BSERV-3469] Pull request references in the Git repository are never removed after merge or decline

            jens added a comment -

            Thanks for reaching out Alexey,

            We are taking great care not to break public APIs. However, if your add-ons make use of internal APIs, you'll have to consider that these APIs may break without notice.

            It sounds like you've build a bunch of awesome functionality on top of Stash and you may still be able to get your plugin running again using the new way the information is stored. But as Bryan mentioned above, unfortunately we won't be able to support it.

            jens added a comment - Thanks for reaching out Alexey, We are taking great care not to break public APIs. However, if your add-ons make use of internal APIs, you'll have to consider that these APIs may break without notice. It sounds like you've build a bunch of awesome functionality on top of Stash and you may still be able to get your plugin running again using the new way the information is stored. But as Bryan mentioned above, unfortunately we won't be able to support it.

            Alexey Efimov added a comment -

            It's ok. If in 2.9.0 i can use standard method for reading conflicts If Stash change storing, it not dramatically. If you retain API to reading conflicts for pull requests in this version.

            Alexey Efimov added a comment - It's ok. If in 2.9.0 i can use standard method for reading conflicts If Stash change storing, it not dramatically. If you retain API to reading conflicts for pull requests in this version.

            Bryan Turner (Inactive) added a comment -

            Sorry Alexey. What you were doing was not, and still is not, supported. How conflicts are handled is an implementation detail of the system, as I have reiterated many times, and is subject to change without notice. We will not make any effort not to break code that makes use of our implementation details.

            Conflict information will still be attached to the diff tree, as it always has been. The way that information is stored in the repository has changed dramatically, but anyone using Stash's public API will not notice the difference.

            Bryan Turner (Inactive) added a comment - Sorry Alexey. What you were doing was not, and still is not, supported. How conflicts are handled is an implementation detail of the system, as I have reiterated many times, and is subject to change without notice. We will not make any effort not to break code that makes use of our implementation details. Conflict information will still be attached to the diff tree, as it always has been. The way that information is stored in the repository has changed dramatically, but anyone using Stash's public API will not notice the difference.

            Alexey Efimov added a comment -

            Hm, how to get list of conflicts in Pull Request after upgrading? I have plugin to show conflicts in tab within pull-request, it used method which reads notes in 2.8.

            Alexey Efimov added a comment - Hm, how to get list of conflicts in Pull Request after upgrading? I have plugin to show conflicts in tab within pull-request, it used method which reads notes in 2.8.

            Bryan Turner (Inactive) added a comment -

            Alexey,

            Old ones are retained because the cost of unpacking them is simply too high with the current mechanisms available to me. However, no new notes refs will be created after upgrading to 2.9.

            Bryan Turner
            Atlassian Stash

            Bryan Turner (Inactive) added a comment - Alexey, Old ones are retained because the cost of unpacking them is simply too high with the current mechanisms available to me. However, no new notes refs will be created after upgrading to 2.9. Bryan Turner Atlassian Stash

            Alexey Efimov added a comment -

            Bryan, conflicts notes refs still retain or you also removed this?

            Alexey Efimov added a comment - Bryan, conflicts notes refs still retain or you also removed this?

            augustoproiete added a comment -

            Wow! Awesome news bturner! Thank you VERY MUCH for implementing this fix!!

            Looking forward for the 2.9 Release, and appreciate all the warnings about the upgrade and consequences... Will do more dry-runs than usual this time.

            Thanks,
            Caio Proiete

            augustoproiete added a comment - Wow! Awesome news bturner ! Thank you VERY MUCH for implementing this fix!! Looking forward for the 2.9 Release, and appreciate all the warnings about the upgrade and consequences... Will do more dry-runs than usual this time. Thanks, Caio Proiete

            Bryan Turner (Inactive) added a comment -

            Stash 2.9 will include substantial changes to how its pull request refs work. The merge-base/merge-clean/merge-conflicted and to refs have been completely removed. The from and merge refs will only exist for open pull requests. Once a pull request is declined or merged, they will be removed. Further, the merge ref will only exist if the pull request merges cleanly. If it has conflicts, only the from ref will exist.

            To support these changes, on startup Stash 2.9 will make backwards-incompatible changes to every existing repository. As a result, I cannot emphasize strongly enough how important it is to take a backup of your system before upgrading after Stash 2.9 is released. Once Stash 2.9 has been started, it is not possible to revert to 2.8 or previous and attempting to do so is likely to corrupt your repositories.

            When 2.9 starts for the first time, all existing pull request refs will be removed during startup and only refs for open pull requests will be retained. Depending on the number of repositories you have, and the number of pull requests you've done, this may take a little time. Tested on a system with ~300 repositories and ~3,000 pull requests (total), the upgrade runs in ~15 seconds. Please allot a little extra time to your 2.9 upgrade for this cleanup processing. The web interface will not be able to show progress for this processing, but the Stash logs will. Please do not kill Stash during startup. This is a general rule, not one specific to the 2.9 upgrade, but I want to repeat it because it's particularly important. If startup takes extra time, please let it finish. Interrupting Stash while it's applying upgrade tasks can corrupt your repositories.

            Bryan Turner (Inactive) added a comment - Stash 2.9 will include substantial changes to how its pull request refs work. The merge-base / merge-clean / merge-conflicted and to refs have been completely removed . The from and merge refs will only exist for open pull requests. Once a pull request is declined or merged, they will be removed. Further, the merge ref will only exist if the pull request merges cleanly. If it has conflicts, only the from ref will exist. To support these changes, on startup Stash 2.9 will make backwards-incompatible changes to every existing repository . As a result, I cannot emphasize strongly enough how important it is to take a backup of your system before upgrading after Stash 2.9 is released. Once Stash 2.9 has been started, it is not possible to revert to 2.8 or previous and attempting to do so is likely to corrupt your repositories. When 2.9 starts for the first time, all existing pull request refs will be removed during startup and only refs for open pull requests will be retained. Depending on the number of repositories you have, and the number of pull requests you've done, this may take a little time. Tested on a system with ~300 repositories and ~3,000 pull requests (total), the upgrade runs in ~15 seconds. Please allot a little extra time to your 2.9 upgrade for this cleanup processing. The web interface will not be able to show progress for this processing, but the Stash logs will. Please do not kill Stash during startup. This is a general rule, not one specific to the 2.9 upgrade, but I want to repeat it because it's particularly important. If startup takes extra time, please let it finish. Interrupting Stash while it's applying upgrade tasks can corrupt your repositories.

            Alexey Efimov added a comment -

            Thanks!

            Alexey Efimov added a comment - Thanks!

            Bryan Turner (Inactive) added a comment -

            Alexey,

            As the status of this issue indicates, the change is in progress. However, it is a far more complicated change that an outside view might indicate, and so it must be implemented with care and tested extensively before we can release it. I can't offer any insight into when the change will ship beyond to say that it will.

            Best regards,
            Bryan Turner
            Atlassian Stash

            Bryan Turner (Inactive) added a comment - Alexey, As the status of this issue indicates, the change is in progress. However, it is a far more complicated change that an outside view might indicate, and so it must be implemented with care and tested extensively before we can release it. I can't offer any insight into when the change will ship beyond to say that it will. Best regards, Bryan Turner Atlassian Stash

            Alexey Efimov added a comment -

            Hello,
            What decision for this task? Will stash remove `from` references at least if PR merged?

            Alexey Efimov added a comment - Hello, What decision for this task? Will stash remove `from` references at least if PR merged?

            Bryan Turner (Inactive) added a comment -

            Caio,

            I believe, without building a plugin, it's probably not possible with the current system to do too much about it. Such a plugin could go in either system:

            • A plugin for Stash which, when a pull request is created or rescoped (we raise events in both cases) prods TeamCity to perform a build and, when a pull request reaches an "end" state (merged or declined) cleans up the associated build
            • A plugin for TeamCity which pairs polling for ref updates with considering the pull request's state, allowing pull requests which are closed or merged to be ignored

            I'm not terribly familiar with the internals of TeamCity, when it comes to building plugins, so I don't know what's involved with that route for certain. I've used their REST API pretty heavily, though, before joining Atlassian, and, assuming you're on at least TeamCity 7, their REST API allows creating, modifying and deleting build configurations. I'd probably prepare a template and then create builds which used it, to reduce the configuration as much as possible.

            With regards to forks, things get a little more complicated. I think it's worth asking if the forks really need to be continuously built, or whether pull requests from forks are what need building. Even for cross-repository pull requests, the same merge commit is created to supply the diff. That means, once a pull request is created from a fork, your CI server can still build the result. With Bamboo we have some special "Fork Builder" configurations people have setup which allows the user initiating the build to explicitly provide the URL for the repository. Assuming TeamCity allows something similar, and paired with anonymous access (STASH-2565), developers could use custom builds to build specific branches in their forks on demand. That'd be a bit trickier to automate, though, for sure. It's a pain point we're also dealing with internally (half the Stash team does all their development using forks and the other half using branches to allow us to thoroughly dogfood both approaches) and we're still investigating solutions.

            One further thought. When you build merges, are you building refs/pull-requests/ID/merge or refs/pull-requests/ID/merge-clean? If the former, you should probably build the latter instead. Building the merge ref can result in builds that can't possibly succeed, since the merge may include committed conflicts. Building merge-clean will never suffer from that problem. If a clean merge becomes conflicted, the merge-clean ref is replaced with merge-conflicted, and vice versa.

            Sorry I don't have more to offer at the moment.
            Bryan Turner
            Atlassian Stash

            Bryan Turner (Inactive) added a comment - Caio, I believe, without building a plugin, it's probably not possible with the current system to do too much about it. Such a plugin could go in either system: A plugin for Stash which, when a pull request is created or rescoped (we raise events in both cases) prods TeamCity to perform a build and, when a pull request reaches an "end" state (merged or declined) cleans up the associated build A plugin for TeamCity which pairs polling for ref updates with considering the pull request's state, allowing pull requests which are closed or merged to be ignored I'm not terribly familiar with the internals of TeamCity, when it comes to building plugins, so I don't know what's involved with that route for certain. I've used their REST API pretty heavily, though, before joining Atlassian, and, assuming you're on at least TeamCity 7, their REST API allows creating, modifying and deleting build configurations. I'd probably prepare a template and then create builds which used it, to reduce the configuration as much as possible. With regards to forks, things get a little more complicated. I think it's worth asking if the forks really need to be continuously built, or whether pull requests from forks are what need building. Even for cross-repository pull requests, the same merge commit is created to supply the diff. That means, once a pull request is created from a fork, your CI server can still build the result. With Bamboo we have some special "Fork Builder" configurations people have setup which allows the user initiating the build to explicitly provide the URL for the repository. Assuming TeamCity allows something similar, and paired with anonymous access ( STASH-2565 ), developers could use custom builds to build specific branches in their forks on demand. That'd be a bit trickier to automate, though, for sure. It's a pain point we're also dealing with internally (half the Stash team does all their development using forks and the other half using branches to allow us to thoroughly dogfood both approaches) and we're still investigating solutions. One further thought. When you build merges, are you building refs/pull-requests/ID/merge or refs/pull-requests/ID/merge-clean ? If the former, you should probably build the latter instead. Building the merge ref can result in builds that can't possibly succeed, since the merge may include committed conflicts. Building merge-clean will never suffer from that problem. If a clean merge becomes conflicted, the merge-clean ref is replaced with merge-conflicted , and vice versa. Sorry I don't have more to offer at the moment. Bryan Turner Atlassian Stash

            augustoproiete added a comment -

            Hi bturner,

            Thank you very much for the detailed explanation.
            I understand the reason now, and can imagine the work involved in making this change.

            In the meantime, is there any workaround for building pull requests in the build server (we use TeamCity)?
            At the moment, every developer pushes feature branches to the server, and they all get automatically built by the CI builds, because they pick up any reference (refs/heads/*). I tried to do the same with pull requests, which is how I noticed all the pull request references there, and the build server just started building thousands of merge commits.

            In separate note: We were looking forward to start using Forks, but we are unable to move to Forks until we figure out a way to have CI builds automatically building pull requests and forks automatically. The solution we have now is not ideal as we end up with lots and lots of branches cluttered in the server...

            Thanks,
            Caio Proiete

            augustoproiete added a comment - Hi bturner , Thank you very much for the detailed explanation. I understand the reason now, and can imagine the work involved in making this change. In the meantime, is there any workaround for building pull requests in the build server (we use TeamCity)? At the moment, every developer pushes feature branches to the server, and they all get automatically built by the CI builds, because they pick up any reference (refs/heads/*). I tried to do the same with pull requests, which is how I noticed all the pull request references there, and the build server just started building thousands of merge commits. In separate note: We were looking forward to start using Forks, but we are unable to move to Forks until we figure out a way to have CI builds automatically building pull requests and forks automatically. The solution we have now is not ideal as we end up with lots and lots of branches cluttered in the server... Thanks, Caio Proiete

            Bryan Turner (Inactive) added a comment -

            caio-atlassian,

            This is not a bug, per se; the system is actually doing it intentionally. Even after a pull request is merged or declined, that pull request can still be opened in Stash so that users can look at it. As a result, its merge commit, which is used to show the diff, may still be needed so Stash keeps that merge commit around, protected by its reference. Note that while an equivalent merge can be produced later, it's not (necessarily) cheap to do so and its hash will not be the same. That has repercussions for comments in the diff, which are anchored in the merge commit by its hash.

            Now, all that said. There are definitely other ways for Stash to handle this, and we're working on switching it up. I can't guarantee a specific version where this will be released, because we're definitely going to want not only to test the change heavily but also the upgrade path (bulk updating thousands of refs could potentially be very slow, depending on how it's done), but I wanted to let you know it's on our radar as something we must address (and soon).

            Best regards,
            Bryan Turner
            Atlassian Stash

            Bryan Turner (Inactive) added a comment - caio-atlassian , This is not a bug, per se; the system is actually doing it intentionally. Even after a pull request is merged or declined, that pull request can still be opened in Stash so that users can look at it. As a result, its merge commit, which is used to show the diff, may still be needed so Stash keeps that merge commit around, protected by its reference. Note that while an equivalent merge can be produced later, it's not (necessarily) cheap to do so and its hash will not be the same. That has repercussions for comments in the diff, which are anchored in the merge commit by its hash. Now, all that said. There are definitely other ways for Stash to handle this, and we're working on switching it up. I can't guarantee a specific version where this will be released, because we're definitely going to want not only to test the change heavily but also the upgrade path (bulk updating thousands of refs could potentially be very slow, depending on how it's done), but I wanted to let you know it's on our radar as something we must address (and soon). Best regards, Bryan Turner Atlassian Stash

              Unassigned Unassigned
              ab4ae5e3f28f augustoproiete
              Votes:
              7 Vote for this issue
              Watchers:
              8 Start watching this issue

                Created:
                Updated:
                Resolved: