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
