Thread: [HACKERS] Possible spelling fixes
Hi. I'm going through project-by-project offering spelling fixes. I have read your submission suggestions [1][2] and am choosing to disregard them. Could someone please review the changes I have [3] and suggest a series of commits that this project might like? It is quite likely that someone will tell me that there are certain types of changes that you don't want. That's fine, I'm happy to drop changes. It's possible that I've chosen the wrong spelling correction or made a mistake in my attempts, while the process of identifying errant words is somewhat automated, the correction process is a mix of me and some scripts, and neither of us are perfect. The patch queue is currently sorted alphabetically, but it contains at least: * branding corrections -- some of which may be wrong * regional English -- I understand there's a discussion about this, I can easily drop them, or you can choose to take them separately * changelog changes -- I could understand not wanting to change the changelog * local variable changes * API changes -- hopefully the changes to functions/enums are within private space and thus not a problem, but if you consider them to be public, dropping them isn't a big deal * changes to tests -- it wouldn't shock me if I messed some of these up. Again, everything can be split/dropped/fixed if requested (within limits -- I refuse to submit 100 emails with patches) * changes to comments, lots of comments Most changes are just changes to comments, and I'd hope it wouldn't be terribly hard for them to be accepted. A complete diff would be roughly 130k. I've recently tried to submit a similarly sized patch to another project and it was stuck in moderation (typically mailing lists limit attachments to around 40k). I do not expect anyone to be able to do anything remotely useful with such a patch. It is certainly possible for someone else to generate such a file if they really like reading a diff in that manner. For reference, I've split my proposal into 172 changes. Each change should represent a single word/concept which has been misspelled (possibly in multiple different manners). I understand that people don't care about diffstats (I certainly don't), but for reference collectively this encompasses 175 files and 305 lines. Note that I've intentionally excluded certain files (by removing them in prior commits), it's possible that changes would be needed to be made to some of those files in order for tests to pass. If you have an automated system for running tests (typically Travis or Appveyor), I'm happy to submit changes to them before offering changes to a list. But I couldn't find anything of the sort. [1] https://wiki.postgresql.org/wiki/Submitting_a_Patch [2] http://petereisentraut.blogspot.ca/2009/09/how-to-submit-patch-by-email.html [3] https://github.com/jsoref/postgres/compare/ignore...jsoref:spelling?expand=1
Hi, On 2017-02-05 21:05:50 -0500, Josh Soref wrote: > Could someone please review the changes I have [3] and suggest a > series of commits that this project might like? I think the current split seem excessive... I'd suggest splitting things first into: - straight up spelling/typo fixes in comments etc. - straight up spelling/typo fixes in variable names etc - straight up spelling/typo fixes that are API relevant - the same split for stuff more dependenant on taste Then afterwards we can see what's remaining. > A complete diff would be roughly 130k. I've recently tried to submit a > similarly sized patch to another project and it was stuck in > moderation (typically mailing lists limit attachments to around 40k). IIRC 130k shouln't get you stuck in filters yet - if you're concerned you can gzip the individual patches. > I understand that people > don't care about diffstats (I certainly don't), but for reference > collectively this encompasses 175 files and 305 lines. FWIW, I do ;) Greetings, Andres Freund
It's now split more or less to your suggestion: https://github.com/jsoref/postgres/commits/spelling -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 02/06/2017 04:50 AM, Josh Soref wrote: > It's now split more or less to your suggestion: > https://github.com/jsoref/postgres/commits/spelling Thanks! I pushed most of these. Except for the below: > optimisation -> optimization et al. Most of our code is written with the American spelling, but the British spelling isn't wrong, so I don't want to go around changing them all. > NUL-terminated -> NULL-terminated When we're talking about NUL-terminated strings, NUL refers to the NUL ASCII character. NULL usually refers to a NULL pointer. We're probably not consistent about this, but in this context, NUL-terminated isn't wrong, so let's leave them as they are. > Ooops -> Oops "Oops" is more idiomatic, but this doesn't really seem worth changing. Maybe "Ooops" indicates a slightly bigger mistake than "oops" :-) > re-entrancy -> reentrancy Googling around, I can see both spellings being used. "Re-entrancy" actually feels more natural to me, although I'm not sure which is more correct. Let's leave them as they are. > passthru -> passthrough "Passthrough" is clearly the correct spelling (or "pass-through"?), but "passthru" seems OK in the context, as an informal shorthand. > --- a/src/backend/tsearch/dict_thesaurus.c > +++ b/src/backend/tsearch/dict_thesaurus.c > @@ -23,7 +23,7 @@ > > > /* > - * Temporay we use TSLexeme.flags for inner use... > + * Temporary we use TSLexeme.flags for inner use... > */ > #define DT_USEASIS 0x1000 Looking at the code real quick, I couldn't understand the original meaning of this. Is it: * DT_USEASIS is a temporary value we use for something. For what? * DT_USEASIS is used temporarily for something. Does this mean, "temporarily" until we get around to write the code differently, or does it happen temporarily at runtime, or what? Just fixing the typo doesn't help much here, and I'm not sure if it should be "temporary" or "temporarily" anyway. > --- a/contrib/spi/timetravel.c > +++ b/contrib/spi/timetravel.c > @@ -51,7 +51,7 @@ static EPlan *find_plan(char *ident, EPlan **eplan, int *nplans); > * and stop_date eq INFINITY [ and update_user eq current user ] > * and all other column values as in new tuple, and insert tuple > * with old data and stop_date eq current date > - * ELSE - skip updation of tuple. > + * ELSE - skip UPDATE of tuple. > * 2. IF a delete affects tuple with stop_date eq INFINITY > * then insert the same tuple with stop_date eq current date > * [ and delete_user eq current user ] I wasn't sure if this changes the meaning of the comment slightly. An "UPDATE" in all-caps refers to an UPDATE statement, is that what's meant here? Or just updating a tuple, i.e. should this rather be "skip updating of the tuple" or "skip update of tuple"? > --- a/src/test/regress/sql/errors.sql > +++ b/src/test/regress/sql/errors.sql > @@ -2,7 +2,7 @@ > -- ERRORS > -- > > --- bad in postquel, but ok in postsql > +-- bad in postquel, but ok in PostgreSQL > select 1; This "postsql" refers to the SQL dialect of PostgreSQL, rather than PostgreSQL the project. I don't remember seeing it called "postsql" anywhere else, though. We hardly care about what was an error in postqual anyway, though, so perhaps this should be rewritten into something else entirely, like "This is not allowed by the SQL standard, but ok on PostgreSQL" (assuming that's correct, I'm not 100% sure). Or just leave it alone. Thanks for the fixes! I was particularly impressed that you caught the typo in Marcel Kornacker's surname. - Heikki
Heikki wrote: > I pushed most of these. Except for the below: > optimisation -> optimization et al. > Most of our code is written with the American spelling, > but the British spelling isn't wrong, > so I don't want to go around changing them all. Sure As you'll see, my approach is to aim for consistency. If you used en-GB 99% of the time, I'd have offered a change to enforcethat. I have a personal preference, but there's no obligation, and I understand the potential costs churn entails(I see you backported to branches). > NUL-terminated -> NULL-terminated > When we're talking about NUL-terminated strings, > NUL refers to the NUL ASCII character. NULL usually refers to a NULL pointer. This wasn't even in my original set (i.e. The dictionary I'm using didn't consider NUL to be misspelled). I ran across itwhile splitting comments out per Andres and figured I'd offer it as well. > We're probably not consistent about this, Hrm, I was going to say "that's correct, you aren't", but rereading, I realize that I'd have to review every instance inorder to prove to myself that statement. I could make the weaker argument that "nul-terminated" should be changed to eitherNUL-terminated or null-terminated . My general approach is to only make changes when I can detect an inconsistency.And I generally tend toward "majority rule". Here, I think the corpus has 4 spellings, and it sounds like it should only have two, but probably (NUL- and null-) not thetwo I picked (NULL- and null-). > but in this context, NUL-terminated isn't wrong, so let's leave them as they are. But that's OK. My goal in posting these is to encourage people to consider their code. >> Ooops -> Oops > "Oops" is more idiomatic, but this doesn't really seem worth changing. Technically oops is in dictionaries whereas the other isn't, but I understood the intent. > Maybe "Ooops" indicates a slightly bigger mistake than "oops" :-) That seemed like the intent. It's certainly not unreasonable to retain it. It's also why I generally offer a queue, so peoplecan reject families of changes. >> re-entrancy -> reentrancy > Googling around, I can see both spellings being used. Both are used, but reentrancy is in most dictionaries (and encyclopedias) and is the form that's used in instruction (certainlyit was when I studied in university, and it isn't likely to regress). It's akin to email vs e-mail. Once the dashlessform becomes accepted (within a domain [1]), it's the correct form, and the other was merely transitional. > "Re-entrancy" actually feels more natural to me, although I'm not sure which is more correct. > Let's leave them as they are. Sure >> passthru -> passthrough > "Passthrough" is clearly the correct spelling (or "pass-through"?), The former is also present in the codebase. (I didn't look for the latter, for the same reason as the previous note.) > but "passthru" seems OK in the context, as an informal shorthand. My goal is consistency. If you always spell a concept a single way, then grepping for that concept is easier and more reliable. I personally recognize quite a few flavors, because they're usable for talking to Coverity / Purify. >> - * Temporay we use TSLexeme.flags for inner use... >> + * Temporary we use TSLexeme.flags for inner use... > Looking at the code real quick, I couldn't understand the original meaning of this. Is it: > * DT_USEASIS is a temporary value we use for something. For what? > * DT_USEASIS is used temporarily for something. > Does this mean, "temporarily" until we get around to write the code differently, or does > it happen temporarily at runtime, or what? > Just fixing the typo doesn't help much here, > and I'm not sure if it should be "temporary" or "temporarily" anyway. Apparently I didn't look at this one much at all. I believe temporarily is the intended word (fwiw, I originally mis-correcteddirectly as directory, that I did spot before submitting). And probably as a runtime concept. But I'm not volunteering to fix all comments in the project ;-). After spelling fixes, I'm more likely to try actual bugs/ usability issues. I have a specific bug which bit me, but fixing that would require more effort than a spelling passand more cooperation. I tend to do a spelling pass to determine if the more expensive activity is viable. So far, theproject is welcoming :-) so, perhaps I'll manage to write the real fix... > I wasn't sure if this changes the meaning of the comment slightly. > An "UPDATE" in all-caps refers to an UPDATE statement, > is that what's meant here? Or just updating a tuple, > i.e. should this rather be "skip updating of the tuple" or "skip update of tuple"? I'm not certain. I do understand that capital UPDATE is special. This one people more familiar with the project will haveto resolve. Fwiw, if it's the former, you could omit the "of". > This "postsql" refers to the SQL dialect of PostgreSQL, I had to look up the other dialect from that line to decide it wasn't a spelling error. > rather than PostgreSQL the project. > I don't remember seeing it called "postsql" anywhere else, though. Nothing within the corpus I was changing shared that spelling, otherwise it too would have been changed :) Oddly, this specific thing feels like a Deja-vu. I wonder if I started a spelling fix series for Postgres a decade ago orsomething... > We hardly care about what was an error in postqual anyway, > though, so perhaps this should be rewritten into something else entirely, > like "This is not allowed by the SQL standard, but ok on PostgreSQL" > (assuming that's correct, I'm not 100% sure). > Or just leave it alone. I'd encourage you to find something that's meaningful and correct. > Thanks for the fixes! You're welcome. Thanks for the quick handling. Some projects take months. Or never respond. > I was particularly impressed that you caught the typo in Marcel Kornacker's surname. My tools identify both spellings as incorrect (and all possibly misspelled words are listed alphabetically), which meansthat I have the opportunity to choose a correct spelling -- generally I'll Google if I'm concerned because there isinsufficient preference within a corpus. Did you want me to submit emails for the remaining portions from https://github.com/jsoref/postgres/commits/spelling [1] https://en.m.wikipedia.org/wiki/Reentrant
On 02/06/2017 12:52 PM, Josh Soref wrote: > Did you want me to submit emails for the remaining portions from > https://github.com/jsoref/postgres/commits/spelling Ah, yes please. Post them over and I'll have a look at those as well. - Heikki
Andres Freund wrote: > On 2017-02-05 21:05:50 -0500, Josh Soref wrote: > > A complete diff would be roughly 130k. I've recently tried to submit a > > similarly sized patch to another project and it was stuck in > > moderation (typically mailing lists limit attachments to around 40k). > > IIRC 130k shouln't get you stuck in filters yet - if you're concerned > you can gzip the individual patches. Our limit for pgsql-hackers is 1 MB. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2017-02-06 10:40, Heikki Linnakangas wrote: > On 02/06/2017 04:50 AM, Josh Soref wrote: >> NUL-terminated -> NULL-terminated > > When we're talking about NUL-terminated strings, NUL refers to the NUL > ASCII character. NULL usually refers to a NULL pointer. We're probably > not consistent about this, but in this context, NUL-terminated isn't > wrong, so let's leave them as they are. The C standard talks about how "a byte with all bits set to 0, called the null character" is used to "terminate a character string"; it mentions '\0' as "commonly used to represent the null character"; and it also talks about when snprintf() produces "null-terminated output". It never mentions ASCII in this context; quite intentionally it avoids assuming ASCII at all, so that a standard-compliant C implementation may co-exist with other encodings (like EBCDIC).
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On 2/6/17 06:03, Heikki Linnakangas wrote: > On 02/06/2017 12:52 PM, Josh Soref wrote: >> Did you want me to submit emails for the remaining portions from >> https://github.com/jsoref/postgres/commits/spelling > > Ah, yes please. Post them over and I'll have a look at those as well. This thread is in the commit fest, but I think there is no current patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mar 1, 2017 9:06 AM, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com> wrote:
On 2/6/17 06:03, Heikki Linnakangas wrote:This thread is in the commit fest, but I think there is no current patch.
> Ah, yes please. Post them over and I'll have a look at those as well.
I sent email on the 6th with a number of patches as attachments...
Josh Soref wrote: > - else if (ControlFile->state == DB_SHUTDOWNING) > + else if (ControlFile->state == DB_SHUTTINGDOWN) SHUTDOWNING and SHUTDOWNED are typos first introduced by hacker emeritus Vadim Mikheev in 1999 together with WAL, commit 47937403676d. It goes to show that not every PG hacker is a learned English speaker. I see no reason to change the spelling. > - List *epq_arowmarks; > + List *epq_arowMarks; I'd not change this one either. Of the large set of words run together in our source, this is one of the easiest ones to read. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Josh Soref wrote: > > <!-- > 2016-04-08 [848ef42bb] Add the "snapshot too old" feature > -2016-05-06 [2cc41acd8] Fix hash index vs "snapshot too old" problemms > +2016-05-06 [2cc41acd8] Fix hash index vs "snapshot too old" problems > 2016-05-06 [7e3da1c47] Mitigate "snapshot too old" performance regression on NU > 2016-08-03 [3e2f3c2e4] Prevent "snapshot too old" from trying to return pruned > 2016-08-07 [9ee1cf04a] Fix TOAST access failure in RETURNING queries. Note that these SGML comment lines are references to git commits, so the typos are present in the commit messages. These should not be changed. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 6 February 2017 at 15:50, Josh Soref <jsoref@gmail.com> wrote: > It's now split more or less to your suggestion: > https://github.com/jsoref/postgres/commits/spelling - * Note that this algrithm is know to not be very effective (O(N^2)) + * Note that this algorithm is know to not be very effective (O(N^2)) know should be known. Perhaps you can include if you have a followup patch. -- David Rowley http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 3/1/17 09:12, Josh Soref wrote: > On Mar 1, 2017 9:06 AM, "Peter Eisentraut" > <peter.eisentraut@2ndquadrant.com > <mailto:peter.eisentraut@2ndquadrant.com>> wrote: > > On 2/6/17 06:03, Heikki Linnakangas wrote: > > Ah, yes please. Post them over and I'll have a look at those as well. > > This thread is in the commit fest, but I think there is no current > patch. > > > I sent email on the 6th with a number of patches as attachments... Yes, some of that was committed, and some comments were offered. If there is more to do, please send a rebased patch set. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I can easily drop the shutdown* items; The reason for rowMarks is consistency with lr_arowMarks. I'll tentatively exclude that from any resubmission I make tonight...
I understood that they were git commits. I could have excluded the
file but opted not to in case people were willing to take a small
drift -- the SHAs are what someone needs to review the commit, and
personally, I'd rather read something without typos than with --
especially in a summary. But, I'll tentatively exclude the lines
containing SHAs.
do you want:
@@ -3116,7 +3116,7 @@ 2016-02-17 [a5c43b886] Add new system vi
<para> This view exposes the same information available from
- the <application>pg_config</> comand-line utility,
+ the <application>pg_config</> command-line utility, namely assorted compile-time configuration
informationfor <productname>PostgreSQL</>. </para>
or should I exclude the file entirely?
I'll include it.
Peter Eisentraut wrote: > Yes, some of that was committed, and some comments were offered. If > there is more to do, please send a rebased patch set. Conflicting comments were offered. And Heikki requested I send along the remainder. Which I did. Only one of those patches would have been impacted by the conflicting comments. The patches in this thread still applied today: spelling: comments -- conflicting comments about NUL/NULL spelling: strings -- no comments / applied cleanly spelling: variables -- no comments / applied cleanly spelling: misc -- no comments / applied cleanly spelling: api -- no comments until today / applied cleanly, may end up being dropped I want to thank Heikki for the initial acceptance and Alvaro and David for their additional comments. I'm not going to send a new submission before tonight. If anyone else wants to make comments before I resubmit, I welcome them... For reference, my current work queue is here: https://github.com/postgres/postgres/compare/master...jsoref:spelling-words?expand=1 The rebased version of the patches that were submitted but ignored are here: https://github.com/postgres/postgres/compare/master...jsoref:spelling?expand=1 I haven't updated them to reflect my work queue, as selecting things into those bundles is a not particularly pleasant, and thus a step I want to do as few times as possible. One thing that would be helpful is if someone could comment on: https://github.com/jsoref/postgres/commit/9050882d601134ea1ba26f77ce5f1aaed75418de -#undef SH_ITERTOR +#undef SH_ITERATOR It's unclear to me what that line is/was doing. It's possible that it could be removed entirely instead of having its spelling changed. If the line is trying to guard against a previous version of the code, which is no longer active, then it deserves a comment.
Josh Soref wrote: > One thing that would be helpful is if someone could comment on: > https://github.com/jsoref/postgres/commit/9050882d601134ea1ba26f77ce5f1aaed75418de > -#undef SH_ITERTOR > +#undef SH_ITERATOR > > It's unclear to me what that line is/was doing. It's possible that it > could be removed entirely instead of having its spelling changed. > If the line is trying to guard against a previous version of the code, > which is no longer active, then it deserves a comment. AFAICS this is a bug. This file can potentially be included several times by the same C source, and it defines SH_ITERATOR every time. The second time it needs to be #undef'ed prior, which this line is supposed to do but fails because of the typo. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2017-03-01 14:40:26 -0300, Alvaro Herrera wrote: > Josh Soref wrote: > > > One thing that would be helpful is if someone could comment on: > > https://github.com/jsoref/postgres/commit/9050882d601134ea1ba26f77ce5f1aaed75418de > > -#undef SH_ITERTOR > > +#undef SH_ITERATOR > > > > It's unclear to me what that line is/was doing. It's possible that it > > could be removed entirely instead of having its spelling changed. > > If the line is trying to guard against a previous version of the code, > > which is no longer active, then it deserves a comment. > > AFAICS this is a bug. This file can potentially be included several > times by the same C source, and it defines SH_ITERATOR every time. The > second time it needs to be #undef'ed prior, which this line is supposed > to do but fails because of the typo. Indeed. Fixed, thanks for noticing. Andres
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers