- "(s)" used 5 times
- "in order to" used 6 times
- "i.e." used 1 time
- "e.g." used 1 time
- "simply" used 3 times
- "basically" used 2 times
- "the following" used 18 times
- "as follows" used 3 times
- "should" used 83 times
Details
Details
- Reviewers
wblock - Commits
- rD50299: Pass the committer's guide through igor.
Diff Detail
Diff Detail
- Repository
- rD FreeBSD doc repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
I've found a few things that should be improved, but overall, good work. +1 for the title of the review. ;-)
| en_US.ISO8859-1/articles/committers-guide/article.xml | ||
|---|---|---|
| 1827 ↗ | (On Diff #19356) | I think there is a word missing now after the "as". Something like "as it" or "as they". |
| 2025 ↗ | (On Diff #19356) | I think you can even drop the "these" here. |
| 4109 ↗ | (On Diff #19356) | s/contains/contain/ |
Comment Actions
Wow!
| en_US.ISO8859-1/articles/committers-guide/article.xml | ||
|---|---|---|
| 480 ↗ | (On Diff #19410) | Can remove that indefinite "It": New committers are assumed to already be familiar... |
| 1003 ↗ | (On Diff #19410) | Passive -> active: |
| 1263 ↗ | (On Diff #19410) | No "s" because of the plural "merges": A note on "should": it's not always wrong. The wrong way to use it is when it implies the writer is not sure of what they are saying, like "the screen output should look like this". The right way is when you are making a recommendation: "You should always use strong passwords." In this case, you've made it more imperative. It's not a recommendation, it's the way things are. Nice! |
| 1269 ↗ | (On Diff #19410) | How about "must be" rather than "is"? |
| 1272 ↗ | (On Diff #19410) | As above, probably better with s/$/must/ |
| 1289 ↗ | (On Diff #19410) | Redundant with the same word used in the next line. How about "process"? |
| 1299 ↗ | (On Diff #19410) | directory where changes are being merged) |
| 1317 ↗ | (On Diff #19410) | It would be terrible if something were to happen and this old, broken, confusing merge process were to be accidentally deleted totally by accident and unintentionally. |
| 1460 ↗ | (On Diff #19410) | Better with "For example, to merge:" |
| 1615 ↗ | (On Diff #19410) | Nice! |
| 1630 ↗ | (On Diff #19410) | As above, probably better as "must be". |
| 1784 ↗ | (On Diff #19410) | This is one of those rare instances where "should" is appropriate. "can" makes it sound like it is not possible rather than something that is to be avoided. |
| 1787 ↗ | (On Diff #19410) | Since we are here, break sentence: Committing is now possible. However, it is good |
| 1788 ↗ | (On Diff #19410) | s/OK/okay/ |
| 1827 ↗ | (On Diff #19410) | conflicts as they will be handled manually.</para> |
| 1871 ↗ | (On Diff #19410) | Good, but the original sentence is poor and the writer probably misused i.e. when they mean e.g., another reason to avoid these. vendor tree but not in the main tree. These are things that |
| 1873 ↗ | (On Diff #19410) | Redundant "like" and "for example". import, like the vendor's <filename>Makefile</filename>s |
| 2014 ↗ | (On Diff #19410) | Hard to keep track of what "it" means here. Rearrange: Avoid setting up a <application>svnsync</application> mirror unless there is a very good reason for it. |
| 2016 ↗ | (On Diff #19410) | "many" and "multiple" are redundant. Remove "many". s/the network/network/ |
| 2020 ↗ | (On Diff #19410) | Spell out numbers up to ten: four to ten |
| 2025 ↗ | (On Diff #19410) | I would keep "these" but lose "three", reason being that counts in text frequently are wrong after text is edited. Or better, just say There are several ways to do this:</para> |
| 2085 ↗ | (On Diff #19410) | Missing "name" after "same". |
| 2128 ↗ | (On Diff #19410) | s/adding to/adding these entries to/ |
| 2262 ↗ | (On Diff #19410) | "your" is okay here, because it is specific. Sometimes these are hard to avoid. |
| 2391 ↗ | (On Diff #19410) | The original sentence is kind of misleading. Reading it does not mean it will be kept. How about or save it elsewhere.</para> |
| 2394 ↗ | (On Diff #19410) | s/processing/processing,/ |
| 2400 ↗ | (On Diff #19410) | s/email/email,/ |
| 2403 ↗ | (On Diff #19410) | End the sentence here: class="fqdomainname">freefall.FreeBSD.org</systemitem>.</para> The rest of the sentence is redundant. |
| 2468 ↗ | (On Diff #19410) | s/commit/commit,/ |
| 2582 ↗ | (On Diff #19410) | Hm. The original is a complex and confusing sentence which appears to include irrelevant information. The commit is from a PR submitted by John Smith. |
| 2584 ↗ | (On Diff #19410) | The commit message <quote>PR</quote> and <quote>Submitted by</quote> fields are filled. |
| 2595 ↗ | (On Diff #19410) | <para>The virtual memory system is being changed. After |
| 2608 ↗ | (On Diff #19410) | <para>Commit a port, after working with``` |
| 2624 ↗ | (On Diff #19410) | <para>Commiting some code based on work done in the |
| 2636 ↗ | (On Diff #19410) | <para>Committing some code which will be merged from``` |
| 2652 ↗ | (On Diff #19410) | It is often necessary to combine these. |
| 2654 ↗ | (On Diff #19410) | This can be third-person: Looking at the PR, the developer sees it is not an area of the tree they normally work in, so they have the change reviewed by the <literal>arch</literal> mailing list. Since the change is complex, the developer opts to <acronym>MFC</acronym> after one month to allow adequate testing.</para> |
| 2680 ↗ | (On Diff #19410) | "Currently" isn't doing anything useful here. s/Currently the/The/ |
| 2812 ↗ | (On Diff #19410) | s/When working on/Working on/ |
| 2814 ↗ | (On Diff #19410) | s/When trying/Trying/ |
| 2818 ↗ | (On Diff #19410) | s/you/a developer/ |
| 2820 ↗ | (On Diff #19410) | s/you/a developer/ |
| 2823 ↗ | (On Diff #19410) | repository, if it s not clear who the active maintainer |
| 2824 ↗ | (On Diff #19410) | s/be,/is,/ |
| 2829 ↗ | (On Diff #19410) | s/your queries/queries/ |
| 2839 ↗ | (On Diff #19410) | <para>If there is any doubt about a commit for any reason at all, have |
| 2842 ↗ | (On Diff #19410) | part of the repository. If a commit does |
| 2843 ↗ | (On Diff #19410) | does result in controversy erupting, |
| 2845 ↗ | (On Diff #19410) | Remove that misused, ugly emdash: settled. Remember, with a version control system we can |
| 2848 ↗ | (On Diff #19410) | <para>Do not impugn the intentions of others. |
| 2849 ↗ | (On Diff #19410) | If they see a different solution to a problem, or even |
| 2850 ↗ | (On Diff #19410) | a different problem, it is probably not because they are stupid, because |
| 2876 ↗ | (On Diff #19410) | <para>When unsure about something, whether it be a |
| 2898 ↗ | (On Diff #19410) | s/will/can/ |
| 3142 ↗ | (On Diff #19410) | Now &man.ssh-add.1; can be used for |
| 3143 ↗ | (On Diff #19410) | authentication once per session. It prompts for |
| 3144 ↗ | (On Diff #19410) | the private key's pass phrase, and then stores it in the |
| 3145 ↗ | (On Diff #19410) | authentication agent (&man.ssh-agent.1;). Use <command>ssh-add -d</command> to remove keys stored in the agent.<para> |
| 3149 ↗ | (On Diff #19410) | Test with a simple remote command: <command>ssh |
| 3385 ↗ | (On Diff #19410) | Phabricator service. The commit will only happen once |
| 3387 ↗ | (On Diff #19410) | does not mean that permission is required before |
| 3389 ↗ | (On Diff #19410) | misspelling, just that it is good to develop a feel |
| 3395 ↗ | (On Diff #19410) | The very best way of making sure that things are on the right |
| 3396 ↗ | (On Diff #19410) | track is to have code reviewed by one or more other |
| 3419 ↗ | (On Diff #19410) | look at the repository logs for the files |
| 3501 ↗ | (On Diff #19410) | cooled back down. Do not air |
| 3502 ↗ | (On Diff #19410) | angry words in public and do not forward private |
| 3509 ↗ | (On Diff #19410) | the person sending a flame-o-gram at least had the |
| 3601 ↗ | (On Diff #19410) | This whole paragraph needs a rewrite. |
| 3679 ↗ | (On Diff #19410) | s/theses/these/ |
| 3772 ↗ | (On Diff #19410) | s/are not/must not be/ |
| 3786 ↗ | (On Diff #19410) | s/are/must be kept/ |
| 3791 ↗ | (On Diff #19410) | are considered private communications. |
| 3792 ↗ | (On Diff #19410) | Permission is required to publish material from these |
| 3948 ↗ | (On Diff #19410) | This is a recommendation, so either "should be" or "must be", depending on how imperative you feel. |
| 3965 ↗ | (On Diff #19410) | s/are/is/ (singular "support") |
| 4275 ↗ | (On Diff #19410) | subdirectory to another, or when the |
| 4276 ↗ | (On Diff #19410) | name of a directory must be changed because the authors renamed their |
| 4619 ↗ | (On Diff #19410) | <para>This is much simpler than a physical category. Only |
| 4620 ↗ | (On Diff #19410) | a few modifications are needed:</para> |
| 4699 ↗ | (On Diff #19410) | "a port's" is redundant and can be removed. |