Add squash merge support #3130
Add squash merge support #3130
Conversation
There was a problem hiding this comment.
Nice, thanks for the contribution. I also use this command a lot, and have configured a custom command for it.
One general thought: in my custom command I went one step further and also make a commit; I found that I want this pretty much 100% of the time, and having to do it manually is annoying. And frankly I wouldn't know what else you would do with the squashed changes in your working copy, other than committing them. Do you think this is a general enough workflow that it makes sense to have your new command do that too? For reference, my custom command is
- key: "S"
command: 'git merge --squash --ff {{.SelectedLocalBranch.Name}} && git commit -m "[SQUASHED] {{.SelectedLocalBranch.Name}}"'
context: "localBranches"
description: "Merge squashed branch"
| } | ||
|
|
||
| func (self *BranchCommands) Merge(branchName string, opts MergeOpts) error { | ||
| cmdArgs := NewGitCmd("merge"). | ||
| Arg("--no-edit"). | ||
| ArgIf(self.UserConfig.Git.Merging.Args != "", self.UserConfig.Git.Merging.Args). | ||
| ArgIf(opts.FastForwardOnly, "--ff-only"). | ||
| ArgIf(opts.Squash, "--squash"). |
There was a problem hiding this comment.
It would be good to check that FastForwardOnly and Squash aren't both true, because that combination doesn't work. (I think it would be appropriate to panic in that case, as that's a programming error; like an assert in other languages.)
Also, it would be good to also add --ff if Squash is true, because it doesn't work otherwise. Yes, that's the default, but people might have merge.ff false in their git config (I do), in which case the command would fail. Now you might argue that this is a bug in git (--squash should imply --ff), but if we can easily work around it, we should.
And finally, we have tests for this function (in branch_test.go), would be good to extend these for the new option.
| func (self *MergeAndRebaseHelper) SquashRefIntoWorkingTree(refName string) error { | ||
| checkedOutBranchName := self.refsHelper.GetCheckedOutRef().Name | ||
| if checkedOutBranchName == refName { | ||
| return self.c.ErrorMsg(self.c.Tr.CantMergeBranchIntoItself) |
There was a problem hiding this comment.
Our new way of dealing with situations like this is to set a DisabledReason on the menu item, so that you can't even invoke it. We just didn't get around yet to converting all existing commands to that new scheme yet. Now I do realize that you just copied this from MergeRefIntoCheckedOutBranch, so you might argue it's out of scope for this PR, but if you feel like doing that extra work, you could add a separate commit (before yours) that converts MergeRefIntoCheckedOutBranch to the new style (it also uses a non-translated text for when you are trying to merge a detached head, which could be fixed at the same time), and use the new pattern for your new command too.
|
@rodakd I haven't heard from you again after giving feedback; do you intend to keep working on this? |
|
Hello since there's no news, should I remake a PR with theses changes ? |
I wouldn't have any objections to that. I would, however, be interested in discussing this question first. Any thoughts on that? |
I personally have no objection but in my use case I don't always make a commit, I might divide the changes in multiple commits or remove some change. However, having a formatted message showing well that it is a merge could be good for clarity and commit are always editable. Maybe having both good be great, but it requires to either open a new window like for rebase options, or having to keys which might a bit too much. What do you think ? |
|
We probably want to have a confirmation panel anyway (this PR has one). Instead of the confirmation panel we could show a menu with two options, something like this, maybe: Or something along these lines. I would vote for making the "and commit" version the first one, as I think it's the more common one. |
- **PR Description** Hello, This PR add merge --squash. A PR already exist #3130, but the author abandoned it, so I remake it. I modified to fit most of the comment made except this one https://github.com/jesseduffield/lazygit/pull/3130/files#r1404808121. I didn't find an existing example and thus didn't know to modify the code to fit it to the new way of doing things. There's still the choice box to commit or not to do as discussed #3130 (comment). I'll do it when I have time, I first need to read the code to see how it really works. Also only english has been made for now.  - **Please check if the PR fulfills these requirements** * [x] Cheatsheets are up-to-date (run `go generate ./...`) * [x] Code has been formatted (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting)) * [x] Tests have been added/updated (see [here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md) for the integration test guide) * [x] Text is internationalised (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation)) * [x] Docs (specifically `docs/Config.md`) have been updated if necessary * [x] You've read through your own file changes for silly mistakes etc <!-- Be sure to name your PR with an imperative e.g. 'Add worktrees view' see https://github.com/jesseduffield/lazygit/releases/tag/v0.40.0 for examples -->
I do
git merge --squash brancha lot so I've added this shortcut for myself. I think some people may want this too so here it is.go generate ./...)docs/Config.md) have been updated if necessary