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

Disable merge rebase commands when branch is local #8517

Conversation

GintasS
Copy link
Collaborator

@GintasS GintasS commented Oct 4, 2020

Fixes #1248

Proposed changes

Like the author of the issue has said, I'm disabling these two buttons when the branch is local.

Screenshots

Before

image

After

image

Test methodology

Manual


✒️ I contribute this code under The Developer Certificate of Origin.

@ghost ghost assigned GintasS Oct 4, 2020
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
Please add tests

@GintasS
Copy link
Collaborator Author

GintasS commented Oct 4, 2020

👍
Please add tests

Will do :)

@GintasS
Copy link
Collaborator Author

GintasS commented Oct 4, 2020

Please let me know if the this test is not sufficient, could not figure out how to make a scenario where a branch becomes a remote one, so it would set true to Enabled, Checked properties.

@gerhardol
Copy link
Member

If the default action is Merge or Rebase, the error message still appears, this should also be handled

The screenshot After does not have the circle checked, it should be set here.

@GintasS
Copy link
Collaborator Author

GintasS commented Oct 4, 2020

Ok, try to see the cases, I removed the circle so it wouldn't confuse that an option is selected, but somehow is disabled.

@gerhardol
Copy link
Member

Ok, try to see the cases, I removed the circle so it wouldn't confuse that an option is selected, but somehow is disabled.

For the disabled options, it is good to disable
I was asking about the remaining enabled option

@GintasS
Copy link
Collaborator Author

GintasS commented Oct 5, 2020

Ok, try to see the cases, I removed the circle so it wouldn't confuse that an option is selected, but somehow is disabled.

For the disabled options, it is good to disable
I was asking about the remaining enabled option

Just to be sure, you're talking about "Do not merge, only fetch remote changes"?

@gerhardol
Copy link
Member

Just to be sure, you're talking about "Do not merge, only fetch remote changes"?

yes

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

GitUI/CommandsDialogs/FormPull.cs Outdated Show resolved Hide resolved
@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 9, 2020
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 10, 2020
@GintasS
Copy link
Collaborator Author

GintasS commented Oct 10, 2020

Just to be sure, you're talking about "Do not merge, only fetch remote changes"?

yes

Fixed this.

@codecov
Copy link

codecov bot commented Oct 10, 2020

Codecov Report

Merging #8517 into master will decrease coverage by 0.01%.
The diff coverage is 52.78%.

@@            Coverage Diff             @@
##           master    #8517      +/-   ##
==========================================
- Coverage   55.07%   55.06%   -0.02%     
==========================================
  Files         899      899              
  Lines       64708    64742      +34     
  Branches    11624    11648      +24     
==========================================
+ Hits        35639    35650      +11     
  Misses      26340    26340              
- Partials     2729     2752      +23     
Flag Coverage Δ
#production 41.97% <45.97%> (-0.01%) ⬇️
#tests 94.70% <72.88%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@GintasS
Copy link
Collaborator Author

GintasS commented Oct 10, 2020

If the default action is Merge or Rebase, the error message still appears, this should also be handled

The screenshot After does not have the circle checked, it should be set here.

I'm not sure how to do it properly, I know that you can set default action via:
AppSettings.DefaultPullAction

@RussKie
Copy link
Member

RussKie commented Oct 11, 2020

I know that you can set default action via:
AppSettings.DefaultPullAction

We shouldn't touch this, this is a user setting.

If the default action is Merge or Rebase, the error message still appears, this should also be handled
The screenshot After does not have the circle checked, it should be set here.

I'm not sure how to do it properl

I presume what @gerhardol is referring to here is that by disabling the two merge options the 3rd is left unchecked. I'm not saying it has be checked until we understand the new behaviour with your fix.
Have you tested what happens if you click the "Pull" button?

@RussKie RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 11, 2020
@GintasS
Copy link
Collaborator Author

GintasS commented Oct 11, 2020

I know that you can set default action via:
AppSettings.DefaultPullAction

We shouldn't touch this, this is a user setting.

If the default action is Merge or Rebase, the error message still appears, this should also be handled
The screenshot After does not have the circle checked, it should be set here.

I'm not sure how to do it properl

I presume what @gerhardol is referring to here is that by disabling the two merge options the 3rd is left unchecked. I'm not saying it has be checked until we understand the new behaviour with your fix.
Have you tested what happens if you click the "Pull" button?

So yes, I disabled the 3rd option with my new fix. Now regarding what happens when I press Pull button:

1.Default action is set to Pull - merge
image

2.Default action is set to Pull - rebase
image

@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 11, 2020
@GintasS
Copy link
Collaborator Author

GintasS commented Oct 11, 2020

Updated "After" image on the first comment.

@RussKie
Copy link
Member

RussKie commented Oct 12, 2020

@gerhardol what do you think of this? To me this feels acceptable.

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few naming suggestions (not commented all occurrences) and setting of checked
Otherwise fine

GitUI/CommandsDialogs/FormPull.cs Outdated Show resolved Hide resolved
GitUI/CommandsDialogs/FormPull.cs Outdated Show resolved Hide resolved
@GintasS
Copy link
Collaborator Author

GintasS commented Oct 16, 2020

Why does the build fail, integration tests are fine from my end?

@gerhardol
Copy link
Member

Why does the build fail, integration tests are fine from my end?

They fail for me too, all on
accessor.Fetch.Checked.Should().Be(fetchChecked);
Some defaults maybe?

@RussKie
Copy link
Member

RussKie commented Oct 16, 2020

I'll have a look in a few days

@RussKie
Copy link
Member

RussKie commented Oct 16, 2020

image
Please don't do that, rebase cleanly on HEAD instead.

@GintasS GintasS force-pushed the disable-merge-rebase-commands-when-branch-is-local branch from 0657d73 to 10e0568 Compare October 17, 2020 11:20
@RussKie
Copy link
Member

RussKie commented Oct 17, 2020

There's something wrong, you have too many changes, and most aren't yours.
Please cleanly rebase only your changes on top of the master branch, and squash. https://github.com/gitextensions/gitextensions/wiki/How-To%3A-Squash-and-Rebase-your-changes

@GintasS
Copy link
Collaborator Author

GintasS commented Oct 17, 2020

There's something wrong, you have too many changes, and most aren't yours.
Please cleanly rebase only your changes on top of the master branch, and squash. https://github.com/gitextensions/gitextensions/wiki/How-To%3A-Squash-and-Rebase-your-changes

I removed the changes, but the rebase thing is till valid.

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.

Disable Merge and Rebase commands when current is local branch
3 participants