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.
