Amend to head shouldn't rebase

[mark2185]

• PR Description Should fix #2170.

• Please check if the PR fulfills these requirements

• [x] Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
• [x] Code has been formatted (see here)
• [x] Tests have been added/updated (see here for the integration test guide)
• [x] Text is internationalised (see here)
• [x] Docs (specifically docs/Config.md) have been updated if necessary
• [x] You've read through your own file changes for silly mistakes etc

[mark2185]

Looks good! Can we add an integration test using the repro steps of the issue, to prevent regressions?

Sure, but in the meantime, the failing test (branch/suggestions, if I'm reading that right) isn't failing on my side. Did #2171 prove they aren't flaky or was it just a fluke?

[jesseduffield]

I'm pretty sure branch/suggestions tests something different, and so is itself flakey

[jesseduffield]

I don't think there's an integration test using the repro steps of the issue

[mark2185]

There are discussions to be had about things that I'd like:

• reword the functionality "Amend to last commit" to "Amend to HEAD commit" since it helps down the road
• disable amending to commits other than HEAD when in an interactive rebase, since it starts another and that's what started this issue, which means pressing A in the commits panel (during a rebase) should always ask Do you want to amend to HEAD
• some nicer, cleaner, helper functions of checking where the HEAD is during a rebase
• I think that input.PressKeys(keys.Universal.Return) didn't do the same as input.Confirm() (even though it looks like it should, according to the underlying code), but that might have been a fluke
• invoking shell.CreateFileAndAdd does not refresh views in the tests, maybe it should? Just as a convenience. I'm not sure if that's maybe fixed in your big PR

Thoughts?

[jesseduffield]

@mark2185 I agree about renaming to 'Amend to HEAD commit'.

disable amending to commits other than HEAD when in an interactive rebase, since it starts another and that's what started this issue, which means pressing A in the commits panel (during a rebase) should always ask Do you want to amend to HEAD

If the user is in an interactive rebase and they try to amend a non-HEAD commit I would raise an alert saying they can't do that. I wouldn't give them to the option to amend to head in that alert, because it's unlikely that that's what they wanted to do and they still have the option to do it themselves if they want to.

I think that input.PressKeys(keys.Universal.Return) didn't do the same as input.Confirm() (even though it looks like it should, according to the underlying code), but that might have been a fluke

'Return' is poorly named. It should be 'Escape'.

invoking shell.CreateFileAndAdd does not refresh views in the tests, maybe it should? Just as a convenience. I'm not sure if that's maybe fixed in your big PR

Interesting, that's not something I had thought about. So is this for when you're creating the file from within the test? There's three approaches we can take here:

1. have the user press shift+R to refresh
2. have the user wait for an auto-refresh
3. trigger an auto-refresh manually from within the test.

I'm not a fan of 3 because it means we're no longer simply mimicking a user (i.e. during a lazygit session, a user is allowed to create random files in the background and they're allowed to press shift+R, so that should also be what we're doing via shell and input.)

Waiting for an auto-refresh is fraught with danger, so I prefer using shift+R directly. The question is whether we want to have shift+R happen automatically after any shell method is called, or do we want to require that it's done manually within the test. I'm leaning towards manually, because we may want to run a series of shell commands before refreshing the views.

FYI I just merged that integration test PR (finally) :)

[mark2185]

If the user is in an interactive rebase and they try to amend a non-HEAD commit I would raise an alert saying they can't do that. I wouldn't give them to the option to amend to head in that alert, because it's unlikely that that's what they wanted to do and they still have the option to do it themselves if they want to.

Fair enough!

'Return' is poorly named. It should be 'Escape'.

Ah, that explains some things. Should I open up a new PR for that change?

3. trigger an auto-refresh manually from within the test.

That's what I opted out for, in the end. I agree with your reasoning about it staying manual!

[mark2185]

I'm marking it for review just so I can run CI.

[jesseduffield]

Ah, that explains some things. Should I open up a new PR for that change?

Please do :)

That's what I opted out for, in the end. I agree with your reasoning about it staying manual!

Sweet!

[mark2185]

@jesseduffield could you try running interactive_rebase/amend_non_HEAD locally? It doesn't fail for me so I'm a bit stumped.

[jesseduffield]

@mark2185 it does fail for me, same as CI. In GetLazygitRootDirectory we check if we've got a .git directory in the current working directory and if so we return the directory. Given that we start lazygit within the test repo dir that means we end up creating the file at e.g. test/blah/actual/test/blah/actual, if that makes sense.

I'm open to just passing the test directory as an environment variable to lazygit so that it doesn't need to worry about the logic. That is the test runner binary passes the environment variable to lazygit

Worth mentioning as well that we should use the 'actual/repo' path instead of just 'actual' so that we're consistent between the pre-test shell and the during-test shell.

[mark2185]

@mark2185 it does fail for me, same as CI. In GetLazygitRootDirectory we check if we've got a .git directory in the current working directory and if so we return the directory. Given that we start lazygit within the test repo dir that means we end up creating the file at e.g. test/blah/actual/test/blah/actual, if that makes sense.

Worth mentioning as well that we should use the 'actual/repo' path instead of just 'actual' so that we're consistent between the pre-test shell and the during-test shell.

I have to be frank, I haven't still wrapped my head around the actual, actual/repo, root and other magical locations, I've had a debug adventure now, but sadly I couldn't go past a certain depth so I never figured out why we need to create the folder when creating a shell in IntegrationTest::Run, but I did see the crashes with really deep (and odd) paths. I did read the docs in paths.go, but not all puzzles have clicked into their places.

I'll debug some more in the following days and hopefully that'll clear things up.

[jesseduffield]

Yeah the confusion is understandable: it could be better documented.

The reason there is an actual/repo/ dir is so that we can also put other repos in the actual/ dir, for example, a remote that you can push to. When the snapshot step occurs, it looks at all repos in the actual dir and compares them. That way if there's a test that involves pushing to remote you can verify that the remote was actually successfully pushed to.

We default the shell to being in the actual/repo dir because it's the one we most likely want to be doing stuff in. The way the shell scripts in the old tests worked is that they would go cd .. (taking them up to the actual/ dir) and then create a remote repo, and then potentially cd back into 'actual/repo/'

[mark2185]

Should be solved by #2485 .