Drop merge commits

[stefanhaller]

• PR Description

Allow deleting a merge commit. We only allow this when the merge commit is the only selected item, and only outside of a rebase. The reason for this is that we don't show the "label" and "reset" todos in lazygit, so deleting a merge commit would leave the commits from the branch that is being merged in the list as "pick" commits, with no indication that they are going to be dropped because they are on a different branch, and the merge commit that would have brought them in is gone. This could be very confusing.

Fixes #3164.

• 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

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for 13e9e1d1	✅ 92.08%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (13e9e1d)	Report Missing	Report Missing	Report Missing
Head commit (078445d)	51323	44298	86.31%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#4094)	101	93	92.08%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

_{Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more}

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[jesseduffield]

Just one thing

[jesseduffield]

If I'm reading this right: this function implicitly handles the case of update-ref todos by checking if the hash is empty. That's not very obvious from reading it, and I wonder if we can make this more explicit, either by moving the length check to the callsite so that equalHash() can assume that the hashes are non-empty, or by doing some kind of check on the type of the todo before comparing hashes.

[stefanhaller]

If I'm reading this right: this function implicitly handles the case of update-ref todos by checking if the hash is empty.

I wouldn't say it like that. This function simply compares two hashes, without making assumptions about the context. Two empty hashes are equal; one empty hash and a non-empty hash are not. Before this change the function was broken in that it returned true when comparing an empty hash against a non-empty one; it just didn't have any bad consequences because it was never called with such arguments. That's what we're fixing here.

Now, whether anything can be made clearer at the call sites is a different question. Adding a check on the type there seems unnecessary to me. It would have to be something like "if both sides are update-refs, we don't have to bother comparing the hashes because we know they will both be empty; and if one side is an update-ref but the other isn't, then we don't have to bother comparing them either because we know one hash will be empty and the other isn't". But it's not worth doing that extra work because the equalHash function does the right thing in these cases now.

I'd be open to adding a comment at the call site though if you think this helps. Wanna make a suggestion?

[jesseduffield]

Adding a check on the type there seems unnecessary to me. It would have to be something like "if both sides are update-refs, we don't have to bother comparing the hashes because we know they will both be empty; and if one side is an update-ref but the other isn't, then we don't have to bother comparing them either because we know one hash will be empty and the other isn't". But it's not worth doing that extra work because the equalHash function does the right thing in these cases now.

That is indeed exactly what I'm thinking of i.e. being explicit about the fact that we don't need to compare hashes unless both sides are not update-refs. I spent some time seeing if I could get it working but the type mismatches were making it awkward. I'd still like to have another crack at it later but I don't want to hold up this PR so I'm happy to approve

[jesseduffield]

LGTM

[jesseduffield]

Adding a check on the type there seems unnecessary to me. It would have to be something like "if both sides are update-refs, we don't have to bother comparing the hashes because we know they will both be empty; and if one side is an update-ref but the other isn't, then we don't have to bother comparing them either because we know one hash will be empty and the other isn't". But it's not worth doing that extra work because the equalHash function does the right thing in these cases now.

That is indeed exactly what I'm thinking of i.e. being explicit about the fact that we don't need to compare hashes unless both sides are not update-refs. I spent some time seeing if I could get it working but the type mismatches were making it awkward. I'd still like to have another crack at it later but I don't want to hold up this PR so I'm happy to approve