Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds a setting that allows opp to keep your master branch clean. #146

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

cupcicm
Copy link
Owner

@cupcicm cupcicm commented Feb 18, 2024

When opp.keep_master_clean is set, if you create a PR, the commits
you have selected in the PR will be removed from the master branch.
This keeps the master branch clean.

@cupcicm
Copy link
Owner Author

cupcicm commented Feb 19, 2024

@gabrielreid it was good to meet you in Paris ! It made me think about you and I tried to implement this over the weekend, which is the feature I think you wanted from opp. I haven't had time to test this extensively, but I think it works.

What's pretty cool (IMO) is that it works even with opp pr -i meaning if I have 4 commits, run opp pr -i, select the 1st one and the 3rd one in my PR, then my master branch will then contain only commit 2 and 4 (if applying 2 then 4 doesn't conflict).

WDYT ? Can you test it and tell me if it works ? Need to set a hidden config parameter in .opp/config.yaml

@cupcicm cupcicm requested a review from gabrielreid February 19, 2024 19:53
@gabrielreid
Copy link
Collaborator

Oh wow, awesome!

I'll give it a try and let you know 👍

@cupcicm
Copy link
Owner Author

cupcicm commented Feb 20, 2024

Thanks I'll do the same. I think it's working in many of the usual cases, but seeing how many git rebase --onto it's taken to make it work, being careful is a good thing I think.

Copy link
Collaborator

@gabrielreid gabrielreid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've mostly just been testing this, and it seems to definitely work for my general flow that I always use, so this definitely seems good to me 👍

This did make me try opp pr -i, which I otherwise never use, and I discovered that it seems to fail for me (regardless of this PR) with the following

Choose what commits you want to include in this PR
panic: tried to abort the rebase but failed: exit status 128

goroutine 1 [running]:
github.com/cupcicm/opp/core.(*Repo).TryRebaseOntoSilently(0x1?, {0x10108b590, 0x1400018ee10}, {0x64, 0xba, 0xea, 0xf2, 0x64, 0x9c, 0xbb, ...}, ...)
	/Users/greid/development/opp/core/repo.go:242 +0x270
github.com/cupcicm/opp/cmd.(*create).RebasePrCommits(0x1400022b8d0, {0x10108b590, 0x1400018ee10}, 0x140003c6060)
	/Users/greid/development/opp/cmd/pr.go:216 +0x130
github.com/cupcicm/opp/cmd.PrCommand.func1(0x14000036280)
	/Users/greid/development/opp/cmd/pr.go:88 +0xbc
github.com/urfave/cli/v2.(*Command).Run(0x14000294420, 0x14000036280, {0x14000080340, 0x2, 0x2})
	/Users/greid/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/command.go:274 +0x730
github.com/urfave/cli/v2.(*Command).Run(0x14000092160, 0x140000361c0, {0x1400019e120, 0x3, 0x3})
	/Users/greid/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/command.go:267 +0x940
github.com/urfave/cli/v2.(*App).RunContext(0x14000296000, {0x10108b590?, 0x1400018ee10}, {0x1400019e120, 0x3, 0x3})
	/Users/greid/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/app.go:332 +0x518
main.main()
	/Users/greid/development/opp/opp.go:54 +0x2f8

Anyhow, that error happens even when running on main though, so no reason to not merge this PR because of it.

cmd/pr.go Outdated
return err
}
if !repo.TryRebaseCurrentBranchSilently(cCtx.Context, localPr) {
fmt.Printf("problem while rebasing %s on %s\n", args.InitialBranch.LocalName(), localPr.LocalName())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit I don't fully get the logic here, but this looks to me like there should be a return err or something similar here (or if it's intentional that we keep going on this point, that a comment explaining why would help).

That's mostly my lack of understanding of fancy git rebasing tricks speaking here though 😬

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right, when the rebase fail we shouldn't continue as if nothing happened.

@cupcicm
Copy link
Owner Author

cupcicm commented Mar 3, 2024

I discovered that it seems to fail for me
Weird 🤔 this doesn't happen to me.
Is it doable to create a chain of commit and show me what gets you in this situation by any chance ? The error means:

  1. opp tried to create a PR that contains just the commits you selected in the rebase todo file but couldn't (e.g. the commits you selected didn't apply cleanly on master)
  2. When this happens opp run git rebase --abort to get you back to a state that's clean but this rebase failed too. Usually when 1. happens the rebase aborts correctly and you don't get the error.

When opp.keep_master_clean is set, if you create a PR, the commits
you have selected in the PR will be removed from the master branch.
This keeps the master branch clean.
@gabrielreid
Copy link
Collaborator

About my opp pr -i issue, I've confirmed that it happens regardless of this feature.

I can reproduce it by checking out a clean version of main of this repo: https://github.com/gabrielreid/testing, configuring opp (i.e. running opp init), and then doing the following:

$ echo something >> test.txt
$ git add test.txt && git commit -m 'test 1'
$ opp pr -i

That reliably gives me this output:

Choose what commits you want to include in this PR
panic: tried to abort the rebase but failed: exit status 128

goroutine 1 [running]:
github.com/cupcicm/opp/core.(*Repo).TryRebaseOntoSilently(0x1?, {0x1057036b0, 0x1400007ce10}, {0x8d, 0xf2, 0x90, 0xb0, 0x12, 0x5d, 0xb3, ...}, ...)
	/Users/greid/development/opp/core/repo.go:242 +0x270
github.com/cupcicm/opp/cmd.(*create).RebasePrCommits(0x1400017b8b0, {0x1057036b0, 0x1400007ce10}, 0x140001d7a40)
	/Users/greid/development/opp/cmd/pr.go:241 +0x130
github.com/cupcicm/opp/cmd.PrCommand.func1(0x140001d6700)
	/Users/greid/development/opp/cmd/pr.go:88 +0xc4
github.com/urfave/cli/v2.(*Command).Run(0x140001e2420, 0x140001d6700, {0x140001da420, 0x2, 0x2})
	/Users/greid/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/command.go:274 +0x730
github.com/urfave/cli/v2.(*Command).Run(0x140001e2dc0, 0x140001d6640, {0x14000020150, 0x3, 0x3})
	/Users/greid/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/command.go:267 +0x940
github.com/urfave/cli/v2.(*App).RunContext(0x140001e4000, {0x1057036b0?, 0x1400007ce10}, {0x14000020150, 0x3, 0x3})
	/Users/greid/go/pkg/mod/github.com/urfave/cli/v2@v2.25.7/app.go:332 +0x518
main.main()
	/Users/greid/development/opp/opp.go:54 +0x2f8

I don't know of anything "special" on my machine, and the output of git version is git version 2.39.3 (Apple Git-145) (if that's worth anything).

@cupcicm cupcicm merged commit df17592 into main Mar 15, 2024
1 check passed
@cupcicm cupcicm deleted the cupcicm/pr/146 branch March 15, 2024 21:15
@cupcicm
Copy link
Owner Author

cupcicm commented Mar 15, 2024

OK @gabrielreid Sorry I didn't have time to look into it before today. So this is due to the fact this branch of the code doesn't check that you have a local diff. If you do, when it's trying to do the rebase magic your local diff gets in the way and 💥 .
We'll add a check that tells you to stash if you have this setting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants