What the best PR workflow is
34 views
Skip to first unread message
Ondřej Čertík
Aug 7, 2012, 9:18:26 PM8/7/12
to sympy
Hi,
Here is what I think the best workflow is when working on a GitHub
Pull Request (PR), and I would be interested if you agree with me or
not.
1) Develop some fix or feature locally in git and commit often [1].
Then when I think that the branch works, I do "git rebase -i master"
and create a nice set of patches.
2) I create a PR
3) Other people comment, tests are being run, both automatically or
possibly also manually and results reported in comments. People also
pull the branch.
4) I address the issues again by repeating 1), but this time at the
end only rebasing code that is *not* yet in the PR, only local to my
computer.
And I *add* new patches to the public branch. In particular, I do not
rebase the branch.
5) Steps 3) and 4) are repeated until the PR is ready to merge.
6) Before it's merged, and the history is really ugly, only then it
makes sense to rebase it and create some nice set of patches.
The other circumstance is if it is some old PR and the code doesn't
merge without conflicts. Then again, rebasing is typically a cleaner
solution, unless people actually have pulled from the branch (which
usually is not the case for some old PR), in which case it's better to
"merge" with the master.
What makes the review quite difficult is that in the step 4) some
people simply rebase the whole thing, typically into one patch. Then
what happens is that
it becomes really difficult to figure out what exactly has changed
from my last review and comments. In particular, I left the following
comment in one PR:
"""
First one note for any PR ---- just keep adding patches to your
branch, so that we can quickly see what has changed and one can read
the whole PR as a history or "story". If you rebase, then suddenly all
the test results stop having any sense.
Only at the very end, when the PR is ready to go in, and the history
is really ugly (which actually is rarely the case), then one can do a
quick rebase and merge it.
"""
What do you think?
Ondrej
[1] My own personal workflow is typically that I have two terminals
open, in one I periodically execute "git commit -a -m 'X'" --- so that
I don't loose my work and also so that I can see all the changes that
I made and can easily debug it if something was briefly working at
some point and stopped working later --- and run tests, in the other
terminal I have a VIM session.
Here is what I think the best workflow is when working on a GitHub
Pull Request (PR), and I would be interested if you agree with me or
not.
1) Develop some fix or feature locally in git and commit often [1].
Then when I think that the branch works, I do "git rebase -i master"
and create a nice set of patches.
2) I create a PR
3) Other people comment, tests are being run, both automatically or
possibly also manually and results reported in comments. People also
pull the branch.
4) I address the issues again by repeating 1), but this time at the
end only rebasing code that is *not* yet in the PR, only local to my
computer.
And I *add* new patches to the public branch. In particular, I do not
rebase the branch.
5) Steps 3) and 4) are repeated until the PR is ready to merge.
6) Before it's merged, and the history is really ugly, only then it
makes sense to rebase it and create some nice set of patches.
The other circumstance is if it is some old PR and the code doesn't
merge without conflicts. Then again, rebasing is typically a cleaner
solution, unless people actually have pulled from the branch (which
usually is not the case for some old PR), in which case it's better to
"merge" with the master.
What makes the review quite difficult is that in the step 4) some
people simply rebase the whole thing, typically into one patch. Then
what happens is that
it becomes really difficult to figure out what exactly has changed
from my last review and comments. In particular, I left the following
comment in one PR:
"""
First one note for any PR ---- just keep adding patches to your
branch, so that we can quickly see what has changed and one can read
the whole PR as a history or "story". If you rebase, then suddenly all
the test results stop having any sense.
Only at the very end, when the PR is ready to go in, and the history
is really ugly (which actually is rarely the case), then one can do a
quick rebase and merge it.
"""
What do you think?
Ondrej
[1] My own personal workflow is typically that I have two terminals
open, in one I periodically execute "git commit -a -m 'X'" --- so that
I don't loose my work and also so that I can see all the changes that
I made and can easily debug it if something was briefly working at
some point and stopped working later --- and run tests, in the other
terminal I have a VIM session.
Aaron Meurer
Aug 7, 2012, 10:21:49 PM8/7/12
to sy...@googlegroups.com
IMHO, rebasing has too high of a potential of making the history not
make sense even if you don't squash, because of the way that changes
can be "rebased out" of commits. So I would recommend not rebasing at
all, if it's not necessary.
I definitely agree that it's a good idea to start a PR as soon as you
have a single commit, so that people can start to review it and test
it as you work. You can just say "work in progress" or "not ready to
merge" in the OP if it needs to wait for all the commits, and then
remove it when it's done.
Aaron Meurer
> --
> You received this message because you are subscribed to the Google Groups "sympy" group.
> To post to this group, send email to sy...@googlegroups.com.
> To unsubscribe from this group, send email to sympy+un...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/sympy?hl=en.
>
make sense even if you don't squash, because of the way that changes
can be "rebased out" of commits. So I would recommend not rebasing at
all, if it's not necessary.
I definitely agree that it's a good idea to start a PR as soon as you
have a single commit, so that people can start to review it and test
it as you work. You can just say "work in progress" or "not ready to
merge" in the OP if it needs to wait for all the commits, and then
remove it when it's done.
Aaron Meurer
> You received this message because you are subscribed to the Google Groups "sympy" group.
> To post to this group, send email to sy...@googlegroups.com.
> To unsubscribe from this group, send email to sympy+un...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/sympy?hl=en.
>
Ondřej Čertík
Aug 7, 2012, 10:54:52 PM8/7/12
to sy...@googlegroups.com
On Tue, Aug 7, 2012 at 7:21 PM, Aaron Meurer <asme...@gmail.com> wrote:
> IMHO, rebasing has too high of a potential of making the history not
> make sense even if you don't squash, because of the way that changes
> can be "rebased out" of commits. So I would recommend not rebasing at
> all, if it's not necessary.
Are you talking about rebasing in the step 1), or step 4) or step 6)?
> IMHO, rebasing has too high of a potential of making the history not
> make sense even if you don't squash, because of the way that changes
> can be "rebased out" of commits. So I would recommend not rebasing at
> all, if it's not necessary.
Are you suggesting to use "merge" only (if there are conflicts)?
>
> I definitely agree that it's a good idea to start a PR as soon as you
> have a single commit, so that people can start to review it and test
> it as you work. You can just say "work in progress" or "not ready to
> merge" in the OP if it needs to wait for all the commits, and then
> remove it when it's done.
title of the pull request,
I've seen it couple times in other projects (wip = work in progress). That way
you can easily see it and not even click on the pull request unless
you are interested.
Ondrej
Aaron Meurer
Aug 7, 2012, 11:15:11 PM8/7/12
to sy...@googlegroups.com
On Tue, Aug 7, 2012 at 8:54 PM, Ondřej Čertík <ondrej...@gmail.com> wrote:
> On Tue, Aug 7, 2012 at 7:21 PM, Aaron Meurer <asme...@gmail.com> wrote:
>> IMHO, rebasing has too high of a potential of making the history not
>> make sense even if you don't squash, because of the way that changes
>> can be "rebased out" of commits. So I would recommend not rebasing at
>> all, if it's not necessary.
>
> Are you talking about rebasing in the step 1), or step 4) or step 6)?
> Are you suggesting to use "merge" only (if there are conflicts)?
I guess the step 1 rebase is OK, because that's when you actually
> On Tue, Aug 7, 2012 at 7:21 PM, Aaron Meurer <asme...@gmail.com> wrote:
>> IMHO, rebasing has too high of a potential of making the history not
>> make sense even if you don't squash, because of the way that changes
>> can be "rebased out" of commits. So I would recommend not rebasing at
>> all, if it's not necessary.
>
> Are you talking about rebasing in the step 1), or step 4) or step 6)?
> Are you suggesting to use "merge" only (if there are conflicts)?
split up the commits. I personally don't do it that way, but I think
it works. But otherwise, I would avoid it.
Actually, I guess the big thing is rebasing over master. If you just
rebase over a commit that's already i your branch, it shouldn't happen
there either (but again, it's usually not necessary).
>
>>
>> I definitely agree that it's a good idea to start a PR as soon as you
>> have a single commit, so that people can start to review it and test
>> it as you work. You can just say "work in progress" or "not ready to
>> merge" in the OP if it needs to wait for all the commits, and then
>> remove it when it's done.
>
> I think it's becoming standard to use [WIP] as the first thing in the
> title of the pull request,
> I've seen it couple times in other projects (wip = work in progress). That way
> you can easily see it and not even click on the pull request unless
> you are interested.
>
> Ondrej
>
Ondřej Čertík
Aug 8, 2012, 12:28:32 AM8/8/12
to sy...@googlegroups.com
On Tue, Aug 7, 2012 at 8:15 PM, Aaron Meurer <asme...@gmail.com> wrote:
> On Tue, Aug 7, 2012 at 8:54 PM, Ondřej Čertík <ondrej...@gmail.com> wrote:
>> On Tue, Aug 7, 2012 at 7:21 PM, Aaron Meurer <asme...@gmail.com> wrote:
>>> IMHO, rebasing has too high of a potential of making the history not
>>> make sense even if you don't squash, because of the way that changes
>>> can be "rebased out" of commits. So I would recommend not rebasing at
>>> all, if it's not necessary.
>>
>> Are you talking about rebasing in the step 1), or step 4) or step 6)?
>> Are you suggesting to use "merge" only (if there are conflicts)?
>
> I guess the step 1 rebase is OK, because that's when you actually
> split up the commits. I personally don't do it that way, but I think
> it works.
Well, since nobody besides you can see your code, then of course it works.
> On Tue, Aug 7, 2012 at 8:54 PM, Ondřej Čertík <ondrej...@gmail.com> wrote:
>> On Tue, Aug 7, 2012 at 7:21 PM, Aaron Meurer <asme...@gmail.com> wrote:
>>> IMHO, rebasing has too high of a potential of making the history not
>>> make sense even if you don't squash, because of the way that changes
>>> can be "rebased out" of commits. So I would recommend not rebasing at
>>> all, if it's not necessary.
>>
>> Are you talking about rebasing in the step 1), or step 4) or step 6)?
>> Are you suggesting to use "merge" only (if there are conflicts)?
>
> I guess the step 1 rebase is OK, because that's when you actually
> split up the commits. I personally don't do it that way, but I think
> it works.
> But otherwise, I would avoid it.
>
> Actually, I guess the big thing is rebasing over master. If you just
> rebase over a commit that's already i your branch, it shouldn't happen
> there either (but again, it's usually not necessary).
But remember that if we have a PR that can't be merged because of conflicts,
we always ask the author "can you please rebase this" (over master)?
Typically it happens for PR's with last change a month (or more) ago.
I think that in this situation it is fine to rebase
instead of merge.
Ondrej
Joachim Durchholz
Aug 8, 2012, 4:38:23 AM8/8/12
to sy...@googlegroups.com
Am 08.08.2012 04:21, schrieb Aaron Meurer:
> IMHO, rebasing has too high of a potential of making the history not
> make sense even if you don't squash, because of the way that changes
> can be "rebased out" of commits.
The only situation where that happened to me was when I accidentally
> IMHO, rebasing has too high of a potential of making the history not
> make sense even if you don't squash, because of the way that changes
> can be "rebased out" of commits.
resolved a merge conflict in favor of "drop my change" without noticing
it. You really need to know what you're doing when rebasing.
However, I wouldn't ban rebasing to the back seat. Commit histories
often need to be cleaned up to remove dead ends (so people don't hit
them during a bisect), or to protect the developer from embarrassment or
publishing private data.
Such a rebase can be done in a separate step that doesn't merge with
master. I don't have the exact incantation memorized, I think its
git rebase -i origin
or something like that.
Just my 2c.
Sergiu Ivanov
Aug 8, 2012, 4:42:55 AM8/8/12
to sy...@googlegroups.com
Hello,
On Wed, Aug 8, 2012 at 4:18 AM, Ondřej Čertík <ondrej...@gmail.com> wrote:
>
> 4) I address the issues again by repeating 1), but this time at the
> end only rebasing code that is *not* yet in the PR, only local to my
> computer.
> And I *add* new patches to the public branch. In particular, I do not
> rebase the branch.
[...]
with a tiny additional detail. Once someone has started reviewing my
pull request, I do not push any changes until the reviewer has
finished the first round. I do modifications to my branch locally.
When the reviewer announces that the first round of review is done, I
push the changes and the second round of review starts. It is only
very rare that I rebase-edit the pull request; I usually just add the
fixes in separate commits, each of them generally addressing only a
couple issues, even if this results in one-line commits. When the
reviewers are done, I never mess with the history in the pull request,
and it is pushed as-is.
This strategy has worked nicely for me so far. I did do a couple
rebases though, but that was mainly about fixing typos in commit
messages. Thus I think rebasing is OK once it only introduces minor
changes.
Sergiu
On Wed, Aug 8, 2012 at 4:18 AM, Ondřej Čertík <ondrej...@gmail.com> wrote:
>
> 4) I address the issues again by repeating 1), but this time at the
> end only rebasing code that is *not* yet in the PR, only local to my
> computer.
> And I *add* new patches to the public branch. In particular, I do not
> rebase the branch.
> What makes the review quite difficult is that in the step 4) some
> people simply rebase the whole thing, typically into one patch. Then
> what happens is that
> it becomes really difficult to figure out what exactly has changed
> from my last review and comments.
Tom and I have established a similar workflow on my pull requests,
> people simply rebase the whole thing, typically into one patch. Then
> what happens is that
> it becomes really difficult to figure out what exactly has changed
> from my last review and comments.
with a tiny additional detail. Once someone has started reviewing my
pull request, I do not push any changes until the reviewer has
finished the first round. I do modifications to my branch locally.
When the reviewer announces that the first round of review is done, I
push the changes and the second round of review starts. It is only
very rare that I rebase-edit the pull request; I usually just add the
fixes in separate commits, each of them generally addressing only a
couple issues, even if this results in one-line commits. When the
reviewers are done, I never mess with the history in the pull request,
and it is pushed as-is.
This strategy has worked nicely for me so far. I did do a couple
rebases though, but that was mainly about fixing typos in commit
messages. Thus I think rebasing is OK once it only introduces minor
changes.
Sergiu
Chris Smith
Aug 8, 2012, 5:23:38 AM8/8/12
to sy...@googlegroups.com
I only ever rebase, and the only thing I have to watch out for is to
not extend acbk into master's commits, e.g. `git rebase -i head~5` to
work with the last 5 commits *of mine*. If the 6th one is a merge that
is part of master then that can be really nasty (it shows up as there
being more than 6 commits that you are working with in the editor --
that's how I know if I made a counting error).
not extend acbk into master's commits, e.g. `git rebase -i head~5` to
work with the last 5 commits *of mine*. If the 6th one is a merge that
is part of master then that can be really nasty (it shows up as there
being more than 6 commits that you are working with in the editor --
that's how I know if I made a counting error).
Jason Moore
Aug 8, 2012, 4:47:27 PM8/8/12
to sy...@googlegroups.com
I like the work in progress idea (WIP). The github pull request conversation view is a really nice way to talk about code with someone, regardless if it is ready to be merged into the stable branch or not. Github's internal workflow uses a pull early model: http://scottchacon.com/2011/08/31/github-flow.html . It would be nice if we could turn off the bots for WIP (early pulls) so that they don't clutter the conversation. Is that possible? Who is controlling the bots, or is it all automated? The bots stress me out when the pull request is young and still needs a lot of work and they don't provide that much useful information.
Jason
Jason
Aaron Meurer
Aug 8, 2012, 6:02:49 PM8/8/12
to sy...@googlegroups.com
I actually think that the bots are useful for these. It's basically
free test running for your code, meaning you can find and fix bugs
sooner in the development process.
Aaron Meurer
> https://groups.google.com/d/msg/sympy/-/1PcG3Mh2hQUJ.
free test running for your code, meaning you can find and fix bugs
sooner in the development process.
Aaron Meurer
> --
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To view this discussion on the web visit
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> https://groups.google.com/d/msg/sympy/-/1PcG3Mh2hQUJ.
Jason Moore
Aug 8, 2012, 6:17:20 PM8/8/12
to sy...@googlegroups.com
What about reducing the length of the SymPy bot's post in the conversation on the PR? I think the travisbot is a one liner. The test results can be found by click the link in the one line.
Example
SymPy bot test fails 32 tests, [details are here].
Jason
Example
SymPy bot test fails 32 tests, [details are here].
Jason
Aaron Meurer
Aug 8, 2012, 7:21:26 PM8/8/12
to sy...@googlegroups.com
That is exactly the goal of my pull request at
https://github.com/sympy/sympy-bot/pull/123. I've given an example of
a before and after:
https://github.com/sympy/sympy-bot/pull/123#issuecomment-7475278.
Perhaps we should upload even that info to the reviews site, and only
actually post one line on the PR as you suggest. That would be a
little bit more work, though (I'm personally not up to it right now,
but patches welcome).
Aaron Meurer
> https://groups.google.com/d/msg/sympy/-/_WSxjcria2IJ.
https://github.com/sympy/sympy-bot/pull/123. I've given an example of
a before and after:
https://github.com/sympy/sympy-bot/pull/123#issuecomment-7475278.
Perhaps we should upload even that info to the reviews site, and only
actually post one line on the PR as you suggest. That would be a
little bit more work, though (I'm personally not up to it right now,
but patches welcome).
Aaron Meurer
Aaron Meurer
Aug 8, 2012, 7:28:07 PM8/8/12
to sy...@googlegroups.com
Actually, I just realized that even that output can be made more
compact. How do people feel about something like
https://github.com/sympy/sympy-bot/pull/123#issuecomment-7600885 ?
Aaron Meurer
compact. How do people feel about something like
https://github.com/sympy/sympy-bot/pull/123#issuecomment-7600885 ?
Aaron Meurer
Chris Smith
Aug 8, 2012, 11:05:12 PM8/8/12
to sy...@googlegroups.com
To clean up the conversation I just delete bot runs (x in circle at
upper right of comment text field) that no longer apply.
upper right of comment text field) that no longer apply.
Aaron Meurer
Aug 8, 2012, 11:07:22 PM8/8/12
to sy...@googlegroups.com
I think only devs with push access can delete other people's comments.
But I think this should be OK, as the reviews will still be on the
reviews site.
Aaron Meurer
On Wed, Aug 8, 2012 at 9:05 PM, Chris Smith <smi...@gmail.com> wrote:
> To clean up the conversation I just delete bot runs (x in circle at
> upper right of comment text field) that no longer apply.
>
But I think this should be OK, as the reviews will still be on the
reviews site.
Aaron Meurer
On Wed, Aug 8, 2012 at 9:05 PM, Chris Smith <smi...@gmail.com> wrote:
> To clean up the conversation I just delete bot runs (x in circle at
> upper right of comment text field) that no longer apply.
>
> --
> You received this message because you are subscribed to the Google Groups "sympy" group.
> You received this message because you are subscribed to the Google Groups "sympy" group.
Joachim Durchholz
Aug 9, 2012, 3:29:38 AM8/9/12
to sy...@googlegroups.com
Am 09.08.2012 01:28, schrieb Aaron Meurer:
> Actually, I just realized that even that output can be made more
> compact. How do people feel about something like
> https://github.com/sympy/sympy-bot/pull/123#issuecomment-7600885 ?
+1.
> Actually, I just realized that even that output can be made more
> compact. How do people feel about something like
> https://github.com/sympy/sympy-bot/pull/123#issuecomment-7600885 ?
Maybe also merge "There were test failures" and "@mrocklin: Please fix
the test failures" into one sentence.
Reply all
Reply to author
Forward
0 new messages