Skip to content

Conversation

@stefanhaller
Copy link
Collaborator

  • PR Description

For users who have the rebase.autoSquash git config set to true (like me :), any regular rebase would squash fixups in addition to rebasing. That's undesirable, squashing fixups should be a deliberate action. This PR fixes that.

@jesseduffield I'm not sure if you appreciate this style of adding a "failing" test first to demonstrate a bug. We use this pattern a lot at work, and I find it useful.

…al rebase

For users who have the rebase.autoSquash git config set to true, any regular
rebase will squash fixups in addition to rebasing. Not good -- we'll fix that in
the next commit.
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2023

Uffizzi Preview deployment-15067 was deleted.

This fixes the problem shown in the previous commit.
@stefanhaller stefanhaller force-pushed the do-not-autosquash-in-regular-rebase branch from cf5036b to 1da762c Compare February 9, 2023 17:21
@jesseduffield
Copy link
Owner

I'm not sure if you appreciate this style of adding a "failing" test first to demonstrate a bug
I'm all for TDD :)

As for the PR itself: I typically err on the side of letting the user's git config decide the behaviour of lazygit. I'm not clear why the typical user who has rebase.autoSquash set globally would want that overridden by lazygit. Could you explain that to me?

@stefanhaller
Copy link
Collaborator Author

I have rebase.autoSquash in my config because I usually want this to be the default when doing an interactive rebase on the command line.

When I do a regular, non-interactive rebase on the command line, it has no effect, which is what I want.

When lazygit does a regular, non-interactive rebase though, it passes --interactive, so the config does take effect in that case. I'm not entirely sure why it does that, so another fix could be to remove the --interactive.

Does that make it clearer?

@Ryooooooga
Copy link
Contributor

How about making it configurable like git.commit.verbose?

https://github.com/jesseduffield/lazygit/blob/master/docs/Config.md#:~:text=signOff%3A%20false-,verbose,-%3A%20default%20%23

git:
  rebase:
    autoSquash: default # 'default', 'always', or 'never'

@stefanhaller
Copy link
Collaborator Author

No, another option doesn't help with this. I'm not trying to find a way to tweak lazygit's behavior to everybody's needs; I'm trying to solve a problem where lazygit behaves in unexpected ways by default.

To reiterate my point: in my mental model, selecting "master" in the branches panel and hitting "r" is equivalent to doing git rebase master on the command line. The latter doesn't autosquash even if the rebase.autoSquash git config is on; but the former does, and that's confusing and unexpected.

@stefanhaller
Copy link
Collaborator Author

This is not really an important issue for me, personally. I can totally just remove the rebase.autoSquash option from my git config, now that I use lazygit for almost all my rebase work. 😄 I just thought it would be nice to fix this in case other people are as confused (or annoyed) by the behavior as I was when I first saw it.

@jesseduffield
Copy link
Owner

The reason for lazygit passing the --interactive flag is that if a rebase is not interactive, it's hard to render it the way I want to i.e. showing the commits between the HEAD and the final commit. I say hard, but it's probably not that hard: we would just need to add a new concept of a 'rebase commit' that you can't actually interact with (e.g. dropping/fixup'ing).

If we take that approach, then we can remove the --interactive flag and lazygit will behave less surprisingly

@jesseduffield
Copy link
Owner

To be more specific: I like being able to show a 'you are here' message when we're rebasing so you have a visual cue as to how far you still have to go.

@stefanhaller
Copy link
Collaborator Author

I see, thanks for the explanation. Makes sense to me. But as long as we do that (pass -i to get nicer rendering), I still think it makes sense to also pass --no-autosquash to make the rebase behave more like a non-interactive rebase would.

@jesseduffield
Copy link
Owner

yep I agree with that

@jesseduffield
Copy link
Owner

I've created an issue for removing the --interactive from the rebase: #2442

@jesseduffield jesseduffield merged commit 31fcec1 into jesseduffield:master Feb 15, 2023
@stefanhaller stefanhaller deleted the do-not-autosquash-in-regular-rebase branch February 15, 2023 15:32
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.

3 participants