When bisecting, always mark the current commit as good/bad, not the selected #2837

stefanhaller · 2023-07-27T17:44:54Z

For marking as good or bad, the current commit is pretty much always the one you want to mark, not the selected. It's different for skipping; sometimes you know already that a certain commit doesn't compile, for example, so you might navigate there and mark it as skipped. So in the case that the current commit is not the selected one, we now offer two separate menu entries for skipping, one for the current commit and one for the selected.

Fixes #2836.

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

jesseduffield · 2023-07-29T09:13:11Z

pkg/gui/controllers/bisect_controller.go

-			Label: self.c.Tr.Bisect.ResetOption,
+	}
+	if info.GetCurrentSha() != "" && info.GetCurrentSha() != commit.Sha {
+		menuItems = append(menuItems, lo.ToPtr(types.MenuItem{


When we're bisecting (meaning there is a 'current' commit), what are your thoughts on showing both skip options unconditionally, for the sake of muscle memory?

I'd vote against it. In the vast majority of cases, the selected commit and the current commit will be the same, so it would be confusing to always see two menu entries for the same sha that only differ in their keyboard shortcut. Also (in my personal experience, at least) it's much more common to skip the current commit rather than some arbitrary different one, so most people will have s in their muscle memory and not S.

Fair point: you've convinced me!

jesseduffield

LGTM

…elected For marking as good or bad, the current commit is pretty much always the one you want to mark, not the selected. It's different for skipping; sometimes you know already that a certain commit doesn't compile, for example, so you might navigate there and mark it as skipped. So in the case that the current commit is not the selected one, we now offer two separate menu entries for skipping, one for the current commit and one for the selected.

Dagur · 2023-08-17T15:03:56Z

I don't understand how to use this fix. If I press b it asks me if I want to mark the current commit as good or bad but it's always the selected commit that gets marked, not the current one.

Dagur · 2023-08-17T15:11:42Z

I think I found a bug because it only works after I've found both a good commit and a bad one.
If I've only found one of the two, then it suggests the selected item as the "current" commit instead of the checked out commit.

stefanhaller · 2023-08-17T15:14:25Z

That's not a bug, that's how bisect works. You tell it a good and a bad commit explicitly (by selecting them), and then it checks out commits for you that you test. It's only from that point on that the "current, not selected" logic makes sense.

Dagur · 2023-08-17T15:32:10Z

I see your point but I still think it makes sense to say that the checked out commit is the "current" one.

Here's why that would be useful to me. I check out tags to find the first good and first bad commits. When I've found a version that fits, I want to be able to mark it good or bad right away. Currently I have to either:

Go to the list of commits and scroll down (often for a very long time) to find the tagged commit
Use the command line and type "git bisect good|bad"

Another solution would be to add a bisect option to tags, I wouldn't mind that at all.

stefanhaller · 2023-08-17T15:48:38Z

OK, I do see two bugs here:

if you check out the good commit and mark it "good", and then go back to the branches panel to check out a newer commit (often master), then this is not shown in the local commits panel. It still only shows the commits from the good one downwards. As a workaround, always mark the bad commit first.
if you have only marked one commit as good or bad, the bisect dialog says "current commit", which is wrong, it should say "selected commit". (The sha in parens is the right one though.)

I agree that it might be useful to show bisect options in the tags panel (only as long as we don't have a good and a bad commit, that is). Feel free to file an issue for that.

Apart from that, I can only recommend to use the / key to find tags in the local commits panel. I use that all the time, works great.

Dagur · 2023-08-17T16:20:52Z

Thanks. I've created an issue here #2941

The / key doesn't search the whole history. I'm not sure how far back it goes, but often it's not enough.

stefanhaller · 2023-08-17T16:29:51Z

That no longer seems to be true; typing / triggers loading all commits. I'm not sure when this has changed, I remember that previously you had to type > manually to trigger loading all commits.

(Actually, I don't like this behavior; our monorepo at work is so big that loading all commits takes a really long time, and consumes so much memory that everything is super slow afterwards. I want to be able to search in the loaded commits only, and then only trigger loading more when needed.)

stefanhaller added the enhancement New feature or request label Jul 27, 2023

stefanhaller mentioned this pull request Jul 27, 2023

Bisect good/bad: current or selected commit? #2836

Closed

jesseduffield reviewed Jul 29, 2023

View reviewed changes

jesseduffield approved these changes Jul 29, 2023

View reviewed changes

stefanhaller force-pushed the bisect-always-mark-current-commit branch from a276b11 to 6794149 Compare July 29, 2023 12:44

stefanhaller merged commit 52447e5 into master Jul 29, 2023
12 checks passed

stefanhaller deleted the bisect-always-mark-current-commit branch July 29, 2023 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

When bisecting, always mark the current commit as good/bad, not the selected #2837

When bisecting, always mark the current commit as good/bad, not the selected #2837

stefanhaller commented Jul 27, 2023

jesseduffield Jul 29, 2023

stefanhaller Jul 29, 2023

jesseduffield Jul 29, 2023

jesseduffield left a comment

Dagur commented Aug 17, 2023

Dagur commented Aug 17, 2023

stefanhaller commented Aug 17, 2023

Dagur commented Aug 17, 2023

stefanhaller commented Aug 17, 2023

Dagur commented Aug 17, 2023

stefanhaller commented Aug 17, 2023

When bisecting, always mark the current commit as good/bad, not the selected #2837

When bisecting, always mark the current commit as good/bad, not the selected #2837

Conversation

stefanhaller commented Jul 27, 2023

jesseduffield Jul 29, 2023

Choose a reason for hiding this comment

stefanhaller Jul 29, 2023

Choose a reason for hiding this comment

jesseduffield Jul 29, 2023

Choose a reason for hiding this comment

jesseduffield left a comment

Choose a reason for hiding this comment

Dagur commented Aug 17, 2023

Dagur commented Aug 17, 2023

stefanhaller commented Aug 17, 2023

Dagur commented Aug 17, 2023

stefanhaller commented Aug 17, 2023

Dagur commented Aug 17, 2023

stefanhaller commented Aug 17, 2023