spr icon indicating copy to clipboard operation
spr copied to clipboard

`git spr update --count` missed some commits at the base and created PRs for items higher up the stack instead.

Open chriscz opened this issue 1 year ago • 5 comments

I recently used update --count X and it skipped creating PRs from the base of my branch and then continued to create PRs for things I didn't want, i.e. items beyond the number that I specified, X+1, X+2 etc.

I will try to get a reproduction before I merge my work, and submit a fix.

Possibly related tickets:

  • https://github.com/ejoffe/spr/issues/385

chriscz avatar Sep 03 '24 07:09 chriscz

It looks like alignLocalCommits in spr/spr.go is dropping these commits because they are not aligning with what is already in the PR on GitHub.

This change was introduced to support merge queues: https://github.com/ejoffe/spr/commit/5f9733c506c5bb71f688acd092739bf93e78b07a

chriscz avatar Sep 03 '24 09:09 chriscz

I tried for a while to debug this though my go is quite rusty and didn't manage to fix it yet.

  • spr was trying to update the stack incorrectly and crashed. Consequently I saw some of the PR targets changed to develop and no longer seem to be stacked
  • After this initial crash spr just tries to create new PRs for everything, probably creating a completely new stack
  • I also notice that all of the prior PRs are no longer being returned in the method below. Instead only a single one is returned. https://github.com/ejoffe/spr/blob/2c5a21f639421f6cbb28557af19b6225e3f76ba4/github/githubclient/client.go#L226

spr probably needs some robustness / recovery in a scenario like this. Possibly looking to the source branch (spr/main/xxxx) for guidance on what the real commit id is, it may already be doing this and I missed it.


My PR stack is currently a mess with targets having been incorrectly set and I'm not unable to get it fixed. I'm going to put a hold on using spr for the moment and maybe look at this again in the future. It's a bit unfortunate as I was enjoying it so much.

I'll see if the last thing I can do is create a decent reproduction, but that may only be in a month or so.

chriscz avatar Sep 04 '24 05:09 chriscz

Hey @ejoffe I see you've been active merging recent PRs, looking good :)

Would really appreciate if this is something you can look into. I had to stop using spr because of this because it's a common need for me to re-order commits.

Do let me know if I can answer any questions regarding this.

chriscz avatar Mar 13 '25 04:03 chriscz

I can revisit this. Can you come up with a sequence that will reproduce this issue starting from a clean branch.

ejoffe avatar Mar 13 '25 04:03 ejoffe

Hi @ejoffe I have created a reproduction. I did not use count in this instance so it is more related to #385.

I think if we address that issue I will try to create another reproduction for --count since I think the bugs are related.

(https://github.com/chriscz/git-spr-reproductions)

chriscz avatar Mar 22 '25 01:03 chriscz