search

ejoffe / spr

Stacked Pull Requests on GitHub
MIT License
717 stars 65 forks source link

PR stacks from different users interfere if they share the same local branch name #308

Closed MichaelSims closed 1 year ago

MichaelSims commented 1 year ago

Unfortunately it seems https://github.com/ejoffe/spr/pull/304 introduced an issue:

  1. Bob creates a stack in his local master branch and runs git spr update.
  2. Alice runs git spr update from her local master branch. git spr assumes the PR branches beginning with spr/main/ belong to her. Her local main happens to be pointing to origin/main, and git spr closes the "missing" PRs

As I mentioned in https://github.com/ejoffe/spr/discussions/301 , in my opinion the better way to handle this is as Gerrit does... filter the PRs by the local commit stack, even if this means losing the "auto close" feature.

ejoffe commented 1 year ago

Yikes, I haven't tested it with multiple users. I can add the PR filter based on local stack, that shouldn't be too bad.

There is still a foreseeable issue with sharing stacks though, I forget how Gerrit handles this. Spr does a force push of your stack on spr update, this means that if Bob pulls Alices's stack and amends a commit, the next time Alice runs spr update, Bob's change will be gone. I'm not quite sure how to handle this case gracefully.

MichaelSims commented 1 year ago

There is still a foreseeable issue with sharing stacks though, I forget how Gerrit handles this. Spr does a force push of your stack on spr update, this means that if Bob pulls Alices's stack and amends a commit, the next time Alice runs spr update, Bob's change will be gone. I'm not quite sure how to handle this case gracefully.

True, I have been giving this some thought... if memory serves, gerrit doesn't handle this any differently... i.e. if you have checked out a series of patches pushed by someone else, then change the history, and then push to refs/for/master, it will essentially replace what was there too. The only real difference is that gerrit would show it as "revision N+1" and "revision N" would still be viewable (and therefore easily restorable). I don't think this is a huge deal... the collab workflow just requires a little more thought and staying in communication w/the people you are collaborating with.

Perhaps some sort of heuristic where you compare commit dates and warn/prompt if the commits being pushed are "older" than the previous revision? Although I don't think git spr really prompts for anything yet so that may not be a line you want to cross.

ejoffe commented 1 year ago

I have fixed this bug, and also removed the branch name from the pr namespace. I'm very happy with this change, thank you for proposing it. PRs actually do mostly get closed automatically. With a few exceptions which could be improved.

ejoffe commented 1 year ago

should be fixed in v0.12.1

MichaelSims commented 1 year ago

I have fixed this bug, and also removed the branch name from the pr namespace. I'm very happy with this change, thank you for proposing it. PRs actually do mostly get closed automatically. With a few exceptions which could be improved.

Thank you! This will make things much nicer for my team. I appreciate all your efforts.