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
Received on 2013-10-18 20:28:20 CEST