Augie, are you planning on posting an updated version of this patch
which addresses Ben's comments? Such a version would be sufficient
for commit. Bonus points for David's suggestion, of course. ;-)
On Jul 22, 2007, at 10:55 AM, Ben Collins-Sussman wrote:
> 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
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 25 00:23:35 2007