DOC: update contributing guide regarding updating a pull request (merge master)
State: closed

Closes https://github.com/pandas-dev/pandas/issues/19412

Reference discussion: https://mail.python.org/pipermail/pandas-dev/2017-November/000632.html

cc @datapythonista 

Labels:
name: Docs

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19990?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@87f5654`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/19990/graphs/tree.svg?src=pr&token=eZ4WkYLtcO&width=650&height=150)](https://codecov.io/gh/pandas-dev/pandas/pull/19990?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master   #19990   +/-   ##
=========================================
  Coverage          ?   91.71%           
=========================================
  Files             ?      150           
  Lines             ?    49112           
  Branches          ?        0           
=========================================
  Hits              ?    45045           
  Misses            ?     4067           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.1% <ø> (?)` | |
| #single | `41.86% <ø> (?)` | |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19990?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19990?src=pr&el=footer). Last update [87f5654...c5b0475](https://codecov.io/gh/pandas-dev/pandas/pull/19990?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Looks great. Is it any reason to not use the `--set-upstream` (or `-u`) in the first push to the branch, so in the other pushes they need to repeat the remote and branch again? I'd say it's more common this way, not sure if there is a reason I'm missing to not use it.
@datapythonista actually should check out tracking branches in the first place, e.g.

```
git checkout -b shiny-new-feature --track origin/master
```

is usually the correct thing to do
> I'd say it's more common this way, not sure if there is a reason I'm missing to not use it.

Yes, the only problem is then that those commands won't work if you didn't do that in the first place. So I am certainly in favor of mentioning this when creating the branch (as @jreback points out) or when doing the first push. But maybe we can still keep the general working method here? (although it being more verbose)

(I personally have just my general setting of git to push to same-named branches, so I never do `-u` or `--track origin/master`)
@jschendel did you delete your comment? I think it was relevant:

> Earlier in this doc, under the "Creating a Branch" section, the prescribed procedure for updating a branch is slightly different:
>
>git fetch upstream
>git rebase upstream/master
>
>Is there a reason to prefer rebase in the former section, and merge in this one? Since these sections are relatively far apart it might be nice to provide a quick link between the two sections, and briefly mention when to use one method vs, another? Or could maybe just use a consistent command in each section?

You could say that as long as this is *before* submitting the PR this is fine, but then it can be confusing that we give different guidelines on how to update a branch before and after you submitted a PR ? 
We could also just remove that section, since this is now covered by my new section.

Yeah, I deleted my comment since I missed part of the mailing list discussion thread, and finding it cleared up some of my confusion.  I'm +1 on removing the `rebase` portion of the "Creating a Branch" section.  Seems like that's the most straightforward option that would result in the least amount of confusion for new contributors, especially if we want to nudge them towards using `merge` instead of `rebase`.
@jschendel @jreback @TomAugspurger or any body else -> Question: how do you typically update your master branch with upstream/master? (before creating a new feature branch)

Because that is currently not really mentioned in the contributing guide (it only mentions how to update a branch with rebase, which I will change to merge).

I have an git alias that does:

```
git fetch upstream
git merge upstream/master --ff-only
```

I have the `--ff-only` added because in principle I never committed myself on master and only in branches, and this ensures that this is the case (which is a check that `git pull upstream master` will not do I think, but of course could do `git pull upstream master --ff-only` as well)

What do others do for this step? Or what would be the best to recommend in the contributing guide?



i have a local tracking branch: master that tracks upstream/master 

so
gilt pull

does it all for me
Yes in that case `git pull upstream master` will do the same as `git pull` I think? 
So for the contributing guide, let's assume no set-up tracking branch, and give the full command that certainly works ? 

(and `git pull upstream master --ff-only` will still further do exactly the same as `git pull` if you have no commits on local master that are not present in upstream master. But again, I would like to give the command that is fully save. Somebody who knows git will know how to simplify it).
> Question: how do you typically update your master branch with upstream/master? (before creating a new feature branch)

I only had basic git skills when I started contributing to pandas, so I just followed the contributing guide, probably too closely, and was using the previously recommended `rebase` procedure of:
```
git fetch upstream
git rebase upstream/master
```
Definitely overkill though, as I never actually make changes on master.
> Definitely overkill though, as I never actually make changes on master.

Yes, and therefore also the wrong recommendation I think, as *if* you made changes by accident to master, it will not do what you want (it should complain instead IMO). 
Although that a plain `git merge upstream/master` certainly does the wrong thing as well if you have commits on master .. That's the reason I would recommend `--ff-only`
> therefore also the wrong recommendation I think

Yes, didn't mean for that to sound like I was suggesting the rebase approach that was previously in the docs; was also trying to say that my approach is not the right one here. The fast-forward approach seems like the most appropriate one to me.
Yes, I didn't interpret it like that! You nicely followed are guidelines :-)
Any more comment?