On 28/04/13 10:24, Daniel Shahaf wrote:
Alan Barrett wrote on Wed, Apr 24, 2013 at 09:44:11 +0200:
On Tue, 23 Apr 2013, Gabriela Gibson wrote:
Also, a minor design nit (sorry, no code review): The "---f1"
construct is something I've never seen before.
That's why I picked it --- I checked extensively, and no-one uses a
triple dash, so it does exactly what we hope it will: never interfere
with anything that people might do. Also, I think it looks quite
'unixy' and it's easy to read. I expect fewer problems on windows.
Speaking as somebody who might use this feature, I would much prefer a
more familiar notation like "%(f1)". To my eyes, "---f1" does not look
unixy or easy to read; familiar constructs are easier to read than
unfamiliar constructs.
In addition to the familiarity issue, there's an issue with escapes: you
need a way of representing a literal "---f1" sequence that does not
expand to anything. With notation like "%(f1)" there's already a
widespread convention of using "%%" to represent a "%" character that
does not introduce an expnsion.
True. However, both cmd.exe and the Subversion config file parser use
'%' as a metacharacter, and each of them escapes it differently, so the
way to generate a single, literal '%' character would be:
%%%% in ~/.subversion/config;
%% in the value of --config-option=... on unix;
^%^% in the value of --config-option=... on windows;
% not guaranteed to, but works in practice when not followed by either
/[(]/ or /\w+[%]/.
That's going to be challenging to document clearly.
What about $(f1)? That's also familiar (makefile syntax) but might be
a little saner to escape in various contexts.
Daniel
P.S. It appears our config files' %-interpolation feature doesn't kick
in for --config-option's argument. I'm not sure whether that's a good
thing or not.
Hi,
I meant to send the patch tonight and then, I realised I forgot to add
a feature that Julian Foad suggested.
However, I have a cunning plan to address the issue that Alan raised,
he has a good point that for plain usage, uniformity and conformity is
desirable.
I hope this idea will fix the issues danielsh raised too -- might need
some polishing, then again, it might not be as useful as I hope it is.
Here is the current svn_io_create_custom_diff_cmd: (the guts will
change tomorrow, but it will do the same thing will stuff like +---f1
working added on.)
Current svn help diff output (for unix) is: (adjusted for email fit)
--invoke-diff-cmd ARG :
use ARG as format string for external diff command invocation.
Substitutions: %f1 %f2 files to compare
%l1 %l2 user defined labels
Examples: --invoke-diff-cmd="diff -y %f1 %f2
--invoke-diff-cmd="kdiff3 -auto -o /home/u/log \
%f1 %f2 --L1 %l1 --L2 "Custom Label"
The switch symbol '%' can be modified by defining an
alternative symbol string, starting with a non-alpha
numeric character. Example:
--invoke-diff-cmd="--- diff -y ---f1 ---f2
[[[
const char **
svn_io_create_custom_diff_cmd(const char *label1,
const char *label2,
const char *label3,
const char *tmpfile1,
const char *tmpfile2,
const char *base,
const char *cmd,
apr_pool_t *pool)
{
apr_array_header_t * external_diff_cmd, *delimiters;
const char ** ret;
char * delimiter_prefix;
char * default_delimiter_prefix;
int has_custom_delimiter, len, i, j;
apr_pool_t *subpool;
subpool = svn_pool_create(pool);
#ifdef WIN32
default_delimiter_prefix = apr_pstrdup(subpool, "---");
else
default_delimiter_prefix = apr_pstrdup(subpool, "%");
#endif
external_diff_cmd = svn_cstring_split(cmd, " ", FALSE, subpool);
if (cmd[0] > 33 && cmd[0] < 43) /* user requests custom delimiter
prefix */
{
delimiter_prefix = APR_ARRAY_IDX(external_diff_cmd,0, char *);
has_custom_delimiter = 1;
}
else
{
delimiter_prefix = apr_pstrdup(subpool, default_delimiter_prefix);
has_custom_delimiter = 0;
}
delimiters = svn_cstring_split("f1 f2 f3 l1 l2 l3", " ", TRUE, subpool);
{ /* add the selected prefix to the delimiters */
char *s;
s = apr_pcalloc(subpool, (1+
strlen(delimiter_prefix)+
delimiters->nelts)*sizeof(char *));
for (i = 0; i < delimiters->nelts; i++)
{
strcat(s, delimiter_prefix);
strcat(s, APR_ARRAY_IDX(delimiters, i, char *));
strcat(s," ");
}
delimiters = svn_cstring_split(s, " ", FALSE, subpool);
}
ret = apr_pcalloc(subpool,
external_diff_cmd->nelts *
external_diff_cmd->elt_size*sizeof(char *));
len = strlen(delimiter_prefix)+2; /* f1 f2 ... is always size 2*/
/* skip first entry of external_diff_cmd if the delimiter is
customised */
for (j = 0, i = has_custom_delimiter; i < external_diff_cmd->nelts;
i++, j++)
{
const char * atom;
char *c;
int p = 1;
atom = APR_ARRAY_IDX(external_diff_cmd, i, char *);
c = apr_pcalloc(subpool, strlen(atom)*sizeof(char*));
if (0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 0, char *),
len)))
strcat(c,svn_dirent_local_style(tmpfile1, subpool));
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 1, char
*), len)))
strcat(c,svn_dirent_local_style(tmpfile2, subpool));
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 2, char
*), len)))
strcat(c,svn_dirent_local_style(base, subpool));
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 3, char
*), len)))
strcat(c, label1);
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 4, char
*), len)))
strcat(c, label2);
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 5, char
*), len)))
strcat(c, label3);
if (0 == p)
ret[j] = strdup(c);
else
ret[j] = atom;
}
ret[j] = NULL;
svn_pool_destroy(subpool);
return ret;
}
]]]
Hi,
I meant to send the patch tonight and then, I realised I forgot to add
a feature that Julian Foad suggested.
However, I have a cunning plan to address the issue that Alan raised,
he has a good point that for plain usage, uniformity and conformity is
desirable.
I hope this idea will fix the issues danielsh raised too -- might need
some polishing, then again, it might not be as useful as I hope it is.
Here is the current svn_io_create_custom_diff_cmd: (the guts will
change tomorrow, but it will do the same thing will stuff like +---f1
working added on.)
Current svn help diff output (for unix) is: (adjusted for email fit)
--invoke-diff-cmd ARG :
use ARG as format string for external diff command invocation.
Substitutions: %f1 %f2 files to compare
%l1 %l2 user defined labels
Examples: --invoke-diff-cmd="diff -y %f1 %f2
--invoke-diff-cmd="kdiff3 -auto -o /home/u/log \
%f1 %f2 --L1 %l1 --L2 "Custom Label"
The switch symbol '%' can be modified by defining an
alternative symbol string, starting with a non-alpha
numeric character. Example:
--invoke-diff-cmd="--- diff -y ---f1 ---f2
[[[
const char **
svn_io_create_custom_diff_cmd(const char *label1,
const char *label2,
const char *label3,
const char *tmpfile1,
const char *tmpfile2,
const char *base,
const char *cmd,
apr_pool_t *pool)
{
apr_array_header_t * external_diff_cmd, *delimiters;
const char ** ret;
char * delimiter_prefix;
char * default_delimiter_prefix;
int has_custom_delimiter, len, i, j;
apr_pool_t *subpool;
subpool = svn_pool_create(pool);
#ifdef WIN32
default_delimiter_prefix = apr_pstrdup(subpool, "---");
else
default_delimiter_prefix = apr_pstrdup(subpool, "%");
#endif
external_diff_cmd = svn_cstring_split(cmd, " ", FALSE, subpool);
if (cmd[0] > 33 && cmd[0] < 43) /* user requests custom delimter prefix */
{
delimiter_prefix = APR_ARRAY_IDX(external_diff_cmd,0, char *);
has_custom_delimiter = 1;
}
else
{
delimiter_prefix = apr_pstrdup(subpool, default_delimiter_prefix);
has_custom_delimiter = 0;
}
delimiters = svn_cstring_split("f1 f2 f3 l1 l2 l3", " ", TRUE, subpool);
{ /* add the selected prefix to the delimiters */
char *s;
s = apr_pcalloc(subpool, (1+
strlen(delimiter_prefix)+
delimiters->nelts)*sizeof(char *));
for (i = 0; i < delimiters->nelts; i++)
{
strcat(s, delimiter_prefix);
strcat(s, APR_ARRAY_IDX(delimiters, i, char *));
strcat(s," ");
}
delimiters = svn_cstring_split(s, " ", FALSE, subpool);
}
ret = apr_pcalloc(subpool,
external_diff_cmd->nelts *
external_diff_cmd->elt_size*sizeof(char *));
len = strlen(delimiter_prefix)+2; /* f1 f2 ... is always size 2*/
/* skip first entry of external_diff_cmd if the delimiter is customised */
for (j = 0, i = has_custom_delimiter; i < external_diff_cmd->nelts; i++, j++)
{
const char * atom;
char *c;
int p = 1;
apr_array_header_t pick;
atom = APR_ARRAY_IDX(external_diff_cmd, i, char *);
c = apr_pcalloc(subpool, strlen(atom)*sizeof(char*));
if (0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 0, char *), len)))
strcat(c,svn_dirent_local_style(tmpfile1, subpool));
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 1, char *),
len)))
strcat(c,svn_dirent_local_style(tmpfile2, subpool));
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 2, char *),
len)))
strcat(c,svn_dirent_local_style(base, subpool));
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 3, char *),
len)))
strcat(c, label1);
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 4, char *),
len)))
strcat(c, label2);
else if(0 == (p = strncmp(atom, APR_ARRAY_IDX(delimiters, 5, char *),
len)))
strcat(c, label3);
if (0 == p)
ret[j] = strdup(c);
else
ret[j] = atom;
}
ret[j] = NULL;
svn_pool_destroy(subpool);
return ret;
}
]]]
