CRITICAL: unwanted squash during rebase

[ilan-schemoul]

Describe the bug/To reproduce
A
B <= start rebase
modify foo.py content in my code editor
in lazygit I select foo;py and amend it (A)
rebase continue
commit A conflicts with B
So I must modify foo.py now and solve the problem
A popup appears should we continue the merge. I hit esape because obviously I gotta check the diff of my commit, possibly edit the commit messag etc.
Commit seems ok. I append (click A).
And here's the problem. Instead of editing last, conflicting, commit it SQUASH without telling me with the last commit.
So now instead of
A
B
I just have
B (B only has the commit message of B. But it has the code of A and B)

So basically the bug is: if you ever happen to A while in a rebase conflict then the commit A wil merge with commit B.

So hopefully no one will say "hum technically it's not a bug". IDK the technicalities but I'll add personnal context: I work for weeks on commit. I have to constantly rebase them (company policy + we use gerrit which is rebase only unlike GH). I have long series (+20 patches) and can spend a day rebasing just one. Having one commit randomly merged inside my series is not always obvious to notice. I cannot unmerge the commit. I gotta manually split them or redo the whole rebase. It is really distraughting to see my very long rebase turned into random merge sessions.
I love your app but I cannot stress enough of critical I feel like this bug/bad UX is.

Expected behavior
Make amend works the same for conflicting commit as for non conflicting commit.

Screenshots

Version info:
commit=c03b89227092b852d50015d289a7c6d8c69811c5, build date=2025-01-17T13:37:10Z, build source=binaryRelease, version=0.45.2, os=linux, arch=amd64, git version=2.34.1

⟩ git --version
git version 2.34.1

Additional context
git config --list
user.name=
user.email=ilan.schemoul@intersec.com
include.path=/home/ilan/dev/tools/dotfiles/gitinclude
core.excludesfile=/.gitignore
core.autocrlf=input
alias.br=branch
alias.co=checkout
alias.ci=commit
alias.ri=rebase -i
alias.st=status
alias.stat=status
alias.pop=reset HEAD
alias.undo=reset HEAD~
alias.glog=log --graph
alias.tempo=commit -a -m tempo
alias.su=submodule update --init --recursive
alias.rh=reset --hard
alias.cp=cherry-pick
alias.amend=commit --amend
alias.squash=commit --amend -C HEAD
alias.fixup=commit --amend -C HEAD
alias.unstash=stash apply
alias.track=add -N
alias.untrack=rm --cached
alias.vimdiff=difftool -y -t vimdiff
alias.workdir=!sh /usr/share/doc/git/contrib/workdir/git-new-workdir
alias.branch-contains=branch origin/* --remotes --contains
alias.find-merge=!sh -c 'commit=$0 && branch=${1:-HEAD} && (git rev-list $commit..$branch --ancestry-path | cat -n; git rev-list $commit..$branch --first-parent | cat -n) | sort -k2 -s | uniq -f1 -d | sort -n | tail -1 | cut -f2'
alias.diff-no-blank=diff --ignore-space-change --ignore-all-space --ignore-blank-lines
alias.diff-word=diff --word-diff --word-diff-regex="[^ ;]+"
alias.show-word=show --word-diff --word-diff-regex="[^ ;]+"
alias.diff-char=diff --word-diff --word-diff-regex=.
alias.show-char=show --word-diff --word-diff-regex=.
alias.picore=cherry-pick
alias.decoupe=bisect
alias.journal=log
alias.graffiti=tag
alias.ajoute=add
alias.dichotomie=bisect
alias.transaction=commit
alias.deploiement=checkout
alias.cerise=cherry
alias.recherche-par-expression-rationelle=grep
alias.fusionne=merge
alias.planque=stash
alias.ramasse-miettes=gc
alias.tire=pull
alias.pousse=push
alias.pousse-fort=push-for
alias.montre=show
alias.rembobine=reset
alias.supprime-cette-merde=revert
alias.balance=blame
alias.poop=stash pop
alias.truite=merge
color.ui=auto
color.diff=auto
color.branch=auto
color.status=auto
color.pager=true
color.interactive=auto
color.diff.meta=green
color.diff.frag=yellow
color.diff.old=magenta
color.diff.new=bold cyan
color.status.header=bold blue
color.status.added=green
color.status.changed=bold red
color.status.untracked=bold yellow
notes.displayref=refs/notes/*
commit.template=/srv/tools/share/dotfiles/git-commit-template
push.default=upstream
merge.verbosity=1
merge.conflictstyle=diff3
merge.stat=true
merge.renamelimit=100000
merge.ff=false
merge.ours.driver=true
rebase.stat=true
rebase.autosquash=true
diff.renames=copies
url.ssh://git.corp:29418/.insteadof=git.corp:/srv/git
url.ssh://git.corp:29418/.insteadof=ssh://git.corp/srv/git
url.ssh://git.corp:29418/.insteadof=gitolite@git.corp:
url.ssh://git.corp:29418/mmsx.insteadof=git.corp:/srv/git/qdb
url.ssh://git.corp:29418/mmsx.insteadof=ssh://git.corp/srv/git/qdb
url.ssh://git.corp:29418/mmsx.insteadof=ssh://git.corp:29418/qdb
url.ssh://git.corp:29418/mmsx.insteadof=gitolite@git.corp:qdb
url.ssh://git.corp:29418/mmsx.insteadof=git.corp:/srv/git/qrrd
url.ssh://git.corp:29418/mmsx.insteadof=ssh://git.corp/srv/git/qrrd
url.ssh://git.corp:29418/mmsx.insteadof=ssh://git.corp:29418/qrrd
url.ssh://git.corp:29418/mmsx.insteadof=gitolite@git.corp:qrrd
url.ssh://git.corp:29418/mmsx.insteadof=ssh://git.corp:29418/platform
rerere.enabled=true
submodule.platform.update=!/srv/tools/share/scripts/git-submodule-update.sh
submodule.lib-common.update=!/srv/tools/share/scripts/git-submodule-update.sh
tig.bind.generic=r !git rebase -i %(commit)
tig.bind.generic=p @sh -c "echo -n %(commit) | xclip"
tig.bind.generic=f !git commit --fixup=%(commit)
pull.rebase=true
maintenance.repo=/home/ilan/dev/mmsx-master
maintenance.repo=/home/ilan/dev/mmsx-2022-dev
maintenance.repo=/home/ilan/dev/mmsx-4
alias.gl=config --global -l
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
core.fsmonitor=false
core.untrackedcache=true
remote.origin.url=ssh://git.corp:29418/mmsx
remote.origin.fetch=+refs/heads/:refs/remotes/origin/
submodule.platform/lib-common.active=true
submodule.platform/lib-common.url=ssh://git.corp:29418/lib-common
submodule.platform/www/modules/tcpdf/external.active=true
submodule.platform/www/modules/tcpdf/external.url=ssh://git.corp:29418/tcpdf
remote.mob.url=ssh://git.corp:29418/mmsx-mob.git
remote.mob.fetch=+refs/heads/:refs/remotes/mob/
branch.2022-dev.remote=origin
branch.2022-dev.merge=refs/heads/2022-dev
maintenance.auto=false
maintenance.strategy=incremental
log.excludedecoration=refs/prefetch/
submodule.platform.active=true
submodule.platform.url=ssh://git.corp:29418/platform
submodule.platform/www/modules/core/htdocs/javascript/ext/ckeditor-releases.active=true
submodule.platform/www/modules/core/htdocs/javascript/ext/ckeditor-releases.url=ssh://git.corp:29418/ckeditor-releases
submodule.platform/www/modules/core/htdocs/javascript/ext/jasmine.active=true
submodule.platform/www/modules/core/htdocs/javascript/ext/jasmine.url=ssh://git.corp:29418/jasmine
submodule.platform/www/modules/core/htdocs/javascript/ext/jasmine-jquery.active=true
submodule.platform/www/modules/core/htdocs/javascript/ext/jasmine-jquery.url=ssh://git.corp:29418/jasmine-jquery
branch.geom.remote=mob
branch.geom.merge=refs/heads/ilan/geom-minimal-lib
branch.2024-dev.remote=origin
branch.2024-dev.merge=refs/heads/2024-dev
branch.ARCHIVE-preview.remote=mob
branch.ARCHIVE-preview.merge=refs/heads/ilan/keyword-replacement
submodule.platform/www/modules/core/htdocs/javascript/ext/highcharts.com.active=true
submodule.platform/www/modules/core/htdocs/javascript/ext/highcharts.com.url=ssh://git.corp:29418/highcharts.com
submodule.platform/www/modules/core/htdocs/javascript/ext/jsplumb.active=true
submodule.platform/www/modules/core/htdocs/javascript/ext/jsplumb.url=ssh://git.corp:29418/jsplumb
submodule.platform/www/modules/core/htdocs/javascript/ext/pace.active=true
submodule.platform/www/modules/core/htdocs/javascript/ext/pace.url=ssh://git.corp:29418/pace

Lazy git config

os:
edit: "command nvim --server $NVIM --remote-send "<C-\>:FromFTToTab {{filename}}""
editAtLine: "command nvim --server $NVIM --remote-send "<C-\>:FromFTToTab {{filename}} {{line}}""
openDirInEditor: "command nvim --server $NVIM --remote-send "<C-\>:FromFTToTab {{filename}}""
open: "command nvim --server $NVIM --remote-send "<C-\>:FromFTToTab {{filename}}""
customCommands:

• key: 'G'
  showOutput: true
  context: 'global'
  command: 'git push-for {{.Form.Branch | quote}}'
  prompts:
  • type: 'input'
    title: 'Which branch ?'
    key: 'Branch'
    initialValue: '2024-dev'
    suggestions:
    preset: 'branches' # use built-in logic for obtaining branches
• key: 'b'
  showOutput: true
  context: 'files'
  command: 'git absorb'
• key: 'B'
  showOutput: true
  context: 'files'
  command: 'git absorb --and-rebase'
  gui:
  nerdFontsVersion: "3"

The number of lines you scroll by when scrolling the main window

scrollHeight: 10
commitHashLength: 3
disableStartupPopups: true
git:
commit:
autoWrapWidth: 72
log:
order: default

Note: please try updating to the latest version or manually building the latest master to see if the issue still occurs.

I won't do master but I have a super fresh version and the behaviour was also exhibited by 0.42. It's a not a new behaviour and unlikely to change in master as 0.45.2 is super new.

[stefanhaller]

At first I was going to say I would chalk this up to user error. This is simply how git works; the same happens when you do these things on the command line.

After thinking about it more, I guess that lazygit makes it more likely to make this mistake because of the way it represents the conflicting commit in the commits panel. This is something that command-line git doesn't do (typing git log doesn't show it). However, I find it important that we show this "pseudo commit", because it gives you more context about where you are in the rebase, and it lets you look at the diff of the original commit, which is often very useful for resolving the conflicts. But it does make it look like amending things would now amend into that pseudo commit, whereas in reality it amends into the last real commit, which is the one before.

This is made even worse by my insistence of having the "You are here" pointer point at that pseudo commit rather than at your real current head commit. We had a long discussion about this here.

So what can we do to improve this? I don't think I'm willing to reconsider where the "You are here" marker points; I really like it the way it is, and I would find it very confusing if we changed this.

But I suppose we could simply disable the "Amend" command when there is a "conflict" pseudo commit; I can't think of a real use case where you want to amend into the previous commit after resolving conflicts, so this might help.

@jesseduffield Any thoughts?

[ilan-schemoul]

I'll just give my user feedback on this: I did not know that it was a pseudo commit.
Indeed the git cli UI is quite different because I think it clearly tell you solve conflict and do git rebase --continue so I couldn't do any mistake.

But as a user I really wish I'd know I'm working on a "pseudo commit" (maybe different colors, diferent indentation or different icon IDK). If you are positive that the only difference between pseudo commit and commit is just this commit amend behaviour then I guess you can leave commit and pseudo commit with same UI.
Otherwise I'd like to be aware that I work on a pseudo commit.

About You are here of course keep it ! It's the most brilliant thing of lazgit. With our tool (gerrit) we gotta rebase all day long (sad but necessary for clean history), git cli is confusing, "You are here" is my north star !

EDIT: if the change is not particularly hard I can tackle it

[jesseduffield]

My belief that the 'you are here' market belongs on the head commit hasn't changed, and I would happily switch to that. I think that this issue is an example of where confusion arises from pointing at the wrong commit. I think that the commits panel needs to talk in terms of commits, not in terms of commits + working tree.

If you include the working tree, then 'here' should indeed point to the conflicting commit given that the working tree is going to match it (ignoring conflicting files), but I think the commits panel is for commits and the state of the working tree shouldn't factor in to what we consider the current commit to be, especially given that when you're outside a rebase, the head commit remains the head commit regardless of what's in your working tree.

I think that disabling amend to patch up a UX problem is the wrong approach, regardless of how likely somebody is to need to do that. It's just going to look silly if a user needs to do it and our excuse for disabling it was that our UX led to the action not doing what people expected it to.

I think having 'you are here' pointing at the head commit, with the head commit clearly labeled as a conflict (which we already have), resolves the confusion.

[stefanhaller]

I kind of expected this reaction given our earlier discussion. 😄 But I still disagree very strongly.

but I think the commits panel is for commits

When you're in a rebase, the commits panel shows a mixture of real commits and virtual (or pseudo) commits. It did so already before I added the "conflict" pseudo entry. The "conflict" pseudo entry is just as much a virtual commit as the other rebase todos are. The only difference is that it is synthesized from different information, because it's no longer contained in the git-rebase-todo file, so we need to synthesize it from some heuristic instead.

not in terms of commits + working tree

What we display in case of a conflict isn't the working tree. What we display is a "pick" todo that just doesn't show up anywhere else (since it was removed from git-rebase-todo already, but isn't a real commit yet; it's kind of in flight). The working tree is just where conflict handling for this pick todo happens.

having 'you are here' pointing at the head commit,

This is the point that I disagree with the most. I have said this before: "You are here" doesn't mean "This is your current head commit". We don't need to visualize that information, because you can already visually distinguish real commits from virtual commits (the latter have an action column and a blue hash). If we need even more visual cues for making it more obvious what the current head is (e.g. by making the colours even more different or whatever) then we can think about that, but putting a marker next to the head is not the way to do that in my opinion. (In a real GUI app rather than a TUI we might think about drawing a line between the real commits and the virtual ones.)

"You are here" means "this is where you are in the rebase", or "this is the current rebase step that is being carried out". In the normal case, when stopping at an "edit" todo, this happens to be the same as the current head, but for a conflicting commit it's not.

with the head commit clearly labeled as a conflict

But that's wrong! It isn't the head commit that has the conflict, it is the next "pick" todo after it.

I think that disabling amend to patch up a UX problem is the wrong approach

I disagree again. I think it's a misfeature of git that you can amend into the previous finished commit while you are resolving conflicts, so we can help people avoid that mistake. Maybe outright disabling it is a bit too drastic, but we can put up a big fat warning. Maybe something like

╭─Amend changes────────────────────────────────────────────────────────────────────────────────╮
│ WARNING: you are about to amend your resolved conflicts into the last finished commit. This  │ 
│ is very unlikely to be what you want at this point. More likely, you simply want to continue │ 
│ the rebase instead.                                                                          │
│                                                                                              │
│ Do you still want to amend your changes?                                                     │
│ - Cancel                                                                                     │
│ - No, continue rebase                                                                        │
│ - Yes, amend into previous commit                                                            │
╰──────────────────────────────────────────────────────────────────────────────────────────────╯

[jesseduffield]

When you're in a rebase, the commits panel shows a mixture of real commits and virtual (or pseudo) commits. It did so already before I added the "conflict" pseudo entry. The "conflict" pseudo entry is just as much a virtual commit as the other rebase todos are. The only difference is that it is synthesized from different information, because it's no longer contained in the git-rebase-todo file, so we need to synthesize it from some heuristic instead.

That is true, however, it doesn't contradict my statement: I'm saying the commits panel is for commits, and each item is indeed a commit (even if put there based on information outside of git log). My point is that saying that 'here' is the conflicting commit only makes sense to me if our definition of 'here' involves both commits and the working tree, and I think it shouldn't. Though it seems like your argument doesn't require this and that you instead think of 'here' as where we are up to in a rebase.

This is the point that I disagree with the most. I have said this before: "You are here" doesn't mean "This is your current head commit". We don't need to visualize that information, because you can already visually distinguish real commits from virtual commits (the latter have an action column and a blue hash). If we need even more visual cues for making it more obvious what the current head is (e.g. by making the colours even more different or whatever) then we can think about that, but putting a marker next to the head is not the way to do that in my opinion. (In a real GUI app rather than a TUI we might think about drawing a line between the real commits and the virtual ones.)

The reason I originally added the 'you are here' marker is because I didn't think that showing the todo commits in blue was sufficient to explain to the user which commit was the head commit, given that in all other situations the head commit is just the top commit. In other cases where we commits with different colours (red, green, yellow), the head commit is still the top commit so I wanted a clearer indicator for rebases. I think we both agree that we don't need to be confined to a single marker, given that a conflict is a kind of half-half state (you've got the head commit plus the commit that you're conflicting with and your tree matches the head commit but the index matches the conflicting commit). But I think it's fine to say 'this is the head' and 'this is the conflicting commit'.

But that's wrong! It isn't the head commit that has the conflict, it is the next "pick" todo after it.

Sorry I misspoke here, I meant the commit above the head commit should be clearly labeled as a conflict (which it currently is)

I disagree again. I think it's a misfeature of git that you can amend into the previous finished commit while you are resolving conflicts, so we can help people avoid that mistake. Maybe outright disabling it is a bit too drastic, but we can put up a big fat warning. Maybe something like

I'm okay with a warning

FWIW I have a twitter poll asking about where to point the 'you are here' marker and early results favour the conflict commit: https://x.com/DuffieldJesse/status/1883640424914628723

Maybe we need to think up some new ways of communicating the separation between commits and pseudo-commits. Perhaps even a marker against the head commit that's there at all times (it would help in situations where you don't realise that you've scrolled down in the commits panel).

[stefanhaller]

My point is that saying that 'here' is the conflicting commit only makes sense to me if our definition of 'here' involves both commits and the working tree, and I think it shouldn't.

No, the working tree is not involved here at all as far as I'm concerned. The "conflict" entry that we're displaying in the commits panel has nothing to do with the working tree. For example, whether the conflicts have already been resolved or not doesn't have any influence on how it is displayed (that information is shown in the files panel, not in the commits panel).

The "conflict" entry that we display is simply what used to be a "pick" todo, and it is conceptually exactly the same as all the other rebase todos that we display. The only difference is that normally, rebase todos are yet to be executed, i.e. they are completely in the future; once executed, they turn into real commits, there is no in between. The conflict entry is the only one that is currently being executed, and I think this deserves to be shown with a marker.

The reason I originally added the 'you are here' marker is because I didn't think that showing the todo commits in blue was sufficient to explain to the user which commit was the head commit,

And my question is why it is so important to know which commit is the head commit. You need to know this because that's where amended changes go to, but if we disallow this while there are conflicts (or strongly warn about it as suggested above), then what other reason remains why you need to know? I find it more important to know what the current rebase step is; I have said this several times now, and I haven't heard a reason yet why you disagree with that.

[jesseduffield]

The twitter poll ended in a 2:1 split in favour of a pointing at the conflicting commit:

So I'm happy to go with majority opinion.

But to answer your last question: The file changes in the working tree only make sense if you know what the head commit is, because those file changes are relative to that commit. When you encounter a conflict, you'll typically have a bunch of staged files (which we hide by default) which are from the conflicting commit as well as some conflict files which contain content from both. Either way, the files changes that appear are relative to the head commit, not the conflicting commit, and that's why I care about what the head commit is.

[stefanhaller]

But to answer your last question: The file changes in the working tree only make sense if you know what the head commit is, because those file changes are relative to that commit.

I disagree with this again, but before I explain why, let me say that I'm starting to feel a bit bad about continuing this discussion. I'm afraid it might come across as "I want to be right at all cost" or something. I can assure you that's not the case, what I'm after is to find better reasons for why I feel it's wrong to have the arrow point at the head. I had tried that for a while and couldn't get used to it, but back then I couldn't give any better reasons than "I find it confusing". This discussion helps me get more clarity about it and find reasons for why I feel this way (and maybe why the people voting in the twitter poll feel this way, perhaps unconsciously). So to me the discussion is valuable.

So, why is the current head commit irrelevant for making sense of the changes in your working copy? For normal (non-conflicting) diffs, you never need the base commit in order to make sense of a diff. You typically just look at the diff itself and that's enough information. This makes it possible, for example, to send patches to a mailing list and get them reviewed there. It also makes it possible to move commits up/down in a branch using ctrl-j/k (as long as there are no conflicts), and for the user's perception nothing changes about the commit when you do that. Sure, it gets a different hash, but apart from that it feels like the same "object" because the diff stays the same, and that's all that matters.

Now this might be a bit different when we're talking about conflicts in the working copy (before or after resolving them). Yes, they are relative to the head commit, but that's not what matters, really. What's more interesting is which commit changed the conflicting code last, and in many cases that's not the head commit, but some commit further down the branch. It might actually be valuable to highlight this commit in some way (it might be different ones for the different conflicts you have), but that would require blaming the conflicting section, and is a potential improvement that we're not really talking about here.

(Side note: it's interesting that for conflicts during a merge, git has a tool that comes somewhat close: you can use the --merge option of git log to see all commits that had changes for the files that are currently conflicting. I have actually used this a few times in the past to better understand complicated merge conflicts, e.g. using git log --merge --left-right --oneline. What this does is simply filter the log to the paths that are conflicting right now, so it's not quite the same as blaming the conflicting regions of code, but it's still quite helpful, and we might consider doing something similar for rebases at some point.)

[stefanhaller]

Meanwhile, here's a PR for the extra confirmation menu: #4222

[jesseduffield]

I disagree with this again, but before I explain why, let me say that I'm starting to feel a bit bad about continuing this discussion. I'm afraid it might come across as "I want to be right at all cost" or something. I can assure you that's not the case, what I'm after is to find better reasons for why I feel it's wrong to have the arrow point at the head.

No worries, makes sense.

You're right that the head commit itself is not special compared to ancestor commits which could contain the original changes. So I suppose it's not so much that I want to know what the head commit is, but I want to know the boundary between the real commits and the pseudo commits, because if you mistake the pseudo commits for real commits, then the changes that appear in your working tree become nonsensical.

But back to our disagreement, I think I fundamentally consider the word 'here' to mean 'what's the state of my repo, ignoring the fact that I'm in a rebase' i.e. git log + working tree. And so in the commits view it's intuitive for me to have 'here' against the head because it best represents the git log, being at the tip of it (and it's not really possible to capture the working tree element from within the commits view). For you, 'here' refers specifically to where you're at in the rebase. I think both views are internally consistent.

[stefanhaller]

@jesseduffield I just had a thought how to solve the situation with the marker: use section headers instead. We have the technology for inserting section headers in lists, so we could do something similar to what we are doing with the Local/Remote headers in the divergence logs:

The "You are here" marker is no longer needed at all then. I'd keep the Conflict marker though, probably, to make it extra clear what you need to deal with (its design is changing a bit with #4442).

We could consider omitting the "Pending todos" header because it's always at the top, but maybe it's better to include it for clarity. (Like we do with the "Remote" header in divergence logs.)

Also, in rarer situations you could even have three sections: rebase todos, cherry-pick or revert todos, and commits. That's when you are stopped in a rebase, and then you do a cherry-pick or a revert that conflicts. I think it would actually be helpful for understanding what's going on if we had the headers for all three sections in that case.