On 6/11/13, Daniel Shahaf <danielsh_at_elego.de> wrote:
> Gabriela Gibson wrote on Mon, Jun 10, 2013 at 23:39:45 +0100:
>> On 6/10/13, Daniel Shahaf <danielsh_at_elego.de> wrote:
>>
>> * @a invoke_diff_cmd takes an argument which is used to call an
>> * external diff program. When invoked, the argument may not be @c
>> * NULL or the empty string "". The command line invocation will
>> * override the invoke-diff-cmd invocation entry (if any) in the
>> * Subversion configuration file.
>>
>> Is that passable?
>>
>
> No, because the argument _may_ be NULL (since the deprecated code path
> (and people updating calls to deprecated functions in their code to call
> the non-deprecated equivalents) needs it), and the docstring says it may
> not be. Note, even people who updated their code to the 1.9 API might
> still want to pass NULL because they don't want to override the
> invoke-diff-cmd in the config (or because they want to use the builtin
> diff implementation).
>
> Does this make sense?
I had a look how TortoiseSVN approaches this. From what I can
see, no matter what we do, they have to adjust their code, because
they turn off the config file options explicitly when they run
their custom code, so adding invoke-diff-cmd to the config file
will scupper that plan[1]
So in that sense, behavior *has* changed and our addition is not
backward compatible with older clients, even if they use ...diff6().
I think I should probably write a short intro to the new code for
client devs to make updating their code easier. Another thing to
consider is whether we should offer a 'switch service' in to make
external surgery of the set config file values unnecessary.
On the bright side, I've finally understood why it's OK to pass
NULL safely because the code path is selected on that value. What
I should have said in the doxygen comment to
svn_io_create_custom_diff_cmd is that thou shall not pass NULL.
New comment for svn_client_diff7 is now:
* @a invoke_diff_cmd takes an argument which is used to call an
* external diff program when not NULL or "".
(Will also update the svn_io_create_custom_diff_cmd doxygen comment.)
==================================================================
About the delimiter to select:
I don't have the expertise to make an informed decisions here, or
even to hold a strong opinion. But I happily code whatever the
team chooses :) I could also rewrite (or try to!) the
svn_io_create_custom_diff_cmd in C printf style if the current
code shape is the wrong approach for what we want to do (it
probably is)
I still think the ---f1 pattern is the easiest to use because
it's guaranteed not to interfere with anything, nothing needs to
be escaped either and everything users want to do can be done
much simpler.
The strongest argument against ---f1 was it's novel and a bit
ugly, but, in the scheme of things, I think this is a smaller
problem than overlaying user's interpolation schemes and
painstakingly counting and escaping %'s or $'s.
Also it visually separates the user's interpolation scheme nicely
from ours.
It's no problem to allow users the choice of two interpolating
syntaxes either, maybe the breadths of possibilities is just to
broad to make a decent swiss army knife here that covers
everyone's needs?
(other stuff has been noted and is on the to-do list)
Gabriela
[1]
[[[
bool SVN::CreatePatch(const CTSVNPath& path1, const SVNRev& revision1,
const CTSVNPath& path2, const SVNRev& revision2,
const CTSVNPath& relativeToDir, svn_depth_t depth,
bool ignoreancestry, bool nodiffadded, bool
nodiffdeleted, bool showCopiesAsAdds, bool ignorecontenttype,
bool useGitFormat, bool ignoreproperties, bool
propertiesonly, const CString& options, bool bAppend, const CTSVNPath&
outputfile)
{
// to create a patch, we need to remove any custom diff tools
which might be set in the config file
svn_config_t * cfg = (svn_config_t *)apr_hash_get (m_pctx->config,
SVN_CONFIG_CATEGORY_CONFIG, APR_HASH_KEY_STRING);
CStringA diffCmd;
CStringA diff3Cmd;
if (cfg)
{
const char * value;
svn_config_get(cfg, &value, SVN_CONFIG_SECTION_HELPERS,
SVN_CONFIG_OPTION_DIFF_CMD, NULL);
diffCmd = CStringA(value);
svn_config_get(cfg, &value, SVN_CONFIG_SECTION_HELPERS,
SVN_CONFIG_OPTION_DIFF3_CMD, NULL);
diff3Cmd = CStringA(value);
svn_config_set(cfg, SVN_CONFIG_SECTION_HELPERS,
SVN_CONFIG_OPTION_DIFF_CMD, NULL);
svn_config_set(cfg, SVN_CONFIG_SECTION_HELPERS,
SVN_CONFIG_OPTION_DIFF3_CMD, NULL);
}
bool bRet = Diff(path1, revision1, path2, revision2,
relativeToDir, depth, ignoreancestry, nodiffadded, nodiffdeleted,
showCopiesAsAdds, ignorecontenttype, useGitFormat, ignoreproperties,
propertiesonly, options, bAppend, outputfile, CTSVNPath());
if (cfg)
{
svn_config_set(cfg, SVN_CONFIG_SECTION_HELPERS,
SVN_CONFIG_OPTION_DIFF_CMD, (LPCSTR)diffCmd);
svn_config_set(cfg, SVN_CONFIG_SECTION_HELPERS,
SVN_CONFIG_OPTION_DIFF3_CMD, (LPCSTR)diff3Cmd);
}
return bRet;
}
]]]
Received on 2013-06-12 15:18:44 CEST