Re: [DISCUSS] Updating contribution guide for gitbox

[James <xu...@gmail.com>]

SGTM then, my new vote is +1 to (a) as it preserves the most information,
just the reviewer need to keep in mind to tell the author to squash the
noise commit before merge.

On Wed, Nov 29, 2017 at 12:09 PM Kenneth Knowles <kl...@google.com> wrote:

> 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 <xu...@gmail.com> 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 <ra...@google.com> wrote:
>>
>>> On Tue, Nov 28, 2017 at 11:47 AM, Thomas Weise <th...@apache.org> 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 <kl...@google.com>
>>>> wrote:
>>>>
>>>>> On Tue, Nov 28, 2017 at 11:16 AM, Raghu Angadi <ra...@google.com>
>>>>> 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 <lc...@google.com>
>>>>>> 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 <kl...@google.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> On Tue, Nov 28, 2017 at 9:51 AM, Ben Chambers <bchambers@google.com
>>>>>>>> > 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 <kl...@google.com>
>>>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>