Re: [patch] New --targets option

On Thu, Mar 07, 2002 at 04:46:21PM -0800, Greg Stein wrote:
> On Fri, Mar 01, 2002 at 06:55:22PM -0500, Ben Collins wrote:
> >...
> > +/* Convert a newline seperated list of items into an apr_array_header_t */
> > +#define IS_NEWLINE(c) (c == '\n' || c == '\r')
> > +apr_array_header_t*
> > +svn_cl__newlinelist_to_array(svn_stringbuf_t *buffer, apr_pool_t *pool)
> > +{
> > + apr_array_header_t *array = apr_array_make(pool, DEFAULT_ARRAY_SIZE,
> > + sizeof(svn_stringbuf_t *));
> >
> > + if (buffer != NULL)
> > + {
> > + apr_size_t start = 0, end = 0;
> > + svn_stringbuf_t *item;
> > + while (end < buffer->len)
> > + {
> > + /* Skip blank lines */
> > + while (IS_NEWLINE(buffer->data[start]))
> > + start++;
>
> Probably want to skip all whitespace, rather than just empty lines.

Actually, I don't want to exclude paths with whitespace. I can see where
maybe the file has this:

-------
foo/bar
    blah/bar/foo
 bar/blah/foo
-------

But maybe it should be up to the person providing the file to ensure
lack of spaces at the start of the line.

> > +
> > + end = start;
> > +
> > + /* If end of the string, we're done */
> > + if (start >= buffer->len)
> > + break;
> > +
> > + while (end < buffer->len && !IS_NEWLINE(buffer->data[end]))
> > + end++;
>
> Whitespace here, too, I'd think. See apr_lib.h, apr_isspace().

Same here. The first case above might be ok to skip over white space,
which is almost never at the start of a path. Here, we may truncate in
the middle of a line, where the path does contain white space. E.g.:

------
foo/bar
blah bar foo
------

> >...
> > +++ ./subversion/clients/cmdline/main.c Fri Mar 1 15:13:56 2002
> > @@ -67,6 +67,7 @@
> > {"username", svn_cl__auth_username_opt, 1, "specify a username ARG"},
> > {"password", svn_cl__auth_password_opt, 1, "specify a password ARG"},
> > {"extensions", 'x', 1, "pass \"ARG\" as bundled options to GNU diff"},
> > + {"targets", 't', 1, "pass contents of file \"ARG\" as additional args"},
>
> I don't think the option will be used to enough to warrant a short option.
> I'd suggest only the long --targets option.

Ok. I can take this out.

> >...
>
> Wow. Looks good but for the '-t' thing. I'm not sure whether people think it
> is a good idea or not to have that as a short option.

Makes no difference to be either way. Thanks for reviewing the patch.

-- 
 .----------=======-=-======-=========-----------=====------------=-=-----.
/       Ben Collins    --    Debian GNU/Linux    --    WatchGuard.com      \
`          bcollins@debian.org   --   Ben.Collins@watchguard.com           '
 `---=========------=======-------------=-=-----=-===-======-------=--=---'
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org