You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Gabriela Gibson <ga...@gmail.com> on 2013/06/04 10:56:57 UTC
Re: Review of invoke-diff-cmd-feature branch
Hi,
I hope I've resolved most issues, here are ones I need to ask about:
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1484305)
+++ subversion/include/svn_client.h (working copy)
@@ -2988,8 +2988,8 @@ svn_client_blame(const char *path_or_url,
*
* @a invoke_diff_cmd is used to call an external diff program but may
* not be @c NULL. The command line invocation will override the
- * invoke-diff-cmd invocation entry(if any) in the Subversion
- * configuration file.
+ * invoke-diff-cmd invocation entry(if any) in @c ctx->config.
+ * ### "May not be NULL" !? log-cmd.c and deprecated.c pass NULL for it.
Hmm, what I was trying to communicate was that if a NULL(or "") is passed,
this is an error that will be caught. I'm not quite sure what to write
here now.
The deprecated function is guaranteed to not have an execution path to
invoke-diff-cmd but the log-cmd.c has been fixed.
---------- **** ------------
--- subversion/libsvn_subr/config_file.c (revision 1484305)
+++ subversion/libsvn_subr/config_file.c (working copy)
@@ -1086,6 +1086,9 @@ svn_config_ensure(const char *config_dir, apr_pool
"# diff3-has-program-arg = [yes | no]" NL
"### Set invoke-diff-cmd to the absolute path of your 'diff'" NL
"### program." NL
+ /* ### Document the replaceables */
(done)
+ /* ### Document how the setting may contain argv too,
+ not only argv[0] */
"### This will override the compile-time default, which is to use" NL
"### Subversion's internal diff implementation." NL
"# invoke-diff-cmd = \"diff -y --label %l1% %f1% --label %l2% %f2%\""NL
Index: subversion/libsvn_subr/io.c
+ /* ### Document how the setting may contain argv too,
+ not only argv[0] */
Not sure what you mean here?
---------- **** ------------
+ /* This reimplements "split a string into argv". Is there an existing
+ svn/apr function that does that can be reused here? (We might gain
+ an "escape spaces" functionality this way.) */
tmp = svn_cstring_split(cmd," ",TRUE, subpool);
I didn't find one, but perhaps I missed it?
---------- **** ------------
+ How does this parse "%%%f1%"? Is "%%f1%%" an error?
%%%f1% becomes %%f1% and %%f1%% becomes %f1%%, neither is an error.
However, %f1%% is parsed out as sub%, also, +%f1% ends up as +sub.
What you cannot do is: %%f1% to get %sub, it will render as %f1%.
If this is a show stopper, we could go back to the triple dash model
and not deal with escaping %'s, or choose another delimiter.
+ (The answers to both of these questions should have been
+ decided prior to coding, and I recall a list thread but I
+ don't recall that thread reached consensus on a specific
+ escaping UI.) Has consensus on the UI implemented here
+ been reached? */
I'm not sure, what do others think?
---------- **** ------------
@@ -3025,6 +3040,7 @@ svn_io_run_external_diff(const char *dir,
if (pexitcode == NULL)
pexitcode = &exitcode;
+ /* if invoke_diff_cmd is "", cmd[0] would be NULL? */
SVN_ERR(svn_io_run_cmd(dir, cmd[0], cmd, pexitcode, NULL, TRUE,
NULL, outfile, errfile, pool));
I check for this condition before it reaches there. Neither "" nor
NULL value for invoke_diff_cmd is accepted, but an error is raised
before it reaches svn_io_create_custom_diff_cmd().
Re: Review of invoke-diff-cmd-feature branch
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Wed, Jun 12, 2013 at 14:18:08 +0100:
> On 6/11/13, Daniel Shahaf <da...@elego.de> wrote:
> > Gabriela Gibson wrote on Mon, Jun 10, 2013 at 23:39:45 +0100:
> >> On 6/10/13, Daniel Shahaf <da...@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]
>
Our usual argument is "This only affects users who use the new features
(the new invoke-diff-cmd config file option)". I'm not sure what's the
best solution here. How should we handle a client written against 1.7
libraries and run against 1.8 libraries?
Should the deprecated wrapper have the invoke-diff-cmd entry in
ctx->config disabled? Are there other options?
> 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.
>
What's a "switch service"? Is that just svn_client_diff6() ?
> 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?
>
Let's not add choices if we don't have to. One interpolation syntax is
perfectly fine. Just pick one, specify it, implement it. The
considerations unambiguity of the syntax specification, ease of
implementation, and clarity to users.
(I'm sure someone will add considerations that I missed, if any.)
I know we already went through the "what syntax to choose", I hope you
aren't getting frustrated by going through it a second time. We're just
trying to pick a good, unambiguous syntax.
Cheers,
Dainel
Re: Review of invoke-diff-cmd-feature branch
Posted by Gabriela Gibson <ga...@gmail.com>.
On 6/11/13, Daniel Shahaf <da...@elego.de> wrote:
> Gabriela Gibson wrote on Mon, Jun 10, 2013 at 23:39:45 +0100:
>> On 6/10/13, Daniel Shahaf <da...@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;
}
]]]
Re: Review of invoke-diff-cmd-feature branch
Posted by Branko Čibej <br...@wandisco.com>.
On 11.06.2013 10:45, Julian Foad wrote:
> If there's a scheme that we're already using in Subversion, that would
> be a good choice. Is there one?
Yes; the one we already use in config files, which is a ripoff from Python.
In this case you'd have to use %%(foo)s in the config file instead of
%(foo)s, otherwise svn_config_get will expand it to some unexpected
value. But that's OK, IMO.
-- Brane
Re: Review of invoke-diff-cmd-feature branch
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> Gabriela Gibson wrote on Mon, Jun 10, 2013 at 23:39:45 +0100:
>> On 6/10/13, Daniel Shahaf <da...@elego.de> wrote:
[...]
>> >> ---------- **** ------------
>> >>
>> >> + How does this parse "%%%f1%"? Is "%%f1%%" an error?
>> >>
>> >> %%%f1% becomes %%f1% and %%f1%% becomes %f1%%, neither is an error.
>> >>
>> >> However, %f1%% is parsed out as sub%, also, +%f1% ends up as +sub.
>> >
>> > I'm not sure I understand. I expect %%%f1% to become %sub and %f1%% to
>> > be either an error or sub%. Is that what is implemented?
>>
>> No. We sub only %f1% where we find it, but if it's escaped with
>> one or more %'s, we eat exactly one of those instead, so %%%f1% becomes
>> %%f1%.
>
> That's not the behaviour I would expect. What I would expect is what I
> mentioned in my question: that the string is parsed left-to-right, and
> whenever an % is seen, the next character is either % in which case a
> literal % is emitted, or 'f1%' (or one of the other five replaceables)
> in which case it is replaced; if it is anything else, the behaviour is
> undefined / not promised.
>
>> %f1%% becomes sub% as per request by Julian Foad, because some
>> diff clients accept syntax like +sub and sub+ (so, you'd use
>> +%f1% and %f1%+ respectively to get that).
>
> Okay. I would have expected '%f1%%%' were required to get 'sub%', and
> leave the case of a 'foobar%' (and '%f1%%') as undefined behaviour.
>
> What do others think about these %-escaping issues?
There are lots of variable-substitution (or 'interpolation') schemes in the world already -- please don't invent another one. Choose an existing scheme, look up the spec on the Internet and reference it, and then implement (the relevant parts of) it.
If there's a scheme that we're already using in Subversion, that would be a good choice. Is there one? If not, I don't much like the Windows '%foo%' scheme because it's rather irregular compared to others, but if that's what we want to use then I guess that's OK with me, but again, find the spec on the Internet and refer to that.
- Julian
Re: Review of invoke-diff-cmd-feature branch
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Mon, Jun 10, 2013 at 23:39:45 +0100:
> On 6/10/13, Daniel Shahaf <da...@elego.de> wrote:
>
> >> Index: subversion/include/svn_client.h
> >> ===================================================================
> >> --- subversion/include/svn_client.h (revision 1484305)
> >> +++ subversion/include/svn_client.h (working copy)
> > Existing API users call svn_client_diff6(). API compatibility rules
> > provide that if they run their code against libsvn_client compiled from
> > yoru branch, it will continue to work as it has. Does your branch
> > behave that way?
>
> I think it does. Because if you still use the deprecated
> function, you can't use the invoke-diff-cmd because it does not
> yet exist in your world.
>
> >
> > If yes, your docstring is wrong. If not, you have a bug.
>
> I see your point. I'll change the text in a new patch to the
> following:
>
> * @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?
> >
> >> ---------- **** ------------
> >>
> >> --- subversion/libsvn_subr/config_file.c (revision 1484305)
> >> +++ subversion/libsvn_subr/config_file.c (working copy)
> >> @@ -1086,6 +1086,9 @@ svn_config_ensure(const char *config_dir, apr_pool
> >> "# diff3-has-program-arg = [yes | no]" NL
> >> "### Set invoke-diff-cmd to the absolute path of your 'diff'" NL
> >> "### program." NL
> >> + /* ### Document how the setting may contain argv too,
> >> + not only argv[0] */
> >> "### This will override the compile-time default, which is to
> >> use" NL
> >> "### Subversion's internal diff implementation." NL
> >> "# invoke-diff-cmd = \"diff -y --label %l1% %f1% --label %l2%
> >> %f2%\""NL
> >> Index: subversion/libsvn_subr/io.c
> >> + /* ### Document how the setting may contain argv too,
> >> + not only argv[0] */
> >>
> >> Not sure what you mean here?
> >>
> >
> > When a program receives a parameter which is executed as a command,
> > there are three common ways to parse that parameter: (a) as a shell
> > command to be passed to system(); (b) as the command name (the first
> > parameter to execv(), either absolute or in PATH; (c) as a list of
> > strings, a la execv(). I was requesting that you document which how the
> > argument is used.
> >
>
> How about:(in config_file.c)
>
> "### Set invoke-diff-cmd to the absolute path of your 'diff'" NL
> "### program and provide a command string with substitutions, which" NL
> "### is passed in execv() style." NL
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Okay. I would also document how you split the string (a config value is
a string) into an argv vector. (IIRC, what you do is "split on
whitespace, without provision for escaping the whitespace.
And that reminds me: what about the escape mechanism of the [autoprops]
section? There, we already have a way to split a string into a vector
of strings: using a ';' delimiter, and there is already code in place
that interprets ';;' (double delimiter) as a single literal ';' in the
propname=propval string. Can we reuse that idea somehow (possibly with
a different delimiter)? The benefit is allowing every ASCII character
in every argument.
> "### This will override the compile-time default, which is to use" NL
> "### Subversion's internal diff implementation." NL
> "### Substitutions: %f1% original file" NL
> "### %f2% changed file"
> NL
> "### %l1% label of the original file"
> NL
> "### %l2% label of the changed file"
> NL
> "### Examples: --invoke-diff-cmd=\"diff -y %f1% %f2%\"" NL
> "### --invoke-diff-cmd=\"kdiff3 -auto -o /home/u/log \\" NL
> "### +%f1% %f2% --L1 %l1% --L2 \"Custom Label\" \"" NL
> "### The switch symbol '%' can be escaped in the usual way" NL
> "### using the '%' character: %%f1% will be passed as %f1%." NL
>
>
> > (I think in this case the answer is "it is split to words using a
> > hand-rolled implementating of splitting". The fact that that answer is
> > effectively "it is parsed using some locally-rolled parsing rules" is a
> > separate problem which I commented about below.)
> >
> >> ---------- **** ------------
> >>
> >> + /* This reimplements "split a string into argv". Is there an existing
> >> + svn/apr function that does that can be reused here? (We might gain
> >> + an "escape spaces" functionality this way.) */
> >> tmp = svn_cstring_split(cmd," ",TRUE, subpool);
> >>
> >> I didn't find one, but perhaps I missed it?
> >>
> >
> > It looks like apr_proc_create() is the only way to create processes.
> >
> > I still think hand-rolling a "split command to words" functionality is
> > not a good idea.
>
> Is there an alternative? If not, should svn have a library
> function that offers this service?
>
I don't know an alternative offhand, perhaps someone else will spot this
remark buried in the middle of this thread and suggest one.
If not... yes, separating the code makes sense. Whether to a new
libsvn_subr function or just a static function within the same file that
can easily be promoted to the libsvn_subr API once it has another
consumer is your call.
> >
> >> ---------- **** ------------
> >>
> >> + How does this parse "%%%f1%"? Is "%%f1%%" an error?
> >>
> >> %%%f1% becomes %%f1% and %%f1%% becomes %f1%%, neither is an error.
> >>
> >> However, %f1%% is parsed out as sub%, also, +%f1% ends up as +sub.
> >>
> >
> > I'm not sure I understand. I expect %%%f1% to become %sub and %f1%% to
> > be either an error or sub%. Is that what is implemented?
>
> No. We sub only %f1% where we find it, but if it's escaped with
> one or more %'s, we eat exactly one of those instead, so %%%f1% becomes
> %%f1%.
>
That's not the behaviour I would expect. What I would expect is what I
mentioned in my question: that the string is parsed left-to-right, and
whenever an % is seen, the next character is either % in which case a
literal % is emitted, or 'f1%' (or one of the other five replaceables)
in which case it is replaced; if it is anything else, the behaviour is
undefined / not promised.
> %f1%% becomes sub% as per request by Julian Foad, because some
> diff clients accept syntax like +sub and sub+ (so, you'd use
> +%f1% and %f1%+ respectively to get that).
Okay. I would have expected '%f1%%%' were required to get 'sub%', and
leave the case of a 'foobar%' (and '%f1%%') as undefined behaviour.
What do others think about these %-escaping issues?
>
Cheers,
Daniel
Re: Review of invoke-diff-cmd-feature branch
Posted by Gabriela Gibson <ga...@gmail.com>.
On 6/10/13, Daniel Shahaf <da...@elego.de> wrote:
>> Index: subversion/include/svn_client.h
>> ===================================================================
>> --- subversion/include/svn_client.h (revision 1484305)
>> +++ subversion/include/svn_client.h (working copy)
>> @@ -2988,8 +2988,8 @@ svn_client_blame(const char *path_or_url,
>> *
>> * @a invoke_diff_cmd is used to call an external diff program but may
>> * not be @c NULL. The command line invocation will override the
>> - * invoke-diff-cmd invocation entry(if any) in the Subversion
>> - * configuration file.
>> + * invoke-diff-cmd invocation entry(if any) in @c ctx->config.
>> + * ### "May not be NULL" !? log-cmd.c and deprecated.c pass NULL for it.
>>
>> Hmm, what I was trying to communicate was that if a NULL(or "") is
>> passed,
>> this is an error that will be caught. I'm not quite sure what to write
>> here now.
>>
>> The deprecated function is guaranteed to not have an execution path to
>> invoke-diff-cmd but the log-cmd.c has been fixed.
>>
>
> Can you explain that, please?
I've added --invoke-diff-cmd to log-cmd.c so it's available in
the same way as --diff-cmd.
Example:
svn log --diff --invoke-diff-cmd="diff -y %f1% %f2%" -r 1484733
> Existing API users call svn_client_diff6(). API compatibility rules
> provide that if they run their code against libsvn_client compiled from
> yoru branch, it will continue to work as it has. Does your branch
> behave that way?
I think it does. Because if you still use the deprecated
function, you can't use the invoke-diff-cmd because it does not
yet exist in your world.
>
> If yes, your docstring is wrong. If not, you have a bug.
I see your point. I'll change the text in a new patch to the
following:
* @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?
>
>> ---------- **** ------------
>>
>> --- subversion/libsvn_subr/config_file.c (revision 1484305)
>> +++ subversion/libsvn_subr/config_file.c (working copy)
>> @@ -1086,6 +1086,9 @@ svn_config_ensure(const char *config_dir, apr_pool
>> "# diff3-has-program-arg = [yes | no]" NL
>> "### Set invoke-diff-cmd to the absolute path of your 'diff'" NL
>> "### program." NL
>> + /* ### Document how the setting may contain argv too,
>> + not only argv[0] */
>> "### This will override the compile-time default, which is to
>> use" NL
>> "### Subversion's internal diff implementation." NL
>> "# invoke-diff-cmd = \"diff -y --label %l1% %f1% --label %l2%
>> %f2%\""NL
>> Index: subversion/libsvn_subr/io.c
>> + /* ### Document how the setting may contain argv too,
>> + not only argv[0] */
>>
>> Not sure what you mean here?
>>
>
> When a program receives a parameter which is executed as a command,
> there are three common ways to parse that parameter: (a) as a shell
> command to be passed to system(); (b) as the command name (the first
> parameter to execv(), either absolute or in PATH; (c) as a list of
> strings, a la execv(). I was requesting that you document which how the
> argument is used.
>
How about:(in config_file.c)
"### Set invoke-diff-cmd to the absolute path of your 'diff'" NL
"### program and provide a command string with substitutions, which" NL
"### is passed in execv() style." NL
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
"### This will override the compile-time default, which is to use" NL
"### Subversion's internal diff implementation." NL
"### Substitutions: %f1% original file" NL
"### %f2% changed file"
NL
"### %l1% label of the original file"
NL
"### %l2% label of the changed file"
NL
"### Examples: --invoke-diff-cmd=\"diff -y %f1% %f2%\"" NL
"### --invoke-diff-cmd=\"kdiff3 -auto -o /home/u/log \\" NL
"### +%f1% %f2% --L1 %l1% --L2 \"Custom Label\" \"" NL
"### The switch symbol '%' can be escaped in the usual way" NL
"### using the '%' character: %%f1% will be passed as %f1%." NL
> (I think in this case the answer is "it is split to words using a
> hand-rolled implementating of splitting". The fact that that answer is
> effectively "it is parsed using some locally-rolled parsing rules" is a
> separate problem which I commented about below.)
>
>> ---------- **** ------------
>>
>> + /* This reimplements "split a string into argv". Is there an existing
>> + svn/apr function that does that can be reused here? (We might gain
>> + an "escape spaces" functionality this way.) */
>> tmp = svn_cstring_split(cmd," ",TRUE, subpool);
>>
>> I didn't find one, but perhaps I missed it?
>>
>
> It looks like apr_proc_create() is the only way to create processes.
>
> I still think hand-rolling a "split command to words" functionality is
> not a good idea.
Is there an alternative? If not, should svn have a library
function that offers this service?
>
>> ---------- **** ------------
>>
>> + How does this parse "%%%f1%"? Is "%%f1%%" an error?
>>
>> %%%f1% becomes %%f1% and %%f1%% becomes %f1%%, neither is an error.
>>
>> However, %f1%% is parsed out as sub%, also, +%f1% ends up as +sub.
>>
>
> I'm not sure I understand. I expect %%%f1% to become %sub and %f1%% to
> be either an error or sub%. Is that what is implemented?
No. We sub only %f1% where we find it, but if it's escaped with
one or more %'s, we eat exactly one of those instead, so %%%f1% becomes
%%f1%.
%f1%% becomes sub% as per request by Julian Foad, because some
diff clients accept syntax like +sub and sub+ (so, you'd use
+%f1% and %f1%+ respectively to get that).
Re: Review of invoke-diff-cmd-feature branch
Posted by Daniel Shahaf <da...@elego.de>.
Gabriela Gibson wrote on Tue, Jun 04, 2013 at 09:56:57 +0100:
> Hi,
>
> I hope I've resolved most issues, here are ones I need to ask about:
>
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1484305)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2988,8 +2988,8 @@ svn_client_blame(const char *path_or_url,
> *
> * @a invoke_diff_cmd is used to call an external diff program but may
> * not be @c NULL. The command line invocation will override the
> - * invoke-diff-cmd invocation entry(if any) in the Subversion
> - * configuration file.
> + * invoke-diff-cmd invocation entry(if any) in @c ctx->config.
> + * ### "May not be NULL" !? log-cmd.c and deprecated.c pass NULL for it.
>
> Hmm, what I was trying to communicate was that if a NULL(or "") is passed,
> this is an error that will be caught. I'm not quite sure what to write
> here now.
>
> The deprecated function is guaranteed to not have an execution path to
> invoke-diff-cmd but the log-cmd.c has been fixed.
>
Can you explain that, please?
Existing API users call svn_client_diff6(). API compatibility rules
provide that if they run their code against libsvn_client compiled from
yoru branch, it will continue to work as it has. Does your branch
behave that way?
If yes, your docstring is wrong. If not, you have a bug.
Am I missing anything?
> ---------- **** ------------
>
> --- subversion/libsvn_subr/config_file.c (revision 1484305)
> +++ subversion/libsvn_subr/config_file.c (working copy)
> @@ -1086,6 +1086,9 @@ svn_config_ensure(const char *config_dir, apr_pool
> "# diff3-has-program-arg = [yes | no]" NL
> "### Set invoke-diff-cmd to the absolute path of your 'diff'" NL
> "### program." NL
> + /* ### Document how the setting may contain argv too,
> + not only argv[0] */
> "### This will override the compile-time default, which is to use" NL
> "### Subversion's internal diff implementation." NL
> "# invoke-diff-cmd = \"diff -y --label %l1% %f1% --label %l2% %f2%\""NL
> Index: subversion/libsvn_subr/io.c
> + /* ### Document how the setting may contain argv too,
> + not only argv[0] */
>
> Not sure what you mean here?
>
When a program receives a parameter which is executed as a command,
there are three common ways to parse that parameter: (a) as a shell
command to be passed to system(); (b) as the command name (the first
parameter to execv(), either absolute or in PATH; (c) as a list of
strings, a la execv(). I was requesting that you document which how the
argument is used.
(I think in this case the answer is "it is split to words using a
hand-rolled implementating of splitting". The fact that that answer is
effectively "it is parsed using some locally-rolled parsing rules" is a
separate problem which I commented about below.)
> ---------- **** ------------
>
> + /* This reimplements "split a string into argv". Is there an existing
> + svn/apr function that does that can be reused here? (We might gain
> + an "escape spaces" functionality this way.) */
> tmp = svn_cstring_split(cmd," ",TRUE, subpool);
>
> I didn't find one, but perhaps I missed it?
>
It looks like apr_proc_create() is the only way to create processes.
I still think hand-rolling a "split command to words" functionality is
not a good idea. Not quite sure what to suggests.
> ---------- **** ------------
>
> + How does this parse "%%%f1%"? Is "%%f1%%" an error?
>
> %%%f1% becomes %%f1% and %%f1%% becomes %f1%%, neither is an error.
>
> However, %f1%% is parsed out as sub%, also, +%f1% ends up as +sub.
>
I'm not sure I understand. I expect %%%f1% to become %sub and %f1%% to
be either an error or sub%. Is that what is implemented?
> What you cannot do is: %%f1% to get %sub, it will render as %f1%.
>
That's good.
> If this is a show stopper, we could go back to the triple dash model
> and not deal with escaping %'s, or choose another delimiter.
> ---------- **** ------------
> @@ -3025,6 +3040,7 @@ svn_io_run_external_diff(const char *dir,
> if (pexitcode == NULL)
> pexitcode = &exitcode;
>
> + /* if invoke_diff_cmd is "", cmd[0] would be NULL? */
> SVN_ERR(svn_io_run_cmd(dir, cmd[0], cmd, pexitcode, NULL, TRUE,
> NULL, outfile, errfile, pool));
>
> I check for this condition before it reaches there. Neither "" nor
> NULL value for invoke_diff_cmd is accepted, but an error is raised
> before it reaches svn_io_create_custom_diff_cmd().
Cool. I must have missed that check.