feat: Allow the fixup helper to return commits on the main branch with a warning.

[mricherzhagen]

PR Description

Sometimes it is permissible/required to fixup a commit, even though it is already on the main branch. Even if it is not permissible it might be useful information which commit would have to be changed.

This PR changes the ctrl-f function to instead of showing an error when the commit is already merged to only show a confirmation warning.

Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run 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)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[mricherzhagen]

This PR now shares a commit with #5201 that moves the creation of the Handler function into a helper function.

[stefanhaller]

I'm against merging this, sorry.

Sometimes it is permissible/required to fixup a commit, even though it is already on the main branch.

I disagree, I see no reason for ever doing this (except when the fixup is for an unpushed commit on main, but in that case ctrl-f works, since origin/main is really the main branch. See #5087 for more discussion of that scenario.)

I find the behavior of showing the warning and then selecting the commit anyway confusing, so I'd rather stay with the current behavior of showing an error.

Even if it is not permissible it might be useful information which commit would have to be changed.

That is true, but it's a different feature, and I wouldn't abuse the ctrl-f command for it. To expand on that: I don't usually use git blame; when I want to do history archeology, I use gitk's "Show origin of this line" command. It allows me to jump to the commit that last changed a given line (and then further back from there if that wasn't yet the change I was looking for). This is super useful functionality, and it would be great to have it in lazygit, however there are lots of challenges to be solved for it. But let's not shoehorn this into the ctrl-f command.

[mricherzhagen]

I disagree, I see no reason for ever doing this (except when the fixup is for an unpushed commit on main, but in that case ctrl-f works, ...)

When I start a new project I normally start developing on main, so my recent use case for using fixup on a pushed main is a relatively fresh private repository, that I want to clean up, before I make it public. It is very frustrating to see, that lazygit found the exact information I want and could give it to me easily, but it doesn't, because it thinks I don't know what I'm doing.

Another use case might be the removal of accidental pushed credentials. There are some valid (but maybe rare) reasons to force push main (One of them is if the user just doesn't care about that sort of thing). lazygit shouldn't make that decision for the users.
There are lots of functions that can seriously mess stuff up in lazygit (e.g. you can just drop commits from main with 2 button presses, pressing lowercase f to fixup a commit into the commit below, even if it isn't a fixup commit and 99% of the time not what you want, ...), so I don't know why this function in particular needs to come with training wheels, when it doesn't even do anything destructive.

So I disagree with your assessment and will slightly change my argumentation for this change: The function is called "Find base commit for fixup". If a base commit is found it should be shown, regardless of which branch it is on. The decision if a commit should be "fixup-able" should not be done in the "find base commit" function. If you want to prevent fixups of commits that are already on main it should be a warning (not an error, please!) when creating the actual fixup! commit and not when searching for the base commit of a change.

I find the behavior of showing the warning and then selecting the commit anyway confusing, so I'd rather stay with the current behavior of showing an error.

The "only added lines" case already did this, before. But I arguably ignored/forgot that case with my added PRs. Though I don't want to have two confirmations after another and don't find that warning particularly helpful anyway. I already know the function isn't always perfect.

From UX side the confirmation behaves exactly the same as the error in the way, that it opens a popup, with what I would consider useful information to display. But the confirmation additionally gives you the choice if you want to jump to that commit or not depending on the information given in the popup. If you used Esc to discard the error before, nothing changes for you.
So maybe the description text just needs to be better to remove that confusion?
And the warning about added lines merged into the warning about main?

I don't usually use git blame; when I want to do history archeology, I use gitk's "Show origin of this line" command.

Yes, that gitk feature is amazing, and I would love to see a way to do this in lazygit. This was not an attempt to shoehorn that, though.

[mricherzhagen]

I disagree, I see no reason for ever doing this (except when the fixup is for an unpushed commit on main, but in that case ctrl-f works, since origin/main is really the main branch. See #5087 for more discussion of that scenario.)

I just ran into this again, and it does not work for repositories that have no remote/are not pushed at all, because then main is the branch that cannot be fixupped.

[stefanhaller]

I can see your point, but I'm still not really convinced.

There are lots of functions that can seriously mess stuff up in lazygit

That's true, and maybe we should consider adding stronger warnings to prevent shooting yourself in the foot when you do these things on a master commit. However, in this case my argument is not that I want to protect the user from doing something stupid, but that I want to make the feature easier to understand, and in my book the current behavior is simpler and easier to understand than your proposed changed behavior.

Also, for the use case that you are describing (cleaning up a fresh, not-yet-published repo) there are easy workarounds to allow you to do what you want. For example, you can rename your main branch to main-to-be as long as you are still reworking its history, and rename it back to main when you are ready to publish. Or you can create a repo-local config file (./.git/lazygit.yml) containing

git:
  mainBranches: []

Both of these things allow you to fixup your main commits to your heart's content. I know what you will respond: "But I shouldn't have to use those workarounds!" For me, this is a good balance between keeping the feature simple (and the code, which I also find crucial) and still allowing people with special requirements to get their work done.

I'll leave this open for now in case others want to chime in.

[mricherzhagen]

Because you already envisioned my first response I will give you my second:

I would have never thought about changing the main branch temporarily or would know how to set that config. That requires deep knowledge and consideration and switching from thinking about the current (actual) work to thinking about how to circumvent this in lazygit - then it's just easier to use gitk to find the correct commit or scroll through them manually. But that feels like lazygit intentionally throwing grit (haha, lazygrit) into my gears which is frustrating and unexpected of such an amazing tool, which usually does quite the opposite in so many different ways.

I also don't really see, how this change makes the feature harder to understand or more complicated, tbh. Quite the opposite, I find the current behavior confusing, because "the commit I want to fixup is right there, just bring me to it, why won't you do it 😭". So I also can't think of a way to make it easier to understand or less complicated.
But I'm obviously also biased, because I envisioned this change. 😅

Crossing my fingers for more people in support of that feature to chime in. 🙂 🤞

[mricherzhagen]

What do you think about only allowing to find base commits on the main branch if the current checked out branch IS the main branch?

[allista]

If I may add to this argument: I tried the proposed workaround by removing main branches.
This is what I got:

...show the generic "not in current view" error. 
This can only happen if a feature branch has more than 300 commits, 

or there is no main branch. <<<

🙃

So basically, whatever the reason I do it for, if I need to simply look up the commit that is the base for the current change, I have to go to a terminal and use log/blame.

For example, in gitk I can do it per line or hunk with a mouse click 👌

---

UPD: correction. It does work for the span of unpublished commits between HEAD and remote. But the ability to find base commit is so basic, that I hardly can understand any reasoning not to allow it. Fixing up published commits is a different matter, but even this is just a place for a modal warning and per-project rules, IMHO.