-
-
Notifications
You must be signed in to change notification settings - Fork 5.9k
branch: Close all the associated PRs when a branch is deleted. #5646
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what I meant by sort the issues.
You have a list of pull-requests,
prs
. Consider two pull-requestsp
andq
, sayp<q
ifq
has a dependency onp
.Assuming there are no cyclic dependencies, this
<
forms a partial order on the set of PRs inprs
. You can then sort the listprs
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Yeah sorry missed that, will change it. :)
Thanks for taking time to review! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)