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

BackgroundFetch & push --force-with-lease : add a warning message #8533

Merged

Conversation

pmiossec
Copy link
Member

@pmiossec pmiossec commented Oct 9, 2020

Proposed changes

  • Add a warning in the "background fetch" plugin to warn the user
    about some risk when using the "git push --force-with-lease" feature

Screenshots

Before

no warning message.

After

image

Test methodology

  • Manual

Test environment(s)

  • Git Extensions 33.33.33
  • Build 905403fe2edfa879c96e293973afebe555eb6e8e
  • Git 2.28.0.windows.1
  • Microsoft Windows NT 10.0.17134.0
  • .NET Framework 4.8.4240.0
  • DPI 192dpi (200% scaling)

@ghost ghost assigned pmiossec Oct 9, 2020
@codecov
Copy link

codecov bot commented Oct 9, 2020

Codecov Report

Merging #8533 into master will decrease coverage by 0.48%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8533      +/-   ##
==========================================
- Coverage   55.59%   55.10%   -0.49%     
==========================================
  Files         888      899      +11     
  Lines       64110    64776     +666     
  Branches    11467    11629     +162     
==========================================
+ Hits        35640    35694      +54     
- Misses      25766    26344     +578     
- Partials     2704     2738      +34     
Flag Coverage Δ
#production 42.02% <ø> (-0.48%) ⬇️
#tests 94.80% <ø> (ø)

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

{
tb.Multiline = true;
tb.Height = 500;
});
Copy link
Member

Choose a reason for hiding this comment

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

Is this textbox readonly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. That's handled by PseudoSetting.

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.

👍

@@ -25,6 +25,11 @@ public BackgroundFetchPlugin() : base(true)
private IDisposable _cancellationToken;
private IGitUICommands _currentGitUiCommands;

private readonly PseudoSetting _warningForceWithLease = new PseudoSetting("Warning: be aware that the push force with lease feature should be used carefully when the periodic background fetch is enabled. Indeed, the protection brought by this feature could be bypassed if the background fetch has updated the remote branches. And you could loose the commits newly pushed by someone else. So be sure to refresh the revsion grid before doing a force push with lease.", textboxSettings: tb =>
Copy link
Member

Choose a reason for hiding this comment

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

Few typos in the original text.

The text looks a bit verbose, what do you think of this variant?

Suggested change
private readonly PseudoSetting _warningForceWithLease = new PseudoSetting("Warning: be aware that the push force with lease feature should be used carefully when the periodic background fetch is enabled. Indeed, the protection brought by this feature could be bypassed if the background fetch has updated the remote branches. And you could loose the commits newly pushed by someone else. So be sure to refresh the revsion grid before doing a force push with lease.", textboxSettings: tb =>
private readonly PseudoSetting _warningForceWithLease = new PseudoSetting("WARNING: be careful when force push with lease having the periodic background fetch enabled but chose not to auto-refresh after each fetch. You could lose new commits pushed by others to the remote branch. Be sure to refresh the revision grid before doing a force push with lease.", textboxSettings: tb =>

Copy link
Member Author

Choose a reason for hiding this comment

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

updated.

@RussKie RussKie added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity area: plugins labels Oct 10, 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 11, 2020
for when using the "git push --force-with-lease" feature
@pmiossec pmiossec force-pushed the warning_bckg_fetch_force_with_lease branch from f2a1102 to f8f0265 Compare October 11, 2020 23:14
@RussKie RussKie merged commit 99df5a6 into gitextensions:master Oct 12, 2020
@ghost ghost added this to the 4.0 milestone Oct 12, 2020
@pmiossec pmiossec deleted the warning_bckg_fetch_force_with_lease branch October 12, 2020 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants