Daniel Shahaf wrote:
> julianfoad_at_apache.org wrote on Mon, 12 Feb 2018 13:17 +0000:
>> +++ subversion/trunk/subversion/svn/shelf-cmd.c Mon Feb 12 13:17:16 2018
>> @@ -71,6 +71,36 @@ friendly_duration_str(apr_time_t duratio
>> +#ifndef WIN32
>> +/* Run CMD with ARGS.
>> + * Send its stdout to the parent's stdout. Disconnect its stdin and stderr.
>> + */
>> +static svn_error_t *
>> +run_cmd(const char *cmd,
>> + const char *const *args,
>> + apr_pool_t *scratch_pool)
>> +++ subversion/trunk/subversion/svn/shelve-cmd.c Mon Feb 12 13:17:16
>> 2018
>> @@ -84,6 +84,36 @@ list_sorted_by_date(apr_array_header_t *
>> +#ifndef WIN32
>> +/* Run CMD with ARGS.
>> + * Send its stdout to the parent's stdout. Disconnect its stdin and stderr.
>> + */
>> +static svn_error_t *
>> +run_cmd(const char *cmd,
>> + const char *const *args,
>> + apr_pool_t *scratch_pool)
>
> Why the duplication? We could deduplicate by renaming to svn_cl__run_cmd()
> and declaring it in subversion/svn/*.h.
The duplication is because 'shelve-cmd.c' is shelving-v1, left in trunk
only for easier backporting to 1.10.x; while 'shelf-cmd.c' is the
current shelving implementation.
I should rename them to make that clearer probably. Not sure what
exactly. 'shelve-v1-cmd' and 'shelving-v2-cmd'? 'shelve-1.10.0-cmd.c'?
That was partly an experiment in feature-toggle development, although I
didn't go far with it in this instance.
>> @@ -79,14 +109,19 @@ show_diffstat(svn_client_shelf_version_t
>> SVN_ERR(svn_client_shelf_get_patch_abspath(&patch_abspath, shelf_version,
>> scratch_pool));
>> + args[0] = "diffstat";
>> + args[1] = "-p0";
>> + args[2] = patch_abspath;
>> + args[3] = NULL;
>> + err = run_cmd("diffstat", args, scratch_pool);
>> @@ -84,6 +84,36 @@ list_sorted_by_date(apr_array_header_t *
>> + args[0] = "diffstat";
>> + args[1] = "-p0";
>> + args[2] = info->patch_path;
>> + args[3] = NULL;
>
> Maybe add "--" guards to these calls? The path arguments are absolute, but
> a "--" guard would minimize the impact of any quoting bug in svn_io_run_cmd().
Could do, and you are welcome to, but I think this is not important.
Thanks for reviewing and making these suggestions.
- Julian
Received on 2018-02-12 16:19:40 CET