Branch 'invoke-diff-cmd-feature' is ready for half-way review

[Gabriela Gibson <ga...@gmail.com>]

Hi,

my branch is ready for a half-way review as the 'invoke-diff-cmd'
part is complete and adding the 'invoke-diff3-cmd' part would
only duplicate current errors.

The BRANCH-README file is here:

https://svn.apache.org/repos/asf/subversion/branches/invoke-diff-cmd-feature/BRANCH-README

thanks for looking!

Gabriela

Re: Branch 'invoke-diff-cmd-feature' is ready for half-way review

[Johan Corveleyn <jc...@gmail.com>]

On Fri, Oct 18, 2013 at 8:29 PM, Gabriela Gibson
<ga...@gmail.com> wrote:
...
> 2.) The delimiter:
>
> There appears to be some team miscommunication here -- afaik, the last
> status was that danielsh asked me to implement the semi-colon, so this
> is what you got^Whad :)
>
> Now I had a long think and realised there are other issues with
> expanding that have the potential to be traps, and so, I removed the
> ability to escape the delimiter and changed the syntax to look like
> so:
>
> [[[
>   struct replace_tokens_tab
>   {
>     const char *delimiter;
>     const char *replace;
>   } tokens_tab[] = {  /* Diff terminology */
>     { "%svn_new_label%", label1 },
>     { "%svn_old_label%", label2 },
>     { "%svn_base_label%", label3 },
>     { "%svn_old%", from },
>     { "%svn_new%", to },
>     { "%svn_base%", base },
>     { NULL, NULL }
>   };
>
>   if (label3) /* Merge terminology */
>     {
>       tokens_tab[0].delimiter = "%svn_to_label%";
>       tokens_tab[1].delimiter = "%svn_from_label%";
>       tokens_tab[3].delimiter = "%svn_to%";
>       tokens_tab[4].delimiter = "%svn_from%";
>
>     }
> ]]]
>
> Rationale:
>
> this new syntax frees the commonly used 'fN' and 'lN' variable names and is
> completely unambiguous, and also fairly unique.  It's more to type, but much
> less to worry about.  Moreover it matches the
> %custom_keyword% syntax for the props, which is new in SVN 1.8:
>
> http://subversion.apache.org/docs/release-notes/1.8#custom-keywords
>
> The issue is that, if we allow escaping (which we need to do if we use
> %f1 %f1% or ;f1 etc) it only is necessary because we're appropriating
> common variable names or a shell character as part of the delimiter.

On Windows, %X% expands to the value of the environment variable X.
I.e. %PATH% yields the value of the PATH variable etc. Best to avoid
it, I think.

On the other hand, %PATH won't be handled specially by the Windows
command shell, so that might be a good option.

Also, the custom-keywords syntax that you referenced above is %r, %u,
%b, ... not %r% etc.

So I think %X is better for your invoke-diff-cmd syntax.

There's one caveat with this syntax on Windows though: it seems you
cannot escape the percent sign at the end of a variable (at least, it
doesn't work on my Windows XP under cmd.exe): if the value of X is
foo, then %X%% will just expand to foo%.

So if you go for the %X syntax, it might not be possible to give the
user a guaranteed way to output

    the_old_label%

as one of the invoke-diff-cmd arguments.


BTW, thanks for continuing your work, and for hanging in there even
though it all takes a while (and sorry for giving the syntax feedback
so late). It would be nice to see this feature ending up in 1.9, if
possible.

-- 
Johan

Re: Branch 'invoke-diff-cmd-feature' is ready for half-way review

[Gabriela Gibson <ga...@gmail.com>]

Hi,

thank you very much for your time!

1.) new vs original issue: Fixed, thank you for spotting this.

2.) The delimiter:

There appears to be some team miscommunication here -- afaik, the last
status was that danielsh asked me to implement the semi-colon, so this
is what you got^Whad :)

Now I had a long think and realised there are other issues with
expanding that have the potential to be traps, and so, I removed the
ability to escape the delimiter and changed the syntax to look like
so:

[[[
   struct replace_tokens_tab
   {
     const char *delimiter;
     const char *replace;
   } tokens_tab[] = {  /* Diff terminology */
     { "%svn_new_label%", label1 },
     { "%svn_old_label%", label2 },
     { "%svn_base_label%", label3 },
     { "%svn_old%", from },
     { "%svn_new%", to },
     { "%svn_base%", base },
     { NULL, NULL }
   };

   if (label3) /* Merge terminology */
     {
       tokens_tab[0].delimiter = "%svn_to_label%";
       tokens_tab[1].delimiter = "%svn_from_label%";
       tokens_tab[3].delimiter = "%svn_to%";
       tokens_tab[4].delimiter = "%svn_from%";

     }
]]]

Rationale:

this new syntax frees the commonly used 'fN' and 'lN' variable names and 
is completely unambiguous, and also fairly unique.  It's more to type, 
but much less to worry about.  Moreover it matches the
%custom_keyword% syntax for the props, which is new in SVN 1.8:

http://subversion.apache.org/docs/release-notes/1.8#custom-keywords

The issue is that, if we allow escaping (which we need to do if we use
%f1 %f1% or ;f1 etc) it only is necessary because we're appropriating
common variable names or a shell character as part of the delimiter.

But what letting users add escapes does (as a side effect) is that it
creates a magic global number(in form of n escapes), and whilst this
is not an issue with immediate use that only occurs on the command line
or the config file, once/if we implement issue 2447 and allow file props
to carry individual invoke-diff-cmds, this ability can get problematic:

http://subversion.tigris.org/issues/show_bug.cgi?id=2447


3.) The pool longevity:

**result[] is allocated to the pool that is passed into
svn_io_run_external_diff(), and so persists past the life of
__create_custom_diff_cmd.  Reassigning word->data has no effect on
what has been put into **result[]. (I checked with GDB)


4.)  The word->elt_size query:

"
result = apr_palloc(pool,
                    (words->nelts+1) * words->elt_size * sizeof(char *) )

Why is words->elt_size needed here - result is an array of char*?"

words->elt_size give us the size of the largest substitution we're
making, word->nelts+1 is the max number of entires possible in
**result[].


5.) The quoting issue:

Weak quotes do not need to be escaped since they are always inside
strong quotes.  The apr procedure apr_proc_create() that does the work
here has been primed in this instance to call the desired program
directly and not through a shell, so no additional interpretation
happens on the way, what you type is what you get!  If the user adds
escapes it makes no difference at all, it works either way, at least
for GNU diff.

Gabriela

Re: Branch 'invoke-diff-cmd-feature' is ready for half-way review

[Branko Čibej <br...@wandisco.com>]

On 16.10.2013 02:02, Gabriela Gibson wrote:
> Hi,
>
> my branch is ready for a half-way review as the 'invoke-diff-cmd'
> part is complete and adding the 'invoke-diff3-cmd' part would
> only duplicate current errors.

Why are we inventing a new syntax for variable substitutions? I wrote
ages ago that we should just use what svn_config uses. This is just
extra code to do something we already have code for.

-- Brane


-- 
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com

Re: Branch 'invoke-diff-cmd-feature' is ready for half-way review

["roderich.schupp@gmail.com" <ro...@gmail.com>]

Looking over __create_custom_diff_cmd() in ./subversion/libsvn_subr/io.c:

(1) The function doesn't strip double quotes, e.g.

_create_custom_diff_cmd(..., /* cmd= */ "duff \"quoted\"", pool)

will return the array { "duff", "\"quoted\"", NULL } 
You probably need to split cmd into tokens yourself  (instead of using

svn_cstring_split and then trying to fix up the result).


(2) 

  result = apr_palloc(pool, 
                      (words->nelts+1) * words->elt_size * sizeof(char *) )

Why is words->elt_size needed here - result is an array of char*?

(3) Lifetime issue:

      result[argv] = word->data;

word (and hence word->data) has been allocated in scratch_pool (which will be destroyed 
prior to return from __create_custom_diff_cmd), while result has been allocated in pool.


Cheers, Roderich

Re: Branch 'invoke-diff-cmd-feature' is ready for half-way review

["roderich.schupp@gmail.com" <ro...@gmail.com>]

On Wednesday, October 16, 2013 3:02:57 AM UTC+2, Gabriela Gibson wrote:
>
> The BRANCH-README file is here: 
>

   Substitutions: ;f1 original file                       
                  ;f2 changed file 


"svn help diff" uses "old" and "new" (instead of "original" and "changed") 
to refer to these items.
Please stick to established usage.


   Examples: --invoke-diff-cmd="diff -y ;f1 ;f2"        
      --invoke-diff-cmd="kdiff3 -auto -o /home/u/log \  
        +;f1 ;l2 --L1 ;l1 --L2 "Custom Label" "        
In the kdiff3 example the inner double quotes need to be escaped.

 The delimiter ';' can be escaped by adding a ';'

Though it's just a matter of taste, I don't know any precedent for using the semicolon as the "substitution introducer". 
Several tools (e.g. GNU diff, GNU find) use the percent sign for this.

Cheers, Roderich