jesseduffield / lazygit

simple terminal UI for git commands
MIT License
53.02k stars 1.85k forks source link

disableForcePushing, true or false by default? #1668

Open oscarmlage opened 2 years ago

oscarmlage commented 2 years ago

disableForcePushing

I would like to ask this community in general if wouldn't be a better approach just set disableForcePushing as true by default in order to not allow to force pushing instead of do it the other way around?.

Had something like a bad experience with the force pushing banner (something similar to #855), so in my mind it's like something that involves --force is incompatible with the lazy word, that's why I'm asking if the other way around (disable force pushing by default and enable it just in case you know what you're doing) would be more appropriate.

Thanks in advance for your thoughts.

jesseduffield commented 2 years ago

Happy to wait on others to see their perspectives before giving my 2c

Menchen commented 2 years ago

In my opinion, the best way for me as a default is an forced countdown(5s), because there's been case for me that I Shift-P and pressed enter out of habit without reading it first.

Another way to do it is to make people type 4~ some random character or something like the repo name(Yes,I just force pushed into wrong repo today..) before accepting it.

As for disableForcePushing, my opinion is true by default with a warning about a way to force push in CLI and there's a config to change it.


Edit: I think lazy for me should be an brainless way to use git, and a serious popup warning me for dangerous operation so I can think about it.

jesseduffield commented 2 years ago

The main reason I don't want to make force pushes disabled by default is because anybody who uses rebasing as part of their daily workflow needs to frequently force push. That includes:

At the time this issue was created, lazygit would do a generic git push --force rather than specifying the upstream remote and branch, but that's since been rectified. So the impact of force pushing is now less than it once was.

When it comes to choosing a default, I don't think it's a good idea to require all users who want to make use of a fairly common git feature to enable it explicitly in their config in order to be able to use it, for the sake of the users who might accidentally use the feature.

I do like the idea of requiring the user to type in 'force' in order to do a force push, but I would not want this to be the default, again because force pushing is something that pretty much anybody who rebases will frequently do. So I would have that safeguard disabled by default.

Menchen commented 2 years ago

Another good way of handling this is an popup in the first force push like the welcome popup asking the user about enabling safeguard or not, the reason is I doubt not many user even know about the safeguards exist before making a mistake, and blaming/regretting about it when google leads them to this issue .

And in my opinion typing a little to let some time for your brain think twice for possible breaking changes thats non trivial to undo is worth the extra time saved digging in git reflog, fearing lost in progress.

———————————

Edit: another way to implement safeguards is having a dedicated keybind for forced push. I think the reason accident happened is because it’s not doing what user expect, a normal push should be safe, tough personally I think typing safeguards is better, maybe we can have hybrid solutions with dedicated keybind that skip the typing safeguards(but leave current popup on)?

jesseduffield commented 2 years ago

I like the idea of a popup that appears once which allows the user to set the config value from the popup

mauroporras commented 2 years ago

The --force-with-lease flag is being used:

https://github.com/jesseduffield/lazygit/pull/188/files#r211100140

That's more than enough guard for me.

AndrewSav commented 1 year ago

I think the current default is sensible. It asks you every time when you are trying force push. If you are using GitHub as an extra layer of protection you can also use protected branches, to those ones where it matters.

patrick-motard commented 8 months ago

I'm a new user of LazyGit. Before using Lazygit, when I force push, 99.9% of the time i used --force-with-lease. Today i rebased my branch in lazygit and then attempted a push. It gave me a popup asking for permission to force push. It suprised me that I was not given the option to select which type of force push to use.

Now I come to this ticket and see @mauroporras 's comment showing that behind the scenes, Lazygit is using --force-with-lease. This was a suprise to me as there is no indication in the UI that this is happening.

I suggest that instead of a popup with a binary decision, force vs cancel, we instead present the user with a list of options. Force, force with lease (safe option, should be the default selection in the list), and cancel.

stefanhaller commented 8 months ago

Lazygit does not use --force-with-lease. It's also a misconception that this is a safer option than --force; it isn't, it offers very little additional protection.

The option that is a lot safer is --force-if-includes, and lazygit used to use it at some point (together with--force). It no longer does though; the idea is that those who want to use the option can enable it globally with a git config. Yes, you could argue that it should be enabled by default, but there are reasons why it isn't.

I have more thoughts and information about these topics, but I'm on my phone right now and can't dig up links.

patrick-motard commented 8 months ago

Lazygit does not use --force-with-lease

@mauroporras 's PR link is no longer what is in master. There are however references to force-with-lease in the codebase.

It's also a misconception that this is a safer option than --force; it isn't, it offers very little additional protection.

As much as I want to engage with this opinion, I feel that discussion about which flags are better on the force flag are besides the point. I think the user should have choices, that are easy to make in the UI.

The issue I have with the current behavior is that the force prompt is either force push or cancel. If the user indicates that a force push is desired, it would be nice if we provided them with the ability to specify which flags they want to add to the force push.

AndrewSav commented 8 months ago

@stefanhaller

Lazygit does not use --force-with-lease.

Why is the command log window in lazygit reporting it then? (v0.40.2)

stefanhaller commented 8 months ago

I apologize for the misinformation, yes it does use --force-with-lease instead of --force. Sorry.

I am inclined to open a PR to change this to --force though. 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. (To elaborate: --force-with-lease is only safer if you use it with an explicit argument, i.e. --force-with-lease=fa1afe1, but you have to keep track of that hash manually (by remembering it from the last time you pulled or pushed the branch), and it's unclear how you would do that. A tool like lazygit can't do it for you. But if you use --force-with-lease without an explicit hash, it defaults to using the remote tracking branch, and this doesn't protect you against overwriting upstream commits at all, especially with a tool like lazygit that periodically fetches in the background. 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.)

it would be nice if we provided them with the ability to specify which flags they want to add to the force push.

When it comes to --force vs --force-with-lease I strongly disagree, as explained above.

The situation is different for --force-if-includes (which only has an effect when used in addition to --force). So far we have gone with the principle that if there is a git config to set this globally, we rely on that instead of providing our own way of including the option (see #2664 for another example).

AndrewSav commented 8 months ago

@stefanhaller

But if you use --force-with-lease without an explicit hash, it defaults to using the remote tracking branch, and this doesn't protect you against overwriting upstream commits at all

There is a bit of mismatch here. The official documentation says:

--force-with-lease alone, without specifying the details, will protect all remote refs that are going to be updated by requiring their current value to be the same as the remote-tracking branch we have for them.

Note how you say that it does not protect at all and the official documentation says that it will protect all remote refs. Could you please elaborate what you mean? I'm sure you have a coherent picture in your mind, but it's a bit hard to follow it without further details.

stefanhaller commented 8 months ago

The git documentation uses the word "protect" in the technical sense to mean that --force-with-lease protects you from pushing a branch when your remote tracking branch doesn't match the upstream branch. That is of course true, but it doesn't protect you from overwriting a co-worker's commits at all. Here's a simple, real-world scenario:

Many people wrongly believe that --force-with-lease guards against losing commits in this way. It doesn't.

--force-if-includes would actually have prevented this push to succeed, so it's a lot better.

AndrewSav commented 8 months ago

The git documentation uses the word "protect" in the technical sense to mean that --force-with-lease protects you from pushing a branch when your remote tracking branch doesn't match the upstream branch.

I'm glad we agree that it gives more protection than just --force. It's also for no cost.

Many people wrongly believe that --force-with-lease guards against losing commits in this way

First of all the claim many people believe seems subjective. Do we have any metric to gauge how many people believe that vs those that do not or those that did not give it any thought? I understand that when you are exposed to certain threads and see things repeated quite a few times, it may lead you to believe that majority, or sizeable percentage of people are wrong(ly believe things). At the same time it is quite possible, that it's just a small percentage of people. A small percentage of people is bound to wrongly believe with regards to almost any topic.

Secondly, the fact that "many people" wrongly believe something is not lazygit's job to fix, even if we could convince ourselves that it's true, which I'm currently struggling with. lazygit's job is to be safe and dependable tool, and since we established that -force-with-lease is (somewhat) safer than --force in the absence of other downsides it looks like a better alternative.

I do not have an opinion with regards to --force-if-includes, the above discussion is trying to compare --force and --force-with-lease only.

stefanhaller commented 8 months ago

I'm glad we agree that it gives more protection than just --force.

No, we don't agree on this. I was just trying to explain the apparent contradiction between what I said above ("doesn't protect at all") and the git documentation ("will protect all remote refs"). I probably didn't explain that well enough, so let me try again: as a user, I want to be protected against accidentally overwriting commits by force-pushing. --force-with-lease does not achieve this, because while it will might make the push fail in some cases (depending on how recently I fetched), it will also in many cases make the push succeed, especially with lazygit that auto-fetches every minute by default. This is no protection, nothing I can rely on.

The only scenario where --force-with-lease does offer reliable protection is when I do all of the following:

I think it's safe to say that for the majority of users this does not apply.

and since we established that -force-with-lease is (somewhat) safer than --force in the absence of other downsides it looks like a better alternative.

It has a big downside, which is that people are fooled into believing that it will guard against overwriting other people's work accidentally, where in most real life scenarios it doesn't. With --force users are scared and maybe go and inspect the remote branch manually to see if there's anything on it that they don't have; with --force-with-lease they think they don't have to do that, which is just wrong.

Yes, I do think it's harmful to use --force-with-lease. It gives you a false sense of security.

AndrewSav commented 8 months ago

For those who are interested there is a video here of Scott Chacon, a founder of GitHub and author of the Pro Git book (IMO the best book about Git) on --force-with-lease. The video itself is long but the block on --force-with-lease is quite short. It is always interesting to me to watch an opinion of a recognized expert.

dtabuenc commented 8 months ago

@AndrewSav Strictly speaking, --force-with-lease does offer some additional protection than just --force. However, as @stefanhaller points out, in most real-world scenarios it offers extremely little additional protection. The argument is that --force-with-lease option is simply a protection of last resort, and should not be causing you to be any less diligent in manually checking your remote hashes before a force push.

I don't think I'd go as far as @stefanhaller and change it back to --force. I think the optimal is to have --force-with-lease as long as people don't use it as an excuse to be careless with their force pushes.

So to summarize, yes.. what Scott Chacon is saying is true. It is a safe option to have on as default, but the minute you let the knowledge of this option being on affect your behavior with regards to force pushing, you are probably at a greater risk of losing commits.

AndrewSav commented 8 months ago

I don't think I'd go as far as @stefanhaller and change it back to --force. I think the optimal is to have --force-with-lease as long as people don't use it as an excuse to be careless with their force pushes.

I agree with that, I'm also in favour of treating the users like adults ;)

stefanhaller commented 8 months ago

Oh wow, I just watched Scott's video. Yes, Scott is an expert, and I admire his work on the Pro Git book as much as you do, but what can I say, the advice that he is giving here is very bad. He literally tells people to "just use this, it's kind of a safe force push". Without any qualification. That's really, really bad advice.

I reached out to him for clarification, and I have to say that I find his response somewhat unsatisfactory:

It's a good point. My assumption is that if people are force pushing on the command line, they probably don't have some GUI they're running but not using that is doing automatic fetches. I also feel like I didn't say it was "safe" but rather "safer". But force-if-includes is a good suggestion, I can put that in my next talk :)

lionel- commented 1 month ago

@stefanhaller Since we rely on the user having set push.useForceIfIncludes to true for the safer variant of force-pushing, and since this option is a no-op if you don't use --force-with-lease, it seems we do need lazygit to use the latter by default so that the git config for using --force-if-includes by default may have an effect?

stefanhaller commented 1 month ago

@lionel- Yes, that's true, and we do.

I know that I said above that I propose to change that to just --force, but I was wrong about that, for the exact reason you mention, so I retracted that a while ago (not in this issue, but over in the other one).