jesseduffield / lazygit

simple terminal UI for git commands
MIT License
48.13k stars 1.73k forks source link

Make the force push confirmation button configurable #2009

Open AndrewSav opened 2 years ago

AndrewSav commented 2 years ago

Is your feature request related to a problem? Please describe. Force pushing is a destructive operation. Currently the enter is used to confirm it. I've force pushed with lazygit on accident before, when I was going too fast or was clumsy.

Describe the solution you'd like The consequences of mistakenly force pushing (e.g. if the user did not carefully read the dialog) could be dire, while the consequences of cancelling are mild, therefore it would be nice if we could configure the confirmation button to something unusual to our liking, e.g. the f key.

Describe alternatives you've considered An easier to implement option would be simply changing the hardcoded enter button to another hardcoded button, but that would be backward incompatible and could upset some people.

Additional context Converted from discussions.

dtabuenc commented 9 months ago

Yes please! Force pushing is way too easy to do accidentally with potentially dire consequences.

James-Firth commented 4 months ago

Maybe this should be a separate discussion but putting it here since it's related to "push safety":

I think --force-with-lease would be a better flag to include (assuming it's not the default already) since it checks to make sure that your local machine's head for the remote branch is the same as the head of the remote.

stefanhaller commented 4 months ago

I think --force-with-lease would be a better flag to include

Ah, another one claiming this. No it's not. See the discussion starting here.

since it checks to make sure that your local machine's head for the remote branch is the same as the head of the remote.

Which isn't worth anything, because lazygit fetches in the background (by default), which means you will fetch your co-worker's commits without realizing it, and --force-with-lease will happily allow you to blow them away by force-pushing.

You want --force-if-includes instead, and you can enable it globally with git config --global push.useforceifincludes true.

AndrewSav commented 4 months ago

@James-Firth for the record, not everybody agrees with Stefen, for example see video of Scott Chancon I linked here.

Lazygit already uses --force-with-lease by default for force pushes, and I agree with you, that this is certainly better than just --force, but I do feel this is out of scope of my original issue - that one was about configuring an alternative key for force push confirmation.

dtabuenc commented 4 months ago

I don't see how Scott Chancon is disagreeing with @stefanhaller is saying. It's not that --force-with-lease is never useful, it just simply forces you to be up-to-date with your fetch before you are allowed to force push . Since lazygit is always fetching in the background and keeping your remote heads up-to-date, --force-with-lease adds little or no value in this particular case since your remote tracking branches will always be-up-to-date, and hence like @stefanhaller said, will in almost all cases happily let you blow away the remote changes on a force.

AndrewSav commented 4 months ago

adds little or no value in this particular case since your remote tracking branches will always be-up-to-date

Thank you for this. Well this is much more palatable, Stefen wrote:

Not because one is better or worse than the other, but because there is this widespread misconception that --force-with-lease is a safer option than --force (as the comments above in this very issue show), and I don't want lazygit to contribute to spreading this misinformation. ... Many people in the git community are now of the opinion that the --force-with-lease option should only be allowed with an explicit argument.

When Stefen was talking about "Many people in the git community" it did not sound like he had this particular case in mind. It sounded more, like he thought that --force-with-lease is strictly inferior to --force lazygit or not, which is obviously not true. @stefanhaller did I misread you and you had not actually meant that?

James-Firth commented 4 months ago

@stefanhaller I just started using lazygit about a week ago (it seems great!) so I didn't know it auto-fetched when I suggested that flag. I don't believe my personal VS Code or Sublime Merge settings have that so in those cases it works well for me. But I agree with auto-fetch (especially frequent) on it's not safe enough to prevent overwrites!

I also didn't know about --force-if-includes, I'll have to look into that thanks for mentioning that and linking to the other discussion.

@AndrewSav I'll have to give that a watch, thanks for the link!

@AndrewSav apologies for hijacking the Issue, I was trying to not bloat the Issue log. @stefanhaller Feel free to mark my posts as Off-Topic so it doesn't clutter this any more.

jwhitley commented 4 months ago

Of note, Stefan's guidance re: --force-with-lease is also spelled out explicitly in the current git push docs: https://git-scm.com/docs/git-push

Likewise, the docs mention that --force-if-includes can be a no-op in certain usages (!!):

If the option is passed without specifying --force-with-lease, or specified along with --force-with-lease=:, it is a "no-op".

So despite attempts to put guard rails around --force, it remains full of sharp edges.

stefanhaller commented 4 months ago

Of note, Stefan's guidance re: --force-with-lease is also spelled out explicitly in the current git push docs: https://git-scm.com/docs/git-push

Yes, the docs have a lengthy section about background fetches. That's good, but background fetches are not the only problem with --force-with-lease, and it's unfortunate that the docs don't say anything about that. Even if you are careful to turn off auto-fetch in all the editors and IDEs that you have running, there are still enough scenarios left where --force-with-lease does not protect you from blowing away your co-worker's commits. Here's one, building on the example in Scott's video:

I am pretty convinced that most average git users will not be aware of these subtleties, and will just use --force-with-lease thinking they are safe, because Scott Chacon told them so.

If the option is passed without specifying --force-with-lease, or specified along with --force-with-lease=:, it is a "no-op".

Ah, thanks for reminding me of this. Turns out I was wrong here again, apologies for that: I wrongly thought that --force-if-includes also works together with --force, but it only works together with --force-with-lease. So I take back my suggestion to change lazygit to use --force instead of --force-with-lease; we can't do that, because the git config push.useforceifincludes would no longer work.

And before somebody asks why lazygit doesn't use --force-if-includes: it used to, but it got in the way of some people's workflows (see here), and instead of making it configurable, we removed it and decided to rely on the existing git config to turn it on for those who want to use it.