1647104 - Fix typos and real errors with codespell (except OpenPGP code)

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

There are some real bugs like:
https://searchfox.org/comm-central/source/chat/content/otr-auth.xhtml#10
https://searchfox.org/comm-central/source/mailnews/build/nsMailModule.cpp#737
https://searchfox.org/comm-central/source/mail/base/content/tabmail.js#124
https://searchfox.org/comm-central/source/mail/extensions/openpgp/content/modules/httpProxy.jsm#30

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1647104-calendar.patch [checked in m-c 79, comment #13] [checked in comm-beta 78]

Calendar isn't so bad this time, but the rest is pretty bad. More patches coming.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1647104-real-bugs.patch [checked in m-c 79, comment #6] [checked in comm-beta 78]

These are pretty bad. No idea what effect fixing the typos will have.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/be8438cf7ca1
fix typos with codespell (manual corrections). rs=comment-only,typo-fix DONTBUILD

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: ignore.txt - Words we ignore

For the record, this is how it's done:
pip install codespell
codespell -w -q4 --skip="./.hg,mozilla,obj-x86_64-pc-mingw32,.mab,third_party,suite,mime-torture,matrix,libical,ical.js,blocklist.xml,twitter-text.jsm,messages.json,.png,.ico,.gif,.env,.SHA1,.SHA256,.SHA384,.SHA512,.db,.dat,.p12,.xpi,.bmp,.wav,.icns,.ogv,.oeaccount,.sfx,.pyc",TeXZilla.js,.msf,dsstore,test-UTF-16*.*,utf16_addressbook.csv --ignore-words=ignore.txt

Apart from the patches presented for review, there's a big one coming with all the comment fixes.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9158024 [details] [diff] [review]
1647104-real-bugs.patch  [checked in m-c 79, comment #6] [checked in comm-beta 78]

Review of attachment 9158024 [details] [diff] [review]:
-----------------------------------------------------------------

Ah yes there would be some hidden bugs in there.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/109b433b19ff
Fix real bugs found with codespell. r=mkmelin

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9ca3551392f6
Fix typos in chat, common, db, editor, mail, mailnews found with codespell. rs=comment-only,typo-fix DONTBUILD

Comment 8

[Jorg K (CEST = GMT+2)]

Only the calendar part left.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 9158024 [details] [diff] [review]
1647104-real-bugs.patch  [checked in m-c 79, comment #6] [checked in comm-beta 78]

Review of attachment 9158024 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/build/nsMailModule.cpp
@@ +738,1 @@
>  

Thanks for finding this bug.
It was added by Patrick in 2012 with this changeset:
https://hg.mozilla.org/comm-central/rev/f0e7508fd586861d18f0de9d43eb62da31503cbe

In addition to this attempt to register a handler for multipart/encrypted (which failed because of the type), there was an additional class registration further down in the patch.
"@mozilla.org/mimecth;1?type=multipart/encrypted"

In initial testing, fixing the typo doesn't change which handler is called for this content type.

Comment 10

[Jorg K (CEST = GMT+2)]

Let's remove what isn't needed.

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9158022 [details] [diff] [review]
1647104-calendar.patch [checked in m-c 79, comment #13] [checked in comm-beta 78]

I really hope the code is in better condition than the comments.

Comment 12

[Jorg K (CEST = GMT+2)]

There are compilers and linting ... and bugs ;-)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c925c65b9784
Fix typos in calendar. r=darktrojan

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9158024 [details] [diff] [review]
1647104-real-bugs.patch  [checked in m-c 79, comment #6] [checked in comm-beta 78]

All the patches here should do to beta to avoid endless uplift merge problems in ESR 78. Rob, I'll handle this myself. This should be done after bug 1643561 goes to beta. I haven't tried, but I think some/most of the four changeset won't apply without it.

Comment 15

[Kai Engert (:KaiE:)]

The changes to the comments make merging much more difficult, if not disruptive. For sane development, while we're trying to complete the development of OpenPGP on the 78.x branch, I'd prefer the changes to the mail/extensions directory either applied to comm-beta, or backed out.

Comment 16

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:KaiE:) from comment #15)

The changes to the comments make merging much more difficult, if not disruptive. For sane development, while we're trying to complete the development of OpenPGP on the 78.x branch, I'd prefer the changes to the mail/extensions directory either applied to comm-beta, or backed out.

I'm referring to this commit, which was checked in without review/coordination:

(In reply to Pulsebot from comment #7)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9ca3551392f6
Fix typos in chat, common, db, editor, mail, mailnews found with codespell.
rs=comment-only,typo-fix DONTBUILD

Comment 17

[Jorg K (CEST = GMT+2)]

All changesets should go to beta as per comment #14. Patches which no longer apply due to the comment changes, can usually be edited to make them apply again.

Comment 18

[Kai Engert (:KaiE:)]

Attachment: 1647104-mail-ext-comment-changes-v2.patch [unnecessary]

Reopening, to clarify what we do about the large comment-only commit (comment 7, comment 16).

The whole patch doesn't apply to beta, so to achieve sync of the openpgp code, we could look at the subset of this commit that touches mail/extensions/ (am-e2e and openpgp).

I'll attach a subset of that commit, for beta consideration.

Comment 19

[Kai Engert (:KaiE:)]

Magnus, should the commit from comment 7 be backed out, or should we try to merge the suggested parts to beta?

Comment 20

[Magnus Melin [:mkmelin]]

I was just going to suggest that as the best course of action. Then we can re-land it with less work later.

Comment 21

[Jorg K (CEST = GMT+2)]

The bug is fixed, no need to reopen. I can handle beta tonight, we're only talking about a few typos here (which according to current rules can be fixed without further review). Backing it out doesn't help since we could never fix trunk then. Deal?

Comment 22

[Jorg K (CEST = GMT+2)]

Relanding later is not an option since you'll have merge conflicts forever. Let me fix it.

Comment 23

[Kai Engert (:KaiE:)]

Jörg, please follow the review process, and please follow majority decisions.

I agree with Magnus about the backout.
Please don't commit anything now without review, to not create more confusion.

Comment 24

[Jorg K (CEST = GMT+2)]

Well, then back out the OpenPGP changes. There were typos in messages, too :-(

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bcdba0d3f13f
Backed out changeset 9ca3551392f6. a=mkmelin

Comment 26

[Kai Engert (:KaiE:)]

Backed out.

Jörg, doing this kind of large changes, while we're in the last phase of release preparation, and backporting to a release branch, is really disruptive and creates extra work. It shouldn't be done last minute without coordination.

You could try to ask to land this again later.
You could split the patch into an openpgp part and a non-openpgp part.

Following the review and approval process will make it easier to avoid merge conflicts and to track the changes in order.

I consider the comment changes not sufficiently important to justify extra merging effort.
Thanks for finding the typo in the user facing string in enigmail.ftl, I will suggest that change for Khushil to pick up in bug 1638822.

Comment 27

[Wayne Mery (:wsmwk)]

Comment on attachment 9158024 [details] [diff] [review]
1647104-real-bugs.patch  [checked in m-c 79, comment #6] [checked in comm-beta 78]

delaying approval, per comment 26

Comment 28

[Kai Engert (:KaiE:)]

Wayne, there were multiple things checked in as part of this patch.

My recent comments (including comment 26) were about the large commit from comment 7, which didn't go through review process. and which we have backed out in the meantime.

The small patch that asked for beta approval (attachment 9158024 [details] [diff] [review]) should be OK to land in comm-beta, it's fixes some real issues. On the other hand, we don't know yet if it has side effects.

Comment 29

[Jorg K (CEST = GMT+2)]

As the code manager for three major releases, 52, 60 and 68 and the person who handled the reformatting of the entire C++ code base, various global clean-ups and the one who coordinated backport of the linting/prettier to TB 68, I can tell you that what you did here created an unfortunate situation where three changesets in a bug were landed, and a fourth one backed out. The proper way forward would have been to move everything forward to beta while TB 78 isn't at ESR yet. Alternatively backing out the OpenPGP changes would have been an option.

For comment-only patches landed by a Thunderbird peer I refer you to https://mail.mozilla.org/pipermail/tb-planning/2016-June/004834.html. This was about adding documentation, surely that covers typo fixes in comments.

Any objection to relanding the non-OpenPGP hunks?

Comment 30

[Kai Engert (:KaiE:)]

(In reply to Jorg K (CEST = GMT+2) from comment #29)

Any objection to relanding the non-OpenPGP hunks?

I think you should go through the review process at this time.

I believe Magnus has already said no to this option in comment 20. He suggested to land at a later time, when it will mean less work.

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to Kai Engert (:KaiE:) from comment #15)

For sane development, while we're trying to complete the development of OpenPGP on the 78.x branch,
I'd prefer the changes to the mail/extensions directory either applied to comm-beta, or backed out.

So initially you requested the hunks that affect OpenPGP development to be backout out, so now why doesn't everything need to be back out leaving the bug half done?

(In reply to Magnus Melin [:mkmelin] from comment #20)

I was just going to suggest that as the best course of action. Then we can re-land it with less work later.

That's fine for the OpenPGP hunks but will create more work otherwise. I'm not sure what Magnus referred to, the entire patch or just the parts that got in the way of the OpenPGP development.

Take note that in bug 1643561 we just hit 524 files with what is basically white-space management (creating a one-year uplift disaster if this is not mitigated right now). Surely we can tolerate fixing typos in 78 files here. What am I missing?

So unless I hear objection form a Thunderbird peer, I'll reland the non-OpenPGP hunks (mail/extensions/*) by COB on Wednesday. As I outlined previously, landing an automatically generated patch which I myself reviewed line by line doesn't need to waste anyone else's time any more.

Comment 32

[Jorg K (CEST = GMT+2)]

Attachment: be8438cf7ca1564fbb8132af051e2978a6e4bc15.patch - manual fixes [checked in m-c 79, comment #13] [checked in comm-beta 78]

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: 1647104-mail-mailnews-etc.patch, landed in comment #35

If anyone thinks that another review is necessary here, may they go ahead. This excludes OpenPGP code in mail/extensions.

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9158325 [details] [diff] [review]
1647104-mail-mailnews-etc.patch, landed in comment #35

Review of attachment 9158325 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/55c81fc7e8d5
Fix typos in chat, common, db, editor, mail, mailnews found with codespell. rs=comment-only,typo-fix r=mkmelin DONTBUILD

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 9158022 [details] [diff] [review]
1647104-calendar.patch [checked in m-c 79, comment #13] [checked in comm-beta 78]

Comment only. Should go to beta to avoid later merge issues.

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 9158024 [details] [diff] [review]
1647104-real-bugs.patch  [checked in m-c 79, comment #6] [checked in comm-beta 78]

Fixes various bugs caused by typos.

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 9158321 [details] [diff] [review]
be8438cf7ca1564fbb8132af051e2978a6e4bc15.patch - manual fixes [checked in m-c 79, comment #13] [checked in comm-beta 78]

Comment only. Should go to beta to avoid later merge issues.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 9158325 [details] [diff] [review]
1647104-mail-mailnews-etc.patch, landed in comment #35

Comment only. Should go to beta to avoid later merge issues.

Comment 40

[Jorg K (CEST = GMT+2)]

Rob, I can handle the beta uplift. This should be done after bug 1643561 goes to beta (who manages that?). That bug hit ~550 C++ files and will make uplifts during the TB 78 ESR cycle a pain.

Comment 41

[Wayne Mery (:wsmwk)]

Comment on attachment 9158024 [details] [diff] [review]
1647104-real-bugs.patch  [checked in m-c 79, comment #6] [checked in comm-beta 78]

Thanks.
Approved for beta

Comment 42

[Wayne Mery (:wsmwk)]

Comment on attachment 9158022 [details] [diff] [review]
1647104-calendar.patch [checked in m-c 79, comment #13] [checked in comm-beta 78]

Thanks.
Approved for beta

Comment 43

[Wayne Mery (:wsmwk)]

Comment on attachment 9158321 [details] [diff] [review]
be8438cf7ca1564fbb8132af051e2978a6e4bc15.patch - manual fixes [checked in m-c 79, comment #13] [checked in comm-beta 78]

Thanks.
Approved for beta

Comment 44

[Wayne Mery (:wsmwk)]

Comment on attachment 9158325 [details] [diff] [review]
1647104-mail-mailnews-etc.patch, landed in comment #35

Thanks.
Approved for beta

Comment 45

[Kai Engert (:KaiE:)]

The patch from comment 43 is simple, and applies.

The patch from comment 44 has multiple merge failures.
(maybe I didn't try in the correct order)

(talking about beta)

Comment 46

[Jorg K (CEST = GMT+2)]

See comment #40.

Comment 47

[Kai Engert (:KaiE:)]

Comment on attachment 9158022 [details] [diff] [review]
1647104-calendar.patch [checked in m-c 79, comment #13] [checked in comm-beta 78]

https://hg.mozilla.org/releases/comm-beta/rev/d033c8f1f3b8fe77ccb74a9958ca761f52e5f1ed

Comment 48

[Kai Engert (:KaiE:)]

Comment on attachment 9158024 [details] [diff] [review]
1647104-real-bugs.patch  [checked in m-c 79, comment #6] [checked in comm-beta 78]

https://hg.mozilla.org/releases/comm-beta/rev/85f78035c5db080f36dacf07e950c7482470f32e

Comment 49

[Kai Engert (:KaiE:)]

Comment on attachment 9158321 [details] [diff] [review]
be8438cf7ca1564fbb8132af051e2978a6e4bc15.patch - manual fixes [checked in m-c 79, comment #13] [checked in comm-beta 78]

https://hg.mozilla.org/releases/comm-beta/rev/a78e35148031051a5b2353ee389eae2508c79c84

Comment 50

[Kai Engert (:KaiE:)]

I've landed three of these patches, because two of them touched pgp, and I already saw patches not merging.

Patch 4 is still pending, and has a dependency on bug 1643561 apparently.

Comment 51

[Jorg K (CEST = GMT+2)]

Thanks, that's appreciated. We'll coordinate the 4th part with bug 1643561.

Comment 52

[Jorg K (CEST = GMT+2)]

Re. part 4: After importing

https://hg.mozilla.org/try-comm-central/rev/08d74fbc96be428ed9b112b0b1dd891a4831d1d3
https://hg.mozilla.org/try-comm-central/rev/05655ae8bba6c8eee4948d263e4820fa1cd77cb6

https://hg.mozilla.org/comm-central/rev/55c81fc7e8d5 applies if you push https://hg.mozilla.org/comm-central/rev/26784c9a7a6c9314f2bb3893a2d2b0788d3e3939 before it.

Comment 53

[Rob Lemley [:rjl]]

Thunderbird 78.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/7ae7f4fa2a7b