You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by gb...@apache.org on 2013/05/22 00:57:24 UTC
svn commit: r1485007 -
/subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
Author: gbg
Date: Tue May 21 22:57:23 2013
New Revision: 1485007
URL: http://svn.apache.org/r1485007
Log:
On the invoke-diff-cmd branch: Repair erroneous initialization.
* subversion/svn/io.c
(svn_io_run_external_diff): Change pcalloc to palloc call.
Modified:
subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
Modified: subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
URL: http://svn.apache.org/viewvc/subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c?rev=1485007&r1=1485006&r2=1485007&view=diff
==============================================================================
--- subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c (original)
+++ subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c Tue May 21 22:57:23 2013
@@ -3036,7 +3036,7 @@ svn_io_run_external_diff(const char *dir
for (i = 0, size = 0; cmd[i]; i++)
size += strlen(cmd[i]) + 1;
- failed_command = apr_pcalloc(pool, size * sizeof(char *));
+ failed_command = apr_palloc(pool, size * sizeof(char *));
for (i = 0; cmd[i]; i++)
{
Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/ io.c
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> Bert Huijben wrote on Wed, May 22, 2013 at 10:50:17 +0200:
>>> failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
>>> failed_command = apr_pstrcat(pool, failed_command, " ");
>>
>> Note that apr_pstrcat needs a final NULL argument :)
>>
>> So you could use apr_pstrcat(pool, failed_command, " ", cmd[i], NULL);
>
> You need to explicitly cast the last NULL:
>
> apr_pstrcat(pool, failed_command, " ", cmd[i], (void *)NULL);
>
> (because it's a variadic function so there is no implicit conversion to
> a pointer, when NULL is #define'd to be a 0 narrower than the pointer)
For a strcat, the arguments are pointers to 'char', so it looks better to use (char *)NULL.
- Julian
Re: svn commit: r1485007 -
/subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/
io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Bert Huijben wrote on Wed, May 22, 2013 at 10:50:17 +0200:
>
>
> > -----Original Message-----
> > From: Philip Martin [mailto:philip.martin@wandisco.com]
> > Sent: woensdag 22 mei 2013 10:46
> > To: dev@subversion.apache.org
> > Subject: Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c
> >
> > gbg@apache.org writes:
> >
> > > Author: gbg
> > > Date: Tue May 21 22:57:23 2013
> > > New Revision: 1485007
> > >
> > > URL: http://svn.apache.org/r1485007
> > > Log:
> > > On the invoke-diff-cmd branch: Repair erroneous initialization.
> > >
> > > * subversion/svn/io.c
> > >
> > > (svn_io_run_external_diff): Change pcalloc to palloc call.
> > >
> > > Modified:
> > > subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c
> > >
> > > Modified: subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c
> > > URL: http://svn.apache.org/viewvc/subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c?rev=1485007&r1=1485006&r2=1485007
> > &view=diff
> > >
> > ==========================================================
> > ====================
> > > --- subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c (original)
> > > +++ subversion/branches/invoke-diff-cmd-
> > feature/subversion/libsvn_subr/io.c Tue May 21 22:57:23 2013
> > > @@ -3036,7 +3036,7 @@ svn_io_run_external_diff(const char *dir
> > > for (i = 0, size = 0; cmd[i]; i++)
> > > size += strlen(cmd[i]) + 1;
> > >
> > > - failed_command = apr_pcalloc(pool, size * sizeof(char *));
> > > + failed_command = apr_palloc(pool, size * sizeof(char *));
> > >
> > > for (i = 0; cmd[i]; i++)
> > > {
> > >
> >
> > That's not right. You have calculated 'size' by summing strlen to give
> > you the total text length. Multiplying the text length by the sizeof a
> > pointer is wrong. You could multiply by sizeof(char) but that is 1. You
> > need to add +1 for the terminating null byte.
> >
> > However this is an error path so I'd go for simplicity rather than
> > efficiency:
> >
> > const char *failed_command = "";
> > for (i = 0; cmd[i]; ++i)
> > {
> > failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
> > failed_command = apr_pstrcat(pool, failed_command, " ");
>
> Note that apr_pstrcat needs a final NULL argument :)
>
> So you could use apr_pstrcat(pool, failed_command, " ", cmd[i], NULL);
You need to explicitly cast the last NULL:
apr_pstrcat(pool, failed_command, " ", cmd[i], (void *)NULL);
(because it's a variadic function so there is no implicit conversion to
a pointer, when NULL is #define'd to be a 0 narrower than the pointer)
Re: svn commit: r1485007 -
/subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/
io.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Philip Martin wrote on Wed, May 22, 2013 at 10:44:33 +0100:
> Philip Martin <ph...@wandisco.com> writes:
>
> > My first loop results in a final trailing " " and my second loop results
> > in a leading " ". Perhaps:
> >
> > const char *failed_command = cmd[0];
> > for (i = 1; cmd[i]; ++i)
> > failed_command = apr_psprintf(pool, "%s %s", failed_command, cmd[i]);
>
> Another problem is that you go on to do:
>
> return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
> _("'%s' was expanded to '%s' and returned %d"),
> external_diff_cmd,
> svn_dirent_local_style(failed_command, pool),
> *pexitcode);
>
> which applies local style to the whole command string not just to paths.
> On Windows that will convert all '/' to '\' and command options on
> Windows can use '/' where Unix would use '-'.
I pointed out this issue already in my branch review.
Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@wandisco.com> writes:
> My first loop results in a final trailing " " and my second loop results
> in a leading " ". Perhaps:
>
> const char *failed_command = cmd[0];
> for (i = 1; cmd[i]; ++i)
> failed_command = apr_psprintf(pool, "%s %s", failed_command, cmd[i]);
Another problem is that you go on to do:
return svn_error_createf(SVN_ERR_EXTERNAL_PROGRAM, NULL,
_("'%s' was expanded to '%s' and returned %d"),
external_diff_cmd,
svn_dirent_local_style(failed_command, pool),
*pexitcode);
which applies local style to the whole command string not just to paths.
On Windows that will convert all '/' to '\' and command options on
Windows can use '/' where Unix would use '-'.
--
Philip
Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> However this is an error path so I'd go for simplicity rather than
>> efficiency:
>>
>> const char *failed_command = "";
>> for (i = 0; cmd[i]; ++i)
>> {
>> failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
>> failed_command = apr_pstrcat(pool, failed_command, " ");
>
> Note that apr_pstrcat needs a final NULL argument :)
>
> So you could use apr_pstrcat(pool, failed_command, " ", cmd[i], NULL);
>
> Bert
>> }
>>
>> or:
>>
>> const char *failed_command = "";
>> for (i = 0; cmd[i]; ++i)
>> failed_command = apr_psprintf(pool, "%s %s", failed_command,
>> cmd[i]);
My first loop results in a final trailing " " and my second loop results
in a leading " ". Perhaps:
const char *failed_command = cmd[0];
for (i = 1; cmd[i]; ++i)
failed_command = apr_psprintf(pool, "%s %s", failed_command, cmd[i]);
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
RE: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: woensdag 22 mei 2013 10:46
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c
>
> gbg@apache.org writes:
>
> > Author: gbg
> > Date: Tue May 21 22:57:23 2013
> > New Revision: 1485007
> >
> > URL: http://svn.apache.org/r1485007
> > Log:
> > On the invoke-diff-cmd branch: Repair erroneous initialization.
> >
> > * subversion/svn/io.c
> >
> > (svn_io_run_external_diff): Change pcalloc to palloc call.
> >
> > Modified:
> > subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c
> >
> > Modified: subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c
> > URL: http://svn.apache.org/viewvc/subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c?rev=1485007&r1=1485006&r2=1485007
> &view=diff
> >
> ==========================================================
> ====================
> > --- subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c (original)
> > +++ subversion/branches/invoke-diff-cmd-
> feature/subversion/libsvn_subr/io.c Tue May 21 22:57:23 2013
> > @@ -3036,7 +3036,7 @@ svn_io_run_external_diff(const char *dir
> > for (i = 0, size = 0; cmd[i]; i++)
> > size += strlen(cmd[i]) + 1;
> >
> > - failed_command = apr_pcalloc(pool, size * sizeof(char *));
> > + failed_command = apr_palloc(pool, size * sizeof(char *));
> >
> > for (i = 0; cmd[i]; i++)
> > {
> >
>
> That's not right. You have calculated 'size' by summing strlen to give
> you the total text length. Multiplying the text length by the sizeof a
> pointer is wrong. You could multiply by sizeof(char) but that is 1. You
> need to add +1 for the terminating null byte.
>
> However this is an error path so I'd go for simplicity rather than
> efficiency:
>
> const char *failed_command = "";
> for (i = 0; cmd[i]; ++i)
> {
> failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
> failed_command = apr_pstrcat(pool, failed_command, " ");
Note that apr_pstrcat needs a final NULL argument :)
So you could use apr_pstrcat(pool, failed_command, " ", cmd[i], NULL);
Bert
> }
>
> or:
>
> const char *failed_command = "";
> for (i = 0; cmd[i]; ++i)
> failed_command = apr_psprintf(pool, "%s %s", failed_command,
> cmd[i]);
>
>
>
> --
> Certified & Supported Apache Subversion Downloads:
> http://www.wandisco.com/subversion/download
Re: svn commit: r1485007 - /subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
Posted by Philip Martin <ph...@wandisco.com>.
gbg@apache.org writes:
> Author: gbg
> Date: Tue May 21 22:57:23 2013
> New Revision: 1485007
>
> URL: http://svn.apache.org/r1485007
> Log:
> On the invoke-diff-cmd branch: Repair erroneous initialization.
>
> * subversion/svn/io.c
>
> (svn_io_run_external_diff): Change pcalloc to palloc call.
>
> Modified:
> subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
>
> Modified: subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c
> URL: http://svn.apache.org/viewvc/subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c?rev=1485007&r1=1485006&r2=1485007&view=diff
> ==============================================================================
> --- subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c (original)
> +++ subversion/branches/invoke-diff-cmd-feature/subversion/libsvn_subr/io.c Tue May 21 22:57:23 2013
> @@ -3036,7 +3036,7 @@ svn_io_run_external_diff(const char *dir
> for (i = 0, size = 0; cmd[i]; i++)
> size += strlen(cmd[i]) + 1;
>
> - failed_command = apr_pcalloc(pool, size * sizeof(char *));
> + failed_command = apr_palloc(pool, size * sizeof(char *));
>
> for (i = 0; cmd[i]; i++)
> {
>
That's not right. You have calculated 'size' by summing strlen to give
you the total text length. Multiplying the text length by the sizeof a
pointer is wrong. You could multiply by sizeof(char) but that is 1. You
need to add +1 for the terminating null byte.
However this is an error path so I'd go for simplicity rather than
efficiency:
const char *failed_command = "";
for (i = 0; cmd[i]; ++i)
{
failed_command = apr_pstrcat(pool, failed_command, cmd[i]);
failed_command = apr_pstrcat(pool, failed_command, " ");
}
or:
const char *failed_command = "";
for (i = 0; cmd[i]; ++i)
failed_command = apr_psprintf(pool, "%s %s", failed_command, cmd[i]);
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download