Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (athena.apache.org: local policy)
Date: Mon, 10 Jun 2013 09:58:39 +0200
From: Daniel Shahaf <danielsh@elego.de>
To: Gabriela Gibson <gabriela.gibson@gmail.com>
Cc: Gabriela Gibson <gbg@apache.org>, dev@subversion.apache.org
Subject: Re: Review of invoke-diff-cmd-feature branch
Message-ID: <20130610075839.GH3490@tarsus.local2>
References: <20130519172313.GA3690@lp-shahaf.local>
 <CALfVaASafgT9NHqP+ZZWyqObJdp78KeD5RCxHS4TjFAYCMFnJA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CALfVaASafgT9NHqP+ZZWyqObJdp78KeD5RCxHS4TjFAYCMFnJA@mail.gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)

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.