Pull Requests and Re-writing History

[Joe Bowser <bo...@gmail.com>]

Hey

I saw the wiki was updated, and I'm not quite sure how I feel about this:
https://wiki.apache.org/cordova/ProcessingPullRequests

# REPO_NAME example: "js"
# PULL_REQUEST_NUMBER example: "44"
curl https://github.com/apache/cordova-REPO_NAME/pull/PULL_REQUEST_NUMBER.patch
| git am
git rebase origin/master -i

First, I'd add git am --signoff so that it adds your git e-mail to it.
 This just looks nice, but I'm not sure how I feel about doing the
commit squishing and pruning, since we want to have a nice audit trail
back to GitHub to see what we were doing.  I know it makes it harder
to do release notes, but the one thing that I get super picky about is
who wrote what code, and it matching up.

Am I just being all Apache about this, and we could be way more lax?
What are people's thoughts?

Joe

Re: Pull Requests and Re-writing History

[Braden Shepherdson <br...@chromium.org>]

That seems like a failing of those GUIs. When you git rebase -i from the
command line, it opens $EDITOR with the logs of all the commits that are
being squashed, but that's intended to give you context as you rewrite them
into a single, meaningful commit message that expresses the changes.

Braden


On Mon, Mar 3, 2014 at 2:37 PM, Shazron <sh...@gmail.com> wrote:

> Another thing that some Git GUIs do (like Tower) is when you squash, the
> log of commits you squash are appended to the commit message. Ugly, and not
> sure if its really useful for back-tracking purposes...
>
>
> On Fri, Feb 28, 2014 at 7:16 PM, Michal Mocny <mm...@chromium.org> wrote:
>
> > Ian: I thought the point was to not keep the original commits in tact
> 100%,
> > but rather to squish and clean them, in a way that keeps attribution.
> >
> > I think Joe's question (and perhaps yours), is: is that okay to do?  A
> > separate question is: how to use the tools to make sure this is obvious.
> >
> > -Michal
> >
> >
> > On Fri, Feb 28, 2014 at 9:52 PM, Ian Clelland <iclelland@chromium.org
> > >wrote:
> >
> > > If you're really concerned about keeping their commits intact, then you
> > can
> > > also do what I did with
> > > https://github.com/apache/cordova-plugin-file/pull/30 --
> > >
> > > I added her repo as a remote, and merged with --no-ff back into dev.
> That
> > > kept all of the original commits intact, any my name goes on the merge
> > > commit.
> > >
> > > (That may have also contributed to the pull request being marked as
> > > 'merged' automagically by GitHub)
> > >
> > >
> > >
> > > On Fri, Feb 28, 2014 at 8:56 PM, Andrew Grieve <ag...@chromium.org>
> > > wrote:
> > >
> > > > Without  --signoff, you already get set as the "committer", while the
> > > > author is maintained. You can verify this by running "coho last-week"
> > and
> > > > see that it separates commits you wrote vs commits that you did from
> > pull
> > > > requests. That said, adding "--signoff" couldn't hurt.
> > > >
> > > > Squishing & fixing up does maintain authorship, so I think that's
> > fine. I
> > > > also sometime clean up whitespace & tabs->spaces.
> > > >
> > > > It's definitely nice to squash & fix the commit messages not just for
> > > > release notes, but so you can figure out what the commit does from
> the
> > > "git
> > > > log", and so that they can be reverted easily.
> > > >
> > > > I've been doing this for several months now and haven't had anyone
> > > > complain, so I don't think those submitting the PRs care too much.
> > > >
> > > >
> > > >
> > > > On Fri, Feb 28, 2014 at 5:51 PM, Michal Mocny <mm...@chromium.org>
> > > wrote:
> > > >
> > > > > Does the squash keep original author info?  I know the hashes
> change
> > so
> > > > > they don't match up, but if we have the author and a reference to
> the
> > > PR
> > > > in
> > > > > the commit, I think thats fine for me.
> > > > >
> > > > > Alternative is to ask the contributor to do the squash, which we do
> > try
> > > > to
> > > > > do, but its usually the non-responsive contributors that submit the
> > > nasty
> > > > > PR in the first place (go figure).
> > > > >
> > > > > -Michal
> > > > >
> > > > >
> > > > > On Fri, Feb 28, 2014 at 5:14 PM, Joe Bowser <bo...@gmail.com>
> > wrote:
> > > > >
> > > > > > Hey
> > > > > >
> > > > > > I saw the wiki was updated, and I'm not quite sure how I feel
> about
> > > > this:
> > > > > > https://wiki.apache.org/cordova/ProcessingPullRequests
> > > > > >
> > > > > > # REPO_NAME example: "js"
> > > > > > # PULL_REQUEST_NUMBER example: "44"
> > > > > > curl
> > > > > >
> > > > >
> > > >
> > >
> >
> https://github.com/apache/cordova-REPO_NAME/pull/PULL_REQUEST_NUMBER.patch
> > > > > > | git am
> > > > > > git rebase origin/master -i
> > > > > >
> > > > > > First, I'd add git am --signoff so that it adds your git e-mail
> to
> > > it.
> > > > > >  This just looks nice, but I'm not sure how I feel about doing
> the
> > > > > > commit squishing and pruning, since we want to have a nice audit
> > > trail
> > > > > > back to GitHub to see what we were doing.  I know it makes it
> > harder
> > > > > > to do release notes, but the one thing that I get super picky
> about
> > > is
> > > > > > who wrote what code, and it matching up.
> > > > > >
> > > > > > Am I just being all Apache about this, and we could be way more
> > lax?
> > > > > > What are people's thoughts?
> > > > > >
> > > > > > Joe
> > > > > >
> > > > >
> > > >
> > >
> >
>

Re: Pull Requests and Re-writing History

[Shazron <sh...@gmail.com>]

Another thing that some Git GUIs do (like Tower) is when you squash, the
log of commits you squash are appended to the commit message. Ugly, and not
sure if its really useful for back-tracking purposes...


On Fri, Feb 28, 2014 at 7:16 PM, Michal Mocny <mm...@chromium.org> wrote:

> Ian: I thought the point was to not keep the original commits in tact 100%,
> but rather to squish and clean them, in a way that keeps attribution.
>
> I think Joe's question (and perhaps yours), is: is that okay to do?  A
> separate question is: how to use the tools to make sure this is obvious.
>
> -Michal
>
>
> On Fri, Feb 28, 2014 at 9:52 PM, Ian Clelland <iclelland@chromium.org
> >wrote:
>
> > If you're really concerned about keeping their commits intact, then you
> can
> > also do what I did with
> > https://github.com/apache/cordova-plugin-file/pull/30 --
> >
> > I added her repo as a remote, and merged with --no-ff back into dev. That
> > kept all of the original commits intact, any my name goes on the merge
> > commit.
> >
> > (That may have also contributed to the pull request being marked as
> > 'merged' automagically by GitHub)
> >
> >
> >
> > On Fri, Feb 28, 2014 at 8:56 PM, Andrew Grieve <ag...@chromium.org>
> > wrote:
> >
> > > Without  --signoff, you already get set as the "committer", while the
> > > author is maintained. You can verify this by running "coho last-week"
> and
> > > see that it separates commits you wrote vs commits that you did from
> pull
> > > requests. That said, adding "--signoff" couldn't hurt.
> > >
> > > Squishing & fixing up does maintain authorship, so I think that's
> fine. I
> > > also sometime clean up whitespace & tabs->spaces.
> > >
> > > It's definitely nice to squash & fix the commit messages not just for
> > > release notes, but so you can figure out what the commit does from the
> > "git
> > > log", and so that they can be reverted easily.
> > >
> > > I've been doing this for several months now and haven't had anyone
> > > complain, so I don't think those submitting the PRs care too much.
> > >
> > >
> > >
> > > On Fri, Feb 28, 2014 at 5:51 PM, Michal Mocny <mm...@chromium.org>
> > wrote:
> > >
> > > > Does the squash keep original author info?  I know the hashes change
> so
> > > > they don't match up, but if we have the author and a reference to the
> > PR
> > > in
> > > > the commit, I think thats fine for me.
> > > >
> > > > Alternative is to ask the contributor to do the squash, which we do
> try
> > > to
> > > > do, but its usually the non-responsive contributors that submit the
> > nasty
> > > > PR in the first place (go figure).
> > > >
> > > > -Michal
> > > >
> > > >
> > > > On Fri, Feb 28, 2014 at 5:14 PM, Joe Bowser <bo...@gmail.com>
> wrote:
> > > >
> > > > > Hey
> > > > >
> > > > > I saw the wiki was updated, and I'm not quite sure how I feel about
> > > this:
> > > > > https://wiki.apache.org/cordova/ProcessingPullRequests
> > > > >
> > > > > # REPO_NAME example: "js"
> > > > > # PULL_REQUEST_NUMBER example: "44"
> > > > > curl
> > > > >
> > > >
> > >
> >
> https://github.com/apache/cordova-REPO_NAME/pull/PULL_REQUEST_NUMBER.patch
> > > > > | git am
> > > > > git rebase origin/master -i
> > > > >
> > > > > First, I'd add git am --signoff so that it adds your git e-mail to
> > it.
> > > > >  This just looks nice, but I'm not sure how I feel about doing the
> > > > > commit squishing and pruning, since we want to have a nice audit
> > trail
> > > > > back to GitHub to see what we were doing.  I know it makes it
> harder
> > > > > to do release notes, but the one thing that I get super picky about
> > is
> > > > > who wrote what code, and it matching up.
> > > > >
> > > > > Am I just being all Apache about this, and we could be way more
> lax?
> > > > > What are people's thoughts?
> > > > >
> > > > > Joe
> > > > >
> > > >
> > >
> >
>

Re: Pull Requests and Re-writing History

[Michal Mocny <mm...@chromium.org>]

Ian: I thought the point was to not keep the original commits in tact 100%,
but rather to squish and clean them, in a way that keeps attribution.

I think Joe's question (and perhaps yours), is: is that okay to do?  A
separate question is: how to use the tools to make sure this is obvious.

-Michal


On Fri, Feb 28, 2014 at 9:52 PM, Ian Clelland <ic...@chromium.org>wrote:

> If you're really concerned about keeping their commits intact, then you can
> also do what I did with
> https://github.com/apache/cordova-plugin-file/pull/30 --
>
> I added her repo as a remote, and merged with --no-ff back into dev. That
> kept all of the original commits intact, any my name goes on the merge
> commit.
>
> (That may have also contributed to the pull request being marked as
> 'merged' automagically by GitHub)
>
>
>
> On Fri, Feb 28, 2014 at 8:56 PM, Andrew Grieve <ag...@chromium.org>
> wrote:
>
> > Without  --signoff, you already get set as the "committer", while the
> > author is maintained. You can verify this by running "coho last-week" and
> > see that it separates commits you wrote vs commits that you did from pull
> > requests. That said, adding "--signoff" couldn't hurt.
> >
> > Squishing & fixing up does maintain authorship, so I think that's fine. I
> > also sometime clean up whitespace & tabs->spaces.
> >
> > It's definitely nice to squash & fix the commit messages not just for
> > release notes, but so you can figure out what the commit does from the
> "git
> > log", and so that they can be reverted easily.
> >
> > I've been doing this for several months now and haven't had anyone
> > complain, so I don't think those submitting the PRs care too much.
> >
> >
> >
> > On Fri, Feb 28, 2014 at 5:51 PM, Michal Mocny <mm...@chromium.org>
> wrote:
> >
> > > Does the squash keep original author info?  I know the hashes change so
> > > they don't match up, but if we have the author and a reference to the
> PR
> > in
> > > the commit, I think thats fine for me.
> > >
> > > Alternative is to ask the contributor to do the squash, which we do try
> > to
> > > do, but its usually the non-responsive contributors that submit the
> nasty
> > > PR in the first place (go figure).
> > >
> > > -Michal
> > >
> > >
> > > On Fri, Feb 28, 2014 at 5:14 PM, Joe Bowser <bo...@gmail.com> wrote:
> > >
> > > > Hey
> > > >
> > > > I saw the wiki was updated, and I'm not quite sure how I feel about
> > this:
> > > > https://wiki.apache.org/cordova/ProcessingPullRequests
> > > >
> > > > # REPO_NAME example: "js"
> > > > # PULL_REQUEST_NUMBER example: "44"
> > > > curl
> > > >
> > >
> >
> https://github.com/apache/cordova-REPO_NAME/pull/PULL_REQUEST_NUMBER.patch
> > > > | git am
> > > > git rebase origin/master -i
> > > >
> > > > First, I'd add git am --signoff so that it adds your git e-mail to
> it.
> > > >  This just looks nice, but I'm not sure how I feel about doing the
> > > > commit squishing and pruning, since we want to have a nice audit
> trail
> > > > back to GitHub to see what we were doing.  I know it makes it harder
> > > > to do release notes, but the one thing that I get super picky about
> is
> > > > who wrote what code, and it matching up.
> > > >
> > > > Am I just being all Apache about this, and we could be way more lax?
> > > > What are people's thoughts?
> > > >
> > > > Joe
> > > >
> > >
> >
>

Re: Pull Requests and Re-writing History

[Ian Clelland <ic...@chromium.org>]

If you're really concerned about keeping their commits intact, then you can
also do what I did with
https://github.com/apache/cordova-plugin-file/pull/30 --

I added her repo as a remote, and merged with --no-ff back into dev. That
kept all of the original commits intact, any my name goes on the merge
commit.

(That may have also contributed to the pull request being marked as
'merged' automagically by GitHub)



On Fri, Feb 28, 2014 at 8:56 PM, Andrew Grieve <ag...@chromium.org> wrote:

> Without  --signoff, you already get set as the "committer", while the
> author is maintained. You can verify this by running "coho last-week" and
> see that it separates commits you wrote vs commits that you did from pull
> requests. That said, adding "--signoff" couldn't hurt.
>
> Squishing & fixing up does maintain authorship, so I think that's fine. I
> also sometime clean up whitespace & tabs->spaces.
>
> It's definitely nice to squash & fix the commit messages not just for
> release notes, but so you can figure out what the commit does from the "git
> log", and so that they can be reverted easily.
>
> I've been doing this for several months now and haven't had anyone
> complain, so I don't think those submitting the PRs care too much.
>
>
>
> On Fri, Feb 28, 2014 at 5:51 PM, Michal Mocny <mm...@chromium.org> wrote:
>
> > Does the squash keep original author info?  I know the hashes change so
> > they don't match up, but if we have the author and a reference to the PR
> in
> > the commit, I think thats fine for me.
> >
> > Alternative is to ask the contributor to do the squash, which we do try
> to
> > do, but its usually the non-responsive contributors that submit the nasty
> > PR in the first place (go figure).
> >
> > -Michal
> >
> >
> > On Fri, Feb 28, 2014 at 5:14 PM, Joe Bowser <bo...@gmail.com> wrote:
> >
> > > Hey
> > >
> > > I saw the wiki was updated, and I'm not quite sure how I feel about
> this:
> > > https://wiki.apache.org/cordova/ProcessingPullRequests
> > >
> > > # REPO_NAME example: "js"
> > > # PULL_REQUEST_NUMBER example: "44"
> > > curl
> > >
> >
> https://github.com/apache/cordova-REPO_NAME/pull/PULL_REQUEST_NUMBER.patch
> > > | git am
> > > git rebase origin/master -i
> > >
> > > First, I'd add git am --signoff so that it adds your git e-mail to it.
> > >  This just looks nice, but I'm not sure how I feel about doing the
> > > commit squishing and pruning, since we want to have a nice audit trail
> > > back to GitHub to see what we were doing.  I know it makes it harder
> > > to do release notes, but the one thing that I get super picky about is
> > > who wrote what code, and it matching up.
> > >
> > > Am I just being all Apache about this, and we could be way more lax?
> > > What are people's thoughts?
> > >
> > > Joe
> > >
> >
>

Re: Pull Requests and Re-writing History

[Andrew Grieve <ag...@chromium.org>]

Without  --signoff, you already get set as the "committer", while the
author is maintained. You can verify this by running "coho last-week" and
see that it separates commits you wrote vs commits that you did from pull
requests. That said, adding "--signoff" couldn't hurt.

Squishing & fixing up does maintain authorship, so I think that's fine. I
also sometime clean up whitespace & tabs->spaces.

It's definitely nice to squash & fix the commit messages not just for
release notes, but so you can figure out what the commit does from the "git
log", and so that they can be reverted easily.

I've been doing this for several months now and haven't had anyone
complain, so I don't think those submitting the PRs care too much.



On Fri, Feb 28, 2014 at 5:51 PM, Michal Mocny <mm...@chromium.org> wrote:

> Does the squash keep original author info?  I know the hashes change so
> they don't match up, but if we have the author and a reference to the PR in
> the commit, I think thats fine for me.
>
> Alternative is to ask the contributor to do the squash, which we do try to
> do, but its usually the non-responsive contributors that submit the nasty
> PR in the first place (go figure).
>
> -Michal
>
>
> On Fri, Feb 28, 2014 at 5:14 PM, Joe Bowser <bo...@gmail.com> wrote:
>
> > Hey
> >
> > I saw the wiki was updated, and I'm not quite sure how I feel about this:
> > https://wiki.apache.org/cordova/ProcessingPullRequests
> >
> > # REPO_NAME example: "js"
> > # PULL_REQUEST_NUMBER example: "44"
> > curl
> >
> https://github.com/apache/cordova-REPO_NAME/pull/PULL_REQUEST_NUMBER.patch
> > | git am
> > git rebase origin/master -i
> >
> > First, I'd add git am --signoff so that it adds your git e-mail to it.
> >  This just looks nice, but I'm not sure how I feel about doing the
> > commit squishing and pruning, since we want to have a nice audit trail
> > back to GitHub to see what we were doing.  I know it makes it harder
> > to do release notes, but the one thing that I get super picky about is
> > who wrote what code, and it matching up.
> >
> > Am I just being all Apache about this, and we could be way more lax?
> > What are people's thoughts?
> >
> > Joe
> >
>

Re: Pull Requests and Re-writing History

[Michal Mocny <mm...@chromium.org>]

Does the squash keep original author info?  I know the hashes change so
they don't match up, but if we have the author and a reference to the PR in
the commit, I think thats fine for me.

Alternative is to ask the contributor to do the squash, which we do try to
do, but its usually the non-responsive contributors that submit the nasty
PR in the first place (go figure).

-Michal


On Fri, Feb 28, 2014 at 5:14 PM, Joe Bowser <bo...@gmail.com> wrote:

> Hey
>
> I saw the wiki was updated, and I'm not quite sure how I feel about this:
> https://wiki.apache.org/cordova/ProcessingPullRequests
>
> # REPO_NAME example: "js"
> # PULL_REQUEST_NUMBER example: "44"
> curl
> https://github.com/apache/cordova-REPO_NAME/pull/PULL_REQUEST_NUMBER.patch
> | git am
> git rebase origin/master -i
>
> First, I'd add git am --signoff so that it adds your git e-mail to it.
>  This just looks nice, but I'm not sure how I feel about doing the
> commit squishing and pruning, since we want to have a nice audit trail
> back to GitHub to see what we were doing.  I know it makes it harder
> to do release notes, but the one thing that I get super picky about is
> who wrote what code, and it matching up.
>
> Am I just being all Apache about this, and we could be way more lax?
> What are people's thoughts?
>
> Joe
>