343027 - (calsynctrees) Trunk and branch are out of sync

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

A diff between calendar/ on the 1_8 branch and calendar/ on the trunk shows several changes.  We need to keep this changeset as small as possible if we're to keep cross-commit sane.

Comment 1

[Joey Minta]

Attachment: diff between branch and trunk

This is the diff for calendar/ between branch and trunk.  I see several sets of changes.  If you're responsible for one or more of them, please either justify the divergence or fix it.

1.) nsIClassInfoImpl - this was a tree-wide change
2.) proxy connection changes - were there interfaces that changed underneath here? lilmatt?
3.) whitespace in calRecurrenceDate.cpp
4.) windows installer bits - lilmatt?
5.) -lt locale on branch - this was discussed in a status meeting
6.) calendar.xpi only stuff - mostafah? is this something we want to fix? or cvs remove?
7.) menuOverlay.xul
8.) spelling fixes - tree-wide changes, we should fix these on our own
9.) resources/jar.mn!!! this looks urgent
10.) sunbird/app/Makefile.in - installer related? lilmatt?
11.) prefs.js - this was a tree-wide change by the illustrious intern mwu.  I think it'll land on branch soon.
12.) IDC_SELECTANCHOR  - tree-wide change
13.) whitespace in sunbird's calendar.xul

If you are responsible for one of these, please claim it.  I'll go through later today and start looking at CVS blame for those that haven't been claimed.

Comment 2

[Matthew (lilmatt) Willis]

(In reply to comment #1)
> 2.) proxy connection changes - were there interfaces that changed underneath
> here? lilmatt?
This is correct. They should be different.

> 4.) windows installer bits - lilmatt?
This should be resolved at this point if rob_strong has landed all his stuff.

> 5.) -lt locale on branch - this was discussed in a status meeting
This is correct.

> 6.) calendar.xpi only stuff - mostafah? is this something we want to fix? or
> cvs remove?
cvs rm

> 9.) resources/jar.mn!!! this looks urgent
Will patch.

> 10.) sunbird/app/Makefile.in - installer related? lilmatt?
The first discrepancy is Cairo related.

The second was bug 314927, checked in on trunk only by gavin. Branch browser/app/Makefile.in also differs from trunk at this point, so I'm assuming this is correct behaviour. (?)

> 11.) prefs.js - this was a tree-wide change by the illustrious intern mwu.  I
> think it'll land on branch soon.
This has already been fixed.

> 13.) whitespace in sunbird's calendar.xul
Will patch.

Comment 3

[Matthew (lilmatt) Willis]

Attachment: trunk patch

trunk patch per my comment

Comment 4

[Matthew (lilmatt) Willis]

Attachment: branch patch

branch patch per my comment

Comment 5

[Joey Minta]

Comment on attachment 227979 [details] [diff] [review]
trunk patch

r=jminta

Comment 6

[Matthew (lilmatt) Willis]

trunk patch checked in.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Note that for stuff like the proxy changes, we need to switch that code to use #ifdef  MOZILLA_1_8_BRANCH, so that cross-commit will continue to work.

Comment 8

[Joey Minta]

Comment on attachment 227980 [details] [diff] [review]
branch patch

r=jminta  Can you (or someone) give an update of what's left?

Comment 9

[Matthew (lilmatt) Willis]

(In reply to comment #8)
> (From update of attachment 227980 [details] [diff] [review] [edit])
> r=jminta

branch patch checked in

Comment 10

[Matthew (lilmatt) Willis]

Attachment: updated diff between branch and trunk

(In reply to comment #8)
> Can you (or someone) give an update of what's left?

Attached.

Comment 11

[Matthew (lilmatt) Willis]

Attachment: patch set 2 - branch patch

Comment 12

[Matthew (lilmatt) Willis]

Attachment: patch set 2 - trunk patch

Comment 13

[Matthew (lilmatt) Willis]

Attachment: diff after patch set 2 is applied

Comment 14

[Reed Loden [:reed]]

The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!

Comment 15

[Joey Minta]

Comment on attachment 229915 [details] [diff] [review]
patch set 2 - branch patch

So... when you create a patch, bugzilla gives you this nice big space to describe what it is you're changing and why.  Short of grabbing a second monitor and sitting with the branch and trunk patches side-by-side.  Please describe what these patches are changing, how they relate, etc.

Comment 16

[Matthew (lilmatt) Willis]

(In reply to comment #15)
> (From update of attachment 229915 [details] [diff] [review] [edit])
> So... when you create a patch, bugzilla gives you this nice big space to
> describe what it is you're changing and why.

Whiner. ;)

Much of each patch is either whitespace or spelling changes that got missed the first time.
The real meaty part of the patch is wrapping the proxy stuff in #ifdef MOZILLA_1_8_BRANCH like dmose asked for.

I didn't touch the #include "nsIClassInfoImpl.h" issues, but if we want those wrapped in #ifndef MOZILLA_1_8_BRANCH we can do that on the next cut.

I also enabled the preprocessor on the .js file with the new #ifdef, and removed an $Id expansion keyword from one file. It's unnecessary, and just causes diff noise.

Comment 17

[Joey Minta]

Comment on attachment 229915 [details] [diff] [review]
patch set 2 - branch patch

+    // We need to set the timezone of the above created property manually.
+    // The only reason this is necessary is because icalproperty_set_value()
It looks to me like this change is in both patch sets.  Please, please, please, don't fix other bugs in patches designed for other purposes.  It makes reviewing exponentially harder.  Not to mention the fact that it ruins cvs blame when you check in with an unrelated comment/bug #.

+    // We need to transfer the timezone from the icalproperty to the
+    // calIDateTime object manually.
+    // The only reason this is necessary is because icalproperty_get_value()
+    // has the somewhat non-intuitive behavior of not handling the TZID
+    // parameter automagically.
     const char *tzid = icalproperty_get_parameter_as_string(prop, "TZID");
and again.

 
-## $Id: mozilla.in,v 1.7.2.2 2006/01/06 01:40:30 dmose%mozilla.org Exp $
 ## 
 ## Usage:
 ##
and yet again.

r=jminta if you remove those extra changes.

Comment 18

[Joey Minta]

Comment on attachment 229916 [details] [diff] [review]
patch set 2 - trunk patch

same comments as the branch patch.

Comment 19

[Matthew (lilmatt) Willis]

Outcome of voice iChat with jminta: 

1. Before committing calRecurrenceDate.cpp, I'll back out the comment cleanup to the minimum required to fix the discrepancy.

2. In mozilla.in, I _will_ delete the $Id$ line. While CVS and cross-commit may be able to handle the keyword expansion, it's superfluous and can only cause us pain.

Comment 20

[Matthew (lilmatt) Willis]

patch set 2 committed on branch and trunk

Comment 21

[Matthew (lilmatt) Willis]

Attachment: patch set 3 - branch patch

The last of it.

Comment 22

[Matthew (lilmatt) Willis]

Attachment: patch set 3 - trunk patch

The last of it.

Comment 23

[Matthew (lilmatt) Willis]

Attachment: diff after patch set 3 is applied

Note that the connectionPref.js file should be deleted. This is awaiting final decisions on where to place preferences for Sunbird.

Comment 24

[Matthew (lilmatt) Willis]

Patch set 3 checked in on branch and trunk

-> FIXED