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.
Received on 2013-06-10 09:59:32 CEST