stg rebase sometimes creates a new patch from an upstream commit

[aczapszys-apex]

I haven't been able to produce a test case, but here's what I'm seeing.

When I have this workflow, sometimes I see a new patch created from a commit on origin/main. This is not the most recent commit either.

from branch main

git switch -c topic
stg init
Create a bunch patches...

stg commit

Create a PR from the committed patch, and get it merged into main

git fetch
stg rebase origin/main
stg series

bottom most patch is seemingly random new patch from a commit from origin/main

[jpgrayson]

Thanks for this issue report @aczapszys-apex. The workflow you describe seems to match a pretty common workflow that I and others use. I have not seen this kind of issue.

Some questions:

On your topic branch it sounds like you have multiple patches, but you submit a PR only for the first (bottom most) patch in the series. Is that right?

Why do you stg commit patches before a PR is merged? That part of the workflow is a little different than what I do. If the PR review demands changes to patches, I want StGit to still be managing those patches so that I can make changes and repush them (without having to uncommit).

random new patch from origin/main

How random? The commit message isn't auto-filled from /dev/urandom, right? Does this new bottom most patch share any characteristics, i.e. either the commit message or patch content, with the PR patch? Or is it perhaps related to other commits (i.e. from other people) that landed on origin/main before your fetch and rebase?

Committing the patches posted to the PR also affects what happens when the PR is merged and you run stg rebase. Maybe this is where the problem emerges. If a merge commit is created when the PR is merged (versus a fast-forward merge being done), then perhaps the post-rebase commit DAG is confused.

Starting point:

      A---B---C topic
     /
D---E origin/main

Assuming A is committed with stg commit and pushed as a PR, then B and C would be the patches remaining in the series.

If a merge commit is created for A, then origin would have this state:

      A
     / \
D---E---A' origin/main

Because patch A was committed, It's not immediately clear to me what would happen when trying to rebase topic to origin/main, which is why I think this might be where the problem occurs.

What you perhaps want from the rebase is:

      A   B---C topic
     / \ /
D---E---A' origin/main

But because A was committed, it would sort of anchor topic. The rebase might thus end up looking like this:

      A   A''--B---C topic
     / \ /
D---E---A' origin/main

Where A'' would be an empty or mostly-empty commit.

I recommend making two adjustments to this workflow:

1. Do not stg commit patches that are pushed for PR. Or, if you do commit them, uncommit before rebasing.
2. Rebase using stg rebase -m. The -m/--merged flag causes StGit to check for patches that may have been merged upstream, leaving such patches empty in your series.
3. Run stg clean after rebasing to remove any merged (and thus empty) patches from your series.

Let me know if any of this helps get closer to the cause of the issue.

[aczapszys-apex]

Thanks for the advice. I wondered if there was something off about my workflow.

FYI, our configuration forces all PRs to be squash committed, which is why I'm doling out one patch at a time.

[lkraav]

Run stg clean after rebasing to remove any merged (and thus empty) patches from your series.

Why wouldn't stg rebase -m auto-clean? Are there any high % risks?

[jpgrayson]

Why wouldn't stg rebase -m auto-clean? Are there any high % risks?

Having empty patches is normal in StGit and historically removing patches has required an explicit user intent. Thus the existence of stg clean and requiring stg branch --delete --force to remove a branch with patches.

So I don't know that there are risks of bad/unrecoverable things happening if stg rebase -m auto-deleted patches, but it would be a breaking change.

I think a config option to auto delete when using --merged (either with rebase or perhaps also with push) could be a nice addition though.

[lkraav]

So I don't know that there are risks of bad/unrecoverable things happening if stg rebase -m auto-deleted patches, but it would be a breaking change.

I think a config option to auto delete when using --merged (either with rebase or perhaps also with push) could be a nice addition though.

Option sounds good, although decisions are a lighter maintenance load, maybe.

Main UX problem is there's no clear indication cleanup should be performed.