Project

General

Profile

Bug #5414

When Opening New Pull Request, Target Revision Default Is Undesireable

Added by Peter Rebholz over 1 year ago. Updated over 1 year ago.

Status:
New
Priority:
High
Assignee:
-
Category:
-
Target version:
-
Start date:
12.01.2018
Due date:
% Done:

0%

Estimated time:
Sorting:
Commit Number:
Affected Version:

Description

Version Info:

RhodeCode 4.10.6 Enterprise Edition
CentOS 7
PostgreSQL 10.1

Issue:

We're testing out upgrading to 4.10.6 from a fairly old version and hit a snag that is going to keep us from upgrading. When opening a new pull request for a Hg repository, initiated from the Changelog screen, the default target revision is particularly poor for our workflow/use case. We're seeing it default to the default branch or, in repositories where the default branch is closed, the very first revision of the repository. It takes a really long time for RhodeCode to count every commit from the beginning of time to the present and further, if you change the branch while it's working, you get a browser alert dialog (see attached).

This has a significant impact on our workflow as every user is going to hit this every time they try and open a pull request. We use bookmarks as the source revision for pull requests so and are usually requesting things be merged into the same branch, thus the best default for us would be the same branch. However, having no default would be preferable to the wrong default and a long processing time + error pop-up.

The only workaround I have found is initiating the pull request from the pull requests page and setting the target revision before setting the source revision. This isn't a great work around.

History

#1 Updated by Marcin Kuzminski [staff] over 1 year ago

Hi Peter,

Thanks for your message. In case of the PR opened from the changelog, there's always an option to select the checkbox next to the changelog entry. (screen1)
Then rhodecode will select the ref of it. In this case (fixes-branch), but this works for bookmarks as well.

Another way of pre-selecting is adding to the url. e.g. http://rc.local:8080/pull-requests/default_reviewers-fork/pull-request/new?bookmark=fix-issue-710

We discussed your case in our team, and we think we should introduce a new option for git/hg to be able to select a default PR target, similar as we have now for landing revision. As a quick workardound, you could basically catch the url schema such as:

/repo_name/pull-request/new and redirect it to /repo_name/pull-request/new?bookmark=bookmark_name, or ?branch=some_other_branch_name

meanwhile, we don't have a better workaround.

Best,

#2 Updated by Peter Rebholz over 1 year ago

Hi Marcin,

Thanks for your response. When you mention:

In case of the PR opened from the changelog, there's always an option to select the checkbox next to the changelog entry. (screen1) Then rhodecode will select the ref of it. In this case (fixes-branch), but this works for bookmarks as well.

This is the workflow that we're using, sorry I was not more clear. Here are the steps we've been following for opening PRs:

  1. Navigate to repo's Changelog
  2. Check the commit you want pulled (usually has a bookmark)
  3. Click "Open new pull request for selected commit"

When following this flow, the source revision is correctly set to the bookmark but the target revision is the issue either defaulting to default or the first commit (same thing when using the URL format you mentioned). Since you mentioned this flow, were you expecting a different result?

With the version we're currently using the behavior is a bit different as the branch name that orders first is selected. This is still not perfect for us but is better as it's usually closer to where we want to be. Further, the alert dialog does not pop up when manually setting the target even if the request that fetches the list of commits to be included is still active.

Adding a new option for PR target would help. We often have several active branches so it wouldn't always be right but it would be closer than what we're seeing now.

I guess one last thing I'll ask is: Is the behavior we're seeing where commit 0 is preselected when default is closed is desirable in any circumstance? It seems like at point not selecting anything would be better than picking commit 0.

#3 Updated by Marcin Kuzminski [staff] over 1 year ago

Peter-

This is the workflow that we're using, sorry I was not more clear. Here are the steps we've been following for opening PRs:

  1. Navigate to repo's Changelog
  2. Check the commit you want pulled (usually has a bookmark)
  3. Click "Open new pull request for selected commit"

When following this flow, the source revision is correctly set to the bookmark but the target revision is the issue either defaulting to default or the first commit (same thing when using the URL format you mentioned). Since you mentioned this flow, were you expecting a different result?

Ahh yes, i confused the source/target. This is expected that default is set to 'default' branch if it exists, the problem here is that "if it exists" checks only active branches, so it then defaults to first commit.

With the version we're currently using the behavior is a bit different as the branch name that orders first is selected. This is still not perfect for us but is better as it's usually closer to where we want to be. Further, the alert dialog does not pop up when manually setting the target even if the request that fetches the list of commits to be included is still active.

Adding a new option for PR target would help. We often have several active branches so it wouldn't always be right but it would be closer than what we're seeing now.

We'd add either default branch or "empty" this way it would never load anything until you manually load the target.

I guess one last thing I'll ask is: Is the behavior we're seeing where commit 0 is preselected when default is closed is desirable in any circumstance? It seems like at point not selecting anything would be better than picking commit 0.

Yes i agree with that, selecting first commit is not a good default. We'll change that to "not selected anything".

You can change that in your code now if you want a quick patch: https://code.rhodecode.com/rhodecode-enterprise-ce/files/62aa75de625a17f6f2418b7290d707648cd74792/rhodecode/model/pull_request.py#L1402

Basically replacing
selected = 'rev:%s:%s' % (rev, rev)
with
`selected = None

and in case of also default branch, here: https://code.rhodecode.com/rhodecode-enterprise-ce/files/62aa75de625a17f6f2418b7290d707648cd74792/rhodecode/model/pull_request.py#L1396

That would do what you want for not selecting anything now.

#4 Updated by Peter Rebholz over 1 year ago

An "empty" option would work for us.

The last last thing I'll point out is that we probably wouldn't have even noticed this if it weren't for the alert window. So, while it will be nice to prevent the server from doing unnecessary work, removing the alert would also be desirable (a user may still get this even they pick the wrong revision the quickly switch). There isn't any useful information in the alert that the user can act on. Plus, it seems like the intended behavior (cancel the existing request and make a new one) and thus shouldn't result in an error to the user.

Any idea on the release/time frame that such a feature would be added in?

Thanks again for looking into this,
Peter

#5 Updated by Marcin Kuzminski [staff] over 1 year ago

Hi Peter,

Yes the alert for canceled request is bad. We'll fix that together with the pick revision in case no default branch exists. Those two things for sure will go in 4.11 which we plan in next ~2 weeks.

As for the picker i think 4.12, so next 4-6 weeks after 4.11

Best,

#7 Updated by Peter Rebholz over 1 year ago

Not sure where you are with this, but I wanted to mention this: some of my colleagues have created bash aliases to help them open pull requests, the one missing ingredient is the ability to set the target branch via the URL string. Here's what they're doing:

alias rp='xdg-open "$(hg paths default)/pull-request/new?bookmark=$(hg parent -T {bookmarks})"'

I can open a new request for that additional parameter if you'd like.

#8 Updated by Marcin Kuzminski [staff] over 1 year ago

Hi Kieran,

THis sounds like a good idea, would be great if you can open a PR, we love contributions! :)

Also available in: Atom PDF