Reordering commits confuses spr

[piefel]

I had a small stack with commits A, B, and C like this:

C B A

Then I noticed a small issue that could (should?) have been commit D, but instead it was so obviously semantically part of A that I decided to change the two files and use git amend, which works splendidly:

C B A’

However, since the unit of PR is commit, not set of commits, each commit must be valid individually. Which is good, just sometimes uncomfortable. Problem was, my fix in A’ depended on something in C, so the Github checks on A’ failed. Instead of just skipping them or trying to amend again somehow and perhaps use that D commit after all, I decided on a nice and clean solution with git rebase --interactive, just reordering the commits:

B A’ C

Well, I guess I did something forbidden here and just have myself to blame. Anyway, git spr update didn’t like that too well and spew out a stacktrace which I have to anonymize a bit:

$ git spr update > git rev-parse --show-toplevel > git fetch > git rebase origin/develop --autostash > github fetch pull requests > git log --format=medium --no-color origin/develop..HEAD > git branch --no-color > git log --format=medium --no-color origin/develop..HEAD > github update 647 : Commit message for A’ > git status --porcelain --untracked-files=no > git push --force --atomic origin ac60c07cee3c8aceeab4c1573c8de6c540c76010:refs/heads/spr/develop/5fc9eaba 57e5fa9fde824c159e4dbf02520e49e0313f5ffd:refs/heads/spr/develop/6ce18307 8f612db3ae8be108308ebcef4c7f58eb05c5de52:refs/heads/spr/develop/c6bfa480 > github create 650 : Commit message for B panic: createPullRequest: A pull request already exists for Organization:spr/develop/6ce18307.

goroutine 1 [running]: github.com/ejoffe/spr/github/githubclient.check({0x995c20, 0xc00041a7f8})      /Users/runner/work/spr/spr/github/githubclient/client.go:696 +0xf4 github.com/ejoffe/spr/github/githubclient.(client).CreatePullRequest(0xc000122810, {0x999390, 0xcc85c0}, {0x9992b0, 0xc0001227f8}, 0xc00031dcc0, {{0xc00021b4f9, 0x8}, {0xc00021b417, 0x28}, ...}, ...)      /Users/runner/work/spr/spr/github/githubclient/client.go:404 +0x599 github.com/ejoffe/spr/spr.(stackediff).UpdatePullRequests(0xc00012a7e0, {0x999390?, 0xcc85c0}, {0xcc85c0, 0x0, 0x0}, 0x0)      /Users/runner/work/spr/spr/spr/spr.go:216 +0xe66 main.main.func4(0xc0001e39e0?)      /Users/runner/work/spr/spr/cmd/spr/main.go:161 +0x10f github.com/urfave/cli/v2.(Command).Run(0xc0001e39e0, 0xc00012d040)      /Users/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.8.1/command.go:169 +0x5eb github.com/urfave/cli/v2.(App).RunContext(0xc000103a00, {0x999390?, 0xcc85c0}, {0xc00012e000, 0x2, 0x2})      /Users/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.8.1/app.go:341 +0xb05 github.com/urfave/cli/v2.(*App).Run(...)      /Users/runner/go/pkg/mod/github.com/urfave/cli/v2@v2.8.1/app.go:247 main.main()      /Users/runner/work/spr/spr/cmd/spr/main.go:230 +0x133b

(Interesting paths embedded in the Go binaries. Make mental note to look out for this when using Go myself.)

Also note “github update 647 : Commit message for A’” which updated the existing PR, and “github create 650 : Commit message for B” which tried a new PR instead of changing the existing 648.

So, could I have done it cleanly with spr? If not, would it be possible to implement? Or shall I submit a proposal for a warning in the README?

[ejoffe]

This sounds like a bug. SPR actually has logic to handle reordering of commits. Everything you did is actually (or should be) a-ok. I will try to recreate this case and see what the root cause it.

[piefel]

Today, I again ran into the situation where I had to reorder commits. It worked fine.

What was different today: I am on a branch, last time I was on develop, the merge target. Perhaps, hmm, I changed things the last time without calling git spr update in between, today I certainly did not.

[steinemann]

Hi,

Today, I had 2 commits and reordered them. After updating, git spr closed the PR's that where open before and created new PR's.

This is not beneficial, as the older PR's had some comments on them that I still wanted to address. Is this the way git spr works ? I would be better to keep the PR's and just change the merge branch.