Etiquettes while merging PR from contributors

[Chetan Mehrotra <ch...@gmail.com>]

Hi Team,

Given the switch to git I am now bit in dilemma on how to handle the
PR merge. With svn it was always done by applying the patch and
committing it. Keeps history fine. With Git there is choice to 'merge'
or rebase.

For external contributors it would be good if commit retains the
original author info. So I was thinking to follow approach mentioned
in [1]. In brief use

- `git merge --no-ff feature` for feature branches involving multiple commits
- `git cherry-pick feature` for simple bug fixes

Used this for SLING-7223. Note that while closing the PR gitbot adds
whole PR diff as comment to the jira

So far I liked the way history looked for any module in svn so bit
more comfortable with any approach which is closer to that

Any guidance here on approach to be taken would be helpful.

Chetan Mehrotra
PS: Do not want to initiate a never ending discussion around rebase or
merge! Just checking what approach we take for our project in general

[1] https://mislav.net/2013/02/merge-vs-rebase/

Re: Etiquettes while merging PR from contributors

[Bertrand Delacretaz <bd...@apache.org>]

Hi Chetan,

On Wed, Nov 1, 2017 at 6:04 AM, Chetan Mehrotra
<ch...@gmail.com> wrote:
> ...In brief use
> - `git merge --no-ff feature` for feature branches involving multiple commits
> - `git cherry-pick feature` for simple bug fixes
>
> Used this for SLING-7223....

I like this, the commit at [1] says "mreutegg committed with
chetanmeh" which is exactly what we want IMO.

> ... Note that while closing the PR gitbot adds whole PR diff as comment to the jira...

That's quite noisy, I'd prefer a link to the commit, but we can look
at that separately.

-Bertrand

[1] https://github.com/apache/sling-org-apache-sling-commons-metrics-rrd4j/commit/d6d22d0f6f431e2f9c2bf465833cf3831095636d

Re: Etiquettes while merging PR from contributors

[Robert Munteanu <ro...@apache.org>]

According to [1], there are 3 options:

1. Merge all commits 
2. Squash the commits
3. Rebase the commits individually

1. is Chetan's 'git merge --no-ff feature'.
2. is my suggestion for 'fix-up' commits, with an added merge commit
3. is Chetan's 'git cherry-pick feature' if there is a single commit
only

So for fix-up commits there is some manual work to be done. But AFAIR
we can ask the PR submitter to ammend the original commit and do a
force push so we should be covered for all situations.


[1]: https://help.github.com/articles/merging-a-pull-request/

On Mon, 2017-11-06 at 13:36 +0100, Carsten Ziegeler wrote:
> Does this mean I need to type the commands instead of using githubs
> UI?
> 
> 
> Robert Munteanu wrote
> > On Wed, 2017-11-01 at 10:34 +0530, Chetan Mehrotra wrote:
> > > For external contributors it would be good if commit retains the
> > > original author info. So I was thinking to follow approach
> > > mentioned
> > > in [1]. In brief use
> > > 
> > > - `git merge --no-ff feature` for feature branches involving
> > > multiple
> > > commits
> > > - `git cherry-pick feature` for simple bug fixes
> > 
> > +1 from me.
> > 
> > Also it would be important IMO to ensure that 'fix-up' commits get
> > squashed.
> > 
> > Assume we have a PR with a simple commit that adds some Javadoc to
> > a
> > method. That commit introduces a typo, so another commit is added
> > to
> > the PR. I think the PR should be included as a single commit, not
> > as
> > two.
> > 
> > Robert
> > 

Re: Etiquettes while merging PR from contributors

[Carsten Ziegeler <cz...@apache.org>]

Does this mean I need to type the commands instead of using githubs UI?


Robert Munteanu wrote
> On Wed, 2017-11-01 at 10:34 +0530, Chetan Mehrotra wrote:
>> For external contributors it would be good if commit retains the
>> original author info. So I was thinking to follow approach mentioned
>> in [1]. In brief use
>>
>> - `git merge --no-ff feature` for feature branches involving multiple
>> commits
>> - `git cherry-pick feature` for simple bug fixes
> 
> +1 from me.
> 
> Also it would be important IMO to ensure that 'fix-up' commits get
> squashed.
> 
> Assume we have a PR with a simple commit that adds some Javadoc to a
> method. That commit introduces a typo, so another commit is added to
> the PR. I think the PR should be included as a single commit, not as
> two.
> 
> Robert
> 
-- 
Carsten Ziegeler
Adobe Research Switzerland
cziegeler@apache.org

Re: Etiquettes while merging PR from contributors

[Robert Munteanu <ro...@apache.org>]

On Wed, 2017-11-01 at 10:34 +0530, Chetan Mehrotra wrote:
> For external contributors it would be good if commit retains the
> original author info. So I was thinking to follow approach mentioned
> in [1]. In brief use
> 
> - `git merge --no-ff feature` for feature branches involving multiple
> commits
> - `git cherry-pick feature` for simple bug fixes

+1 from me.

Also it would be important IMO to ensure that 'fix-up' commits get
squashed.

Assume we have a PR with a simple commit that adds some Javadoc to a
method. That commit introduces a typo, so another commit is added to
the PR. I think the PR should be included as a single commit, not as
two.

Robert