Re: [DISCUSS] Updating contribution guide for gitbox

[Ismaël Mejía]

+1 to Kenneth proposal, using reviewer and asignee, for the merge strategy
(a) +1 with the same arguments (preserving commits when they are
meaningful and isolated, ask committers to do extra squash if needed.

I don't really favor having one big commit per PR (in particular if
the change is big) because you lose information with this approach. We
should encourage contributors to do meaningful and isolated commits,
of course if this is not the case then we can go and squash them, but
this is something to review per case.

Regards,
Ismaël

On Wed, Nov 29, 2017 at 4:37 PM, Aljoscha Krettek  wrote:
> I think I agree with Kenn on the "merge question":
>  - There should be a merge commit because this records important information, 
> for example, I like having the option of figuring out what PR certain commits 
> came from
>  - Individual meaningful commits of a PR should be preserved, I think having 
> commits as small as possible is nice and the git history tells a story of 
> where the code came from
>  - fixup commits should be squashed
>
> The question of whether to keep or squash commits could also be solved by 
> enforcing 1 PR = 1 commit and making people open several PRs where they would 
> previously open one PR with several distinct and meaningful commits. This 
> might introduce quite some overhead, though.
>
> Best,
> Aljoscha
>
>> On 29. Nov 2017, at 09:40, Jean-Baptiste Onofré  wrote:
>>
>> Hi,
>>
>> I don't see why gitbox merge button change what we are doing.
>>
>> I agree with Kenn for 1 (reviewer field) & 2 (assignee field).
>>
>> IMHO, for 3, I think the reviewer should only use rebase & merge. The squash 
>> should be under the contributor scope. The reviewer can ask to squash some 
>> commits, but he should not do it himself (the contributor should update the 
>> PR with the squashes).
>>
>> My $0.01 ;)
>>
>> Regards
>> JB
>>
>> On 11/28/2017 06:45 PM, Kenneth Knowles wrote:
>>> Hi all,
>>> James brought up a great question in Slack, which was how should we use the 
>>> merge button, illustrated [1]
>>> I want to broaden the discussion to talk about all the new capabilities:
>>> 1. Whether & how to use the "reviewer" field
>>> 2. Whether & how to use the "assignee" field
>>> 3. Whether & how to use the merge button
>>> My preferences are:
>>> 1. Use the reviewer field instead of "R:" comments.
>>> 2. Use the assignee field to keep track of who the review is blocked on 
>>> (either the reviewer for more comments or the author for fixes)
>>> 3. Use merge commits, but editing the commit subject line
>>> To expand on part 3, GitHub's merge button has three options [1]. They are 
>>> not described accurately in the UI, as they all say "merge" when only one 
>>> of them performs a merge. They do the following:
>>> (a) Merge the branch with a merge commit
>>> (b) Squash all the commits, rebase and push
>>> (c) Rebase and push without squash
>>> Unlike our current guide, all of these result in a "merged" status for the 
>>> PR, so we can correctly distinguish those PRs that were actually merged.
>>> My votes on these options are:
>>> (a) +1 this preserves the most information
>>> (b) -1 this erases the most information
>>> (c) -0 this is just sort of a middle ground; it breaks commit hashes, does 
>>> not have a clear merge commit, but preserves other info
>>> Kenn
>>> [1] https://apachebeam.slack.com/messages/C1AAFJYMP/
>>> Kenn
>>
>> --
>> Jean-Baptiste Onofré
>> jbono...@apache.org
>> http://blog.nanthrax.net
>> Talend - http://www.talend.com
>

Re: [DISCUSS] Updating contribution guide for gitbox

[Aljoscha Krettek]

I think I agree with Kenn on the "merge question":
 - There should be a merge commit because this records important information, 
for example, I like having the option of figuring out what PR certain commits 
came from
 - Individual meaningful commits of a PR should be preserved, I think having 
commits as small as possible is nice and the git history tells a story of where 
the code came from
 - fixup commits should be squashed

The question of whether to keep or squash commits could also be solved by 
enforcing 1 PR = 1 commit and making people open several PRs where they would 
previously open one PR with several distinct and meaningful commits. This might 
introduce quite some overhead, though.

Best,
Aljoscha

> On 29. Nov 2017, at 09:40, Jean-Baptiste Onofré  wrote:
> 
> Hi,
> 
> I don't see why gitbox merge button change what we are doing.
> 
> I agree with Kenn for 1 (reviewer field) & 2 (assignee field).
> 
> IMHO, for 3, I think the reviewer should only use rebase & merge. The squash 
> should be under the contributor scope. The reviewer can ask to squash some 
> commits, but he should not do it himself (the contributor should update the 
> PR with the squashes).
> 
> My $0.01 ;)
> 
> Regards
> JB
> 
> On 11/28/2017 06:45 PM, Kenneth Knowles wrote:
>> Hi all,
>> James brought up a great question in Slack, which was how should we use the 
>> merge button, illustrated [1]
>> I want to broaden the discussion to talk about all the new capabilities:
>> 1. Whether & how to use the "reviewer" field
>> 2. Whether & how to use the "assignee" field
>> 3. Whether & how to use the merge button
>> My preferences are:
>> 1. Use the reviewer field instead of "R:" comments.
>> 2. Use the assignee field to keep track of who the review is blocked on 
>> (either the reviewer for more comments or the author for fixes)
>> 3. Use merge commits, but editing the commit subject line
>> To expand on part 3, GitHub's merge button has three options [1]. They are 
>> not described accurately in the UI, as they all say "merge" when only one of 
>> them performs a merge. They do the following:
>> (a) Merge the branch with a merge commit
>> (b) Squash all the commits, rebase and push
>> (c) Rebase and push without squash
>> Unlike our current guide, all of these result in a "merged" status for the 
>> PR, so we can correctly distinguish those PRs that were actually merged.
>> My votes on these options are:
>> (a) +1 this preserves the most information
>> (b) -1 this erases the most information
>> (c) -0 this is just sort of a middle ground; it breaks commit hashes, does 
>> not have a clear merge commit, but preserves other info
>> Kenn
>> [1] https://apachebeam.slack.com/messages/C1AAFJYMP/
>> Kenn
> 
> -- 
> Jean-Baptiste Onofré
> jbono...@apache.org
> http://blog.nanthrax.net
> Talend - http://www.talend.com

Re: [DISCUSS] Updating contribution guide for gitbox

[Jean-Baptiste Onofré]

Hi,

I don't see why gitbox merge button change what we are doing.

I agree with Kenn for 1 (reviewer field) & 2 (assignee field).

IMHO, for 3, I think the reviewer should only use rebase & merge. The squash 
should be under the contributor scope. The reviewer can ask to squash some 
commits, but he should not do it himself (the contributor should update the PR 
with the squashes).


My $0.01 ;)

Regards
JB

On 11/28/2017 06:45 PM, Kenneth Knowles wrote:

Hi all,

James brought up a great question in Slack, which was how should we use the 
merge button, illustrated [1]


I want to broaden the discussion to talk about all the new capabilities:

1. Whether & how to use the "reviewer" field
2. Whether & how to use the "assignee" field
3. Whether & how to use the merge button

My preferences are:

1. Use the reviewer field instead of "R:" comments.
2. Use the assignee field to keep track of who the review is blocked on (either 
the reviewer for more comments or the author for fixes)

3. Use merge commits, but editing the commit subject line

To expand on part 3, GitHub's merge button has three options [1]. They are not 
described accurately in the UI, as they all say "merge" when only one of them 
performs a merge. They do the following:


(a) Merge the branch with a merge commit
(b) Squash all the commits, rebase and push
(c) Rebase and push without squash

Unlike our current guide, all of these result in a "merged" status for the PR, 
so we can correctly distinguish those PRs that were actually merged.


My votes on these options are:

(a) +1 this preserves the most information
(b) -1 this erases the most information
(c) -0 this is just sort of a middle ground; it breaks commit hashes, does not 
have a clear merge commit, but preserves other info


Kenn

[1] https://apachebeam.slack.com/messages/C1AAFJYMP/





Kenn


--
Jean-Baptiste Onofré
jbono...@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com

Re: [DISCUSS] Updating contribution guide for gitbox

[Kenneth Knowles]

Let's assume that when I say (a) the author has arranged commits to be
meaningful. That's what I meant to say in each of my descriptions of the
option. If they are noise, it doesn't apply.

On Tue, Nov 28, 2017 at 8:04 PM, James  wrote:

> Thanks Kenn for bring up this expanded discussion, my vote is:
>
> (a) -1 this preserves noise log like 'fix review comments'
> (b) +0 this keeps the commit log clean, but without a rebase
> (c) -1 similar to option a), it preserves noise log like 'fix review
> comments'
>
> My ideal option is the current manual merge process: `rebase + squash`,
> maybe we should consider introducing mergebot?
>
>
> On Wed, Nov 29, 2017 at 4:01 AM Raghu Angadi  wrote:
>
>> On Tue, Nov 28, 2017 at 11:47 AM, Thomas Weise  wrote:
>>
>>>
>>> (a) -0 due to extra noise in the commit log
>>>
>>
>>
>>> (b) -1 (as standard/default) this should be done by contributor as there
>>> may be few situation where individual commits should be preserved
>>>
>>
>> It is better to preserve the commit history of the PR at least in the
>> committer branch (and PR).
>> In addition having to force push squashed commit to remote git branch
>> each time is quite painful. If we squash at all, final merge into master
>> seems like the best place.
>>
>>
>>> (c) +1 the rebase will also record the committer (which would be merge
>>> commit author otherwise)
>>>
>>> In general the process should result in "merged" status for a merged PR
>>> as opposed to "closed" as seen often currently.
>>>
>>> Thanks,
>>> Thomas
>>>
>>>
>>>
>>> On Tue, Nov 28, 2017 at 11:23 AM, Kenneth Knowles 
>>> wrote:
>>>
 On Tue, Nov 28, 2017 at 11:16 AM, Raghu Angadi 
 wrote:

> -1 for (a): no need to see all the private branch commits from
> contributor. It often makes me more conscious of local commits.
>

 I want to note that on my PRs these are not private commits. Each one
 is a meaningful isolated change that can be rolled back and is useful to
 keep separate when looking at `git blame` or the history of a file. I would
 encourage every contributor to also do this. A PR is the unit of code
 review, but the unit of meaningful change to a repository is often much
 smaller.

 Kenn


> +1 for (b): with committer replacing the squashed commit messages with
> '[BEAM-jira or PRID]: Brief cut-n-paste (or longer if it contributor
> provided one)'.
> -1 for (c): This is quite painful for contributors to work with if
> there has been merge conflict with master. Especially for larger PRs with
> multiple updates.
>
> On Tue, Nov 28, 2017 at 10:24 AM, Lukasz Cwik 
> wrote:
>
>> Is it possible for mergebot to auto squash any fixup! and perform the
>> merge commit as described in (a), if so then I would vote for mergebot.
>>
>> Without mergebot, I vote:
>> (a) 0 I like squashing fixup!
>> (b) -1
>> (c) +1 Most of our PRs are for focused singular changes which is why
>> I would rather squash everything over not squashing anything
>>
>>
>>
>> On Tue, Nov 28, 2017 at 9:57 AM, Kenneth Knowles 
>> wrote:
>>
>>> On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers 
>>> wrote:
>>>
 One risk to "squash and merge" is that it may lead to commits that
 don't have clean descriptions -- for instance, commits like "Fixing 
 review
 comments" will show up. If we use (a) these would also show up as 
 separate
 commits. It seems like there are two cases of multiple commits in a PR:

 1. Multiple commits in a PR that have semantic meaning (eg., a PR
 performed N steps, split across N commits). In this case, keeping the
 descriptions and performing either a merge (if the commits are 
 separately
 valid) or squash (if we want the commits to become a single commit in
 master) probably makes sense.

>>>
>>> Keep 'em
>>>
>>>
 2. Multiple commits in a PR that just reflect the review history.
 In this case, we should probably ask the PR author to explicitly rebase
 their PR to have semantically meaningful commits prior to merging. 
 (Eg., do
 a rebase -i).

>>>
>>> Ask the author to squash 'em.
>>>
>>> Kenn
>>>
>>>

 On Tue, Nov 28, 2017 at 9:46 AM Kenneth Knowles 
 wrote:

> Hi all,
>
> James brought up a great question in Slack, which was how should
> we use the merge button, illustrated [1]
>
> I want to broaden the discussion to talk about all the new
> capabilities:
>
> 1. Whether & how to use the "reviewer" field
> 2. Whether & how 

Re: [DISCUSS] Updating contribution guide for gitbox

[James]

Thanks Kenn for bring up this expanded discussion, my vote is:

(a) -1 this preserves noise log like 'fix review comments'
(b) +0 this keeps the commit log clean, but without a rebase
(c) -1 similar to option a), it preserves noise log like 'fix review
comments'

My ideal option is the current manual merge process: `rebase + squash`,
maybe we should consider introducing mergebot?


On Wed, Nov 29, 2017 at 4:01 AM Raghu Angadi  wrote:

> On Tue, Nov 28, 2017 at 11:47 AM, Thomas Weise  wrote:
>
>>
>> (a) -0 due to extra noise in the commit log
>>
>
>
>> (b) -1 (as standard/default) this should be done by contributor as there
>> may be few situation where individual commits should be preserved
>>
>
> It is better to preserve the commit history of the PR at least in the
> committer branch (and PR).
> In addition having to force push squashed commit to remote git branch each
> time is quite painful. If we squash at all, final merge into master seems
> like the best place.
>
>
>> (c) +1 the rebase will also record the committer (which would be merge
>> commit author otherwise)
>>
>> In general the process should result in "merged" status for a merged PR
>> as opposed to "closed" as seen often currently.
>>
>> Thanks,
>> Thomas
>>
>>
>>
>> On Tue, Nov 28, 2017 at 11:23 AM, Kenneth Knowles  wrote:
>>
>>> On Tue, Nov 28, 2017 at 11:16 AM, Raghu Angadi 
>>> wrote:
>>>
 -1 for (a): no need to see all the private branch commits from
 contributor. It often makes me more conscious of local commits.

>>>
>>> I want to note that on my PRs these are not private commits. Each one is
>>> a meaningful isolated change that can be rolled back and is useful to keep
>>> separate when looking at `git blame` or the history of a file. I would
>>> encourage every contributor to also do this. A PR is the unit of code
>>> review, but the unit of meaningful change to a repository is often much
>>> smaller.
>>>
>>> Kenn
>>>
>>>
 +1 for (b): with committer replacing the squashed commit messages with
 '[BEAM-jira or PRID]: Brief cut-n-paste (or longer if it contributor
 provided one)'.
 -1 for (c): This is quite painful for contributors to work with if
 there has been merge conflict with master. Especially for larger PRs with
 multiple updates.

 On Tue, Nov 28, 2017 at 10:24 AM, Lukasz Cwik  wrote:

> Is it possible for mergebot to auto squash any fixup! and perform the
> merge commit as described in (a), if so then I would vote for mergebot.
>
> Without mergebot, I vote:
> (a) 0 I like squashing fixup!
> (b) -1
> (c) +1 Most of our PRs are for focused singular changes which is why I
> would rather squash everything over not squashing anything
>
>
>
> On Tue, Nov 28, 2017 at 9:57 AM, Kenneth Knowles 
> wrote:
>
>> On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers 
>> wrote:
>>
>>> One risk to "squash and merge" is that it may lead to commits that
>>> don't have clean descriptions -- for instance, commits like "Fixing 
>>> review
>>> comments" will show up. If we use (a) these would also show up as 
>>> separate
>>> commits. It seems like there are two cases of multiple commits in a PR:
>>>
>>> 1. Multiple commits in a PR that have semantic meaning (eg., a PR
>>> performed N steps, split across N commits). In this case, keeping the
>>> descriptions and performing either a merge (if the commits are 
>>> separately
>>> valid) or squash (if we want the commits to become a single commit in
>>> master) probably makes sense.
>>>
>>
>> Keep 'em
>>
>>
>>> 2. Multiple commits in a PR that just reflect the review history. In
>>> this case, we should probably ask the PR author to explicitly rebase 
>>> their
>>> PR to have semantically meaningful commits prior to merging. (Eg., do a
>>> rebase -i).
>>>
>>
>> Ask the author to squash 'em.
>>
>> Kenn
>>
>>
>>>
>>> On Tue, Nov 28, 2017 at 9:46 AM Kenneth Knowles 
>>> wrote:
>>>
 Hi all,

 James brought up a great question in Slack, which was how should we
 use the merge button, illustrated [1]

 I want to broaden the discussion to talk about all the new
 capabilities:

 1. Whether & how to use the "reviewer" field
 2. Whether & how to use the "assignee" field
 3. Whether & how to use the merge button

 My preferences are:

 1. Use the reviewer field instead of "R:" comments.
 2. Use the assignee field to keep track of who the review is
 blocked on (either the reviewer for more comments or the author for 
 fixes)
 3. Use merge commits, but editing the commit subject 

Re: [DISCUSS] Updating contribution guide for gitbox

[Kenneth Knowles]

On Tue, Nov 28, 2017 at 11:16 AM, Raghu Angadi  wrote:

> -1 for (a): no need to see all the private branch commits from
> contributor. It often makes me more conscious of local commits.
>

I want to note that on my PRs these are not private commits. Each one is a
meaningful isolated change that can be rolled back and is useful to keep
separate when looking at `git blame` or the history of a file. I would
encourage every contributor to also do this. A PR is the unit of code
review, but the unit of meaningful change to a repository is often much
smaller.

Kenn


> +1 for (b): with committer replacing the squashed commit messages with
> '[BEAM-jira or PRID]: Brief cut-n-paste (or longer if it contributor
> provided one)'.
> -1 for (c): This is quite painful for contributors to work with if there
> has been merge conflict with master. Especially for larger PRs with
> multiple updates.
>
> On Tue, Nov 28, 2017 at 10:24 AM, Lukasz Cwik  wrote:
>
>> Is it possible for mergebot to auto squash any fixup! and perform the
>> merge commit as described in (a), if so then I would vote for mergebot.
>>
>> Without mergebot, I vote:
>> (a) 0 I like squashing fixup!
>> (b) -1
>> (c) +1 Most of our PRs are for focused singular changes which is why I
>> would rather squash everything over not squashing anything
>>
>>
>>
>> On Tue, Nov 28, 2017 at 9:57 AM, Kenneth Knowles  wrote:
>>
>>> On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers 
>>> wrote:
>>>
 One risk to "squash and merge" is that it may lead to commits that
 don't have clean descriptions -- for instance, commits like "Fixing review
 comments" will show up. If we use (a) these would also show up as separate
 commits. It seems like there are two cases of multiple commits in a PR:

 1. Multiple commits in a PR that have semantic meaning (eg., a PR
 performed N steps, split across N commits). In this case, keeping the
 descriptions and performing either a merge (if the commits are separately
 valid) or squash (if we want the commits to become a single commit in
 master) probably makes sense.

>>>
>>> Keep 'em
>>>
>>>
 2. Multiple commits in a PR that just reflect the review history. In
 this case, we should probably ask the PR author to explicitly rebase their
 PR to have semantically meaningful commits prior to merging. (Eg., do a
 rebase -i).

>>>
>>> Ask the author to squash 'em.
>>>
>>> Kenn
>>>
>>>

 On Tue, Nov 28, 2017 at 9:46 AM Kenneth Knowles  wrote:

> Hi all,
>
> James brought up a great question in Slack, which was how should we
> use the merge button, illustrated [1]
>
> I want to broaden the discussion to talk about all the new
> capabilities:
>
> 1. Whether & how to use the "reviewer" field
> 2. Whether & how to use the "assignee" field
> 3. Whether & how to use the merge button
>
> My preferences are:
>
> 1. Use the reviewer field instead of "R:" comments.
> 2. Use the assignee field to keep track of who the review is blocked
> on (either the reviewer for more comments or the author for fixes)
> 3. Use merge commits, but editing the commit subject line
>
> To expand on part 3, GitHub's merge button has three options [1]. They
> are not described accurately in the UI, as they all say "merge" when only
> one of them performs a merge. They do the following:
>
> (a) Merge the branch with a merge commit
> (b) Squash all the commits, rebase and push
> (c) Rebase and push without squash
>
> Unlike our current guide, all of these result in a "merged" status for
> the PR, so we can correctly distinguish those PRs that were actually 
> merged.
>
> My votes on these options are:
>
> (a) +1 this preserves the most information
> (b) -1 this erases the most information
> (c) -0 this is just sort of a middle ground; it breaks commit hashes,
> does not have a clear merge commit, but preserves other info
>
> Kenn
>
> [1] https://apachebeam.slack.com/messages/C1AAFJYMP/
>
>
>
>
>
> Kenn
>

>>>
>>
>

Re: [DISCUSS] Updating contribution guide for gitbox

[Raghu Angadi]

-1 for (a): no need to see all the private branch commits from contributor.
It often makes me more conscious of local commits.
+1 for (b): with committer replacing the squashed commit messages with
'[BEAM-jira or PRID]: Brief cut-n-paste (or longer if it contributor
provided one)'.
-1 for (c): This is quite painful for contributors to work with if there
has been merge conflict with master. Especially for larger PRs with
multiple updates.

On Tue, Nov 28, 2017 at 10:24 AM, Lukasz Cwik  wrote:

> Is it possible for mergebot to auto squash any fixup! and perform the
> merge commit as described in (a), if so then I would vote for mergebot.
>
> Without mergebot, I vote:
> (a) 0 I like squashing fixup!
> (b) -1
> (c) +1 Most of our PRs are for focused singular changes which is why I
> would rather squash everything over not squashing anything
>
>
>
> On Tue, Nov 28, 2017 at 9:57 AM, Kenneth Knowles  wrote:
>
>> On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers 
>> wrote:
>>
>>> One risk to "squash and merge" is that it may lead to commits that don't
>>> have clean descriptions -- for instance, commits like "Fixing review
>>> comments" will show up. If we use (a) these would also show up as separate
>>> commits. It seems like there are two cases of multiple commits in a PR:
>>>
>>> 1. Multiple commits in a PR that have semantic meaning (eg., a PR
>>> performed N steps, split across N commits). In this case, keeping the
>>> descriptions and performing either a merge (if the commits are separately
>>> valid) or squash (if we want the commits to become a single commit in
>>> master) probably makes sense.
>>>
>>
>> Keep 'em
>>
>>
>>> 2. Multiple commits in a PR that just reflect the review history. In
>>> this case, we should probably ask the PR author to explicitly rebase their
>>> PR to have semantically meaningful commits prior to merging. (Eg., do a
>>> rebase -i).
>>>
>>
>> Ask the author to squash 'em.
>>
>> Kenn
>>
>>
>>>
>>> On Tue, Nov 28, 2017 at 9:46 AM Kenneth Knowles  wrote:
>>>
 Hi all,

 James brought up a great question in Slack, which was how should we use
 the merge button, illustrated [1]

 I want to broaden the discussion to talk about all the new capabilities:

 1. Whether & how to use the "reviewer" field
 2. Whether & how to use the "assignee" field
 3. Whether & how to use the merge button

 My preferences are:

 1. Use the reviewer field instead of "R:" comments.
 2. Use the assignee field to keep track of who the review is blocked on
 (either the reviewer for more comments or the author for fixes)
 3. Use merge commits, but editing the commit subject line

 To expand on part 3, GitHub's merge button has three options [1]. They
 are not described accurately in the UI, as they all say "merge" when only
 one of them performs a merge. They do the following:

 (a) Merge the branch with a merge commit
 (b) Squash all the commits, rebase and push
 (c) Rebase and push without squash

 Unlike our current guide, all of these result in a "merged" status for
 the PR, so we can correctly distinguish those PRs that were actually 
 merged.

 My votes on these options are:

 (a) +1 this preserves the most information
 (b) -1 this erases the most information
 (c) -0 this is just sort of a middle ground; it breaks commit hashes,
 does not have a clear merge commit, but preserves other info

 Kenn

 [1] https://apachebeam.slack.com/messages/C1AAFJYMP/





 Kenn

>>>
>>
>

Re: [DISCUSS] Updating contribution guide for gitbox

[Lukasz Cwik]

Is it possible for mergebot to auto squash any fixup! and perform the merge
commit as described in (a), if so then I would vote for mergebot.

Without mergebot, I vote:
(a) 0 I like squashing fixup!
(b) -1
(c) +1 Most of our PRs are for focused singular changes which is why I
would rather squash everything over not squashing anything



On Tue, Nov 28, 2017 at 9:57 AM, Kenneth Knowles  wrote:

> On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers 
> wrote:
>
>> One risk to "squash and merge" is that it may lead to commits that don't
>> have clean descriptions -- for instance, commits like "Fixing review
>> comments" will show up. If we use (a) these would also show up as separate
>> commits. It seems like there are two cases of multiple commits in a PR:
>>
>> 1. Multiple commits in a PR that have semantic meaning (eg., a PR
>> performed N steps, split across N commits). In this case, keeping the
>> descriptions and performing either a merge (if the commits are separately
>> valid) or squash (if we want the commits to become a single commit in
>> master) probably makes sense.
>>
>
> Keep 'em
>
>
>> 2. Multiple commits in a PR that just reflect the review history. In this
>> case, we should probably ask the PR author to explicitly rebase their PR to
>> have semantically meaningful commits prior to merging. (Eg., do a rebase
>> -i).
>>
>
> Ask the author to squash 'em.
>
> Kenn
>
>
>>
>> On Tue, Nov 28, 2017 at 9:46 AM Kenneth Knowles  wrote:
>>
>>> Hi all,
>>>
>>> James brought up a great question in Slack, which was how should we use
>>> the merge button, illustrated [1]
>>>
>>> I want to broaden the discussion to talk about all the new capabilities:
>>>
>>> 1. Whether & how to use the "reviewer" field
>>> 2. Whether & how to use the "assignee" field
>>> 3. Whether & how to use the merge button
>>>
>>> My preferences are:
>>>
>>> 1. Use the reviewer field instead of "R:" comments.
>>> 2. Use the assignee field to keep track of who the review is blocked on
>>> (either the reviewer for more comments or the author for fixes)
>>> 3. Use merge commits, but editing the commit subject line
>>>
>>> To expand on part 3, GitHub's merge button has three options [1]. They
>>> are not described accurately in the UI, as they all say "merge" when only
>>> one of them performs a merge. They do the following:
>>>
>>> (a) Merge the branch with a merge commit
>>> (b) Squash all the commits, rebase and push
>>> (c) Rebase and push without squash
>>>
>>> Unlike our current guide, all of these result in a "merged" status for
>>> the PR, so we can correctly distinguish those PRs that were actually merged.
>>>
>>> My votes on these options are:
>>>
>>> (a) +1 this preserves the most information
>>> (b) -1 this erases the most information
>>> (c) -0 this is just sort of a middle ground; it breaks commit hashes,
>>> does not have a clear merge commit, but preserves other info
>>>
>>> Kenn
>>>
>>> [1] https://apachebeam.slack.com/messages/C1AAFJYMP/
>>>
>>>
>>>
>>>
>>>
>>> Kenn
>>>
>>
>

Re: [DISCUSS] Updating contribution guide for gitbox

[Kenneth Knowles]

On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers  wrote:

> One risk to "squash and merge" is that it may lead to commits that don't
> have clean descriptions -- for instance, commits like "Fixing review
> comments" will show up. If we use (a) these would also show up as separate
> commits. It seems like there are two cases of multiple commits in a PR:
>
> 1. Multiple commits in a PR that have semantic meaning (eg., a PR
> performed N steps, split across N commits). In this case, keeping the
> descriptions and performing either a merge (if the commits are separately
> valid) or squash (if we want the commits to become a single commit in
> master) probably makes sense.
>

Keep 'em


> 2. Multiple commits in a PR that just reflect the review history. In this
> case, we should probably ask the PR author to explicitly rebase their PR to
> have semantically meaningful commits prior to merging. (Eg., do a rebase
> -i).
>

Ask the author to squash 'em.

Kenn


>
> On Tue, Nov 28, 2017 at 9:46 AM Kenneth Knowles  wrote:
>
>> Hi all,
>>
>> James brought up a great question in Slack, which was how should we use
>> the merge button, illustrated [1]
>>
>> I want to broaden the discussion to talk about all the new capabilities:
>>
>> 1. Whether & how to use the "reviewer" field
>> 2. Whether & how to use the "assignee" field
>> 3. Whether & how to use the merge button
>>
>> My preferences are:
>>
>> 1. Use the reviewer field instead of "R:" comments.
>> 2. Use the assignee field to keep track of who the review is blocked on
>> (either the reviewer for more comments or the author for fixes)
>> 3. Use merge commits, but editing the commit subject line
>>
>> To expand on part 3, GitHub's merge button has three options [1]. They
>> are not described accurately in the UI, as they all say "merge" when only
>> one of them performs a merge. They do the following:
>>
>> (a) Merge the branch with a merge commit
>> (b) Squash all the commits, rebase and push
>> (c) Rebase and push without squash
>>
>> Unlike our current guide, all of these result in a "merged" status for
>> the PR, so we can correctly distinguish those PRs that were actually merged.
>>
>> My votes on these options are:
>>
>> (a) +1 this preserves the most information
>> (b) -1 this erases the most information
>> (c) -0 this is just sort of a middle ground; it breaks commit hashes,
>> does not have a clear merge commit, but preserves other info
>>
>> Kenn
>>
>> [1] https://apachebeam.slack.com/messages/C1AAFJYMP/
>>
>>
>>
>>
>>
>> Kenn
>>
>

Re: [DISCUSS] Updating contribution guide for gitbox

[Thomas Groh]

I am strongly in favor of (1); I have no strong feelings about (2); I agree
on (3), but generically am not hugely concerned, so long as back-references
to the original PR are maintained, which is where most of the context
lives. It is nice to have the change broken up into as many individually
useful parts as possible, so I wouldn't really choose (b) or (c).

Of note, (1) will not be possible if you would like another contributor to
review and they have not set up their gitbox account. Notably this is
always going to be the case for contributors who are not committers - we
should maintain use of the "R: @reviewer" comments in those cases.

On Tue, Nov 28, 2017 at 9:45 AM, Kenneth Knowles  wrote:

> Hi all,
>
> James brought up a great question in Slack, which was how should we use
> the merge button, illustrated [1]
>
> I want to broaden the discussion to talk about all the new capabilities:
>
> 1. Whether & how to use the "reviewer" field
> 2. Whether & how to use the "assignee" field
> 3. Whether & how to use the merge button
>
> My preferences are:
>
> 1. Use the reviewer field instead of "R:" comments.
> 2. Use the assignee field to keep track of who the review is blocked on
> (either the reviewer for more comments or the author for fixes)
> 3. Use merge commits, but editing the commit subject line
>
> To expand on part 3, GitHub's merge button has three options [1]. They are
> not described accurately in the UI, as they all say "merge" when only one
> of them performs a merge. They do the following:
>
> (a) Merge the branch with a merge commit
> (b) Squash all the commits, rebase and push
> (c) Rebase and push without squash
>
> Unlike our current guide, all of these result in a "merged" status for the
> PR, so we can correctly distinguish those PRs that were actually merged.
>
> My votes on these options are:
>
> (a) +1 this preserves the most information
> (b) -1 this erases the most information
> (c) -0 this is just sort of a middle ground; it breaks commit hashes, does
> not have a clear merge commit, but preserves other info
>
> Kenn
>
> [1] https://apachebeam.slack.com/messages/C1AAFJYMP/
>
>
>
>
>
> Kenn
>

Re: [DISCUSS] Updating contribution guide for gitbox

[Ben Chambers]

One risk to "squash and merge" is that it may lead to commits that don't
have clean descriptions -- for instance, commits like "Fixing review
comments" will show up. If we use (a) these would also show up as separate
commits. It seems like there are two cases of multiple commits in a PR:

1. Multiple commits in a PR that have semantic meaning (eg., a PR performed
N steps, split across N commits). In this case, keeping the descriptions
and performing either a merge (if the commits are separately valid) or
squash (if we want the commits to become a single commit in master)
probably makes sense.
2. Multiple commits in a PR that just reflect the review history. In this
case, we should probably ask the PR author to explicitly rebase their PR to
have semantically meaningful commits prior to merging. (Eg., do a rebase
-i).

On Tue, Nov 28, 2017 at 9:46 AM Kenneth Knowles  wrote:

> Hi all,
>
> James brought up a great question in Slack, which was how should we use
> the merge button, illustrated [1]
>
> I want to broaden the discussion to talk about all the new capabilities:
>
> 1. Whether & how to use the "reviewer" field
> 2. Whether & how to use the "assignee" field
> 3. Whether & how to use the merge button
>
> My preferences are:
>
> 1. Use the reviewer field instead of "R:" comments.
> 2. Use the assignee field to keep track of who the review is blocked on
> (either the reviewer for more comments or the author for fixes)
> 3. Use merge commits, but editing the commit subject line
>
> To expand on part 3, GitHub's merge button has three options [1]. They are
> not described accurately in the UI, as they all say "merge" when only one
> of them performs a merge. They do the following:
>
> (a) Merge the branch with a merge commit
> (b) Squash all the commits, rebase and push
> (c) Rebase and push without squash
>
> Unlike our current guide, all of these result in a "merged" status for the
> PR, so we can correctly distinguish those PRs that were actually merged.
>
> My votes on these options are:
>
> (a) +1 this preserves the most information
> (b) -1 this erases the most information
> (c) -0 this is just sort of a middle ground; it breaks commit hashes, does
> not have a clear merge commit, but preserves other info
>
> Kenn
>
> [1] https://apachebeam.slack.com/messages/C1AAFJYMP/
>
>
>
>
>
> Kenn
>

Re: [DISCUSS] Updating contribution guide for gitbox

[Jean-Baptiste Onofré]

Hi,

In other Apache projects using gitbox, I experiment, the following cinematic:

1. use the review button to assign someone
2. once changes approved, I use the merge button (supporting squash and merge)

It's very convenient and works fine.

So, +1 to (b)

Regards
JB

On 11/28/2017 06:45 PM, Kenneth Knowles wrote:

Hi all,

James brought up a great question in Slack, which was how should we use the 
merge button, illustrated [1]


I want to broaden the discussion to talk about all the new capabilities:

1. Whether & how to use the "reviewer" field
2. Whether & how to use the "assignee" field
3. Whether & how to use the merge button

My preferences are:

1. Use the reviewer field instead of "R:" comments.
2. Use the assignee field to keep track of who the review is blocked on (either 
the reviewer for more comments or the author for fixes)

3. Use merge commits, but editing the commit subject line

To expand on part 3, GitHub's merge button has three options [1]. They are not 
described accurately in the UI, as they all say "merge" when only one of them 
performs a merge. They do the following:


(a) Merge the branch with a merge commit
(b) Squash all the commits, rebase and push
(c) Rebase and push without squash

Unlike our current guide, all of these result in a "merged" status for the PR, 
so we can correctly distinguish those PRs that were actually merged.


My votes on these options are:

(a) +1 this preserves the most information
(b) -1 this erases the most information
(c) -0 this is just sort of a middle ground; it breaks commit hashes, does not 
have a clear merge commit, but preserves other info


Kenn

[1] https://apachebeam.slack.com/messages/C1AAFJYMP/





Kenn


--
Jean-Baptiste Onofré
jbono...@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com