On Feb 1, 2008, at 1:09 PM, Augie Fackler wrote:
>
> On Feb 1, 2008, at 12:42 PM, David Glasser wrote:
>
>> On Jan 31, 2008 8:15 PM, <kfogel_at_tigris.org> wrote:
>>> Author: kfogel
>>> Date: Thu Jan 31 20:15:40 2008
>>> New Revision: 29127
>>>
>>> Log:
>>> Fix issue #3026: _svn and .svn are silently ignored as file
>>> arguments.
>>>
>>> Patch by: Augie Fackler: <durin42_at_gmail.com>
>>> (Test by me.)
>>> Modified: trunk/subversion/include/svn_error_codes.h
>>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_error_codes.h?pathrev=29127&r1=29126&r2=29127
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- trunk/subversion/include/svn_error_codes.h (original)
>>> +++ trunk/subversion/include/svn_error_codes.h Thu Jan 31
>>> 20:15:40 2008
>>> @@ -1124,6 +1124,10 @@
>>> SVN_ERRDEF(SVN_ERR_UNKNOWN_CHANGELIST,
>>> SVN_ERR_MISC_CATEGORY_START + 24,
>>> "Unknown changelist")
>>> +
>>> + SVN_ERRDEF(SVN_ERR_RESERVED_FILENAME_SPECIFIED,
>>> + SVN_ERR_MISC_CATEGORY_START + 25,
>>> + "Reserved directory name in command line
>>> arguments")
>>
>> Indentation is weird.
>
> Whoops - someone already caught that...sorry.
>
>>>
>>> /* command-line client errors */
>>>
>>>
>>> Modified: trunk/subversion/include/svn_opt.h
>>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/include/svn_opt.h?pathrev=29127&r1=29126&r2=29127
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- trunk/subversion/include/svn_opt.h (original)
>>> +++ trunk/subversion/include/svn_opt.h Thu Jan 31 20:15:40 2008
>>> @@ -490,6 +490,18 @@
>>> apr_array_header_t *known_targets,
>>> apr_pool_t *pool);
>>>
>>> +/**
>>> + * This is the same as svn_opt_args_to_target_array2() except
>>> that it returns
>>> + * errors when a path has the same name as a working copy
>>> directory.
>>> + *
>>> + * @since New in 1.5.
>>> + */
>>
>> We should always document the newest version, and change the old
>> version to say how it differs. I also find this to be confusing: I'd
>> say "a working copy administrative directory". Also, it should be
>> explicit about the fact that while it returns an error (and probably
>> should say which one), it still actually completes the operation.
>> (Please do not backport this revision without fixing this, and the
>> related issue mentioned below!)
>
> *nod* I'll fix that and the below issue in a single patch to clean
> things up.
>
> <snip issues that others fixed>
>>> Modified: trunk/subversion/svn/util.c
>>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/util.c?pathrev=29127&r1=29126&r2=29127
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- trunk/subversion/svn/util.c (original)
>>> +++ trunk/subversion/svn/util.c Thu Jan 31 20:15:40 2008
>>> @@ -1010,6 +1010,28 @@
>>> }
>>>
>>>
>>> +svn_error_t *
>>> +svn_cl__args_to_target_array_print_reserved(apr_array_header_t
>>> **targets,
>>> + apr_getopt_t *os,
>>> + apr_array_header_t
>>> *known_targets,
>>> + apr_pool_t *pool)
>>> +{
>>> + svn_error_t *error = svn_opt_args_to_target_array3(targets, os,
>>> +
>>> known_targets, pool);
>>> + if (error)
>>> + {
>>> + if (error->apr_err == SVN_ERR_RESERVED_FILENAME_SPECIFIED)
>>> + {
>>> + svn_handle_error2(error, stderr, FALSE, "svn: Skipping
>>> argument: ");
>>> + svn_error_clear(error);
>>
>> This is making the interesting assumption that any chain containing
>> this sort of error *only* contains this sort of error. This
>> assumption should be documented in the
>> svn_opt_args_to_target_array3 docstring.
>
> Is there some better way to do this that might be preferable? I'd be
> happy to change svn_opt_args_to_target_array3() to work a bit more
> sanely (I didn't see anything else that could return multiple
> errors, and nesting them seemed the logical choice).
>
>>> + }
>>> + else
>>> + return error;
>>> + }
>>> + return SVN_NO_ERROR;
>>> +}
>>> +
>>> +
>>> /* Helper for svn_cl__get_changelist(); implements
>>> svn_changelist_receiver_t. */
>>> static svn_error_t *
>>>
>
> I'll start working on making these two fixes. Can anyone point me to
> the "correct" way to return multiple errors from a function?
>
> Thanks,
> Augie
Having seen the newer code that was checked in at r29136 and r29138,
is it acceptable to have nested errors like this and just document it
in the docstring?
Augie
(who promises to read newer threads before replying next
time...hopefully)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-02-01 20:13:47 CET