On 6/10/13, Daniel Shahaf <danielsh_at_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).
Received on 2013-06-11 00:40:20 CEST