Re: [PATCH 1/5] Fix some typos, grammar or wording issues in the documentation
- From
Štěpán Němec <stepnem@smrk.net>
- Date
- Oct 3, 2023, 20:09 UTC
- Message-ID
- <20231003220951+0200.221551-stepnem@smrk.net>
- In-Reply-To
- <CAPig+cR2wgAsGCTphqHsf_pbM0q_xLcMx=acVD6MDKhpEt3HNA@mail.gmail.com>
On Tue, 3 Oct 2023 14:30:38 -0400 Eric Sunshine wrote:
> On Tue, Oct 3, 2023 at 4:28 AM Štěpán Němec <stepnem@smrk.net> wrote: >> Fix some typos, grammar or wording issues in the documentation > > SubmittingPatches suggests: s/Fix/fix
I think that only applies to subjects of the form "area: description".
Come to think of it, 'doc: fix some typos, grammar and wording issues' might have made sense to begin with; I don't suppose C header comments are off-limits to doc:.
Show 8 quoted lines
>> diff --git a/Documentation/git.txt b/Documentation/git.txt >> @@ -96,7 +96,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git config >> This is useful for cases where you want to pass transitory >> -configuration options to git, but are doing so on OS's where >> +configuration options to git, but are doing so on OSes where > > Since you're touching this anyhow, it could probably be made clearer > by simply spelling it out: "operating systems"
Probably. Checking for other occurrences now I see I haven't done my homework, anyway: there are "OS's" as plural in config/transfer.txt and fsmonitor--daemon.h, too. I'll include those and use "operating systems" in the next version, thanks.
Show 7 quoted lines
>> other processes might be able to read your cmdline >> (e.g. `/proc/self/cmdline`), but not your environ >> (e.g. `/proc/self/environ`). That behavior is the default on > > I wonder if "cmdline" and "environ" would be better spelled out, as > well, since the examples which immediately follow them give the > necessary context.
Agreed.
Show 5 quoted lines
> other processes might be able to read your command-line > (e.g. `/proc/self/cmdline`), but not your environment > (e.g. `/proc/self/environ`). > > But perhaps that's outside the scope of this patch?
I don't think so; I'll include those.
Show 10 quoted lines
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt >> @@ -1151,7 +1151,7 @@ will be stored via placeholder `%P`. >> This attribute controls the length of conflict markers left in >> -the work tree file during a conflicted merge. Only setting to >> +the work tree file during a conflicted merge. Only setting >> the value to a positive integer has any meaningful effect. > > Or, more simply: > > Only a positive integer has a meaningful effect.
That's better, thanks.
Show 7 quoted lines
>> diff --git a/contrib/README b/contrib/README >> @@ -24,14 +24,14 @@ lesser degree various foreign SCM interfaces, so you know the >> I expect that things that start their life in the contrib/ area >> -to graduate out of contrib/ once they mature, either by becoming >> +graduate out of contrib/ once they mature, either by becoming > > You probably want to add a comma after "area".
That would read awkward to me. How about going the other way,
I expect things that start their life in the contrib/ area to graduate out of contrib/ once they mature
instead?
Show 6 quoted lines
> Do we want to correct the formatting of this pathname: > > ...in the `contrib/` area... > ...out of `contrib/` once... > > or is that outside the scope of this patch?
I'd prefer not to: this is a plain-text README, no other markup present, and I don't think the backquotes would help here.
Show 18 quoted lines
>> diff --git a/strbuf.h b/strbuf.h >> @@ -12,9 +12,9 @@ >> - * strbuf's are meant to be used with all the usual C string and memory >> + * strbufs are meant to be used with all the usual C string and memory > > Alternatively: > > strbuf is meant to be used... > >> * APIs. Given that the length of the buffer is known, it's often better to >> - * use the mem* functions than a str* one (memchr vs. strchr e.g.). >> + * use the mem* functions than a str* one (e.g., memchr vs. strchr). >> * Though, one has to be careful about the fact that str* functions often >> * stop on NULs and that strbufs may have embedded NULs. > > Similar: > > ... and that a strbuf may have...
Hm, not sure about this one. I think I'd prefer keeping the plurals, but it's not a strong preference.
Show 10 quoted lines
>> diff --git a/t/README b/t/README >> @@ -262,8 +262,8 @@ The argument for --run, <test-selector>, is a list of description >> suite to include (or exclude, if negated) in the run. A range is two >> -numbers separated with a dash and matches a range of tests with both >> -ends been included. You may omit the first or the second number to >> +numbers separated with a dash and matches an inclusive range of tests >> +to run. You may omit the first or the second number to > > Not the fault of this patch, but "matches" seems an odd choice. > Perhaps "specifies" would feel more natural.
Indeed, will fix.
Show 8 quoted lines
>> The argument to --run is split on commas into separate strings, >> @@ -579,11 +579,10 @@ This test harness library does the following things: >> -Here are some recommented styles when writing test case. > > Do you want to fix the spelling error while you're here or is that > done in a later patch? > > s/recommented/recommended/
You really had me double-checking both the branch and the patch I sent here. :-D Unless I'm very much missing something, that line is _removed_ by the patch (seemed redundant given the title immediately preceding it).
Show 11 quoted lines
>> - - Keep test title the same line with test helper function itself. >> + - Keep test titles and helper function invocations on the same line. > > This would be clearer if it was switched around. Either: > > Keep the test_expect_* function call and test title on the same line. > > or, more verbosely: > > Place the test title on the same line as the test_expect_* call > which precedes it.
I'll take your first suggestion.
Thank you for the review!
-- Štěpán