Code

Opened 11 months ago

Closed 12 days ago

#20953 closed Cleanup/optimization (wontfix)

git push -f in docs needs a stern warning

Reported by: EvilDMP Owned by: nobody
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/working-with-git/#rebasing-branches:

git push -f origin ticket_xxxxx

This should be accompanied by a note saying something like:

Never use push -f without specifying a remote, otherwise you may be force-pushing changes to one or more unexpected remotes without even realising it. If you force-push to the wrong remote, you will be sorry.

Ways of avoiding accidents with push -f include:

  • thinking twice and three times before using push -f
  • never using push -f, and instead:
    • deleting the branch on GitHub and pushing again, or:
    • creating a new local branch with `git checkout -b <new-branch-name> and pushing that
  • using a password (Git with HTTPS) rather than your key (Git with SSH) for your remotes' URLs

(perhaps there are some more elegant solutions).

Attachments (1)

pre-push (1.8 KB) - added by akaariai 11 months ago.
pre-push script to prevent force-pusing

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 months ago by mjtamlyn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I believe the elegant solution is to get the most up to date version of git, or set the parameter (can't remember off the top of my head) that will make sure that push by default only pushes the current branch.

comment:2 Changed 11 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Cleanup/optimization

comment:3 Changed 11 months ago by EvilDMP

It should also be possible to prevent force-pushes to Django on GitHub: https://enterprise.github.com/help/articles/disable-force-pushes

comment:4 Changed 11 months ago by mjtamlyn

That is a piece of functionality in github enterprise, I don't think it can be done on normal github.

comment:5 Changed 11 months ago by EvilDMP

Where the Working with Git docs say things like: git remote add upstream git@github.com:django/django.git this should say instead: git remote add upstream https://github.com/django/django.

This isn't always read-only in the way that GitHub's now-deprecated git:// URLs are, but at least it requires a password, even if you have write-access to the repository, when pushing.

Then of course the git push -f section in the docs needs a note with the stern warnings mentioned above, and a mention of alternatives to push -f.

(It's actually possible to delete a remote's push URL to prevent pushing, but this is probably beyond the scope of this document.)

comment:6 Changed 11 months ago by mjtamlyn

I disagree. I do not want to encourage people to have to know their github password well enough to type it regularly. HTTPS methods are largely there to provide more help for people who can't use SSH - not as a better alternative.

I think adding a section about the git functionality could be useful, but mainly just never force push!

comment:7 Changed 11 months ago by EvilDMP

People who do want to push should use SSH. This is for people who don't want to find themselves pushing inadvertently, as I did last week.

It's true that there will be a very small number of people who find themselves in my position - with write access, and just enough knowledge to be dangerous - but even one is quite enough.

I've been through a certain amount of discussion on #git and #github, and even conversations with GitHub support about the deprecated 'git://' format, and using HTTPS rather than SSH does seem to be regarded as an acceptable way of putting up a barrier for oneself to accidental pushing.

But if that's not a good solution, and the documentation should no longer say to use push -f after squashing, what should it say?

Last edited 11 months ago by EvilDMP (previous) (diff)

comment:8 Changed 11 months ago by mjtamlyn

Well there's no alternative to force pushing your squashed changes. Really the only good solution here IMO is to document (for now) the improved git options, and perhaps increase the warning.

Anyway, it's a very small edge case - it's not happened often and one learns the lesson pretty quickly! The category of people with push access who don't want to push to github is a pretty small category.

comment:9 Changed 11 months ago by akaariai

Hmmh, git has a pre-push hook as of 1.8.2. I am not sure if this could be used to disable force-push, but this is worth exploring. I don't have 1.8.2 available on this machine, so I can't test this just now.

comment:10 Changed 11 months ago by akaariai

It seems the attached pre-push script can be used to prevent force pushing (and accidental pushing/deletion of upstream branches, too). Requirements are:
1) git 1.8.2 or later
2) change the pre-push variable django_origin to your django/django's remote name (for me, upstream).
3) place the pre-push script (with that name) to .git/hooks/

Do not test this against django/django. This is a quick hack, so I don't trust it enough for that. You can however test it against your own github branches by changing the django_origin name.

Changed 11 months ago by akaariai

pre-push script to prevent force-pusing

comment:11 Changed 4 months ago by anubhav9042

Can't we just replace this in the docs:
git push origin ticket_xxxxx --force

I think that will not create any problem of not finding remote as we are adding --force at the end.
As now if he/she skips adding force, then the repo will not be updated

Last edited 4 months ago by anubhav9042 (previous) (diff)

comment:12 Changed 12 days ago by timo

  • Resolution set to wontfix
  • Status changed from new to closed

I don't think we need to get into the ramifications of force pushing as these docs are aimed at contributors who won't be able to push to Django anyway.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.