Re: invoke-diff-cmd branch is ready for trunk inclusion

Gabriela Gibson wrote:
> the latest invoke-diff-cmd branch:
>
> http://svn.apache.org/viewvc?view=revision&revision=1538071
>
> is ready to be merged into the trunk, iff everyone agrees that the
> current substitution syntax is just right and the documentation and the
> code itself is of acceptable standard.

Hi Gabriela.

It looks to me like this work is near complete. I have at last got around to reviewing it and I think there's only one significant thing to do before merging to trunk.

Your BRANCH-README file

<http://svn.apache.org/repos/asf/subversion/branches/invoke-diff-cmd-feature/BRANCH-README>

is a fantastic window into the branch, much more detailed and helpful than the rest of us ever write. I can easily see exactly what the whole feature does, how it's structured, and what your plans are, without having been paying close attention. I'm mentioning this mainly so anybody else contemplating taking a look at this knows this is a good place to start reading. And I'm pleased to see you have taken care to adhere to our coding style.

I'll mention my main two points first:

1. The interpolation/substitution syntax.

2. Duplication between --diff-cmd and --invoke-diff-cmd.

and then a few coding-style and nit-picking trivia at the end.

> If at all possible, it would be convenient if the invoke-diff-cmd
> branch is not deleted quite yet, because I'm using a particular
> merge revision in that branch for testing the forthcoming
> invoke-merge-cmd feature.

That's no problem. In fact, once we're happy, we'll ask you to do the merge into trunk. Although one would normally delete it at that time, you can keep the branch as long as you need to.

=== Interpolation/substitution Syntax ===

I said at the beginning of this exercise that we should leave the syntax issue till later, and now it is later.

I tried a few things to see how the parsing handles things at the moment. One case that doesn't work right at the moment is this:

$ svn diff subversion/svn/svn.c \
  --invoke-diff-cmd 'echo %svn_old_label%,%svn_new_label%'

which prints:

subversion/svn/svn.c  (revision 1536635),%svn_new_label%

I also tried command strings involving "%%", "%%%", "%svn_old_label%svn_new_label%" and other variations to see if I could get it to output a plain '%' character when I wanted it and if it would substitute correctly in all combinations and so on.

Basically I'm looking for the substitution syntax to (1) be regular, easy to predict the result, with a small number of easy-to-remember rules; (2) be possible to programatically 'escape' any arbitrary string X (even if it contains sequences like '%%' or '%old_label%' or anything that would otherwise have special meaning) to produce a result Y in such a way that putting Y through the substitution will recreate X exactly; and (3) be exactly the same syntax that we already use somewhere else in Subversion.

So we need to follow up one of the suggestions that have been made about using the syntax from config files, or from user-defined keyword definitions, or something. I haven't paid close attention to those. If nobody else does, I'll see if I can make a concrete recommendation.

Argument splitting:

The function svn_io_run_external_diff() is now the main entry point for running a diff command, and you have made the old svn_io_run_diff2() into a wrapper which forwards to svn_io_run_external_diff(). That's fine in principle. However, there is a reason the old one took its arguments as an array rather than as a space-separated string. Parsing a space-separated string into a list of arguments is hard to get right in terms of syntax design and in terms of implementation, when it comes to the edge cases involving spaces in arguments, zero-length arguments and so on. So avoid it. Don't join the arguments into a single string, keep them as an array all the way through the system.

=== Duplication ===

It would be nice if we could find a way to duplicate less of the settings and the implementation: instead of --diff-cmd and --invoke-diff-cmd being handled by two separate options, variables, code paths, etc, could we reduce them to sharing a single implementation at some level in the code? If so, it would lead to a number of simplifications.

In subversion/libsvn_client/diff.c:

+  if (diff_cmd_baton->diff_cmd && diff_cmd_baton->invoke_diff_cmd)
+      return svn_error_create(SVN_ERR_CLIENT_DIFF_CMD, NULL,

The need for this error might go away entirely.

In set_up_diff_cmd_and_options(): Not sure about the precedence of old and new options. And instead of keeping them in separate variables, can we combine them?

In subversion/include/svn_io.h and subversion/libsvn_subr/io.c:

svn_io_run_external_diff() is very similar to svn_io_run_diff2(), and I think it makes sense to make its interface even more similar and then keep only the newer one, deprecating the older one. Choose one style: either have the caller do the argument interpolation before calling it, and pass a list of literal arguments -- like svn_io_run_diff2() does already, except without adding extra arguments; or interpolate the arguments inside it, defining a way for the old callers to avoid triggering this.

Finally,

=== Minor Nits ===

In svn_client.h:

+/** Similar to svn_client_diff7(), but with @a invoke_diff_cmd.

s/with/without/

+/**
+ * Similar to svn_client_peg7(), but with @a no_diff_added set to
+ * FALSE, @a ignore_properties set to FALSE and @a properties_only
+ * set to FALSE.

s/with ... FALSE./without @a invoke_diff_cmd./

+/**
+ * Similar to svn_client_diff6_peg6(), but with @a outfile and @a errfile,

s/_diff6_peg6/_diff_peg6/

In subversion/tests/cmdline/getopt_tests_data/getopt_help_log_switch_stdout:

Add the --invoke-diff-cmd descriptive text, to fix getopt_tests.py 8, which currently fails just because the help text has changed. (I know, it's a bit silly testing that particular help text comes out and thus having to edit the test data each time we change the help, but that's how it is.)

In subversion/libsvn_client/diff.c:

+  /* external custom diff command */
+  const char *invoke_diff_cmd;

Put this next to the existing 'diff_cmd' member at the top of the struct, just to keep closely related things together. (This struct isn't public, so its layout can change.)

+                    _("diff-cmd and invoke-diff-cmd are"
+                      "mutually exclusive."));

Missing space between "are" and "mutually".

In subversion/include/svn_io.h and subversion/libsvn_subr/io.c:

+const char **
+__create_custom_diff_cmd(const char *label1,

The name for a logically private, physically public function should start with "svn_io__" rather than plain "__". Its declaration should go in subversion/include/private/svn_io_private.h (rather than svn_io.h). The doc string of this function needs updating with the new syntax.

That's about it. Thanks.

- Julian
Received on 2013-11-06 16:16:22 CET