Re: svn commit: r1823966 - in /subversion/trunk/subversion/svn: shelf-cmd.c shelve-cmd.c

2018-02-12 Thread Julian Foad



Daniel Shahaf wrote:

julianf...@apache.org wrote on Mon, 12 Feb 2018 13:17 +:

+++ 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(_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


Re: svn commit: r1823966 - in /subversion/trunk/subversion/svn: shelf-cmd.c shelve-cmd.c

2018-02-12 Thread Daniel Shahaf
julianf...@apache.org wrote on Mon, 12 Feb 2018 13:17 +:
> +++ 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.

> @@ -79,14 +109,19 @@ show_diffstat(svn_client_shelf_version_t
>SVN_ERR(svn_client_shelf_get_patch_abspath(_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().

Cheers,

Daniel