Bug 343027 (calsynctrees)

Trunk and branch are out of sync

VERIFIED FIXED

Status

Calendar
Internal Components
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: Joey Minta, Assigned: Matthew (lilmatt) Willis)

Tracking

Details

Attachments

(9 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 227447 [details] [diff] [review]
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.
(Assignee)

Comment 2

12 years ago
(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.


(Assignee)

Comment 3

12 years ago
Created attachment 227979 [details] [diff] [review]
trunk patch

trunk patch per my comment
Attachment #227979 - Flags: first-review?(jminta)
(Assignee)

Comment 4

12 years ago
Created attachment 227980 [details] [diff] [review]
branch patch

branch patch per my comment
Attachment #227980 - Flags: first-review?(jminta)
(Reporter)

Comment 5

12 years ago
Comment on attachment 227979 [details] [diff] [review]
trunk patch

r=jminta
Attachment #227979 - Flags: first-review?(jminta) → first-review+
(Assignee)

Comment 6

12 years ago
trunk patch checked in.

Comment 7

12 years ago
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.
(Reporter)

Comment 8

12 years ago
Comment on attachment 227980 [details] [diff] [review]
branch patch

r=jminta  Can you (or someone) give an update of what's left?
Attachment #227980 - Flags: first-review?(jminta) → first-review+
(Assignee)

Comment 9

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

branch patch checked in
(Assignee)

Comment 10

12 years ago
Created attachment 228189 [details] [diff] [review]
updated diff between branch and trunk

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

Attached.
Attachment #227447 - Attachment is obsolete: true
(Assignee)

Comment 11

12 years ago
Created attachment 229915 [details] [diff] [review]
patch set 2 - branch patch
Attachment #229915 - Flags: first-review?(jminta)
(Assignee)

Comment 12

12 years ago
Created attachment 229916 [details] [diff] [review]
patch set 2 - trunk patch
Attachment #229916 - Flags: first-review?(jminta)
(Assignee)

Comment 13

12 years ago
Created attachment 229917 [details]
diff after patch set 2 is applied
(Assignee)

Updated

12 years ago
Alias: calsynctrees
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
(Reporter)

Comment 15

12 years ago
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.
(Assignee)

Comment 16

12 years ago
(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.


(Reporter)

Comment 17

12 years ago
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.
Attachment #229915 - Flags: first-review?(jminta) → first-review+
(Reporter)

Comment 18

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

same comments as the branch patch.
Attachment #229916 - Flags: first-review?(jminta) → first-review+
(Assignee)

Comment 19

12 years ago
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.
(Assignee)

Comment 20

12 years ago
patch set 2 committed on branch and trunk
(Assignee)

Comment 21

12 years ago
Created attachment 230538 [details] [diff] [review]
patch set 3 - branch patch

The last of it.
Assignee: nobody → mattwillis
Status: NEW → ASSIGNED
Attachment #230538 - Flags: first-review?(jminta)
(Assignee)

Comment 22

12 years ago
Created attachment 230539 [details] [diff] [review]
patch set 3 - trunk patch

The last of it.
Attachment #230539 - Flags: first-review?(jminta)
(Assignee)

Comment 23

12 years ago
Created attachment 230540 [details]
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.
(Reporter)

Updated

12 years ago
Attachment #230538 - Flags: first-review?(jminta) → first-review+
(Reporter)

Updated

12 years ago
Attachment #230539 - Flags: first-review?(jminta) → first-review+
(Assignee)

Comment 24

12 years ago
Patch set 3 checked in on branch and trunk

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.