Yes, I'm hoping to work on it presently. I was out of town all
weekend, but I should be getting it touched up this week. I didn't
figure I'd put any rush on it since Ben is at OSCON all week.
Thanks,
Augie
On Jul 24, 2007, at 5:31 PM, Daniel Rall wrote:
> 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 03:19:51 2007