Closed Bug 1647104 Opened 2 months ago Closed 2 months ago

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

Categories

(Thunderbird :: General, defect)

defect

Tracking

(thunderbird78 fixed)

RESOLVED FIXED
Thunderbird 79.0
Tracking Status
thunderbird78 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

Details

Attachments

(5 files, 1 obsolete file)

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

Assignee: nobody → jorgk-bmo
Status: NEW → ASSIGNED
Attachment #9158022 - Flags: review?(geoff)

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

Attachment #9158024 - Flags: review?(mkmelin+mozilla)
Keywords: leave-open
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

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 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.
Attachment #9158024 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → Thunderbird 79.0

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

Only the calendar part left.

Keywords: leave-open
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.

Let's remove what isn't needed.

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.
Attachment #9158022 - Flags: review?(geoff) → review+

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

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c925c65b9784
Fix typos in calendar. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
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.
Attachment #9158024 - Flags: approval-comm-beta?

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.

(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

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.

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.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

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

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(mkmelin+mozilla)

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?

Status: REOPENED → RESOLVED
Closed: 2 months ago2 months ago
Resolution: --- → FIXED

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

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.

Attachment #9158151 - Attachment description: 1647104-mail-ext-comment-changes-v2.patch → 1647104-mail-ext-comment-changes-v2.patch [unnecessary]
Attachment #9158151 - Attachment is obsolete: true

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

Pushed by kaie@kuix.de:
https://hg.mozilla.org/comm-central/rev/bcdba0d3f13f
Backed out changeset 9ca3551392f6. a=mkmelin

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 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
Attachment #9158024 - Flags: approval-comm-beta? → approval-comm-beta-

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.

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?

Summary: Fix typos and real errors with codespell → Fix typos and real errors with codespell (except OpenPGP code)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

(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.

(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.

Attachment #9158022 - Attachment description: 1647104-calendar.patch → 1647104-calendar.patch, landed in comment #13
Attachment #9158024 - Attachment description: 1647104-real-bugs.patch → 1647104-real-bugs.patch, landed in comment #6

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

Attachment #9158325 - Flags: review?(mkmelin+mozilla)
Attachment #9158325 - Flags: review?(geoff)
Attachment #9158321 - Attachment is patch: true
Comment on attachment 9158325 [details] [diff] [review]
1647104-mail-mailnews-etc.patch, landed in comment #35

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

LGTM
Attachment #9158325 - Flags: review?(mkmelin+mozilla)
Attachment #9158325 - Flags: review?(geoff)
Attachment #9158325 - Flags: review+

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

Status: REOPENED → RESOLVED
Closed: 2 months ago2 months ago
Resolution: --- → FIXED
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.
Attachment #9158022 - Flags: approval-comm-beta?
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.
Attachment #9158024 - Flags: approval-comm-beta- → approval-comm-beta?
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.
Attachment #9158321 - Flags: approval-comm-beta?
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.
Attachment #9158325 - Flags: approval-comm-beta?

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.

Attachment #9158325 - Attachment description: 1647104-mail-mailnews-etc.patch → 1647104-mail-mailnews-etc.patch, landed in comment #35
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
Attachment #9158024 - Flags: approval-comm-beta? → approval-comm-beta+
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
Attachment #9158022 - Flags: approval-comm-beta? → approval-comm-beta+
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
Attachment #9158321 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 9158325 [details] [diff] [review]
1647104-mail-mailnews-etc.patch, landed in comment #35

Thanks.
Approved for beta
Attachment #9158325 - Flags: approval-comm-beta? → approval-comm-beta+

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 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
Attachment #9158022 - Attachment description: 1647104-calendar.patch, landed in comment #13 → 1647104-calendar.patch [checked in m-c 79, comment #13] [checked in comm-beta 78]
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
Attachment #9158024 - Attachment description: 1647104-real-bugs.patch, landed in comment #6 → 1647104-real-bugs.patch [checked in m-c 79, comment #6] [checked in comm-beta 78]
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
Attachment #9158321 - Attachment description: be8438cf7ca1564fbb8132af051e2978a6e4bc15.patch - manual fixes, landed in comment #3 → be8438cf7ca1564fbb8132af051e2978a6e4bc15.patch - manual fixes [checked in m-c 79, comment #13] [checked in comm-beta 78]

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.

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

You need to log in before you can comment on or make changes to this bug.