Gabriela Gibson wrote on Mon, Jun 10, 2013 at 23:39:45 +0100:
> 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)
> > 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
Received on 2013-06-11 07:21:43 CEST