Handling PR's with stgit

150 views
Skip to first unread message

Ismael Bouya

unread,
Nov 30, 2018, 3:52:05 AM11/30/18
to st...@googlegroups.com
Hi everyone,
I’m starting to use stgit to handle patches, and it seems like a great
tool!

I’m wondering how you’re handling PR's with stgit, i.e. selecting a
bunch of patches and turn them into a PR. The current workflow I have is
creating a new branch on top of the patches, rebasing, removing the
commits I’m not interested in and push that branch remotely to create a
PR. It works fine but not ideal (lingering branches, losing track of
changes required during the review of the PR, etc.)

I thought to use the "publish" command for that, but it seems not to
work for what I want to do. Did I miss something? How do you handle this
kind of things?

--
Ismael
signature.asc

Catalin Marinas

unread,
Nov 30, 2018, 5:49:10 AM11/30/18
to ismael....@normalesup.org, st...@googlegroups.com
Hi Ismael,

On Fri, 30 Nov 2018 at 08:52, Ismael Bouya
<ismael....@normalesup.org> wrote:
> I’m wondering how you’re handling PR's with stgit, i.e. selecting a
> bunch of patches and turn them into a PR. The current workflow I have is
> creating a new branch on top of the patches, rebasing, removing the
> commits I’m not interested in and push that branch remotely to create a
> PR. It works fine but not ideal (lingering branches, losing track of
> changes required during the review of the PR, etc.)

StGit has a "commit" command to "freeze" the state of a branch and
avoid inadvertently changing the sha1 of the patches to be published
(assuming that the patches you don't want are popped from the stack).
Ideally you'd send the PR with a (signed) tag that points to the
stable sha1. This way, after the PR, you can continue to work on that
branch, push additional patches on top and even publish them as long
as the tag in the PR is not changed (and you don't "uncommit" any
patches).

That's pretty much my workflow combining StGit and Git. I even have a
"git series" alias to show me all the patches in the current branch
that have not been merged (including the stg committed ones),
equivalent to:

git log --oneline origin/master..

> I thought to use the "publish" command for that, but it seems not to
> work for what I want to do. Did I miss something? How do you handle this
> kind of things?

I was wondering whether to remove this command or change its
behaviour, I don't use it at all these days. The original intent of
this command was to be able to continue preparing your patches
(rebase, refresh etc.) while providing a stable for merge public
branch. It tries to generate a completely new commit with the diff
between the published branch and your working branch; a bit like git
fixups but probably worse as an additional commit could cover changes
in multiple patches.

We could make publish behave as "commit" + resetting/fast-forwarding
the head of a separate branch to be published (<branch>.public) if
anyone thinks it's useful.

Catalin

Peter Maydell

unread,
Nov 30, 2018, 7:05:07 AM11/30/18
to Catalin Marinas, ismael....@normalesup.org, st...@googlegroups.com
On Fri, 30 Nov 2018 at 10:49, Catalin Marinas <catalin...@gmail.com> wrote:
> I was wondering whether to remove this command or change its
> behaviour, I don't use it at all these days. The original intent of
> this command was to be able to continue preparing your patches
> (rebase, refresh etc.) while providing a stable for merge public
> branch. It tries to generate a completely new commit with the diff
> between the published branch and your working branch; a bit like git
> fixups but probably worse as an additional commit could cover changes
> in multiple patches.
>
> We could make publish behave as "commit" + resetting/fast-forwarding
> the head of a separate branch to be published (<branch>.public) if
> anyone thinks it's useful.

I don't use "stg publish" now, but I used to, for maintaining
the qemu-linaro tree. That tree was a mess of patches which I
was gradually cleaning up and occasionally upstreaming parts of.
I wanted to provide a user-facing "this branch never rebases"
view, but mostly work on the "this is a stack of patches which
I shuffle around, move code from one place to another, rebase
on newer upstream versions" view. stg publish worked perfectly
for that.

thanks
-- PMM

Ismael Bouya

unread,
Nov 30, 2018, 7:26:17 AM11/30/18
to st...@googlegroups.com
Hi Catalin,
Thanks for the answer!

> StGit has a "commit" command to "freeze" the state of a branch and
> avoid inadvertently changing the sha1 of the patches to be published
> (assuming that the patches you don't want are popped from the stack).
> Ideally you'd send the PR with a (signed) tag that points to the
> stable sha1. This way, after the PR, you can continue to work on that
> branch, push additional patches on top and even publish them as long
> as the tag in the PR is not changed (and you don't "uncommit" any
> patches).

But how do you handle needed changes with that workflow? (usually when
you do a PR, you expect a review and possibly some changes to do to that
PR)
With "bare git" I commit --amend (/rebase) and repush and it updates on
github/gitlab side. With a tag, this cannot work.

> I was wondering whether to remove this command or change its
> behaviour, I don't use it at all these days. The original intent of
> this command was to be able to continue preparing your patches
> (rebase, refresh etc.) while providing a stable for merge public
> branch. It tries to generate a completely new commit with the diff
> between the published branch and your working branch; a bit like git
> fixups but probably worse as an additional commit could cover changes
> in multiple patches.

Thanks!

> We could make publish behave as "commit" + resetting/fast-forwarding
> the head of a separate branch to be published (<branch>.public) if
> anyone thinks it's useful.

Here is a scenario that comes to mind of what I’d like to do:

I am on the branch "master", with a bunch of patches on top of it,
I want to push patches A B and E to a new PR:
- stg create-pull-request --branch=cool_feature A B E
- git push origin cool_feature
- (edit patch A with the maintainer feedback, the commands to do that
already exist in stgit)
- stg update-pull-request --branch=cool_feature
- git push origin +cool_feature
- (maybe some comment to "close" the PR once merged or refused)

(stg could also handle the pushing part but it’s details)
In the workflow, I never "leave" my "master+patches", but I can still
easily push new PR’s extracted from my patches.

The idea of being able to specify a branch is that usually you will have
more than one PR open for a given remote, so giving them a name is
important.

How easily would this kind of thing be done, and is it in the scope of
stgit's goal? I may try to have a look at it if you tell me it can be of
some use (I didn’t look at stgit’s code yet, but I have quite a good
knowledge of git).
--
Ismael
signature.asc

Pete Grayson

unread,
Nov 30, 2018, 1:35:48 PM11/30/18
to Catalin Marinas, Peter Maydell, ismael....@normalesup.org, st...@googlegroups.com
On 18/11/30 12:04, Peter Maydell wrote:
> On Fri, 30 Nov 2018 at 10:49, Catalin Marinas <catalin...@gmail.com> wrote:
> > I was wondering whether to remove this command or change its
> > behaviour, I don't use it at all these days. The original intent of
> > this command was to be able to continue preparing your patches
> > (rebase, refresh etc.) while providing a stable for merge public
> > branch. It tries to generate a completely new commit with the diff
> > between the published branch and your working branch; a bit like git
> > fixups but probably worse as an additional commit could cover changes
> > in multiple patches.
> >
> > We could make publish behave as "commit" + resetting/fast-forwarding
> > the head of a separate branch to be published (<branch>.public) if
> > anyone thinks it's useful.

This is an interesting idea. I wonder if it might make sense as an
option to `stg commit` instead of radically altering the semantics of
`stg publish`?

> I don't use "stg publish" now, but I used to, for maintaining
> the qemu-linaro tree. That tree was a mess of patches which I
> was gradually cleaning up and occasionally upstreaming parts of.
> I wanted to provide a user-facing "this branch never rebases"
> view, but mostly work on the "this is a stack of patches which
> I shuffle around, move code from one place to another, rebase
> on newer upstream versions" view. stg publish worked perfectly
> for that.

I'm glad to hear this discussion about `stg publish` since I had not
really understood its use case.

Regarding whether to remove `stg publish`, I'm imagining there is
probably a stgit user or two out there still getting value from it. And
since it is already implemented using the "lib" framework, I do not see
too much maintenance burden in keeping it alive; the exception being
that its test suite is a bit thin. At some point I will get around to
improving its test coverage. TBD if I may have already inadvertently
broken it...

Pete

Catalin Marinas

unread,
Nov 30, 2018, 2:19:35 PM11/30/18
to st...@googlegroups.com
On Fri, 30 Nov 2018 at 12:26, Ismael Bouya
<ismael....@normalesup.org> wrote:
> > StGit has a "commit" command to "freeze" the state of a branch and
> > avoid inadvertently changing the sha1 of the patches to be published
> > (assuming that the patches you don't want are popped from the stack).
> > Ideally you'd send the PR with a (signed) tag that points to the
> > stable sha1. This way, after the PR, you can continue to work on that
> > branch, push additional patches on top and even publish them as long
> > as the tag in the PR is not changed (and you don't "uncommit" any
> > patches).
>
> But how do you handle needed changes with that workflow? (usually when
> you do a PR, you expect a review and possibly some changes to do to that
> PR)
> With "bare git" I commit --amend (/rebase) and repush and it updates on
> github/gitlab side. With a tag, this cannot work.

In the Linux kernel world, when you submitted a pull request the
patches have already been reviewed, so no further rebase is necessary.
I didn't have a need for amending it (well, not very often).

> > We could make publish behave as "commit" + resetting/fast-forwarding
> > the head of a separate branch to be published (<branch>.public) if
> > anyone thinks it's useful.
>
> Here is a scenario that comes to mind of what I’d like to do:
>
> I am on the branch "master", with a bunch of patches on top of it,
> I want to push patches A B and E to a new PR:
> - stg create-pull-request --branch=cool_feature A B E
> - git push origin cool_feature
> - (edit patch A with the maintainer feedback, the commands to do that
> already exist in stgit)
> - stg update-pull-request --branch=cool_feature
> - git push origin +cool_feature
> - (maybe some comment to "close" the PR once merged or refused)

There is also a "stg sync" command. You can create a PR branch where
you cherry-pick some of the patches but continue to work on your
development branch. Before the PR, you switch to the PR branch and run
a "stg sync -a" which, in theory, should synchronise the patches with
the corresponding ones in your master branch. Now, I don't remember
when I last used this command, I even forgot it existed ;).

We could probably do this as well with an extension to "sync" to be
able to update patches in a different branch ("--to=" option) without
switching the current one. We could also replace the more complicated
3-way merge of "sync" with a re-write of the patches on the --to=
branch since handling a 3-way merge outside the current index is
problematic anyway.

But I'm open to having a completely different command for preparing
PRs. "commit" has been sufficient for my workflow so far.

--
Catalin

Pete Grayson

unread,
Nov 30, 2018, 3:49:35 PM11/30/18
to Catalin Marinas, Ismael Bouya, st...@googlegroups.com
On 18/11/30 07:19, Catalin Marinas wrote:
> On Fri, 30 Nov 2018 at 12:26, Ismael Bouya
> <ismael....@normalesup.org> wrote:
> > > StGit has a "commit" command to "freeze" the state of a branch and
> > > avoid inadvertently changing the sha1 of the patches to be published
> > > (assuming that the patches you don't want are popped from the stack).
> > > Ideally you'd send the PR with a (signed) tag that points to the
> > > stable sha1. This way, after the PR, you can continue to work on that
> > > branch, push additional patches on top and even publish them as long
> > > as the tag in the PR is not changed (and you don't "uncommit" any
> > > patches).
> >
> > But how do you handle needed changes with that workflow? (usually when
> > you do a PR, you expect a review and possibly some changes to do to that
> > PR)
> > With "bare git" I commit --amend (/rebase) and repush and it updates on
> > github/gitlab side. With a tag, this cannot work.

The workflow Catalin described involves pushing new commits for any
changes needed to the PR after it is initially published.

Another workflow can be used if it is acceptable to change the commits
associated with a published PR instead of just adding new commits. Given
a series with patches A-F where A, B, and C are the patches for the PR,
you can simply apply and push the patches associated with your PR:

stg goto C
git push origin master

A PR can then be created based on your master branch. If you have to
make changes to any of your patches (i.e. due to comments on your PR):

stg goto B
<edit files>
stg refresh
stg goto C
git push origin +master

Rinse and repeat as necessary.

While the PR is pending, you can continue working on other patches in
your queue (D, E, F, ...).

Once the PR is merged into the upstream repo, you can cleanup your
patches:

stg goto C
stg pull --merged upstream

Patches A, B, and C should now be empty. Verify with:

stg series -e

Finally, remove all the now-empty patches with:

stg clean

There are a couple of things about the above workflow that do not meet
your goals:

1. PR is on master, not a named branch.
2. Only works for one PR at a time.

However, this workflow can be good in cases where only having one
outstanding PR at a time is acceptable.

> > Here is a scenario that comes to mind of what I’d like to do:
> >
> > I am on the branch "master", with a bunch of patches on top of it,
> > I want to push patches A B and E to a new PR:
> > - stg create-pull-request --branch=cool_feature A B E
> > - git push origin cool_feature
> > - (edit patch A with the maintainer feedback, the commands to do that
> > already exist in stgit)
> > - stg update-pull-request --branch=cool_feature
> > - git push origin +cool_feature
> > - (maybe some comment to "close" the PR once merged or refused)
>
> There is also a "stg sync" command. You can create a PR branch where
> you cherry-pick some of the patches but continue to work on your
> development branch. Before the PR, you switch to the PR branch and run
> a "stg sync -a" which, in theory, should synchronise the patches with
> the corresponding ones in your master branch. Now, I don't remember
> when I last used this command, I even forgot it existed ;).

I had never successfully used `stg sync`. I just did a little
experimentation, but when I initialized my PR branch by `stg pick`ing
patches from master, I ended up with merge conflicts even in trivial
cases where I only modified the patch on master. Also, it doesn't seem
like `stg sync` synchronizes the commit messages.

> We could probably do this as well with an extension to "sync" to be
> able to update patches in a different branch ("--to=" option) without
> switching the current one. We could also replace the more complicated
> 3-way merge of "sync" with a re-write of the patches on the --to=
> branch since handling a 3-way merge outside the current index is
> problematic anyway.

+1 for this idea. The 3-way merge behavior does not really fit the use
case where canonical patches are maintained in a master series and
farmed-out to other branches for creating PR's. Maybe a `--replace`
option as well to tell `stg sync` to do simple replacement of patches on
the target branch instead of the 3-way merge.

stg branch --create cool_feature
stg pick -B master A B E
stg branch master
git push origin cool_feature
<create PR, get feedback>
stg goto A
<edit>
stg refresh
stg sync --replace --to=cool_feature A B E
git push origin +cool_feature

> But I'm open to having a completely different command for preparing
> PRs. "commit" has been sufficient for my workflow so far.

Agreed. StGit features that help with managing GitHub-style PR's would
be welcome. I'm open to both new subcommands and improvements to
existing subcommands like `sync`, `publish`, `branch`, and `pick`.

Pete

Ismael Bouya

unread,
Dec 1, 2018, 6:32:01 AM12/1/18
to st...@googlegroups.com
Hi Pete,

(Fri, Nov 30, 2018 at 03:49:33PM -0500) Pete Grayson :
> The workflow Catalin described involves pushing new commits for any
> changes needed to the PR after it is initially published.

Yes, but usually (at least everywhere I worked) you’re expected to have
a list of relevant commits and rebase them before merging, that’s the
workflow I want to have.

> Another workflow can be used if it is acceptable to change the commits
> associated with a published PR instead of just adding new commits. Given
> a series with patches A-F where A, B, and C are the patches for the PR,
> you can simply apply and push the patches associated with your PR:
>
> (...)
> 1. PR is on master, not a named branch.
> 2. Only works for one PR at a time.
>
> However, this workflow can be good in cases where only having one
> outstanding PR at a time is acceptable.

Yes, that’s more or less the workflow I have currently (with more git
commands as I’m still not used to stgit commands enough :p ), but having
several concurrent PR's plus having the whole patches applied locally is
important to me:
My use case is that I have a bunch of private patches to projects (be it
a work project with local debug features, or a public project for which
I have very specific features not used by anyone), and I also contribute
to these projects. Locally, I want all the features I coded, and
upstream, I want to be able to cherry-pick patches and push them in the
order I’m pleased to, sometimes (often) even concurrently.

Currently (before knowing about stgit, now it’s a bit better :) ) I have
a branch merge nightmare (some projects with 10's branch, some depending
on others) on top of upstream master’s, following as good as I can but
it’s not practical.

Anyway, I’ll learn a bit more about stgit to get used to it, and maybe
create the ~patches~PR's I like to it at some points, and we’ll see!

Thanks for your helps!
--
Ismael
signature.asc

Ismael Bouya

unread,
Dec 1, 2018, 6:32:01 AM12/1/18
to st...@googlegroups.com
> In the Linux kernel world, when you submitted a pull request the
> patches have already been reviewed, so no further rebase is necessary.
> I didn't have a need for amending it (well, not very often).

Yes, I figured out that stgit was written with that workflow in mind,
but this project is the exception rather than the rule I think :)

> There is also a "stg sync" command. You can create a PR branch where
> you cherry-pick some of the patches but continue to work on your
> development branch. Before the PR, you switch to the PR branch and run
> a "stg sync -a" which, in theory, should synchronise the patches with
> the corresponding ones in your master branch. Now, I don't remember
> when I last used this command, I even forgot it existed ;).

I tried to use it, but it requires a lot of preparation (the branch
needs to exist and be properly initialised with stgit for instance)

> We could probably do this as well with an extension to "sync" to be
> able to update patches in a different branch ("--to=" option) without
> switching the current one. We could also replace the more complicated
> 3-way merge of "sync" with a re-write of the patches on the --to=
> branch since handling a 3-way merge outside the current index is
> problematic anyway.
>
> But I'm open to having a completely different command for preparing
> PRs. "commit" has been sufficient for my workflow so far.

Thanks, I’ll try to think of it and maybe submit something.

Kind regards,
--
Ismael
signature.asc
Reply all
Reply to author
Forward
0 new messages