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

add support for git push --force-if-includes #2333

Merged
merged 3 commits into from
Dec 30, 2022

Conversation

Ryooooooga
Copy link
Contributor

@Ryooooooga Ryooooooga commented Dec 26, 2022

  • PR Description

  • Please check if the PR fulfills these requirements

  • Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • Docs (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@Ryooooooga Ryooooooga force-pushed the push-force-if-includes branch 2 times, most recently from ad0d0db to 6cfafac Compare December 26, 2022 16:10
@jesseduffield
Copy link
Owner

Reading the docs on the new arg, I don't really understand how it works, or whether it would be unnecessarily strict. Do you have insights into that @Ryooooooga ?

As for the code itself, it's fantastic! Being able to easily check the git version is greater than some version will be very useful

@Ryooooooga
Copy link
Contributor Author

Ryooooooga commented Dec 29, 2022

@jesseduffield If autoFetch is enabled, --force-with-lease is not safe (so I have disabled autoFetch).

@Ryooooooga Ryooooooga force-pushed the push-force-if-includes branch from 6cfafac to ba78ddd Compare December 29, 2022 01:41
@jesseduffield
Copy link
Owner

Can you confirm my understanding is correct: --force-with-lease means if the remote branch has more commits than your remote tracking branch, the push is rejected. With auto-fetch enabled, the remote branch is almost always aligned with the remote tracking branch, meaning it's no safer than using --force.

--force-if-includes is about only allowing a force push if you have the commits locally (for example, on a remote tracking branch), so that if you accidentally lose commits on the remote, you can get them back.

In terms of lazygit, given auto-fetch is enabled by default (and I think that it should be), it sounds like using --force-if-includes is a fairly safe option that shouldn't make things worse for users.

Does that all sound right?

@Ryooooooga Ryooooooga force-pushed the push-force-if-includes branch from ba78ddd to e00f248 Compare December 30, 2022 11:01
@Ryooooooga
Copy link
Contributor Author

@jesseduffield My thoughts are the same as yours.

This PR will cause force push to fail in some cases, but it will have protected the user from loss of commits.

@jesseduffield
Copy link
Owner

alrighty let's merge it and see if it causes any issues

@jesseduffield jesseduffield merged commit 31bdd27 into jesseduffield:master Dec 30, 2022
@Ryooooooga Ryooooooga deleted the push-force-if-includes branch December 31, 2022 13:10
@mtritab
Copy link

mtritab commented Apr 13, 2023

This flag breaks my workflow. Obviously you shouldn't make changes for one user, but is there any chance --force-if-includes could be made an option, or include an option to disable it?

My use case is a CI/CD tool making commits to a repo only I use. In 0.36.0, I could force push my commits as I don't care about commits from the tool being overwritten.

Either way, lazygit is great and I can always compile my own version if necessary (reverted to 0.36.0 for now).

@stefanhaller
Copy link
Collaborator

I wasn't around yet when the original discussion happened, so here's a very late contribution:

--force-if-includes is about only allowing a force push if you have the commits locally (for example, on a remote tracking branch)

That's not quite true. --force-if-includes is only effective if you use --force-with-lease without specifying a sha; in this case, the head of the remote-tracking branch is used as the sha, and this can be unsafe if you have background fetch enabled; or even without background fetch, e.g. if you checkout a different branch and pull it, because the git fetch that runs as part of the pull will (usually) also update other branches' tracking branches. So if you specify --force-if-includes in addition in this case, it will check whether the head of your remote tracking branch appears somewhere in the reflog of your local branch, i.e. if you "had it on your local branch" at some point. If not, the push is rejected.

This option is very useful, and it did save my ass a few times already, e.g. when I work on the same branch from both my Mac and Windows machines and push commits from either.

However, I have also run into false positives with it more than once, and then that's a bit annoying because the only way around it is to drop to the command line and do a git push -f there. One such situation is when you delete a local branch, create a new one from the same commit, and try to push that to the same upstream branch (since deleting a branch also deletes its reflog).

As for @mtritab's use case, I'm not sure how I feel about it; it does seem strange to routinely wipe out commits that someone else (even a tool) commited. I wonder if you will learn to appreciate the safety guard when you ever work from multiple machines and overwrite your own commits by accident. 😄

Anyway, one way to solve this use case could be to remove the option again from lazygit and let people turn it on in their git config if they want it (push.useForceIfIncludes).

@jesseduffield
Copy link
Owner

I agree: given this can be handled in the git config, I'm happy to let users do that.

@Ryooooooga
Copy link
Contributor Author

Indeed, on second thought, it seems better to follow the user's push.useForceIfIncludes setting.
For now, I created a PR to revert e00f248.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants