Re: [evolution-patches] seeking review for patch for bug 55102

[Jeffrey Stedfast <fejj ximian com>]

looks mostly ok, but a few comments below (and PLEASE, send patches with
Content-Disposition: inline so it is easier to reply to them and comment
on the patch)

On Wed, 2004-03-03 at 03:54, Calvin Liu wrote:
> Hi, there,
> 
> This bug will affect all platforms. If the mail account name contains
> special characters like "#", user can't access that account.
> 
> The patch is coming from the code of shell folder name validation.
> Please review it.
> 
> Thanks.
> Calvin

Index: mail/ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
retrieving revision 1.3142
diff -u -r1.3142 ChangeLog
--- mail/ChangeLog      3 Mar 2004 08:11:15 -0000       1.3142
+++ mail/ChangeLog      3 Mar 2004 09:01:38 -0000
@@ -1,3 +1,9 @@
+2004-03-03  Calvin Liu <calvin liu sun com>
+       * mail-account-gui.c
+       add mail_account_name_is_valid () and change
mail_account_gui_save
+       to check if account name is valid before save it.
+       fixes bug 55102
+
 2004-03-03  Not Zed  <NotZed Ximian com>
 
        ** See bug #54924.

Index: mail/mail-account-gui.c
===================================================================
RCS file: /cvs/gnome/evolution/mail/mail-account-gui.c,v
retrieving revision 1.148
diff -u -r1.148 mail-account-gui.c
--- mail/mail-account-gui.c     24 Feb 2004 21:31:19 -0000      1.148
+++ mail/mail-account-gui.c     3 Mar 2004 08:58:01 -0000
@@ -1949,6 +1949,7 @@
        const char *new_name;
        gboolean is_storage;
        GSList *signatures;
+       const char *reason = NULL;


probably doesn't need to be set to NULL here, but ok...

        
        if (!mail_account_gui_identity_complete (gui, NULL) ||
            !mail_account_gui_source_complete (gui, NULL) ||
@@ -1963,6 +1964,12 @@
         * here. */
        
        new_name = gtk_entry_get_text (gui->account_name);
+        if (! mail_account_name_is_valid (new_name, &reason)){

remove the space after the ! and add a space before the {
(to be consistent with the rest of the code formatting)

+                e_notice (gui->account_name, GTK_MESSAGE_ERROR,
+                                  _("The specified account name is not
valid: %s"), reason);

looks like you used spaces here rather than tab to indent the wrapped
line (use tab)


+                return FALSE;
+        }
+
        account = mail_config_get_account_by_name (new_name);
        
        if (account && account != new) {
@@ -2092,3 +2099,43 @@
        g_free (gui->sent_folder_uri);
        g_free (gui);
 }
+
+gboolean
+mail_account_name_is_valid (const char *name,
+                              const char **reason_return)


don't bother wrapping this line. this is the age of 1024x768
resolutions, we don't need to wrap at 80 columns :-)

also, "reason_return" is inconsistent naming with the way we do this
everywhere else in the code. Just call it "reason".


+{
+        if (name == NULL || *name == '\0') {
+                if (reason_return != NULL)

since this function is only ever called from one place which always
passes in &reason, do we really need this check? I don't think we do...

+                        *reason_return = _("No account name
specified.");
+                return FALSE;
+        }
+                                                                                                                                         

instead of doing the following check, what I would do is check for \r
above the empty-name check and simply remove \r's. No need to trouble
the user about characters he clearly didn't mean to have as part of his
account name.

+        /* GtkEntry is broken - if you hit KP_ENTER you get a \r
inserted... */
+        if (strchr (name, '\r')) {
+                if (reason_return != NULL)
+                        *reason_return = _("Account name cannot contain
the Return character.");
+                return FALSE;
+        }
+                                                                                                                                         
+        if (strchr (name, '/') != NULL) {
+                if (reason_return != NULL)
+                        *reason_return = _("Account name cannot contain
the character \"/\".");
+                return FALSE;
+        }
+                                                                                                                                         
+        if (strchr (name, '#') != NULL) {
+                if (reason_return != NULL)
+                        *reason_return = _("Account name cannot contain
the character \"#\".");
+                return FALSE;
+        }
+                                                                                                                                         
+        if (strcmp (name, ".") == 0 || strcmp (name, "..") == 0) {
+                if (reason_return != NULL)
+                        *reason_return = _("Account name cannot contain
the character \".\".");
+                return FALSE;
+        }
+                                                                                                                                         
+        *reason_return = NULL;

hehe, you went thru all the trouble of checking that reason_return !=
NULL in the error conditions above and didn't bother checking it here?
Shame on you :-)

also, get rid of this. it is unneeded (reason_return only gets used if
this function returns FALSE, so no need to ever set it if we return
TRUE).

+                                                                                                                                         
+        return TRUE;
+}