Honour configured comment char on interactive rebase

[gustavokrieger]

• PR Description

The changes were made based on the suggestion by @jesseduffield at #2639 (comment). This makes sure that interactive rebases do not fail when the user sets a 'core.commentChar' that is different from '#'.

• Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.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)
• Docs (specifically docs/Config.md) have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[gustavokrieger]

Guys, do you believe I should add unit or integrations tests for this change? What about a comment in the code explaining why this is needed?

[jesseduffield]

Yep, an integration test would be good where you set the git config to have the comment character be ';' and then verify that you can do a basic interactive rebase. And a comment would also be appreciated.

For posterity I'll mention that we still want the longer-term fix of using the configured comment character given it'll allow us to open lazygit mid-rebase that the user started outside lazygit, but this PR is still a step in the right direction.

[stefanhaller]

For posterity I'll mention that we still want the longer-term fix of using the configured comment character given it'll allow us to open lazygit mid-rebase that the user started outside lazygit, but this PR is still a step in the right direction.

Good point, I hadn't thought about that. In that light, I do wonder whether this PR is actually a step in the right direction: if we merge the PR now, and then later do the "real" fix, then parsing will fail again, unless we revert this PR, because the real fix will entail looking at the global core.commentChar config, so it has no way of knowing that we overrode the setting for just one git call.

Doesn't that mean we should rather do the real fix now?

[jesseduffield]

Good point, I hadn't considered that edge case. In that case yes, we should do the proper fix. Sorry @gustavopcassol ! Let me know if you're up to doing the bigger fix.

To be clear on what's needed, we need to create a PR in the git-todo-parser repo that allows us to pass a comment char ourselves (it's currently defined as a constant in https://github.com/fsmiamoto/git-todo-parser/blob/main/todo/parse.go). We'll want @fsmiamoto 's input but we could create new methods that accept a configuration object including the comment char. This affects the Parse and Write methods of that package.

Then from within lazygit we need to check which comment char is being used and pass that to the todo parser. We can add a method in pkg/commands/git_commands/config.go for getting the comment char, and then we need to call that method before calling todo.Parse, which we do in two places. The first place we call it is from pkg/commands/git_commands/commit_loader.go so we'll need to pass *GitCommon to the CommitLoader struct in order to get access to the new method, and the other place we call it is from pkg/utils/rebase_todo.go which unfortunately happens when we don't have access to our GitCommon struct, so we'll need to pass it through the instruction args in pkg/app/daemon/daemon.go (there are multiple instructions that will need to be updated).

We'll also need to give similar treatment in the place we call todo.Write.

[gustavokrieger]

Ok, I'll give it a shot.

[gustavokrieger]

I created an issue there fsmiamoto/git-todo-parser#5 (comment).

[jesseduffield]

just confirming that this test fails under the old code and passes under the new code? Or are we yet to implement the fix that makes this test pass?

[gustavokrieger]

The former. Not sure I understand what you are asking in the latter...

[gustavokrieger]

@jesseduffield, would it make sense to have a git config struct that is created when the program starts and that is easily accessible by any part of the code without the need for passing it around (like a singleton, for example)? It could have only private fields with public getters to make sure it never mutates after it's created. I have the impression that reading the config can happen at any point of the application, which would always demand changes from a bunch of parts of the app so that the struct can be passed around.

[stefanhaller]

There's already the ConfigCommands struct that sounds like what you want. It has methods like GetShowUntrackedFiles that query a particular git config; adding a new method CommentChar seems like the right thing to do.

[gustavokrieger]

Guys, I need some input on how to proceed. Currently, the last fixup commit is partially implementing the changes related to the Parse call at pkg/utils/rebase_todo.go. All of the current errors should be related to it. Should I proceed by adding the parameter to the run method? Screenshot of the erros:

[stefanhaller]

Sorry for the delay, this somehow slipped from my radar. Feel free to ping earlier next time if you don't get responses. 😄

I had a look, and I think for the case of local_commits_controller you are doing too much work to pass the comment char as a parameter in places where you don't need to. Only the code in rebase.go needs to set the comment char, and it has access to the git config already. I pushed a fixup (04b8fc7) to undo some of your changes here, and to also implement a solution for daemon.go. In this case I'd go the easy route and just have a local function that queries the git config manually, I think that's an ok thing to do for the daemon.

Then I pushed another fixup (1ff7a16) that implements the commentChar logic inside of GetCoreCommentChar, since the same logic is needed from two places. It might seem questionable to have logic like this here, as the responsibility of ConfigCommands could be seen as just returning git config values as is, but I'd say it's still acceptable in this case.

And finally a third fixup (9019baf) that adds the missing WriteRebaseTodoFile arguments, otherwise I couldn't have tested if it works now. It looks like it does. 😄

The next step would be to squash the commits (and rebase on master) so that we can do a proper review.

[gustavokrieger]

To get the updated git-todo-parser I ran these:

go get github.com/fsmiamoto/git-todo-parser@v.0.0.5
go mod vendor

Let me know if it's all that's needed.

[gustavokrieger]

When writing this test, I saw that this part ended up not failing like I believe it should. Am I misunderstanding how this works?

[stefanhaller]

That's an unfortunate consequence of how the test framework works. After dropping the commit, lazygit runs a refresh of the commits list, but it runs in the background and takes a while to complete. The test framework has logic to wait a while and retry the check if it doesn't succeed right away, but in this case the check succeeds because it still matches the old state before the refresh.

@jesseduffield I wonder if there's some way to teach the test infrastructure that any queued-up refresh requests are still pending, and wait until they are all carried out. That should fix this kind of problem, no?

[jesseduffield]

Agreed, it would be great if the test framework had a concept of pending actions. It would take some work to support that though (most of the challenge is in tracking that something is pending when we're using goroutines to do work off the UI thread)

[stefanhaller]

Naïvely, it sounds simply like a matter of incrementing a counter when posting a refresh, and decrementing it again when the refresh was handled. But I'm probably missing something...

[jesseduffield]

That's certainly something we could do when refreshing, and it should be straightforward to implement, but I'm thinking more broadly about all actions that make use of goroutines for doing work off the UI thread

[stefanhaller]

Don't let the Perfect be the enemy of the Good... 😉

[gustavokrieger]

Are the tests I wrote enough?

Also, as a side note, the integration tests were very easy to work with, and overall very awesome 🚀

[stefanhaller]

To get the updated git-todo-parser I ran these:

go get github.com/fsmiamoto/git-todo-parser@v.0.0.5
go mod vendor

Let me know if it's all that's needed.

Normally yes (except that a git mod tidy doesn't hurt to clean out old stuff). In this case I messed up and created the upstream tag wrong (using v.0.0.5 instead of v0.0.5), that's why a simple go get github.com/fsmiamoto/git-todo-parser didn't work. I fixed this upstream and pushed a fixup (ce4f104).

[stefanhaller]

The test drop_in_edit_with_custom_comment_char doesn't catch the bug, it already succeeded before. The reason is that pressing e doesn't parse the todo-file at all; it only prepends a break line. And then, when pressing d during the rebase, git has already stripped all comments from the todo-file, so that succeeded too. I'd suggest to just drop this test.

Instead, I'd probably add one more test that moves a commit up or down (outside of a rebase). For my feeling that should be enough coverage.

[stefanhaller]

Also, I'm a big fan of introducing tests first to demonstrate the bug. I know @jesseduffield didn't always agree in the past, but I'm still trying to convince him of the value of this. 😄

This has two benefits:

• it shows more clearly how the bug manifested itself, so there's less to explain in the commit message
• it proves that the test actually works for catching the bug, which wasn't true for the drop_in_edit_with_custom_comment_char test, for example.

Do you feel like trying this in this case?

[stefanhaller]

I'm not a fan of having local functions like this in tests. It makes the flow of what happens in which order harder to understand, so I'd just have inlined this below where you need it.

However, I see that you copied this from other existing tests that do it in the same way. How about extracting this to a new file in pkg/integration/tests/shared? Doing this for the existing tests could be a separate commit at the beginning of the branch.

[jesseduffield]

I'm okay with local functions if they remove lots of repetition (e.g. if you need to drop 5 commits in the one test), but I agree about erring on the side of inlining everything.

I'm also fine with this being a shared function. You can chuck it in pkg/integration/components/common.go if you like

[gustavokrieger]

I felt the function was simple enough, so I inlined it.

[jesseduffield]

I actually like writing failing tests first! But I do so within the same commit that fixes it. I think my confusion was around committing the failing test and whether it would cause a test failure on that commit, which could interfere with a git bisect. If we're not doing that then I'm open to the two-commit approach

[stefanhaller]

I actually like writing failing tests first! But I do so within the same commit that fixes it. I think my confusion was around committing the failing test and whether it would cause a test failure on that commit, which could interfere with a git bisect. If we're not doing that then I'm open to the two-commit approach

Of course, all commits need to be green, so the test must not fail in the first commit. But that's the point: the test demonstrates the buggy behavior in the first commit, and you see how the test changes in the commit that fixes the bug, which is useful information for future readers of the git history.

Sometimes I go as far as writing something like this in the first commit, but maybe that's overkill:

	Lines(
		/* EXPECTED:
		Contains("commit 04"),
		ACTUAL: */
		Contains("commit 05"),
	)

And of course it doesn't always work, e.g. if it was a crash bug.

[jesseduffield]

That's sensible. I'll give it a go next time I do a bugfix

[gustavokrieger]

The test drop_in_edit_with_custom_comment_char doesn't catch the bug, it already succeeded before. The reason is that pressing e doesn't parse the todo-file at all; it only prepends a break line. And then, when pressing d during the rebase, git has already stripped all comments from the todo-file, so that succeeded too. I'd suggest to just drop this test.

Instead, I'd probably add one more test that moves a commit up or down (outside of a rebase). For my feeling that should be enough coverage.

I ended up adding drop_in_edit_with_custom_comment_char because, even though it did already succeeded before, I noticed that the test drop_with_custom_comment_char does not cover the changes made here and here. Do you think this alone does not justify the test's existence? I have no strong opinion about it. The test just seemed simple enough to me, so I didn't worry much about it's necessity.

[gustavokrieger]

See if I understand what you guys want me to do with the PR: it should be two commits, the first being only the tests (skip the crashing ones), and the second the rest of the code. The tests should not be failing in either commit, so in the first one they will be testing for the behavior before the change. Is this correct?

[jesseduffield]

Yep!

[stefanhaller]

I ended up adding drop_in_edit_with_custom_comment_char because, even though it did already succeeded before, I noticed that the test drop_with_custom_comment_char does not cover the changes made here and here.

But drop_in_edit_with_custom_comment_char doesn't either, that was my point. If you want to cover these changes, you'll have to add one that actually fails before these changes. For the first one you have one already (drop_in_edit_with_custom_comment_char covers it), but for the second you'll have to use one of the tests that result in a "conflict" being shown in the commits panel. We have several of these, search for Contains("conflict") to find them; maybe amend_commit_with_conflict would be a good candidate.

See if I understand what you guys want me to do with the PR: it should be two commits, the first being only the tests (skip the crashing ones),

I'm not sure what you mean by "skip the crashing ones"; we don't have any tests that would crash before the change, do we? If you mean tests that result in an error message, then those should not be skipped; it's good to document the error that we are getting before the change.

[stefanhaller]

If you want to go about this more systematically, then you could do the following (this is actually what I would have done if it were my PR):

1. Make a commit that adds all the new tests, in their "failing" state (i.e. documenting the brokenness, including any error panels)
2. Make a second commit that only bumps the git-todo-parser module. Get the code to compile by passing a hard-coded '#' at every call site of todo.Parse and todo.Write. This commit shouldn't change any tests, of course.
3. Now you are in a position to make a series of more fine-grained commits to pass the real comment char instead of the '#'; each of these commits changes only a single call site. In each of these commits you should have to adapt at least one test. If you find yourself changing one call site where you don't have to adapt any test, then you are missing test coverage.

After this you might decide to squash all the small commits of step 3 into one, this might be a matter of taste. However, if in doubt I prefer smaller commits over larger ones (as long as each one is green).

[gustavokrieger]

But drop_in_edit_with_custom_comment_char doesn't either, that was my point. If you want to cover these changes, you'll have to add one that actually fails before these changes.

I was not clear on what I meant. With "drop_with_custom_comment_char does not cover the changes" I meant to say that those specific lines of code did not execute at all on the drop_with_custom_comment_char test, which was the only one that set the comment char to ;.

We have several of these, search for Contains("conflict") to find them; maybe amend_commit_with_conflict would be a good candidate.

I ended up dropping the drop_in_edit_with_custom_comment_char test in favor of the "move" one, as suggested earlier. Should I still add this new one?

I'm not sure what you mean by "skip the crashing ones"; we don't have any tests that would crash before the change, do we?

Never mind, we don't.

[stefanhaller]

Thanks for bearing with my nitpicking Gustavo, this is getting really, really nice. Great work!

But drop_in_edit_with_custom_comment_char doesn't either, that was my point. If you want to cover these changes, you'll have to add one that actually fails before these changes.

I was not clear on what I meant. With "drop_with_custom_comment_char does not cover the changes" I meant to say that those specific lines of code did not execute at all on the drop_with_custom_comment_char test, which was the only one that set the comment char to ;.

Yes, I got that. My point was that having a test that just executes those lines is not enough. Specifically, for the test drop_in_edit_with_custom_comment_char those lines were executed at a point where there are no comment characters in the file any more, so the test succeeds no matter what core.commentChar is set to, and no matter whether we pass the correct one or a hard-coded #. So that's a worthless test.

But I thought about this a little more, and actually I can't come up with any scenario (not just in tests, also in real usage) where we execute those two places in commit_loader.go while there are comments in the files being parsed; which means that we make the change only for the theoretical case that a future git version decides to put comments in the those files at that point, but today we have no way to cover these with tests.

[stefanhaller]

@jesseduffield Did you want to review this too? I'd be happy to merge this in its current state.

[jesseduffield]

One thought: we're currently obtaining the comment char in two different places: what are our thoughts on passing the comment char as an env var to the daemon so that we only fetch it in a single place and so that the fetching process can be cached (given that the git config struct we use caches results already)?

[stefanhaller]

I thought about that, but the code is so much simpler if the daemon just makes its own call to git config. And I doubt that we need caching for performance reasons here, so I'd be happy with the way we have it now.

[jesseduffield]

Fair enough. It would be worth finding a go package for parsing a git config so that we don't need to invoke the git binary; then we wouldn't need to care about the overhead there (obvs not needed for this PR).

Merge away :)

[stefanhaller]

git config core.commentChar takes less than 3ms to execute on my machine. Granted, it's probably worse on Windows, but I really don't think it's worth manually parsing the config file to improve that.