Skip to content

fix(push): fix tracking reference not well selected #11954

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 19, 2024
Merged

fix(push): fix tracking reference not well selected #11954

merged 1 commit into from
Oct 19, 2024

Conversation

pmiossec
Copy link
Member

@pmiossec pmiossec commented Oct 2, 2024 •
edited
Loading

by retrieving the gitref corresponding to local branch combobox

fixes #11899

Screenshots

Before

image

After

image

Test methodology

  • Manual

Test environment(s)

  • GIT
  • Windows

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

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

@pmiossec
Copy link
Member Author

pmiossec commented Oct 2, 2024

Alternative to #11950 / poke @RussKie
I think this change handle better the case where push dialog is open when no branch is checked out.

mstv
mstv reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@mstv mstv left a comment

Choose a reason for hiding this comment

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

Looks sensible, have not run either

src/app/GitUI/CommandsDialogs/FormPush.cs Outdated
@@ -780,24 +780,27 @@ private void BranchSelectedValueChanged(object sender, EventArgs e)
{
if (PushToRemote.Checked)
{
if (_NO_TRANSLATE_Branch.SelectedItem is GitRef branch)
IGitRef selectedRef = _NO_TRANSLATE_Branch.SelectedItem as IGitRef;
Copy link
Member

Choose a reason for hiding this comment

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

Why change the name?

Suggested change
IGitRef selectedRef = _NO_TRANSLATE_Branch.SelectedItem as IGitRef;
IGitRef branch = _NO_TRANSLATE_Branch.SelectedItem as IGitRef;

or

Suggested change
IGitRef selectedRef = _NO_TRANSLATE_Branch.SelectedItem as IGitRef;
IGitRef branchRef = _NO_TRANSLATE_Branch.SelectedItem as IGitRef;

Copy link
Member Author

Choose a reason for hiding this comment

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

branch is not expressive at all for me.
Because we take the SelectedItem it seems natural to me to have selected in the name.
I would prefer selectedBranchRef or, as a 2nd choice, branchRef.

Copy link
Member

Choose a reason for hiding this comment

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

I, too, am fine with selectedBranchRef.

RussKie
RussKie reviewed Oct 3, 2024
View reviewed changes
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.

I'm not sure this is a more correct fix than proposed by me in #11950.
The reason is that the order of the dropdown initialisation is still incorrect, which means _NO_TRANSLATE_Branch.SelectedItem is null when the dialog first loaded and this method executes.

gerhardol
gerhardol approved these changes Oct 6, 2024
View reviewed changes
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.

I believe this PR addresses the core problem I see: The selected ref is not always correct. I have seen that occasionally but not have been able to reproduce. The remote ref may not always match the local.
It can well be merged together with #11950 that also addresses the standard case

@pmiossec
Copy link
Member Author

pmiossec commented Oct 8, 2024

The reason is that the order of the dropdown initialisation is still incorrect, which means _NO_TRANSLATE_Branch.SelectedItem is null when the dialog first loaded and this method executes.

It was made on purpose to not load the items of the combobox (and load them only when interacting with it) so that there is no slow down on opening (especially when you have a lot of local branches). As a consequence, indeed, _NO_TRANSLATE_Branch.SelectedItem is null.
The goal is to not pay the cost of loading the branches every times the form is opened for something that is very rarely used (we push the current branch or multiple branch, nearly never another local branch) and so handle the case of the current branch specifically as this fix is intended to do...

I have seen that occasionally but not have been able to reproduce

You could reproduce it when trying to push to a tracking remote branch that has not the same name than the local branch.

I'm not sure this is a more correct fix than proposed by me in #11950.

@RussKie If you prefer your fix, you will have to handle the case where you are in detached HEAD state.
When I started to work on the fix for this issue, I end up with the same fix before seeing the regression in this edge case. And this PR is the result of trying to provide a better fix that handle the 2 cases....

@RussKie
Copy link
Member

RussKie commented Oct 10, 2024

The reason is that the order of the dropdown initialisation is still incorrect, which means _NO_TRANSLATE_Branch.SelectedItem is null when the dialog first loaded and this method executes.

It was made on purpose to not load the items of the combobox (and load them only when interacting with it) so that there is no slow down on opening (especially when you have a lot of local branches). As a consequence, indeed, _NO_TRANSLATE_Branch.SelectedItem is null. The goal is to not pay the cost of loading the branches every times the form is opened for something that is very rarely used (we push the current branch or multiple branch, nearly never another local branch) and so handle the case of the current branch specifically as this fix is intended to do...

Oh, that's the insights I wasn't aware of. This makes sense, and it'd be great to add comments capturing this knowledge, so no one would try to "fix" it again.
With that I'll close my change.

@RussKie RussKie mentioned this pull request Oct 10, 2024
Closed
@RussKie RussKie added this to the 5.0.1 milestone Oct 10, 2024
@pmiossec pmiossec mentioned this pull request Oct 10, 2024
Closed
@pmiossec
Copy link
Member Author

it'd be great to add comments capturing this knowledge, so no one would try to "fix" it again.

Done.

mstv
mstv approved these changes Oct 18, 2024
View reviewed changes
src/app/GitUI/CommandsDialogs/FormPush.cs Outdated
@@ -780,24 +780,27 @@ private void BranchSelectedValueChanged(object sender, EventArgs e)
{
if (PushToRemote.Checked)
{
if (_NO_TRANSLATE_Branch.SelectedItem is GitRef branch)
IGitRef selectedRef = _NO_TRANSLATE_Branch.SelectedItem as IGitRef;
Copy link
Member

Choose a reason for hiding this comment

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

I, too, am fine with selectedBranchRef.

@pmiossec
fix(push): fix tracking reference not well selected
279fd4c 
by retrieving the gitref corresponding to local branch combobox text

fixes gitextensions#11899
@pmiossec pmiossec force-pushed the fix/push_tracking_ref branch from 4516d69 to 279fd4c Compare October 18, 2024 22:46
@pmiossec
Copy link
Member Author

Renamed. Squashed and rebased....

@pmiossec pmiossec merged commit 7c244e4 into gitextensions:master Oct 19, 2024
4 checks passed
@pmiossec pmiossec deleted the fix/push_tracking_ref branch October 19, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RussKie RussKie RussKie approved these changes

@gerhardol gerhardol gerhardol approved these changes

@mstv mstv mstv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

"Replace tracking reference" doesn't work on the Push dialog
4 participants
@pmiossec @RussKie @gerhardol @mstv