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

Fix #8188: cannot force push tags #8529

Merged

Conversation

lhiginbotham
Copy link
Contributor

Fix #8188: cannot force push tags

  • Checking the "force push" option in the tags tab of the push form resulted in passing ForcePushOptions.ForceWithLease to the git push command builder
  • Add a check for the force push checkbox for the tags tab in order to pass the correct ForcePushOptions.Force value to the command builder

Test methodology

  • I've tested this out with a dummy remote repo on GitHub and verified that I am now able to force push a tag from one commit to another

Test environment(s)

  • GIT 2.28.0.windows.1
  • Windows 10

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

@ghost ghost assigned lhiginbotham Oct 8, 2020
@codecov
Copy link

codecov bot commented Oct 8, 2020

Codecov Report

Merging #8529 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8529      +/-   ##
==========================================
+ Coverage   54.99%   55.11%   +0.12%     
==========================================
  Files         902      903       +1     
  Lines       65170    65222      +52     
  Branches    11751    11752       +1     
==========================================
+ Hits        35841    35950     +109     
+ Misses      26555    26490      -65     
- Partials     2774     2782       +8     
Flag Coverage Δ
#production 42.06% <100.00%> (+0.12%) ⬆️
#tests 94.83% <100.00%> (+0.02%) ⬆️

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

@vbjay
Copy link
Contributor

vbjay commented Oct 8, 2020 via email

@RussKie
Copy link
Member

RussKie commented Oct 9, 2020

Force push with lease is there to prevent code stomps.

You can't push tags with lease.

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.

👍
Can you please add a test for GetForcePushOption as well?

@RussKie RussKie added hacktoberfest-accepted 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity labels Oct 9, 2020
@vbjay
Copy link
Contributor

vbjay commented Oct 9, 2020 via email

@lhiginbotham
Copy link
Contributor Author

@RussKie

Can you please add a test for GetForcePushOption as well?

I am not sure how I would go about adding a test for this method: 1) it is a private method of GitUI.CommandsDialogs.FormPush and 2) there do not appear to be any tests for the FormPush class that I can see... Are you suggesting I write some tests that somehow execute the form for the various possible "force push" button states and then somehow check what git command was generated? Otherwise, I can change the code to expose GetForcePushOption() as a public static method of FormPush, taking in the three button states as parameters and then just simply test this new public method in a simple test suite.

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

RussKie commented Oct 9, 2020

  1. it is a private method of GitUI.CommandsDialogs.FormPush

We test non-public API with a help of TestAccessor struct. There are few examples, you can have a look at FormPull.TestAccessor, this would probably be the closest to what you'd need.

  1. there do not appear to be any tests for the FormPush

Bummer! But we can fix this :)
I think you can pretty much copy FormPullTest class, remove all but one test that would toggle checkboxes and assert that form.GetTestAccessor().GetForcePushOption() returns the expected.

This should be fairly straight forward, but holler if you get stuck.
Thank you

@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
@lhiginbotham
Copy link
Contributor Author

@RussKie
Done! I hope this is what you were looking for in terms of tests.

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.

👍

GitUI/CommandsDialogs/FormPush.cs Outdated Show resolved Hide resolved
@ghost ghost added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity labels Oct 10, 2020
@RussKie
Copy link
Member

RussKie commented Oct 11, 2020

Please squash and it is ready to go.

@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
	- checking the "force push" option in the tags tab of the push form resulted in passing ForcePushOptions.ForceWithLease to the git push command builder
	- add a check for the force push checkbox for the tags tab in order to pass the correct ForcePushOptions.Force value to the command builder
	- add tests for FormPush.GetForcePushOption()
@ghost ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 25, 2020
@lhiginbotham
Copy link
Contributor Author

@RussKie this is good to be merged, thanks!

@RussKie RussKie merged commit 0e28345 into gitextensions:master Oct 26, 2020
@ghost ghost added this to the 4.0 milestone Oct 26, 2020
@RussKie
Copy link
Member

RussKie commented Oct 26, 2020

Thank you

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.

Unable force push tag
3 participants