Skip to content

Conversation

mstv
Copy link
Member

@mstv mstv commented Nov 22, 2024

Fixes #12035

Proposed changes

  • FormResetAnotherBranch: Use quicker command "update-ref"
  • Enable the OK button using a dry-run of the previous command "push"

Screenshots

N/A

Test methodology

  • add testcase

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.

Enable OK button using dry-run of previous command
@mstv mstv self-assigned this Nov 22, 2024
@gerhardol gerhardol requested a review from pmiossec November 23, 2024 18:42
Set BackColor in order to indicate what is wrong
or that the check is going on
@gerhardol
Copy link
Member

@pmiossec update-ref was discussed when the command was added. Do you remember why it was not used?

@mstv
Copy link
Member Author

mstv commented Nov 27, 2024

@pmiossec update-ref was discussed when the command was added. Do you remember why it was not used?

#12035 (comment)

@pmiossec
Copy link
Member

@pmiossec update-ref was discussed when the command was added. Do you remember why it was not used?

I have not found such discussion and from what I remembered it was from the beginning using 'push' because I remembered that it was a clever use of it to have this security check.

But I might be wrong because I didn't remembered that I contributed this very useful feature 😅
Pretty sure because the original idea was from @mdonatas .
Maybe he will remember better.

I still have to find some time* to have a look at the PR but it seems to be a clever move as the security check is still there.

If the dry-run deliver the perf improvement, it's great.
I still don't get why the push is slower than the update-ref but if the improvement is there I think it is a good comprise.

I wanted to have a look at it to be sure we don't have a regression at the security check because it is a very important point for me. But I was really waiting for the perf improvement feedback from the user and as far as I known we still not have it...

*: and it will maybe be difficult in the near future due to some health problems taking some time and energy to settle...

mstv added 2 commits November 28, 2024 21:26
Skip check if ForceReset.Checked
Add cancellation
Do not enable Ok button if branch is invalid
@mdonatas
Copy link
Contributor

This was done so long ago that I have no clue. But it's very likely that I didn't know of the update-ref existence at the time.

ComboBox.TextChanged and ComboBox.BackColor are not fun!
Copy link
Member

@pmiossec pmiossec left a comment

Choose a reason for hiding this comment

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

The case where the branch to reset is automatically filled in the combobox (FormResetAnotherBranch.cs line 78) is not well handled.
Normally it's the case where you sync the branch with its remote and the button should nearly every times be enabled by default.
The Validate() event is well called after the branch name is set but the 1st pre-requisit in the Validate() method is not satisfied and so the button is not enabled.

Validate default candidate at once
Validate once
@mstv mstv merged commit 19e253a into gitextensions:master Dec 2, 2024
3 of 4 checks passed
@mstv mstv deleted the feature/update-ref branch December 2, 2024 18:01
@mstv mstv added this to the v5.2 milestone Jan 11, 2025
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.

The "Reset another branch to here..." option hangs in a large repo
5 participants