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

[jorisvandenbossche]

Closes #19412

Reference discussion: https://mail.python.org/pipermail/pandas-dev/2017-November/000632.html

cc @datapythonista

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@87f5654). Click here to learn what that means.
The diff coverage is n/a.

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87f5654...c5b0475. Read the comment docs.

[datapythonista]

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.

[jreback]

@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

[jorisvandenbossche]

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)

[jorisvandenbossche]

@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.

[jschendel]

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.

[jorisvandenbossche]

@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?

[jreback]

i have a local tracking branch: master that tracks upstream/master

so
gilt pull

does it all for me

[jorisvandenbossche]

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).

[jschendel]

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.

[jorisvandenbossche]

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

[jschendel]

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.

[jorisvandenbossche]

Yes, I didn't interpret it like that! You nicely followed are guidelines :-)

[jreback]

lgtm. very minor comments.

[jreback]

need to make this an http ref

[jorisvandenbossche]

sphinx does that automatically

[jreback]

maybe add a reference to git stash here (git docs)

[jorisvandenbossche]

done

[jreback]

maybe add a comment about -f force option of push (they shouldn't use it)

[jorisvandenbossche]

Any more comment?