[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r40493 - in trunk/subversion: libsvn_fs_base libsvn_fs_fs libsvn_ra_neon libsvn_ra_serf tests/cmdline

From: Paul Burba <ptburba_at_gmail.com>
Date: Fri, 13 Nov 2009 12:59:04 -0500

On Fri, Nov 13, 2009 at 11:43 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> Paul Burba wrote:
>> On Fri, Nov 13, 2009 at 1:59 AM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
>>> Paul T. Burba wrote:
>>>> Author: pburba
>>>> Date: Thu Nov 12 18:37:57 2009
>>>> New Revision: 40493
>>> [...]
>>>
>>>> Modified: trunk/subversion/libsvn_fs_base/lock.c
>>>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/libsvn_fs_base/lock.c?pathrev=40493&r1=40492&r2=40493
>>>> ==============================================================================
>>>> --- trunk/subversion/libsvn_fs_base/lock.c    Thu Nov 12 15:37:51 2009        (r40492)
>>>> +++ trunk/subversion/libsvn_fs_base/lock.c    Thu Nov 12 18:37:57 2009        (r40493)
>>>> @@ -98,9 +98,18 @@ txn_body_lock(void *baton, trail_t *trai
>>>>    /* While our locking implementation easily supports the locking of
>>>>       nonexistent paths, we deliberately choose not to allow such madness. */
>>>>    if (kind == svn_node_none)
>>>> -    return svn_error_createf(SVN_ERR_FS_NOT_FOUND, NULL,
>>>> -                             "Path '%s' doesn't exist in HEAD revision",
>>>> -                             args->path);
>>>> +    {
>>>> +      if (SVN_IS_VALID_REVNUM(args->current_rev))
>>>> +        return svn_error_createf(
>>>> +          SVN_ERR_FS_OUT_OF_DATE, NULL,
>>>> +          _("Path '%s' doesn't exist in HEAD revision"),
>>>> +          args->path);
>>>> +      else
>>>> +        return svn_error_createf(
>>>> +          SVN_ERR_FS_NOT_FOUND, NULL,
>>>> +          _("Path '%s' doesn't exist in HEAD revision"),
>>>> +          args->path);
>>>> +    }
>>> Did you intend to leave both error messages the same even though the error
>>> *codes* differ?  That seems non-normal, but perhaps has good reasoning in
>>> this case that I can't detect from the provided context?
>>
>> Mike,
>>
>> I kept the old error in place if ((LOCK_ARGS *)BATON)->CURRENT_REV was
>> invalid because I wasn't sure if any callers depend on that.
>
> It's not the continued use of the old error code that concerns me.  It's the
> weirdness that this function can return two different error codes that both
> use the same *error message*.

Sorry Mike, I misunderstood your concern :-\

> Should the error in the 'else' clause

I assume you mean the first error? The error in the else clause if
for when *no* valid current rev is specified.

> there say something like, "Path 'foo'
> doesn't exist in revision REV?" instead of mentioning "HEAD"?  Like I said,
> maybe not -- I can't see enough of the function in your diff to know.

We shouldn't say that in either case. If we are trying to lock a file
in a WC then current_rev is that path's base revision, which does
exist, the problem is that the path doesn't exist at HEAD -- here is
the context you need to see this (In particular note that we call
svn_fs_base__get_path_kind, which give us the kind of
args->path_at_HEAD):

static svn_error_t *
txn_body_lock(void *baton, trail_t *trail)
{
  struct lock_args *args = baton;
  svn_node_kind_t kind = svn_node_file;
  svn_lock_t *existing_lock;
  const char *fs_username;
  svn_lock_t *lock;

  SVN_ERR(svn_fs_base__get_path_kind(&kind, args->path, trail, trail->pool));

  /* Until we implement directory locks someday, we only allow locks
     on files or non-existent paths. */
  if (kind == svn_node_dir)
    return SVN_FS__ERR_NOT_FILE(trail->fs, args->path);

  /* While our locking implementation easily supports the locking of
     nonexistent paths, we deliberately choose not to allow such madness. */
  if (kind == svn_node_none)
    {
      if (SVN_IS_VALID_REVNUM(args->current_rev))
        return svn_error_createf(
          SVN_ERR_FS_OUT_OF_DATE, NULL,
          _("Path '%s' doesn't exist in HEAD revision"),
          args->path, args->current_rev);
      else
        return svn_error_createf(
          SVN_ERR_FS_NOT_FOUND, NULL,
          _("Path '%s' doesn't exist in HEAD revision"),
          args->path);
    }

Perhaps the first error should say something to the effect of 'the
path in your working copy no longer exists at head', e.g.:

[[[
Index: subversion/libsvn_fs_base/lock.c
===================================================================
--- subversion/libsvn_fs_base/lock.c (revision 40495)
+++ subversion/libsvn_fs_base/lock.c (working copy)
@@ -102,8 +102,8 @@
       if (SVN_IS_VALID_REVNUM(args->current_rev))
         return svn_error_createf(
           SVN_ERR_FS_OUT_OF_DATE, NULL,
- _("Path '%s' doesn't exist in HEAD revision"),
- args->path);
+ _("Path '%s@%d' doesn't exist in HEAD revision"),
+ args->path, args->current_rev);
       else
         return svn_error_createf(
           SVN_ERR_FS_NOT_FOUND, NULL,
]]]

Paul

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2417673
Received on 2009-11-13 18:59:16 CET

This is an archived mail posted to the Subversion Dev mailing list.