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

Let's talk about nearest_branch selection for rebase #1166

Closed
kaste opened this issue Sep 24, 2019 · 4 comments
Closed

Let's talk about nearest_branch selection for rebase #1166

kaste opened this issue Sep 24, 2019 · 4 comments

Comments

@kaste
Copy link
Collaborator

kaste commented Sep 24, 2019

The rebase dashboard preselects a good base candidate automatically. The implementation is imo flawed. Let's look at it.

For the simple case:

image

A: The feature branch I just pushed here is still on top of dev, and nearest_branch suggest dev 👍

nearest_branch: found 1 relatives: ['dev']
nearest_branch: Found best candidate dev

Let's look at this. colorize-paths is a PR which is already behind dev. follow-paths-upwards is an upcoming PR derived from the 'colorize' feat.

image

B: If I check out 'colorize' nearest_branch fails 😮 and falls back to the 'upstream' fork/colorize-paths-in-graph-view, which is a no-op because 'local' and 'remote' are synced. (Without an 'upstream' it would fallback to 'master'!)

nearest_branch: found 1 relatives: ['dev']
nearest_branch: No valid nearest found. Possibly on a root / detached branch!

C: If I checkout follow-paths-upwards, it selects colorize-paths. 👍

nearest_branch: found 2 relatives: ['colorize-paths-in-graph-view', 'dev']
nearest_branch: Found best candidate colorize-paths-in-graph-view

What are good candidates for rebasing?

First, 'dev' is always a good candidate here ('dev' would be ' master' in other repos).

For B: You have options:

  1. 'dev', the branch you originally started your feat which moved along. E.g. you need to rebase on 'master' because of merge conflicts.

  2. the actual merge-base, t.i. the commit you forked originally. E.g. you re-arrange the PR but don't want to resolve possible merge conflicts, you want to stay in the feat branch.

  3. the upstream if available, t.i. the commit you already pushed. If you rebase from upstream you don't have to force-push afterwards, it's safe.

Since we currently don't offer candidates to the user but just select one good candidate, should we just go cheap and take the first relative found. Are there known cases we do want to handle differently?

The main bug was imo introduced here 2dcc3aa with switching from --no-merged to --merged. --merged wasn't correct either bc it breaks C. (For C --merged would select 'dev'.) Imo we don't want neither, just omit this arg. But then the check git branch --contains <common> cannot fail so we could omit altogether. (We know that the sha is in relative bc we have it from its log.)

WDYT 🤷‍♀

@randy3k
Copy link
Collaborator

randy3k commented Sep 24, 2019

That also doesn’t work for me most of the time. I use a setting (forget the name now, on mobile) to fix the target branch.

@kaste
Copy link
Collaborator Author

kaste commented Sep 24, 2019

Yeah, the setting is rebase_default_base_ref but it takes precedence t.i. it is static!

kaste added a commit to kaste/GitSavvy that referenced this issue Sep 24, 2019
@randy3k
Copy link
Collaborator

randy3k commented Sep 24, 2019

You could specify it in project setting https://github.com/divmain/GitSavvy/blob/master/docs/README.md#editing-settings

@kaste
Copy link
Collaborator Author

kaste commented Nov 14, 2019

Implemented and merged into dev. Works fine so far. Way better than before at least.

@kaste kaste closed this as completed Nov 14, 2019
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

No branches or pull requests

2 participants