Re: Diff Project --invoke-diff-cmd part

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;
}
]]]