Preserve pending commit message when closing/re-opening

[AzraelSec]

• PR Description
  This PR allows lazygit to preserve the commit messages when the commit popup gets closed.

While discussing the feature as part of its issue, two approaches were taken into consideration:

• to store the commit content as part of the global state file
• to store the commit content in a special file to place inside the .git folder

I opted for the second approach to avoid worrying about associating each preserved message to the worktree it belongs to. I am happy to reconsider this and opt for the alternative approach, but I wanted to discuss this with the maintainers before deciding.

Note: The preserving file (.git/LAZYGIT_PENDING_COMMIT) is deleted when the commit is finalized or when the commit content becomes empty.

• 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

[AzraelSec]

@jesseduffield @stefanhaller - I gave a shot to the approach of storing the pending commit message in a custom file placed in the .git folder instead of using the global state. This should guarantee that it gets preserved between different worktrees, but let me know if you prefer the other approach, and I'll jump on it.

Also, I created two commits so that we could ensure that the original commit persistence test ran correctly before updating it.

[jesseduffield]

Nice, I'll give this a test today

[stefanhaller]

This looks mostly good, and I tested it and it works great. I also like the decision to store the message in a file in .git/.

I only have a concern about error handling; I don't like it that so many functions now return an error that previously didn't. For example, CloseCommitMessagePanel now returns an error, and this feels wrong because closing a window is one of the operations that ought to be possible to do without having to handle errors.

The problem with this is that we now have two different classes of errors, really important ones and then these less important ones when storing the commit message fails (which, let's be honest, will never fail in practice). And this is a problem because it makes error handling more complex; for a function that first runs a git operation and then closes the panel, it now has to check if the git operation failed, and if so return that error (even if the closePanel function also returned one). This is cumbersome and error-prone, so I'd prefer it if these minor, unimportant failures were not reported through error return values, but maybe only logged (like we do for SaveAppStateAndLogError, for example).

[AzraelSec]

@stefanhaller - I refactored the branch to log errors instead of handling them when setting or getting the persisted commit message, as you suggested (it honestly makes sense - I wasn't aware that pattern was already part of the codebase). Let me know if this version works better.

[stefanhaller]

Much better, looks great to me now. Just two small things below.

One word about this convention of prefixing commit messages with feat: or test: (again): I really don't like it, and I wish we wouldn't use it here. It looks ugly and provides no value to me; on the contrary, it constantly makes me go and verify whether the prefixes are really correct. I find this distracting. If the goal is that this helps you make sure you separate refactorings from adding features, then I do this anyway all the time, and I don't need those prefixes for that.

[stefanhaller]

I find the logic of this method a bit unnecessarily complicated, and pushed a fixup to simplify it (6f07e2c). Let me know what you think.

[AzraelSec]

@stefanhaller - happy to accept your suggestion, we don't really need to distinguish between presence vs deletion errors. I'll rebase-squash, thanks.

[stefanhaller]

I don't really see a reason why this shouldn't be squashed into the first commit of the branch.

[AzraelSec]

One word about this convention of prefixing commit messages with feat: or test: (again): I really don't like it, and I wish we wouldn't use it here. It looks ugly and provides no value to me; on the contrary, it constantly makes me go and verify whether the prefixes are really correct. I find this distracting. If the goal is that this helps you make sure you separate refactorings from adding features, then I do this anyway all the time, and I don't need those prefixes for that.

I'm used to conventional commits since they allow us to automate the creation of release messages, but since we don't use it that way, I'm happy to drop it. In this specific case, I created a test-specific commit so that we could ensure that the test was working with the new code before updating it. In other circumstances, I would have used a single commit to keep the feature + its test(s) in the same place.

@stefanhaller - I rebased the branch on the current master, squashed your fixup and modified the commits to strip the conventional commits prefixes.

[stefanhaller]

In this specific case, I created a test-specific commit so that we could ensure that the test was working with the new code before updating it. In other circumstances, I would have used a single commit to keep the feature + its test(s) in the same place.

Yes, I noticed that and I support the decision. Still, I think the conventional commits prefixes don't help with understanding that. (Personally, I would have mentioned this rationale in the commit message body of the second commit.)