Add Git Bootcamp page#144
Conversation
orsenthil
left a comment
There was a problem hiding this comment.
Contents LGTM. For the page itself, suggest getting a sign-off from Brett.
willingc
left a comment
There was a problem hiding this comment.
This is wonderful! 🍰 🍪
It's great as is. Do you want to add "how to merge a PR into master" too?
|
|
||
| $ git checkout master | ||
| $ git fetch upstream | ||
| $ git rebase upstream/master |
There was a problem hiding this comment.
Why do you have to rebase the master branch if you haven't committed to it? Is this a step that git pull does for you?
There was a problem hiding this comment.
Difference in workflow approach. Some (myself included) use rebase/fetch instead of git pull.
There was a problem hiding this comment.
Why is that? What's the pros/cons of the two approaches? (Basically why do you like the extra steps since this is meant for people not familiar with git and thus we might want to prefer the fewer-steps approach?)
There was a problem hiding this comment.
git pull --rebase would be similar in approach and one could configure git pull to use rebase by default instead of merge.
There was a problem hiding this comment.
I've only ever used git pull upstream master in any other projects I've used. To me it says more clearly what it's doing - "pull what's new on upstream onto my master branch". AFAIK, that's the normal advice (I don't recall where I learned the workflow from, but likely from various tutorials online). I think we should stick to the "normal" approach, as best as we can determine it, so we don't get newcomers being confused as to why we do things differently on Python.
I'd assume that fetch/rebase is intended more for cases where you're making changes on your local master branch. But my normal workflow is to only ever have master as a pristine copy of upstream's master, so pull upstream master says what I'd expect.
There was a problem hiding this comment.
OK, cool. I wasn't sure if this was intended as a recommendation, is all - to me, it's more confusing than a simple git pull, and I'd tend to go for the simpler suggestion by default. But if we're saying the same thing, that's fine :-)
There was a problem hiding this comment.
I think @pfmoore picked up on my initial confusion. As the instructions are just to keep master in sync then the rebase step seemed odd to me since you're not rebasing any previous commits you made locally. Maybe a comment at the start saying "there are alternative ways to perform the synchronization, but the approach taken to provide steps that are the same across scenarios"?
There was a problem hiding this comment.
That's why I suggest git pull --rebase upstream master.
There was a problem hiding this comment.
I think @zware's suggestion is the best in terms of simplicity and flexibility.
There was a problem hiding this comment.
I'd be inclined to prefer @zware's git pull --rebase upstream master, with a comment "you only need --rebase if you have local changes to the branch", That's simple enough for newcomers, explains when --rebase is needed, and remains consistent across scenarios.
|
|
||
| Go to https://github.com/python/cpython to create the pull request. Select | ||
| ``3.6`` as the base branch, and ``backport-someissue-3.6`` as the head branch. | ||
|
|
There was a problem hiding this comment.
You forgot to mention your script. 😄
There was a problem hiding this comment.
Once it's moved to core-workflow, I'll update this :)
|
LGTM, but honestly I think having Ezio look over this would be great since I know he has asked for more of a git intro in the devguide. |
| $ cd UserX | ||
| $ git clone https://github.com/UserX/cpython | ||
| $ cd cpython | ||
| $ git checkout fixspamlib |
There was a problem hiding this comment.
This can be simplified significantly. GitHub offers pull requests as remote branches that are not pulled automatically, but you can pull them explicitly by doing git fetch upstream pull/${1}/head:pr/${1} && git checkout pr/${1} where ${1} is the PR number. @berkerpeksag has this nicely packaged up as an alias:
[alias]
pr = !sh -c \"git fetch upstream pull/${1}/head:pr/${1} && git checkout pr/${1}\" -
which allows one to do git pr 615 to fetch and check out python/cpython#615. The local branch name (pr/${1} or pr/615 here) can be adjusted as you see fit; I might prefer pr_${1}.
There was a problem hiding this comment.
Very interesting.. I didn't know about this :)
There was a problem hiding this comment.
As an FYI (though we don't need to document), hub makes a maintainer's job easier (similar to @zware's mention of @berkerpeksag's alias).
- explain how to set up `git pr` alias - explain how to accept and merge a PR - provide an example of good squash commit message
|
Made some adjustments:
|
|
|
||
| From your command line:: | ||
|
|
||
| $ git clone git@github.com:UserName/cpython.git |
There was a problem hiding this comment.
Might want to add a note about substituting UserName or use something like <username> in case UserName on GitHub actually has CPython forked. 😉
|
|
||
| $ git checkout -b some-branch master # creates a new branch off master | ||
|
|
||
| which is equal to:: |
There was a problem hiding this comment.
Might want to mention it's equivalent if you are starting from the master branch, otherwise explicitly add the master part to the git branch step.
There was a problem hiding this comment.
Thanks, I made the change in line 56.
| $ git pr 777 | ||
|
|
||
|
|
||
| Accepting and Merging Pull Request |
There was a problem hiding this comment.
"Accepting and Merging a Pull Request"
|
|
||
| 3. Select the base fork: ``python/cpython`` and base branch: ``master`` | ||
|
|
||
| 4. Select the head fork: ``UserName/cpython`` and base branch: the branch |
- change UserName into <username> - be explicit about branching off master
| which is equal to:: | ||
|
|
||
| $ git branch some-branch # create 'some-branch', without checking it out | ||
| $ git branch some-branch master # create 'some-branch' off 'master, without checking it out |
There was a problem hiding this comment.
I think you're missing an apostrophe at the end of "master".
ezio-melotti
left a comment
There was a problem hiding this comment.
Even though I left lot of comments, mostly are minor issues, and overall I like the page.
This however adds yet another page, so in the future this might end up replacing or being merged with the other git-related pages.
| Git Bootcamp and Cheat Sheet | ||
| ============================ | ||
|
|
||
| In this section, we'll go over some commonly used git commands that are |
There was a problem hiding this comment.
Just a nit: I think this should either be "Git" or :cmd:`git`.
|
|
||
| 1. Go to https://github.com/python/cpython. | ||
|
|
||
| 2. Press ``Fork``. |
There was a problem hiding this comment.
"Press the Fork button on the top-right".
|
|
||
| 3. When asked where to fork the repository, choose to fork it to your username. | ||
|
|
||
| 4. A fork will be created at https://github.com/<username>/cpython. |
| Cloning The Forked CPython Repository | ||
| ------------------------------------- | ||
|
|
||
| From your command line:: |
There was a problem hiding this comment.
In this section you could also mention that this is only necessary once.
|
|
||
| $ git clone git@github.com:<username>/cpython.git | ||
| $ cd cpython | ||
| $ git remote add upstream git@github.com:python/cpython.git |
There was a problem hiding this comment.
Using $ has the disadvantage that is not possible to copy/paste all the instructions at once, but it makes clear that these are shell commands. Since most of the commands are one-liners, it might be ok to leave the $.
|
|
||
| For example, to fetch and checkout pull request #777:: | ||
|
|
||
| $ git pr 777 |
There was a problem hiding this comment.
I think this is the only example in the page -- elsewhere you only used .
I'm not sure it's needed -- it doesn't hurt having it, but it's not particularly useful either, and it's a bit inconsistent with the rest.
There was a problem hiding this comment.
I'll remove the example to make it consistent.
| For example, to fetch and checkout pull request #777:: | ||
|
|
||
| $ git pr 777 | ||
|
|
There was a problem hiding this comment.
Another scenario that is not covered here is: how can I update a PR that I pulled from a contributor (e.g. to add Misc/NEWS)?
This came up on #python-dev a few days ago, and afaiu:
- you can add your changes and push to the contributor repo using
git push -u git@github.com:<contributor>/cpython.git <branch>(or add a remote withgit remote add <contributor> git@github.com:<contributor>/cpython.gitand thengit push -u <contributor> <branch>). In order to do this the contributor must have allowed the maintainers to edit the prs (should be allowed by default). - you can add your changes, push the branch to your fork, and create a new and separate PR that replaces the original one.
It was not clear how the author is determined if multiple users (contributor and core-dev) worked on the same PR: author of the first commit? owner of the repo the PR comes from? author with most commits/loc in the PR?
There was a problem hiding this comment.
I haven't had to do this myself, so I don't have the answer ...
| If the change has been committed, and now you want to reset it to whatever | ||
| the origin is at:: | ||
|
|
||
| $ git reset --hard HEAD |
There was a problem hiding this comment.
There are two more scenarios where I used hg revert that are not covered here:
- how to revert all changes in the working copy (i.e. the equivalent of
hg revert -a--git reset?git reset --hard?). - how to revert the fix while leaving the test to verify if they fail without the fix.
The second case works differently since we are using PRs. With HG I could apply a patch and do hg revert Lib/ to revert all the changes in Lib while leaving the tests unchanged. I think the git equivalent would be something like git checkout master Lib/, after I pulled a PR as described in a later section.
| Accepting and Merging A Pull Request | ||
| ------------------------------------ | ||
|
|
||
| Pull requests can be accepted and merged by a Python Core Developer. |
There was a problem hiding this comment.
A question about "merging etiquette": if another core-dev submitted a PR, and it looks good to me, should I go ahead and merge it, or should I just leave a positive review and let the original core-dev merge it?
I think the latter is better, but I've seen the former happen.
Perhaps this could be clarified here.
There was a problem hiding this comment.
Perhaps this is to be asked at core-workflow or python-committers? :)
|
|
||
|
|
||
| Accepting and Merging A Pull Request | ||
| ------------------------------------ |
There was a problem hiding this comment.
This should be marked as core-devs only, since contributors can't accept/merge.
There was a problem hiding this comment.
Mentioned in the next line.
|
@ezio-melotti Wouldn't it make more sense since we are now on GitHub to merge this PR as is and then create a new PR for any other changes you wish to make? |
|
That would be fine with me, since there are no major problems with the patch. |
|
Thanks @ezio-melotti. Your flexibility is appreciated. I suspect this will get iterated over time as well. 👍 |
|
Thanks for reviewing, @ezio-melotti 😃 I will prepare another update to this PR later today to address some of your feedback. There are definitely more topics that need to be covered here, but we can address them in future PRs :)
Thanks :) |
- replace current instruction to backport with a reference to cherry_picker.py - add ..highlight:: console - some formatting
|
The latest changeset LGTM. |
|
Thanks for all the feedback and for reviewing 🙋 💯 🎉 |
Provide a list of useful Git commands that are relevant to CPython's workflow, such as: - forking and cloning the repo - branching - committing - backporting - creating pull requests
The no frills guide to git.
Closes #125, python/core-workflow#27