Workflow for failed pull requests

On Fri, Dec 2, 2011 at 4:58 PM, Stefan Zeiger wrote:
> I'm wondering what's the suggested workflow for failed pull requests. Simply
> adding another commit to the branch in question is probably not an option
> because the pull requests are supposed to be in a state that allows merging
> into trunk without further squashing or rewriting.

Pull requests get automatically updated if you push to the
corresponding branches, even if it is a rebase. You lose a bit of
context in the pull request discussion (because the old state of the
branch isn't easily accessible any more) but otherwise this should
work.

Conventionally, you would add another commit to the pull request.  Git conventions are very strict that you do not rewrite history that is not private to you.  Pull requests count as "not private".  This would of course mean that we could add commits to pull requests, which would then get merged, but I see no problem with that.  Commit history is supposed to be representative.  Indiscriminate squashing is just as bad as incessant merging of checkpoint commits.

Incidentally, a case where the "submit a new squashed pull request" idea breaks down is where multiple people are collaborating on the same pull request (which is a situation that we want to encourage).

In other words, I would say we should keep the history clean, but it is vastly more important to avoid public history rewriting.  The kernel developers have been dealing with these sorts of questions for a long time; let's not reinvent the process wheel.

Daniel

On Fri, Dec 2, 2011 at 9:58 AM, Stefan Zeiger <szeiger@novocode.com> wrote:

Hi,

I'm wondering what's the suggested workflow for failed pull requests. Simply adding another commit to the branch in question is probably not an option because the pull requests are supposed to be in a state that allows merging into trunk without further squashing or rewriting.

So, rewrite the branch in the pull request? That's what I just did. I also had to rewrite another pull request with a follow-up patch. I then notified Paul about the changes so he could retry integrating them. But rewriting public branches is ugly, plus I'm wondering if this would this work with an automated system.

Or delete the public branch and pull request, rewrite the local one, and publish it again? Is this any better? The branch name would still be the same but that's what we want if fixes for tickets should come in branches named e.g. issue/1234, right?

I'm used to much simpler git workflows, so maybe one of the git experts can advise?

Cheers,
Stefan

On 2 December 2011 17:39, Daniel Spiewak wrote:
> Conventionally, you would add another commit to the pull request.  Git
> conventions are very strict that you do not rewrite history that is not
> private to you.  Pull requests count as "not private".  This would of course
> mean that we could add commits to pull requests, which would then get
> merged, but I see no problem with that.  Commit history is supposed to be
> representative.  Indiscriminate squashing is just as bad as incessant
> merging of checkpoint commits.
>
> Incidentally, a case where the "submit a new squashed pull request" idea
> breaks down is where multiple people are collaborating on the same pull
> request (which is a situation that we want to encourage).
>
> In other words, I would say we should keep the history clean, but it is
> vastly more important to avoid public history rewriting.  The kernel
> developers have been dealing with these sorts of questions for a long time;
> let's not reinvent the process wheel.

Yup. Also, github ui has no support for rewritten pull requests which
means your previous comments.

If pull request needs a major rework then close it and start with a
new one that has clean history. For minor tweaking just keep appending
commits.

On Fri, Dec 02, 2011 at 05:43:58PM +0100, Grzegorz Kossakowski wrote:
> Yup. Also, github ui has no support for rewritten pull requests which
> means your previous comments.
>
> If pull request needs a major rework then close it and start with a
> new one that has clean history. For minor tweaking just keep appending
> commits.

Yeah, on projects I work on the convention is usually for submitters to
create a branch for each pull request, and then don't touch it. This
way it doesn't block the submitter (who can return to their work
branch, master, wherever) and it's easier to avoid accidental rebasing
and what-have-you.

Incidentally, I haven't seen serious projects that regularly use the
Github UI to merge pull requests... the idea of taking someone's patch
and merging directly into your main repo is terrifying (at least
compared to merging it locally, testing things out, and only pushing
once things are fine).

On 2 December 2011 17:50, Erik Osheim wrote:
>
> Incidentally, I haven't seen serious projects that regularly use the
> Github UI to merge pull requests... the idea of taking someone's patch
> and merging directly into your main repo is terrifying (at least
> compared to merging it locally, testing things out, and only pushing
> once things are fine).

We'll have automatic tools for that (jenkins).

Yeah, on projects I work on the convention is usually for submitters to
create a branch for each pull request, and then don't touch it. This
way it doesn't block the submitter (who can return to their work
branch, master, wherever) and it's easier to avoid accidental rebasing
and what-have-you.

I think it's ok to add stuff to a pull request branch even after opening the pull request.  Maybe you found something that you wanted to revise, or perhaps the review was taking a long time and you were able to get more done.  My policy (on projects that I run on GitHub) is that contributors should absolutely isolate their pull requests in a branch, but they should feel perfectly free to keep working on it as the spirit moves.  GitHub will update the pull request, and all I need to do (as a reviewer) is be sure to grab the branch and look at it in its entirety.  It's no harder than reviewing the pull request would otherwise be, and it removes another barrier for continued contribution.

Incidentally, I haven't seen serious projects that regularly use the
Github UI to merge pull requests... the idea of taking someone's patch
and merging directly into your main repo is terrifying (at least
compared to merging it locally, testing things out, and only pushing
once things are fine).

Big +1 here.  GitHub's merge UI is cute, but I never touch it.  It's really not that hard to add a remote, fetch, checkout, build, merge and push.  There are also other advantages to this.  For example, GitHub's merge creates unnecessary merge commits, even when the pull request is a fast-forward.  So, if you value uncluttered history, don't push the button!

Daniel

On Fri, Dec 2, 2011 at 5:10 PM, Daniel Spiewak <djspiewak@gmail.com> wrote:

Big +1 here.  GitHub's merge UI is cute, but I never touch it.  It's really not that hard to add a remote, fetch, checkout, build, merge and push.  There are also other advantages to this.  For example, GitHub's merge creates unnecessary merge commits, even when the pull request is a fast-forward.  So, if you value uncluttered history, don't push the button!

One comment, it's definitely a good idea to avoid merge commits for single commit branches. Having said that, it _is_ a good idea to have the merge commit if there are multiple commits in the branch (even if it's a fast forward).
For a clean history, I suggest rebasing _before_ the merge and then merging it _with_ the merge commit. That way the history is linear with a merge commit showing clearly the commits that were part of a given feature branch.
Best,Ismael

Linear history is nice, but rebasing public history is a big no-no in terms of Git best practices.  Contributors should rebase prior to opening the pull request.  Once the pull request is open, neither the contributor nor the reviewer should be rebasing anything in that commit set.  For example, imagine master is at rev abc123 and someone creates commit def456 (parented on abc123).  Meanwhile, master has moved forward to ghi789.  The contributor should rebase def456 on top of ghi789 before opening the pull request (creating def456').  Now, a reviewer comes along and wants to merge this pull request, but master has moved forward to jkl098.  The reviewer should not rebase def456', since that commit is now in public ownership and other contributors may have based work on it.  Instead, they should merge def456' on top of jkl098.  Non-linear history does have its place.

Note that if a contributor bases subsequent work on def456' (maybe the reviewer took too long and someone wanted to continue that enhancement), they should feel free to rebase any of their subsequent work prior to publication, but they cannot rebase def456', since they don't "own" that commit.

Linus sent an epic email on the kernel list a while back outlining all of this (and more).  Basically, these restrictions are intended to allow the maximum flexibility in keeping history pretty without causing any forced updates and history version conflicts.  This in turn makes the contribution process smoother and less prone to frustration.

Daniel

On Fri, Dec 2, 2011 at 11:20 AM, Ismael Juma <ismael@juma.me.uk> wrote:

On Fri, Dec 2, 2011 at 5:10 PM, Daniel Spiewak <djspiewak@gmail.com> wrote:

Big +1 here.  GitHub's merge UI is cute, but I never touch it.  It's really not that hard to add a remote, fetch, checkout, build, merge and push.  There are also other advantages to this.  For example, GitHub's merge creates unnecessary merge commits, even when the pull request is a fast-forward.  So, if you value uncluttered history, don't push the button!

One comment, it's definitely a good idea to avoid merge commits for single commit branches. Having said that, it _is_ a good idea to have the merge commit if there are multiple commits in the branch (even if it's a fast forward).
For a clean history, I suggest rebasing _before_ the merge and then merging it _with_ the merge commit. That way the history is linear with a merge commit showing clearly the commits that were part of a given feature branch.
Best,Ismael

I should clarify that in the previous narrative, I was using the "pull request is open" state to represent "commits have been pushed into a public repo".  Contributors will push commits into their own forks prior to opening a pull request.  It is that moment at which they can no longer rebase, since that is the moment at which they no longer hold private ownership of those commits (i.e. history rewriting would potentially disrupt others).

Daniel

On Fri, Dec 2, 2011 at 11:44 AM, Daniel Spiewak <djspiewak@gmail.com> wrote:

Linear history is nice, but rebasing public history is a big no-no in terms of Git best practices.  Contributors should rebase prior to opening the pull request.  Once the pull request is open, neither the contributor nor the reviewer should be rebasing anything in that commit set.  For example, imagine master is at rev abc123 and someone creates commit def456 (parented on abc123).  Meanwhile, master has moved forward to ghi789.  The contributor should rebase def456 on top of ghi789 before opening the pull request (creating def456').  Now, a reviewer comes along and wants to merge this pull request, but master has moved forward to jkl098.  The reviewer should not rebase def456', since that commit is now in public ownership and other contributors may have based work on it.  Instead, they should merge def456' on top of jkl098.  Non-linear history does have its place.

Note that if a contributor bases subsequent work on def456' (maybe the reviewer took too long and someone wanted to continue that enhancement), they should feel free to rebase any of their subsequent work prior to publication, but they cannot rebase def456', since they don't "own" that commit.

Linus sent an epic email on the kernel list a while back outlining all of this (and more).  Basically, these restrictions are intended to allow the maximum flexibility in keeping history pretty without causing any forced updates and history version conflicts.  This in turn makes the contribution process smoother and less prone to frustration.

Daniel

On Fri, Dec 2, 2011 at 11:20 AM, Ismael Juma <ismael@juma.me.uk> wrote:

On Fri, Dec 2, 2011 at 5:10 PM, Daniel Spiewak <djspiewak@gmail.com> wrote:

Big +1 here.  GitHub's merge UI is cute, but I never touch it.  It's really not that hard to add a remote, fetch, checkout, build, merge and push.  There are also other advantages to this.  For example, GitHub's merge creates unnecessary merge commits, even when the pull request is a fast-forward.  So, if you value uncluttered history, don't push the button!

One comment, it's definitely a good idea to avoid merge commits for single commit branches. Having said that, it _is_ a good idea to have the merge commit if there are multiple commits in the branch (even if it's a fast forward).
For a clean history, I suggest rebasing _before_ the merge and then merging it _with_ the merge commit. That way the history is linear with a merge commit showing clearly the commits that were part of a given feature branch.
Best,Ismael

On Fri, Dec 2, 2011 at 5:44 PM, Daniel Spiewak <djspiewak@gmail.com> wrote:

Linear history is nice, but rebasing public history is a big no-no in terms of Git best practices.

I was talking about private branches in my example.
Ismael

On Fri, Dec 2, 2011 at 5:46 PM, Daniel Spiewak <djspiewak@gmail.com> wrote:

I should clarify that in the previous narrative, I was using the "pull request is open" state to represent "commits have been pushed into a public repo".  Contributors will push commits into their own forks prior to opening a pull request.  It is that moment at which they can no longer rebase, since that is the moment at which they no longer hold private ownership of those commits (i.e. history rewriting would potentially disrupt others).

I don't agree with this. There is no rule that _any_ branch in _any_ public repository can't be rebased. They don't do that for the kernel either.
It doesn't make sense to say that one has to keep their work private just so that they have the flexibility to rebase. It's important to make it clear to users that those branches can be rebased.
Best,Ismael