Re: Diff Project --invoke-diff-cmd part

Gabriela Gibson wrote:
> This patch plugs in a new option --invoke-diff-cmd into the existing
> diff command structure, but does leave the existing "diff-cmd" option
> untouched.
>
> This addition allows the user to define a complex diff command, to be
> applied in place of the internal diff, for example:
>
> svn diff --invoke-diff-cmd="kdiff3 -auto -o /home/g/log ---f1 ---f2 --L1
> ---l1 --L2 "MyLabel""
>
> either on the command line(for example):
>
>   svn diff --invoke-diff-cmd="diff -y ---f1 ---f2"
>
> which expands to "diff -y *from *to",
>
> or, in the config file by adding
>
>   invoke-diff-cmd= diff -y ---f1 ---f2
>
> where ---f1 ---f2 are file 1 and 2. and ---l1 ---l2 are labels.

This is cool.  Nice work.  I tried it with a few different diff programs and it works.

I tried 'diff' (GNU diff), 'kdiff3', 'xxdiff', 'meld', and a few others.  Really I was just looking to see if there are any diff programs for which this form of argument substitution wouldn't be good enough.  I think it would be desirable to be able to create arguments that contain the substituted file name or label as well as some other text, such as "--file1=foo" or "+foo" (which is a form accepted by kdiff3).  But although forms such as these are accepted by some of the diff programs I tried, it wasn't necessary for any of them.  I didn't come across any other problems.

The first thing I tried was a 'pegged' diff invocation such as "svn diff -r10:20 foo" and that didn't work, it just ran the old diff code, as you haven't updated the second code path in diff-cmd.c (it still calls svn_client_diff6 instead of _diff7).

> What's missing:
> --------------
>
> * The merge part, --invoke-diff3-cmd.
>
> * Tests.  I will write them, but I have to spent some time reading
>   into the test suite first.
>
> * This patch breaks the override --internal-diff for now, because
>   this part has to be revised anyway when the invoke-diff3-cmd part
>   gets added.

OK, we'll have to decide what should override what.  I have not formed any opinion on that yet.

> * I added the help blurb to subversion/svn/svn.c:340 but svn help is
>   not showing it.  I will hunt for th reason over the weekend, I
>   didn't want to delay the patch any longer.

"svn help diff" shows your help text, for me, but the formatting is messed up as you have no line breaks in it.

> Thanks for looking!

A pleasure.  It was fun.

I'm not able to do a full review of the patch, at least today.  At a quick scan through, I noticed a few things that need attention: the statement that "substitutions not mentioned get a default value" which doesn't seem to make sense, doc strings needing more detail, an apr_palloc() not allocating enough bytes, two copies of 'invoke_diff_cmd' member in svn_cl__opt_state_t.  Those are important but relatively minor things now that you've found your way through the code base.

And I might as well give you some feedback on the log message since it's right here...

> [[[
> Add new diff option "--invoke-diff-cmd" which allows the user to
> define a custom command line or config file entry for an external
> diff program.
>
> * subversion/include/svn_client.h
>   (svn_client_diff7): Add new function.
>   (svn_client_diff6): Remove deprecated function.

You don't remove it, you just deprecate it.  You can just say "Deprecate.".

> * subversion/include/svn_config.h
>   (): Add new definition.

The top-level symbol or object affected here is the #defined symbol itself, so put that in the parens.  I would write:

  (SVN_CONFIG_OPTION_INVOKE_DIFF_CMD): New definition.

> * subversion/include/svn_io.h
>   (svn_io_create_custom_diff_cmd): Add new function.
>   (svn_io_run_external_diff): Add new function.

For new things, we typically just say "New." or "New function.".  That helps to distinguish the meaning "this thing is new" from ...

> * subversion/libsvn_client/deprecated.c
>   (svn_client_diff6): Add deprecated function.   
>
> * subversion/libsvn_client/diff.c
>   (struct diff_cmd_baton): Add new variable.

... this other meaning, "the change I made to this thing is: add a new variable [inside it]".

>   (diff_content_changed): Add new conditional function call.

Here there's an actual behaviour change.  Describe the behaviour change rather than the shape of the source code.  Say something like "Do a wibble diff if the foo option was specified, otherwise do the old kind of diff."

>   (set_up_diff_cmd_and_options): Add new condition.

Another behaviour change, I guess?  ("Add new condition" doesn't really say much.)

> * subversion/libsvn_subr/config_file.c
>   (svn_config_ensure): Add help text.
>
> * subversion/libsvn_subr/io.c
>   (svn_io_create_custom_diff_cmd): Add new function.
>   (svn_io_run_external_diff): Add new function.
>
> * subversion/svn/cl.h
>   (struct svn_cl__opt_state_t): Add new variable.
>   (struct svn_cl__opt_state_t.diff): Add new variable.
>
> * subversion/svn/diff-cmd.c
>   (svn_cl__diff): Modify call to deprecated function.
>
> * subversion/svn/svn.c
>   (svn_cl__options[]): Add new variable and help info.
> ]]]

- Julian
Received on 2013-04-12 01:59:28 CEST