#133: Update Devguide pull request commit documentation #134

louisom · 2017-02-24T08:19:07Z

Closes #133

willingc · 2017-02-24T13:48:50Z

pullrequest.rst

+
+   git commit
+
+Inside the commit editor, commit messages title should startswith


Inside the commit editor, the commit message's title should start with

willingc · 2017-02-24T13:50:29Z

pullrequest.rst

+   git commit
+
+Inside the commit editor, commit messages title should startswith
+``bpo-NNNNN`` and the major changed of this commit, for example::


changed -> change

willingc · 2017-02-24T13:54:04Z

pullrequest.rst

+
+   bpo-29624: Adds purge step and layout test after uploading files.
+
+You will saw hash tag number in ``git log``, e.g.


I would remove this paragraph and edit the next paragraph to read:

Two commands, ``git log`` and ``git status``, provide helpful information and detail about commits and changes.

I've rearrange the paragraph here. First introduce these command, and a note block to remind contributer won't need to add PR id to title. (I already make a mistake here......)

willingc · 2017-02-24T13:57:09Z

Thanks for the PR @grapherd. I've reviewed it and found a few typo/grammar items to edit. I would also recommend simplifying the sections on git log and git status. I've left some suggested text for you to consider.

berkerpeksag · 2017-02-24T14:25:52Z

pullrequest.rst

+changes to your branch. Assume you want to add a file which you have changed,
+you can add it to the list of changes to be committed (the "staging area")::
+
+   git add <file>


I think this level of detail is not needed. GitHub has pretty good introductory level resources (example) about using Git and I prefer to refer the readers to them.

I've add the chect sheet and git-scm to the end.

And remove the last line about staging area, but still reserve most word, I think we still need to provide a clear message to contributer.

berkerpeksag · 2017-02-24T14:28:48Z

pullrequest.rst

+Inside the commit editor, commit messages title should startswith
+``bpo-NNNNN`` and the major changed of this commit, for example::
+
+   bpo-29624: Adds purge step and layout test after uploading files.


+1. Adding an example commit message is a good idea.

willingc · 2017-02-24T15:46:32Z

pullrequest.rst


   git commit

-Inside the commit editor, commit messages title should startswith
-``bpo-NNNNN`` and the major changed of this commit, for example::
+Inside the commit editor, commit message's title should start with


...editor, the commit message's

willingc · 2017-02-24T15:49:31Z

pullrequest.rst

-You can always run ``git status`` to see what changes are outstanding.
+.. note::
+
+   You will saw hashtag with number in git log, e.g.


...will see a hashtag followed by a number that references the commit in the git log

should this be more specific to "... that references the pull request id in cpython repo."

willingc

Thanks for making changes in a timely manner. I like the addition of the cheatsheet and the git book as reference.

willingc · 2017-02-24T15:52:49Z

pullrequest.rst

+   You will saw hashtag with number in git log, e.g.
+   「bpo-29624: Adds purge step and layout test after uploading files.
+   **(#258)**」, you won't need to add this into your commit title, it will
+   be auto added when making a pull request on GitHub.

 When all of your changes are committed (i.e. ``git status`` doesn't
 list anything), you will want to push your branch to your fork::


...push your branch to your fork

louisom · 2017-02-24T14:51:21Z

pullrequest.rst

+
+   * `Git Cheat Sheet <https://services.github.com/on-demand/downloads/github-git-cheat-sheet.pdf>`_
+   * `ProGit <https://git-scm.com/book/en/v2>`_
+
 Now you want to
 `create a pull request from your fork <https://help.github.com/articles/creating-a-pull-request-from-a-fork/>`_.
 If this is a pull request in response to a pre-existing issue on the


I'm not sure the correct flow for a GitHub PR, still have some question here.

Because we will need the bpo id for commit title, should the above content about creating bpo issue put to the front of the chapter? Or the contributor maybe confuse about bpo title we mention before.

Yes, it should be moved earlier, and mention that creating an issue is not necessary for trivial changes.

louisom · 2017-02-25T07:44:39Z

pullrequest.rst

-You can always run ``git status`` to see what changes are outstanding.
+.. note::
+
+   You will saw hashtag with number in git log, e.g.


should this be more specific to "... that references the pull request id in cpython repo."

ezio-melotti · 2017-03-01T19:45:29Z

pullrequest.rst

-what changes are outstanding.
+Once you are satisfied with your work, you will need to commit your
+changes to your branch. Assume you want to add a file which you have changed,
+you will need to enter this command::


This sentence is confusing. If I changed a file, the file was probably there already, so I don't need to add it.
I think this part about git add can be removed, for the following reasons:

Adding files is uncommon -- most of the times existing files are changed

If the file is not added, I believe git will give an error during commit

The command to add files is very simple and similar for all the VCS

@ezio-melotti I'm confused about your comments. It's a common workflow pattern with git to:

git add <files> git commit -m 'Commit message here'

Apparently I assumed this worked like Mercurial, but it doesn't...

So git add doesn't mean "add this file to the tracked files starting from the next commit", but something like "add this file to the list of files that will get committed when I do git commit, right?
Also will git commit -am include all the modified files, including new files that were previously untracked?

git add will add files (modified or new) to the staging area. Files in the staging area will be committed when git commit is issued.

-a modifier will only include modified files and deleted files (it will not add new untracked) https://git-scm.com/docs/git-commit Plenty of folks use -a; I just prefer the more conservative two step git add git commit -m.

Do you think it would then be ok to suggest git command -am '...'?

I think it's better practice to use -m, but I'm happy to defer to you and Berker on that.

personally speaking, I usually use -v option. Then I do final self review and write commit message in vim.

I don't sure if we were using git description? If yes, we are using description, I think we should mention git commit, because this will open the editor and let user to enter title and description.

If not, I think using git commit -m is fine.

ezio-melotti · 2017-03-01T19:48:00Z

pullrequest.rst

@@ -119,10 +119,30 @@ homepage.
 Submitting
 ----------

-Once you are satisfied with your work you will want to commit your
-changes to your branch. In general you can run ``git commit -a`` and
-that will commit everything. You can always run ``git status`` to see


I would keep git status, and perhaps mention it together with git diff as both are useful to check if the right thing is being committed.

ezio-melotti · 2017-03-01T19:50:51Z

pullrequest.rst

+Inside the commit editor, commit message's title should start with
+``bpo-NNNNN`` and the major change of this commit, for example::
+
+   bpo-29624: Adds purge step and layout test after uploading files.


I think it would be better to suggest git commit -am 'bpo-NNNN: commit message.' directly instead of git commit + the editor.

I agree that git commit -m is better than the editor. I take a more conservative approach and rarely use -a to avoid inadvertently adding extraneous files that are not in the .gitignore.

Scratch the last part as -a will not add new files.

ezio-melotti · 2017-03-01T20:11:04Z

pullrequest.rst

+   bpo-29624: Adds purge step and layout test after uploading files.
+
+Two commands, ``git log`` and ``git status``, provide helpful information
+and detail about commits cand changes.


In my experience git log is not particularly useful while committing, whereas git status and git diff are useful before committing.
Another useful thing to check before committing is that the correct branch is selected (git branch) and that the repo is up-to-date. With HG I also used to use hg out after committing and before pushing.

Typo cand

FWIW git status will display the branch before committing. I agree git diff is useful prior to commit. git log is helpful for reviewing history and whether the cloned fork is up to date.

Good to know about git status -- that makes git branch unnecessary.
To check if the clone is up to date, I've been using git show --name-status (this should show the latest commit and be more or less equivalent to hg tip).

I suspect that there are multiple ways to check if the clone is up to date 👍 I tend to git fetch remote git rebase remote/branch if on my cloned fork. Some folks git pull from the remote. It may make sense to look at GitHub's new opensource.guide site and see what is most recommended there.

another small-ish typo: 'detail' -> 'details'.

Also, diff, status should be mentioned someplace (don't know if they are elsewhere) because they provide all the information you need before adding and commiting: what files you changed and how you changed them.

ezio-melotti · 2017-03-01T20:17:17Z

pullrequest.rst

+   You will saw hashtag with number in git log, e.g.
+   「bpo-29624: Adds purge step and layout test after uploading files.
+   **(#258)**」, you won't need to add this into your commit title, it will
+   be auto added when making a pull request on GitHub.


I find this note confusing. IIUC you are trying to say that the PR number shouldn't be included in the commit message, but why would the PR number show up in git log if I didn't include it in the commit message?

Also:

I wouldn't call the PR num "hashtag"

will saw -> will see

Use regular ASCII quote or rst markup for the message

ezio-melotti · 2017-03-01T20:17:52Z

pullrequest.rst

@@ -131,6 +151,13 @@ list anything), you will want to push your branch to your fork::

 This will get your changes up to GitHub.

+.. seealso::
+
+   For more information about git commands, please refer to:


Git should be capitalized.

ezio-melotti · 2017-03-01T20:21:52Z

pullrequest.rst

+
+   * `Git Cheat Sheet <https://services.github.com/on-demand/downloads/github-git-cheat-sheet.pdf>`_
+   * `ProGit <https://git-scm.com/book/en/v2>`_
+
 Now you want to
 `create a pull request from your fork <https://help.github.com/articles/creating-a-pull-request-from-a-fork/>`_.
 If this is a pull request in response to a pre-existing issue on the


Yes, it should be moved earlier, and mention that creating an issue is not necessary for trivial changes.

DimitrisJim · 2017-03-27T00:37:12Z

pullrequest.rst

+   bpo-29624: Adds purge step and layout test after uploading files.
+
+Two commands, ``git log`` and ``git status``, provide helpful information
+and detail about commits cand changes.


another small-ish typo: 'detail' -> 'details'.

Also, diff, status should be mentioned someplace (don't know if they are elsewhere) because they provide all the information you need before adding and commiting: what files you changed and how you changed them.

DimitrisJim · 2017-03-27T00:40:18Z

pullrequest.rst

+
+   git add <file>
+
+After adding all files of changed, you can commit your changes::


"files of changed" -> "the files you changed"

Though I think rewording "After adding all files of changed, you can commit your changes" to "After adding all the files that you changed, you can commit them with" might be better.

willingc · 2017-04-13T14:34:10Z

@lulouie Is this still an active PR (i.e. you are planning to update) or was the subject already addressed and this should be closed? Either one is fine. Just looking for an update. Thanks. Carol

louisom · 2017-04-13T15:05:09Z

@willingc I think #140 solve this problem, if there is any further improve, I'll open another PR.

willingc · 2017-04-13T17:09:00Z

@lulouie Thanks for the update. Also, thanks for your comments on other PRs too. 🍪

the-knights-who-say-ni added the CLA signed label Feb 24, 2017

ezio-melotti self-assigned this Feb 24, 2017

willingc requested changes Feb 24, 2017

View reviewed changes

berkerpeksag requested changes Feb 24, 2017

View reviewed changes

berkerpeksag reviewed Feb 24, 2017

View reviewed changes

willingc reviewed Feb 24, 2017

View reviewed changes

willingc requested changes Feb 24, 2017

View reviewed changes

willingc reviewed Feb 24, 2017

View reviewed changes

louisom commented Feb 25, 2017

View reviewed changes

brettcannon changed the title ~~#133: Update Devguide pull reuqest commit documentation~~ #133: Update Devguide pull request commit documentation Feb 27, 2017

ezio-melotti requested changes Mar 1, 2017

View reviewed changes

DimitrisJim reviewed Mar 27, 2017

View reviewed changes

#133: Update Devguide pull reuqest commit documentation

9c2c724

louisom closed this Apr 13, 2017


		git commit

		Inside the commit editor, commit messages title should startswith


		bpo-29624: Adds purge step and layout test after uploading files.

		You will saw hash tag number in ``git log``, e.g.


		git add <file>

		After adding all files of changed, you can commit your changes::

#133: Update Devguide pull request commit documentation #134

#133: Update Devguide pull request commit documentation #134

Conversation

louisom commented Feb 24, 2017 • edited by Mariatta Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willingc commented Feb 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willingc Feb 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willingc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezio-melotti Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willingc Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willingc commented Apr 13, 2017

louisom commented Apr 13, 2017

willingc commented Apr 13, 2017

louisom commented Feb 24, 2017 •

edited by Mariatta

Loading

willingc Feb 24, 2017 •

edited

Loading

ezio-melotti Mar 1, 2017 •

edited

Loading

willingc Mar 1, 2017 •

edited

Loading