Re: Diff Project --invoke-diff-cmd part
From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Fri, 12 Apr 2013 00:58:55 +0100 (BST)
Gabriela Gibson wrote:
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:
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
"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...
You don't remove it, you just deprecate it. You can just say "Deprecate.".
> * subversion/include/svn_config.h
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
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
... 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
This is an archived mail posted to the Subversion Dev mailing list.