You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Küng <to...@gmail.com> on 2007/04/04 11:04:52 UTC
bug in libsvn_wc/questions.c
Hi,
There's a problem in the function
svn_wc__text_modified_internal_p() right at the beginning:
/* No matter which way you look at it, the file needs to exist. */
err = svn_io_stat(&finfo, filename,
APR_FINFO_SIZE | APR_FINFO_MTIME | APR_FINFO_TYPE
| APR_FINFO_LINK, pool);
if ((err && APR_STATUS_IS_ENOENT(err->apr_err))
|| !(finfo.filetype & (APR_REG | APR_LNK)))
{
The if clause checks for an error and the wrong filetype. The problem
is that finfo.filetype is an apr_filetype_e enum, but it's treated
like a bitmask in the check.
I think the correct way to check this would be something like this:
if ((err && APR_STATUS_IS_ENOENT(err->apr_err))
|| ((finfo.filetype != APR_REG) && (finfo.filetype != APR_LNK)))
(I discovered this because if the filetype is APR_DIR, the check will
not complain - but later the 'svn st' will call the abort() function)
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in libsvn_wc/questions.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
Lieven Govaerts wrote:
> Peter Lundblad wrote:
>
>> Stefan Küng writes:
>>
>>
>>> Hi,
>>>
>>> There's a problem in the function
>>> svn_wc__text_modified_internal_p() right at the beginning:
>>>
>>> /* No matter which way you look at it, the file needs to exist. */
>>> err = svn_io_stat(&finfo, filename,
>>> APR_FINFO_SIZE | APR_FINFO_MTIME | APR_FINFO_TYPE
>>> | APR_FINFO_LINK, pool);
>>> if ((err && APR_STATUS_IS_ENOENT(err->apr_err))
>>> || !(finfo.filetype & (APR_REG | APR_LNK)))
>>> {
>>>
>>>
>>>
>> Thanks, Stefan! ...and the person who fixes this could also make sure it
>> doesn't give a bogus result if err is set but with another error code.
>>
>>
> If err is set with another error code then that error code will be
> returned to the caller. What's wrong with that? I must be missing
> something obvious:
>
> if ((err && APR_STATUS_IS_ENOENT(err->apr_err))
> || !(finfo.filetype & (APR_REG | APR_LNK)))
> {
> /* There is no entity, or, the entity is not a regular file or link */
> /* So, it can't be modified. */
> svn_error_clear(err);
> *modified_p = FALSE;
> return SVN_NO_ERROR;
> }
> else if (err)
> return err;
>
> @Stefan: can you reproduce that abort just by using the svn client? I'd
> like to add a test for this change.
>
With some extra info I received from Stefan I was able to reproduce the
issue:
- checkout a working copy containing a symlink.
- delete that symlink locally and create a folder with the same name
- run 'svn status' on the working copy
With svn 1.4.3, the folder will be shown as obstructing the original item.
With svn trunk, svn will abort.
I committed the fix for the issue and a regression test in r24506.
There's one thing strange though. Committing a working copy with such an
obstructed item will result on nothing, no output, no new revision, no
error. In fact, if I do the same thing with a normal file (replace a
file with a directory), the commit doesn't give an error either. Is that
expected behavior?
Thanks for the patch Stefan!
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in libsvn_wc/questions.c
Posted by Lieven Govaerts <sv...@mobsol.be>.
Peter Lundblad wrote:
> Stefan Küng writes:
>
>> Hi,
>>
>> There's a problem in the function
>> svn_wc__text_modified_internal_p() right at the beginning:
>>
>> /* No matter which way you look at it, the file needs to exist. */
>> err = svn_io_stat(&finfo, filename,
>> APR_FINFO_SIZE | APR_FINFO_MTIME | APR_FINFO_TYPE
>> | APR_FINFO_LINK, pool);
>> if ((err && APR_STATUS_IS_ENOENT(err->apr_err))
>> || !(finfo.filetype & (APR_REG | APR_LNK)))
>> {
>>
>>
>
> Thanks, Stefan! ...and the person who fixes this could also make sure it
> doesn't give a bogus result if err is set but with another error code.
>
If err is set with another error code then that error code will be
returned to the caller. What's wrong with that? I must be missing
something obvious:
if ((err && APR_STATUS_IS_ENOENT(err->apr_err))
|| !(finfo.filetype & (APR_REG | APR_LNK)))
{
/* There is no entity, or, the entity is not a regular file or link */
/* So, it can't be modified. */
svn_error_clear(err);
*modified_p = FALSE;
return SVN_NO_ERROR;
}
else if (err)
return err;
@Stefan: can you reproduce that abort just by using the svn client? I'd
like to add a test for this change.
Lieven
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: bug in libsvn_wc/questions.c
Posted by Peter Lundblad <pl...@google.com>.
Stefan Küng writes:
> Hi,
>
> There's a problem in the function
> svn_wc__text_modified_internal_p() right at the beginning:
>
> /* No matter which way you look at it, the file needs to exist. */
> err = svn_io_stat(&finfo, filename,
> APR_FINFO_SIZE | APR_FINFO_MTIME | APR_FINFO_TYPE
> | APR_FINFO_LINK, pool);
> if ((err && APR_STATUS_IS_ENOENT(err->apr_err))
> || !(finfo.filetype & (APR_REG | APR_LNK)))
> {
>
Thanks, Stefan! ...and the person who fixes this could also make sure it
doesn't give a bogus result if err is set but with another error code.
> The if clause checks for an error and the wrong filetype. The problem
> is that finfo.filetype is an apr_filetype_e enum, but it's treated
> like a bitmask in the check.
>
> I think the correct way to check this would be something like this:
>
> if ((err && APR_STATUS_IS_ENOENT(err->apr_err))
> || ((finfo.filetype != APR_REG) && (finfo.filetype != APR_LNK)))
^ ! err &&
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org