Fix typos and real errors with codespell (except OpenPGP code)
Categories
(Thunderbird :: General, defect)
Tracking
(thunderbird78 fixed)
Tracking | Status | |
---|---|---|
thunderbird78 | --- | fixed |
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
Details
Attachments
(5 files, 1 obsolete file)
12.83 KB,
patch
|
darktrojan
:
review+
wsmwk
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
3.62 KB,
patch
|
mkmelin
:
review+
wsmwk
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
497 bytes,
text/plain
|
Details | |
7.24 KB,
patch
|
wsmwk
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
88.09 KB,
patch
|
mkmelin
:
review+
wsmwk
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•3 years ago
|
||
Calendar isn't so bad this time, but the rest is pretty bad. More patches coming.
Assignee | ||
Comment 2•3 years ago
|
||
These are pretty bad. No idea what effect fixing the typos will have.
Assignee | ||
Updated•3 years ago
|
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/be8438cf7ca1 fix typos with codespell (manual corrections). rs=comment-only,typo-fix DONTBUILD
Assignee | ||
Comment 4•3 years ago
|
||
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•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/109b433b19ff
Fix real bugs found with codespell. r=mkmelin
Pushed by mozilla@jorgk.com: 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 9•3 years ago
|
||
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.
Assignee | ||
Comment 10•3 years ago
|
||
Let's remove what isn't needed.
Comment 11•3 years ago
|
||
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.
Assignee | ||
Comment 12•3 years ago
|
||
There are compilers and linting ... and bugs ;-)
Comment 13•3 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c925c65b9784
Fix typos in calendar. r=darktrojan
Assignee | ||
Comment 14•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
(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 mozilla@jorgk.com:
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
Assignee | ||
Comment 17•3 years ago
|
||
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•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 19•3 years ago
|
||
Magnus, should the commit from comment 7 be backed out, or should we try to merge the suggested parts to beta?
Comment 20•3 years ago
|
||
I was just going to suggest that as the best course of action. Then we can re-land it with less work later.
Assignee | ||
Comment 21•3 years ago
|
||
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?
Assignee | ||
Comment 22•3 years ago
|
||
Relanding later is not an option since you'll have merge conflicts forever. Let me fix it.
Comment 23•3 years ago
|
||
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.
Updated•3 years ago
|
Assignee | ||
Comment 24•3 years ago
|
||
Well, then back out the OpenPGP changes. There were typos in messages, too :-(
Comment 25•3 years ago
|
||
Pushed by kaie@kuix.de: https://hg.mozilla.org/comm-central/rev/bcdba0d3f13f Backed out changeset 9ca3551392f6. a=mkmelin
Comment 26•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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.
Assignee | ||
Comment 29•3 years ago
|
||
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?
Assignee | ||
Updated•3 years ago
|
Comment 30•3 years ago
|
||
(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.
Assignee | ||
Comment 31•3 years ago
|
||
(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.
Assignee | ||
Comment 32•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 33•3 years ago
|
||
If anyone thinks that another review is necessary here, may they go ahead. This excludes OpenPGP code in mail/extensions.
Assignee | ||
Updated•3 years ago
|
Comment 34•3 years ago
|
||
Comment on attachment 9158325 [details] [diff] [review] 1647104-mail-mailnews-etc.patch, landed in comment #35 Review of attachment 9158325 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Assignee | ||
Updated•3 years ago
|
Comment 35•3 years ago
|
||
Pushed by mozilla@jorgk.com:
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
Assignee | ||
Comment 36•3 years ago
|
||
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.
Assignee | ||
Comment 37•3 years ago
|
||
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.
Assignee | ||
Comment 38•3 years ago
|
||
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.
Assignee | ||
Comment 39•3 years ago
|
||
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.
Assignee | ||
Comment 40•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Comment 41•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
Comment on attachment 9158325 [details] [diff] [review] 1647104-mail-mailnews-etc.patch, landed in comment #35 Thanks. Approved for beta
Comment 45•3 years ago
•
|
||
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)
Assignee | ||
Comment 46•3 years ago
|
||
See comment #40.
Comment 47•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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.
Assignee | ||
Comment 51•3 years ago
|
||
Thanks, that's appreciated. We'll coordinate the 4th part with bug 1643561.
Updated•3 years ago
|
Assignee | ||
Comment 52•3 years ago
|
||
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•3 years ago
|
||
bugherderuplift |
Thunderbird 78.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/7ae7f4fa2a7b
Updated•3 years ago
|
Description
•