You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2005/05/26 20:52:02 UTC
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
lundblad@tigris.org writes:
> Author: lundblad
> Date: Wed May 25 16:25:57 2005
> New Revision: 14842
>
> Modified:
> trunk/subversion/clients/cmdline/notify.c
>
> Log:
> Print non-fatal locking errors as warnings instead of errors.
>
> * subversion/clients/cmdline/notify.c (notify): Use svn_handle_warning
> instead of svn_handl_error.
>
>
> Modified: trunk/subversion/clients/cmdline/notify.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/clients/cmdline/notify.c?rev=14842&p1=trunk/subversion/clients/cmdline/notify.c&p2=trunk/subversion/clients/cmdline/notify.c&r1=14841&r2=14842
> ==============================================================================
> --- trunk/subversion/clients/cmdline/notify.c (original)
> +++ trunk/subversion/clients/cmdline/notify.c Wed May 25 16:25:57 2005
> @@ -361,7 +361,7 @@
>
> case svn_wc_notify_failed_lock:
> case svn_wc_notify_failed_unlock:
> - svn_handle_error (n->err, stderr, FALSE);
> + svn_handle_warning (stderr, n->err);
> break;
The first thing that comes to mind is that svn_handle_warning only
prints the first error in the error chain, and so it might be suitable
here as we might be dropping information. I don't know why
svn_handle_warning behaves like that, perhaps it's a bug.
When I try it:
$ svn lock wc/a
svn: warning: Path '/a' is already locked by user 'pm' in filesystem '/home/pm/sw/subversion/obj/repo/db'
It's not really clear why that's a warning rather than an error. Is
it something to do with it being "non-fatal" perhaps?
Also I'm not really clear what constitutes a "non-fatal locking
error":
$ svn lock wc/a wc/x
svn: warning: Path '/a' is already locked by user 'pm' in filesystem '/home/pm/sw/subversion/obj/repo/db'
../svn/subversion/libsvn_fs_fs/err.c:286: (apr_err=160017)
svn: '/x' is not a file in filesystem '/home/pm/sw/subversion/obj/repo/db'
One of those comes out as a warning and the other as an error, but I
see no reason why they should be different. I can't ensure that wc/x
is a file before making the call any more than I can ensure that wc/a
is unlocked.
Finally, svn_handle_warning doesn't print the debugging stuff, I guess
that's another bug in svn_handle_warning.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Philip Martin <ph...@codematters.co.uk> writes:
>> - svn_handle_error (n->err, stderr, FALSE);
>> + svn_handle_warning (stderr, n->err);
>> break;
>
> The first thing that comes to mind is that svn_handle_warning only
> prints the first error in the error chain, and so it might be suitable
s/might/might not/
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by Branko Čibej <br...@xbc.nu>.
Julian Foad wrote:
> Peter N. Lundblad wrote:
>
>> On Wed, 3 Aug 2005, Julian Foad wrote:
>>
>>> Ping! Did this bug ever get fixed? I think the bug being discussed
>>> is that
>>> when "svn lock" fails because somebody else already has the lock, it
>>> was only
>>> giving a warning but should be giving an error.
>>
>>
>> AFAICT, it didn't:-( I think a problematic thing is the
>> SVN_ERR_IS_LOCK_ERROR macro that hardcodes which errors are non-fatal
>> locking errors. That's a public macro:-(
>
>
> Goodness knows how we allowed that to become a public macro. It seems
> to be just a convenience for the RA layers.
>
> Fortunately, changing the implementation of it won't break binary or
> source compatibility (at least not directly, if it has been used
> sensibly). But anyway, even if we can't change it, it doesn't matter
> because we don't have to use it.
>
> So that's not a problem. Would you care to propose a fix, or shall we
> file this bug in the issue tracker?
Just deprecate the damn thing and put a private implementatin somewhere
else (the RA loader? Yikes, do RA layers even have RA-common headers?)
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Which locking errors should be warnings? (was: Re: svn commit: r14842
- trunk/subversion/clients/cmdline)
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 8 Aug 2005, Julian Foad wrote:
> Peter N. Lundblad wrote:
> > AFAICT, it didn't:-( I think a problematic thing is the
> > SVN_ERR_IS_LOCK_ERROR macro that hardcodes which errors are non-fatal
> > locking errors. That's a public macro:-(
>
> Goodness knows how we allowed that to become a public macro. It seems to be
> just a convenience for the RA layers.
>
It was converted from a function to a macro in r13618 FWIW. I don't know
why, but it does't matter that much.
> Fortunately, changing the implementation of it won't break binary or source
> compatibility (at least not directly, if it has been used sensibly). But
> anyway, even if we can't change it, it doesn't matter because we don't have to
> use it.
>
Good points. I think deprecation is best here.
> So that's not a problem. Would you care to propose a fix, or shall we file
> this bug in the issue tracker?
>
Might be better to try to get it resolved right away. Let's start with
the subject of this mail: what errors should be considered non-fatal.
Since the RA layers take multiple paths, this is not up to the clients
(but it is still on the client side of the RA layers in code that we have
released.)
Note that I don't remember any design discussions regarding which errors
should be non-fatal. That's why I'm not sure about the rationale for some
of the errors. Maybe fitz can clarify somewhat here.
I've been trying to come up with a proposal regarding which errors should
and shouldn't be warnings, but I'm getting stuck. I think I need the
following question answered first:
- what is our general policy regarding when something is a warning or an
error?
For example, if I try to lock a file in my WC and I already have it locked
in this WC, then you could view the locking operation as a no-op
(forgetting about metadata such as creation time and lock comment). But
when I try to lock a file locked by me, but without a lock token in this
WC, or if I'm locking a file that is out-of-date, why is that error less
fatal than a "path doesn't exist" error? I'm afraid I don't get the
philosophy here.
Someone with a more well-organized head than me needs to sort this out
before we can get any further. Sorry for this confused mail, but Julian,
now you know why this answer is a little late...
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Wed, 3 Aug 2005, Julian Foad wrote:
>
>>Ping! Did this bug ever get fixed? I think the bug being discussed is that
>>when "svn lock" fails because somebody else already has the lock, it was only
>>giving a warning but should be giving an error.
>
> AFAICT, it didn't:-( I think a problematic thing is the
> SVN_ERR_IS_LOCK_ERROR macro that hardcodes which errors are non-fatal
> locking errors. That's a public macro:-(
Goodness knows how we allowed that to become a public macro. It seems to be
just a convenience for the RA layers.
Fortunately, changing the implementation of it won't break binary or source
compatibility (at least not directly, if it has been used sensibly). But
anyway, even if we can't change it, it doesn't matter because we don't have to
use it.
So that's not a problem. Would you care to propose a fix, or shall we file
this bug in the issue tracker?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 3 Aug 2005, Julian Foad wrote:
> Ping! Did this bug ever get fixed? I think the bug being discussed is that
> when "svn lock" fails because somebody else already has the lock, it was only
> giving a warning but should be giving an error.
>
AFAICT, it didn't:-( I think a problematic thing is the
SVN_ERR_IS_LOCK_ERROR macro that hardcodes which errors are non-fatal
locking errors. That's a public macro:-(
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by Julian Foad <ju...@btopenworld.com>.
Ping! Did this bug ever get fixed? I think the bug being discussed is that
when "svn lock" fails because somebody else already has the lock, it was only
giving a warning but should be giving an error.
- Julian
Peter N. Lundblad wrote:
> On Fri, 27 May 2005, Brian W. Fitzpatrick wrote:
>
>>On May 27, 2005, at 11:40 AM, Philip Martin wrote:
>>
>>> I also checked 'wc/b' and it too
>>>was an unlocked file, in that case somebody beat me and deleted it.
>>>
>>>> It's the same thing
>>>>with attempting to 'svn add' a file that's already been added:
>>>>
>>>>$ touch 1 2 3
>>>>$ svn add 2
>>>>A 2
>>>>$ svn add ?
>>>>A 1
>>>>svn: warning: '2' is already under version control
>>>>A 3
>>>>
>>>>In your case, however, wc/x has been replaced with a directory and I
>>>>consider that to be a different case--you can't just lock the
>>>>directory, and you don't already have a lock on it.
>>>>
>>>>Does that make sense? Am I misunderstanding what you're saying?
>>>
>>>No, it doesn't really make sense. It seems to be arbitrary whether a
>>>particular failure is treated as non-fatal. I suppose we do have
>>>other arbitrary behaviour, whether unversioned files are skipped or
>>>not, but it still strikes me as strange.
>>
>>Well, I think you're right--this is a bug. I think that it's OK to
>>warn on 'svn lock' if we already have the lock in our working copy,
>>but if someone else has the lock, I think an error is in order.
>
> Seems reasonable to me. So this is a bug, but not in the notifier:-) It
> can't do anything about it because it has a void return value. So it is
> probably the svn_client_lock/unlcok that need to decide which errors are
> to be treated as warnings and pass them to the notifier.
>
> Regards,
> //Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 27 May 2005, Brian W. Fitzpatrick wrote:
>
> On May 27, 2005, at 11:40 AM, Philip Martin wrote:
>
> > I also checked 'wc/b' and it too
> > was an unlocked file, in that case somebody beat me and deleted it.
> >
> >
> >> It's the same thing
> >> with attempting to 'svn add' a file that's already been added:
> >>
> >> $ touch 1 2 3
> >> $ svn add 2
> >> A 2
> >> $ svn add ?
> >> A 1
> >> svn: warning: '2' is already under version control
> >> A 3
> >>
> >> In your case, however, wc/x has been replaced with a directory and I
> >> consider that to be a different case--you can't just lock the
> >> directory, and you don't already have a lock on it.
> >>
> >> Does that make sense? Am I misunderstanding what you're saying?
> >>
> >
> > No, it doesn't really make sense. It seems to be arbitrary whether a
> > particular failure is treated as non-fatal. I suppose we do have
> > other arbitrary behaviour, whether unversioned files are skipped or
> > not, but it still strikes me as strange.
>
> Well, I think you're right--this is a bug. I think that it's OK to
> warn on 'svn lock' if we already have the lock in our working copy,
> but if someone else has the lock, I think an error is in order.
>
Seems reasonable to me. So this is a bug, but not in the notifier:-) It
can't do anything about it because it has a void return value. So it is
probably the svn_client_lock/unlcok that need to decide which errors are
to be treated as warnings and pass them to the notifier.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by "Brian W. Fitzpatrick" <fi...@collab.net>.
On May 27, 2005, at 11:40 AM, Philip Martin wrote:
> "Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
>
>
>> I think it's all about intent. Given 'wc/a' and 'wc/b', if you
>> already have a lock on wc/a and then run 'svn lock wc/a wc/b', you've
>> already got a lock on wc/a, so why error? We just inform you that
>> you already had a lock, then move on to wc/b.
>>
>
> I don't have already have the lock, the lock belongs to somebody else.
> Before locking I checked the repository and 'wc/a' was an unlocked
> file, but I was beaten to the lock.
Ah! Now I understand... I thought that your wc already had wc/a
locked. Sorry for being dense.
> I also checked 'wc/b' and it too
> was an unlocked file, in that case somebody beat me and deleted it.
>
>
>> It's the same thing
>> with attempting to 'svn add' a file that's already been added:
>>
>> $ touch 1 2 3
>> $ svn add 2
>> A 2
>> $ svn add ?
>> A 1
>> svn: warning: '2' is already under version control
>> A 3
>>
>> In your case, however, wc/x has been replaced with a directory and I
>> consider that to be a different case--you can't just lock the
>> directory, and you don't already have a lock on it.
>>
>> Does that make sense? Am I misunderstanding what you're saying?
>>
>
> No, it doesn't really make sense. It seems to be arbitrary whether a
> particular failure is treated as non-fatal. I suppose we do have
> other arbitrary behaviour, whether unversioned files are skipped or
> not, but it still strikes me as strange.
Well, I think you're right--this is a bug. I think that it's OK to
warn on 'svn lock' if we already have the lock in our working copy,
but if someone else has the lock, I think an error is in order.
Sorry for the misunderstanding.
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
"Brian W. Fitzpatrick" <fi...@red-bean.com> writes:
> I think it's all about intent. Given 'wc/a' and 'wc/b', if you
> already have a lock on wc/a and then run 'svn lock wc/a wc/b', you've
> already got a lock on wc/a, so why error? We just inform you that
> you already had a lock, then move on to wc/b.
I don't have already have the lock, the lock belongs to somebody else.
Before locking I checked the repository and 'wc/a' was an unlocked
file, but I was beaten to the lock. I also checked 'wc/b' and it too
was an unlocked file, in that case somebody beat me and deleted it.
> It's the same thing
> with attempting to 'svn add' a file that's already been added:
>
> $ touch 1 2 3
> $ svn add 2
> A 2
> $ svn add ?
> A 1
> svn: warning: '2' is already under version control
> A 3
>
> In your case, however, wc/x has been replaced with a directory and I
> consider that to be a different case--you can't just lock the
> directory, and you don't already have a lock on it.
>
> Does that make sense? Am I misunderstanding what you're saying?
No, it doesn't really make sense. It seems to be arbitrary whether a
particular failure is treated as non-fatal. I suppose we do have
other arbitrary behaviour, whether unversioned files are skipped or
not, but it still strikes me as strange.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by "Brian W. Fitzpatrick" <fi...@red-bean.com>.
On May 27, 2005, at 10:51 AM, Philip Martin wrote:
> "Brian W. Fitzpatrick" <fi...@collab.net> writes:
>
>
>> On May 26, 2005, at 3:52 PM, Philip Martin wrote:
>>
>>
>>> Also I'm not really clear what constitutes a "non-fatal locking
>>> error":
>>>
>>> $ svn lock wc/a wc/x
>>> svn: warning: Path '/a' is already locked by user 'pm' in
>>> filesystem '/home/pm/sw/subversion/obj/repo/db'
>>> ../svn/subversion/libsvn_fs_fs/err.c:286: (apr_err=160017)
>>> svn: '/x' is not a file in filesystem '/home/pm/sw/subversion/obj/
>>> repo/db'
>>>
>>> One of those comes out as a warning and the other as an error, but I
>>> see no reason why they should be different. I can't ensure that
>>> wc/x
>>> is a file before making the call any more than I can ensure that
>>> wc/a
>>> is unlocked.
>>>
>>
>> Sure, but you're attempting to lock a file here, not to 'svn add' a
>> file and then lock it. Finding an existing lock ("This file is
>> already locked") is quite different than attempting to lock a non-
>> existent or non-versioned file ("Lock wc/x? I see no wc/x here.").
>>
>
> I don't really understand why they are "quite different".
>
> Both wc/a and wc/x are files in my working copy, neither is locked.
> If I do a check (status or ls) before issuing the lock command I see
> two unlocked files. There's a window between my check and lock
> command during which somebody locks one file and replaces the other
> with a directory, so both lock attempts fail.
> It's not clear to me why we have chosen to make one of those
> "non-fatal". The best I can come up with is that "already locked"
> occurs more often. Is that really it? If an error occurs frequently
> we make it non-fatal?
I think it's all about intent. Given 'wc/a' and 'wc/b', if you
already have a lock on wc/a and then run 'svn lock wc/a wc/b', you've
already got a lock on wc/a, so why error? We just inform you that
you already had a lock, then move on to wc/b. It's the same thing
with attempting to 'svn add' a file that's already been added:
$ touch 1 2 3
$ svn add 2
A 2
$ svn add ?
A 1
svn: warning: '2' is already under version control
A 3
In your case, however, wc/x has been replaced with a directory and I
consider that to be a different case--you can't just lock the
directory, and you don't already have a lock on it.
Does that make sense? Am I misunderstanding what you're saying?
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
"Brian W. Fitzpatrick" <fi...@collab.net> writes:
> On May 26, 2005, at 3:52 PM, Philip Martin wrote:
>
>> Also I'm not really clear what constitutes a "non-fatal locking
>> error":
>>
>> $ svn lock wc/a wc/x
>> svn: warning: Path '/a' is already locked by user 'pm' in
>> filesystem '/home/pm/sw/subversion/obj/repo/db'
>> ../svn/subversion/libsvn_fs_fs/err.c:286: (apr_err=160017)
>> svn: '/x' is not a file in filesystem '/home/pm/sw/subversion/obj/
>> repo/db'
>>
>> One of those comes out as a warning and the other as an error, but I
>> see no reason why they should be different. I can't ensure that wc/x
>> is a file before making the call any more than I can ensure that wc/a
>> is unlocked.
>
> Sure, but you're attempting to lock a file here, not to 'svn add' a
> file and then lock it. Finding an existing lock ("This file is
> already locked") is quite different than attempting to lock a non-
> existent or non-versioned file ("Lock wc/x? I see no wc/x here.").
I don't really understand why they are "quite different".
Both wc/a and wc/x are files in my working copy, neither is locked.
If I do a check (status or ls) before issuing the lock command I see
two unlocked files. There's a window between my check and lock
command during which somebody locks one file and replaces the other
with a directory, so both lock attempts fail.
It's not clear to me why we have chosen to make one of those
"non-fatal". The best I can come up with is that "already locked"
occurs more often. Is that really it? If an error occurs frequently
we make it non-fatal?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14842 - trunk/subversion/clients/cmdline
Posted by "Brian W. Fitzpatrick" <fi...@collab.net>.
On May 26, 2005, at 3:52 PM, Philip Martin wrote:
> lundblad@tigris.org writes:
>
>
>> Author: lundblad
>> Date: Wed May 25 16:25:57 2005
>> New Revision: 14842
>>
>> Modified:
>> trunk/subversion/clients/cmdline/notify.c
>>
>> Log:
>> Print non-fatal locking errors as warnings instead of errors.
>>
>> * subversion/clients/cmdline/notify.c (notify): Use
>> svn_handle_warning
>> instead of svn_handl_error.
>>
>>
>> Modified: trunk/subversion/clients/cmdline/notify.c
>> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/clients/
>> cmdline/notify.c?rev=14842&p1=trunk/subversion/clients/cmdline/
>> notify.c&p2=trunk/subversion/clients/cmdline/
>> notify.c&r1=14841&r2=14842
>> =====================================================================
>> =========
>> --- trunk/subversion/clients/cmdline/notify.c (original)
>> +++ trunk/subversion/clients/cmdline/notify.c Wed May 25
>> 16:25:57 2005
>> @@ -361,7 +361,7 @@
>>
>> case svn_wc_notify_failed_lock:
>> case svn_wc_notify_failed_unlock:
>> - svn_handle_error (n->err, stderr, FALSE);
>> + svn_handle_warning (stderr, n->err);
>> break;
>>
>
> The first thing that comes to mind is that svn_handle_warning only
> prints the first error in the error chain, and so it might be suitable
> here as we might be dropping information. I don't know why
> svn_handle_warning behaves like that, perhaps it's a bug.
>
> When I try it:
>
> $ svn lock wc/a
> svn: warning: Path '/a' is already locked by user 'pm' in
> filesystem '/home/pm/sw/subversion/obj/repo/db'
>
> It's not really clear why that's a warning rather than an error. Is
> it something to do with it being "non-fatal" perhaps?
Yes. By running 'svn lock foo', you're attempting to lock it, and if
it's already locked there's no need to error out--just warn and move
along.
> Also I'm not really clear what constitutes a "non-fatal locking
> error":
>
> $ svn lock wc/a wc/x
> svn: warning: Path '/a' is already locked by user 'pm' in
> filesystem '/home/pm/sw/subversion/obj/repo/db'
> ../svn/subversion/libsvn_fs_fs/err.c:286: (apr_err=160017)
> svn: '/x' is not a file in filesystem '/home/pm/sw/subversion/obj/
> repo/db'
>
> One of those comes out as a warning and the other as an error, but I
> see no reason why they should be different. I can't ensure that wc/x
> is a file before making the call any more than I can ensure that wc/a
> is unlocked.
Sure, but you're attempting to lock a file here, not to 'svn add' a
file and then lock it. Finding an existing lock ("This file is
already locked") is quite different than attempting to lock a non-
existent or non-versioned file ("Lock wc/x? I see no wc/x here.").
> Finally, svn_handle_warning doesn't print the debugging stuff, I guess
> that's another bug in svn_handle_warning.
Agreed. Care to file an issue?
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org