branch: Close all the associated PRs when a branch is deleted. #5646
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5646 +/- ##
=========================================
Coverage ? 41.35%
=========================================
Files ? 432
Lines ? 59576
Branches ? 0
=========================================
Hits ? 24639
Misses ? 31694
Partials ? 3243
Continue to review full report at Codecov.
|
bkcsoft
added
the
lgtm/need 2
HarshitOnGitHub
force-pushed
the
Issue_2466
branch
from
3a3c9f1
to
0f81683
Compare
HarshitOnGitHub
force-pushed
the
Issue_2466
branch
from
0f81683
to
1a6ba3f
Compare
HarshitOnGitHub
force-pushed
the
Issue_2466
branch
from
1a6ba3f
to
3d2957a
Compare
| return | ||
| } | ||
| prs := append(baseprs, headprs...) | ||
| for statusChanged := true; statusChanged; { |
There was a problem hiding this comment.
This is not what I meant by sort the issues.
You have a list of pull-requests, prs. Consider two pull-requests p and q, say p<q if q has a dependency on p.
Assuming there are no cyclic dependencies, this < forms a partial order on the set of PRs in prs. You can then sort the list prs by this partial order. (There will be multiple valid sortations of the list but who cares.)
You should not have to ask the database potentially n^2 times whether an issue still has dependencies.
Further, this whole section should be done in a transaction - you're at risk of data changing under your feet here.
There was a problem hiding this comment.
Yeah I had understood what you meant in your comment but after looking at our code for issue dependency I found that it is indeed possible to create a cycle involving three or more issues like A->B->C->A, in this case sorting can't be done without first detecting and removing the issues involved in the cycle, right? If you are concerned about the database query I can remove that by the replacing the code with some less straightforward code, but shouldn't that database query be cheap since we are performing only a count operation and not actually loading the data?
I think we should fix the issue dependency code anyway to avoid having circular dependency, right?
Further, this whole section should be done in a transaction - you're at risk of data changing under your feet here.
Yeah sorry missed that, will change it. :)
Thanks for taking time to review! :)
There was a problem hiding this comment.
Eurgh. I hadn't realised we weren't enforcing non-cyclicity - which I guess is hard without always forcing the transitive closure on dependency additional - and is itself prone to exponential blow up.
Ok. Let's think about this slightly differently - does it ever make sense to have an open pull request from a deleted branch? Probably not, even if that request has dependencies that it should prevent its closure - the fact that the code isn't there means it must be closed despite this.
So, if it doesn't break the database and the app layer to force it closed we should force it closed in spite of its dependencies. (I'm not sure for definite if this is the case - but I'd be surprised if it was the case.)
I guess therefore either issue.ChangeStatus should have additional optional argument to force or, there needs to be a ForceClose function. I could imagine a possible use for a ForceClose for cases where there has been a rogue user - although it might make sense in such situations to just delete everything they've done.
(I appreciate that I'm nitpicking over what will be usually one PR but people have a tendency to deliberately break stuff.)
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions. |
lunny
added
the
issue/confirmed
|
Please resolve the conflicts. |
|
replaced by #9927 |
Fixes: #2466.