Skip to content
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

Only delete source branch if it was forced. #193

Merged
merged 2 commits into from
Dec 10, 2019
Merged

Only delete source branch if it was forced. #193

merged 2 commits into from
Dec 10, 2019

Conversation

Viatorus
Copy link
Contributor

If the option Delete source branch when merge request is accepted. is not checked, the branch will not be removed.

@Viatorus
Copy link
Contributor Author

WTF:
R: 11, 0: Too many public methods (31/30) (too-many-public-methods)

@Viatorus
Copy link
Contributor Author

@JaimeLennox Could we merge this?

@JaimeLennox
Copy link
Contributor

Sorry it took so long to get to this!

I've just had a great time reading through why there are two options called force_remove_source_branch and should_remove_source_branch, and I'm more confused then when I started.

The best information I could find is from https://gitlab.com/gitlab-org/gitlab/issues/18283#note_214998578, which seems to indicate that force_remove_source_branch is the option on the merge request and should_remove_source_branch is for merge time.

And if this wasn't enough the Gitlab API docs also have a plain remove_source_branch option for when updating the MR: https://docs.gitlab.com/ee/api/merge_requests.html#update-mr

In any case, we use should_remove_source_branch for accepting the MR, and this seems to be the right one for that particular endpoint. Setting this to the same option as force_remove_source_branch seems fine, as we're just respecting the user's wishes, although I also think removing the parameter would have the same effect since then we're not overriding anything at merge time. This also seems a bit cleaner than messing around with these options in the first place, given they're quite confusing!

What do you think?

@Viatorus
Copy link
Contributor Author

I just remember, if someone says the branch should not be deleted, it was deleted anyways.

The GUI checkbox option remove source branch inside a MR is ignored if you merge via remote API.

@JaimeLennox
Copy link
Contributor

I see! Well that just adds to the fun I guess. I'm happy to merge this in then.

@JaimeLennox JaimeLennox merged commit 6f0a8f1 into smarkets:master Dec 10, 2019
@Viatorus Viatorus deleted the fix_force_remove_branch branch December 11, 2019 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants