Re: [PATCH] issue #3071: allow built-in diff to be specified as diff-cmd

On Mon, Mar 24, 2008 at 05:05:06AM -0400, Daniel Danger Bentley wrote:
> Ping.
>
> Is there something I can do to help this process along or that I forgot to
> do initially? Should I pick a reviewer based on revision history?

Since no-one else has commented so far, let me just give you my
(non-authoritative!) take on the design of your patch.

I think making an empty string carry special semantics is wrong UI design.
It's not obvious enough.

What would be better IMHO (and various standard UNIX tools do the
same thing) is adding an additional option that overrides all previous
--diff-cmd options, and also config file settings.

Maybe --force-internal-diff ?

Coding-wise, your patch looks great.

> P.S. One potential objection I could imagine is that this breaks
> parallelism with other options in the "[helpers]" section. Should I
> expand the scope of this patch to fix up every such use of editor-cmd
> and diff3-cmd as well?

What should the semantics of editor-cmd "" be?

For diff3, --force-internal-diff3 would be suitable also. If you can
make diff3 consistent with diff, all the better :)

> * subversion/libsvn_client/diff.c
> (diff_content_changed): Changed to use "" to mean "use built-in
> diff". This allows a default in config to be overridden on command
> line.

We usually indent log messages like this, using single spaces:

      * subversion/libsvn_client/diff.c
        (diff_content_changed): Changed to use "" to mean "use built-in
         diff". This allows a default in config to be overridden on command
         line.
       ^^^

Hope this helps,

-- 
Stefan Sperling <stsp_at_elego.de>                 Software Developer
elego Software Solutions GmbH                            HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12        Tel:  +49 30 23 45 86 96 
13355 Berlin                              Fax:  +49 30 23 45 86 95
http://www.elego.de                 Geschaeftsfuehrer: Olaf Wagner