[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] fix for interactive merge-callback not supporting spaces in file paths

From: Ben Collins-Sussman <sussman_at_red-bean.com>
Date: 2007-07-22 19:55:10 CEST

OK, before I apply this patch, about some stylistic nits? :-)

{
- const char *editor, *cmd;
+ const char *editor, *cmd, *base_dir, *file_name, *base_dir_apr;
+ char *old_cwd;
   int sys_err;
+ apr_status_t apr_err;
+ svn_error_t *err = SVN_NO_ERROR;

Our convention, usually, is to end all functions with 'return SVN_NO_ERROR'.

+ base_dir = svn_path_dirname(path, pool);
+ file_name = svn_path_basename(path, pool);

Couldn't you do this all at once with a single call to svn_path_split()?

+
   SVN_ERR(find_editor_binary(&editor, editor_cmd, config));

- cmd = apr_psprintf(pool, "%s %s", editor, path);
+ apr_err = apr_filepath_get(&old_cwd, APR_FILEPATH_NATIVE, pool);
+ if (apr_err)
+ return svn_error_wrap_apr(apr_err, _("Can't get working directory"));
+
+ /* APR doesn't like "" directories */
+ if (base_dir[0] == '\0')
+ base_dir_apr = ".";
+ else
+ SVN_ERR(svn_path_cstring_from_utf8(&base_dir_apr, base_dir, pool));
+
+ apr_err = apr_filepath_set(base_dir_apr, pool);
+ if (apr_err)
+ {
+ return svn_error_wrap_apr
+ (apr_err, _("Can't change working directory to '%s'"), base_dir);
+ }

No need for curly braces around a single-line block.

+
+ cmd = apr_psprintf(pool, "%s \'%s\'", editor, file_name);
   sys_err = system(cmd);
   if (sys_err != 0)
- /* Extracting any meaning from sys_err is platform specific, so just
- use the raw value. */
- return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
- _("system('%s') returned %d"), cmd, sys_err);
+ {
+ /* Extracting any meaning from sys_err is platform specific, so just
+ use the raw value. */
+ err = svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
+ _("system('%s') returned %d"), cmd, sys_err);
+ }

Same thing here.

- return SVN_NO_ERROR;
+ apr_err = apr_filepath_set(old_cwd, pool);
+ if (apr_err)
+ {
+ svn_handle_error2(svn_error_wrap_apr
+ (apr_err, _("Can't restore working directory")),
+ stderr, TRUE /* fatal */, "svn: ");
+ }
+
+ return err;
 }

Same thing here.

Actually, why do you define 'err' at the top of the function at all?
I don't see it used.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Jul 22 19:54:08 2007

This is an archived mail posted to the Subversion Dev mailing list.