You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ne...@apache.org on 2010/03/11 14:52:15 UTC
svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Author: neels
Date: Thu Mar 11 13:52:15 2010
New Revision: 921848
URL: http://svn.apache.org/viewvc?rev=921848&view=rev
Log:
* subversion/libsvn_wc/adm_ops.c
(svn_wc__get_pristine_contents, svn_wc_get_pristine_contents2):
Improve error handling. Bail on non-file node kind and "weird" node stati
(callers should know better than calling this function in those cases).
Assert that obstructed stati do not appear on file nodes.
svn_wc_get_pristine_contents2() wraps svn_wc__get_pristine_contents() and
is obviously also affected by this patch.
Modified:
subversion/trunk/subversion/libsvn_wc/adm_ops.c
Modified: subversion/trunk/subversion/libsvn_wc/adm_ops.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/adm_ops.c?rev=921848&r1=921847&r2=921848&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_wc/adm_ops.c (original)
+++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Thu Mar 11 13:52:15 2010
@@ -2195,13 +2195,23 @@ svn_wc__get_pristine_contents(svn_stream
apr_pool_t *scratch_pool)
{
svn_wc__db_status_t status;
+ svn_wc__db_kind_t kind;
const char *text_base;
- SVN_ERR(svn_wc__db_read_info(&status,
+ SVN_ERR(svn_wc__db_read_info(&status, &kind,
NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
- NULL, NULL, NULL, NULL, NULL, NULL, NULL,
+ NULL, NULL, NULL, NULL, NULL, NULL,
db, local_abspath, scratch_pool, scratch_pool));
+
+ /* Sanity */
+ if (kind != svn_wc__db_kind_file)
+ return svn_error_createf(SVN_ERR_NODE_UNEXPECTED_KIND, NULL,
+ _("Can only get the pristine contents of files; "
+ "'%s' is not a file"),
+ svn_dirent_local_style(local_abspath,
+ scratch_pool));
+
if (status == svn_wc__db_status_added)
{
/* For an added node, we return an empty stream. Make sure this is not
@@ -2219,9 +2229,32 @@ svn_wc__get_pristine_contents(svn_stream
return SVN_NO_ERROR;
}
}
+ else
+ if (status == svn_wc__db_status_base_deleted)
+ /* We know that the delete of this node has been committed.
+ This should be the same as if called on an unknown path. */
+ return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
+ _("Cannot get the pristine contents of '%s' "
+ "because its delete is already committed"),
+ svn_dirent_local_style(local_abspath,
+ scratch_pool));
+ else
+ if (status == svn_wc__db_status_absent
+ || status == svn_wc__db_status_excluded
+ || status == svn_wc__db_status_not_present
+ || status == svn_wc__db_status_incomplete)
+ return svn_error_createf(SVN_ERR_WC_PATH_UNEXPECTED_STATUS, NULL,
+ _("Cannot get the pristine contents of '%s' "
+ "because it has an unexpected status"),
+ svn_dirent_local_style(local_abspath,
+ scratch_pool));
+ else
+ /* We know that it is a file, so we can't hit the _obstructed stati. */
+ SVN_ERR_ASSERT(status != svn_wc__db_status_obstructed
+ && status != svn_wc__db_status_obstructed_add
+ && status != svn_wc__db_status_obstructed_delete);
/* ### TODO: use pristine store. */
- /* ### TODO: check for non-file node. */
SVN_ERR(svn_wc__text_base_path(&text_base, db, local_abspath, FALSE,
scratch_pool));
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by Neels J Hofmeyr <ne...@elego.de>.
Hyrum K. Wright wrote:
>> Hm, I've used this before, always have.
>> IMHO it looks much better this way, and is easier to edit around...
>> I do take care to have the 'else' in the line just above 'if'.
>
> It's unfortunate if this style has found its way into other places in our codebase.
>
>> But whatever, if Greg is surprised by it, not many people will be using my
>> way. Will fix, but now it's high time for bed.
>
> I don't think it's a question of anybody being surprised, or one style is better than another or Greg being the bees-knees or anything like that. Long ago we picked a coding standard, and it just happened to be the "else if (..." style. I'm sure I don't have to enumerate the benefits to being consistent in our style, but suffice it to say it's the way The World is.
>
> (FWIW, I personally prefer "if (...) {", but we don't do that, either. Old habits die hard.)
>
> -Hyrum
I said *whatever* already :)
we have boundaries in which style is allowed. wasn't aware that this is such
a catastrophe... will fix and heed. whatever.
This is definitely less confusing and less important than some of our API
docco. :P
~Neels
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Mar 11, 2010, at 9:11 PM, Neels J Hofmeyr wrote:
> Greg Stein wrote:
>> On Thu, Mar 11, 2010 at 08:52, <ne...@apache.org> wrote:
>>> ...
>>> +++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Thu Mar 11 13:52:15 2010
>>> ...
>>> @@ -2219,9 +2229,32 @@ svn_wc__get_pristine_contents(svn_stream
>>> return SVN_NO_ERROR;
>>> }
>>> }
>>> + else
>>> + if (status == svn_wc__db_status_base_deleted)
>>
>> Woah. This formatting is incorrect. We always do "else if (...". The
>> code above almost makes it look like the "if" is totally separate
>> since it is at the same indentation level, but it ISN'T. The else
>> dramatically changes the meaning.
>>
>> The above style is used nowhere else in our code. Please fix the
>> several uses in this function.
>
> Hm, I've used this before, always have.
> IMHO it looks much better this way, and is easier to edit around...
> I do take care to have the 'else' in the line just above 'if'.
It's unfortunate if this style has found its way into other places in our codebase.
> But whatever, if Greg is surprised by it, not many people will be using my
> way. Will fix, but now it's high time for bed.
I don't think it's a question of anybody being surprised, or one style is better than another or Greg being the bees-knees or anything like that. Long ago we picked a coding standard, and it just happened to be the "else if (..." style. I'm sure I don't have to enumerate the benefits to being consistent in our style, but suffice it to say it's the way The World is.
(FWIW, I personally prefer "if (...) {", but we don't do that, either. Old habits die hard.)
-Hyrum
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Mar 12, 2010 at 08:15, neels <ne...@gmail.com> wrote:
> On 12 March 2010 04:11, Neels J Hofmeyr <ne...@elego.de> wrote:
>> Greg Stein wrote:
>>>> + else
>>>> + if (status == svn_wc__db_status_base_deleted)
>>>
>>> Woah. This formatting is incorrect. We always do "else if (...". The
>
> So I've grep'd around for else(\n)if occurences.
> Notably I'm the only one that lined up the 'if' with the 'else'. All
> other occurences indent the 'if'.
>
> hyrum, gstein, would you be fine with me adopting the indented-if
> variant? It pleases my eyes *slightly* more. Feel free to say "nah,
> come on, just use 'else if' and done".
You found 10 occurrences of else\n if.
I found 1239 occurrences of else if.
Guess my answer :-)
Cheers,
-g
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by neels <ne...@gmail.com>.
On 12 March 2010 04:11, Neels J Hofmeyr <ne...@elego.de> wrote:
> Greg Stein wrote:
>>> + else
>>> + if (status == svn_wc__db_status_base_deleted)
>>
>> Woah. This formatting is incorrect. We always do "else if (...". The
So I've grep'd around for else(\n)if occurences.
Notably I'm the only one that lined up the 'if' with the 'else'. All
other occurences indent the 'if'.
hyrum, gstein, would you be fine with me adopting the indented-if
variant? It pleases my eyes *slightly* more. Feel free to say "nah,
come on, just use 'else if' and done".
lundblad(r24045)
subversion/libsvn_client/merge.c:1425: else
subversion/libsvn_client/merge.c-1426- if (prop_state)
--
hwright(r26317) / sussman(r3908)
subversion/svn/propset-cmd.c:91: else
subversion/svn/propset-cmd.c-92- if (opt_state->encoding)
--
kfogel(r5500) / julianfoad (r31978)
subversion/svnlook/main.c:1755: else
subversion/svnlook/main.c-1756- if (xml)
--
neelsr(921848)
subversion/libsvn_wc/adm_ops.c:2231: else
subversion/libsvn_wc/adm_ops.c-2232- if (status ==
svn_wc__db_status_not_present)
--
neels(r906110)
subversion/libsvn_wc/update_editor.c:1775: else
subversion/libsvn_wc/update_editor.c-1776- if (reason ==
svn_wc_conflict_reason_added)
subversion/libsvn_wc/update_editor.c:1861: else
subversion/libsvn_wc/update_editor.c-1862- if (base_kind ==
svn_wc__db_kind_dir)
--
gstein(r35917) / rhuijben(r39084)
subversion/libsvn_wc/old-and-busted.c:197: else
subversion/libsvn_wc/old-and-busted.c-198- if (!
svn_uri_is_canonical(*result, pool))
--
kfogel(r31614)
subversion/tests/libsvn_repos/repos-test.c:2155: else
subversion/tests/libsvn_repos/repos-test.c-2156- if
(strcmp(prop_key, SVN_PROP_REVISION_LOG) != 0)
subversion/tests/libsvn_repos/repos-test.c:2216: else
subversion/tests/libsvn_repos/repos-test.c-2217- if (err->apr_err
!= SVN_ERR_BAD_PROPERTY_VALUE)
subversion/tests/libsvn_repos/repos-test.c:2236: else
subversion/tests/libsvn_repos/repos-test.c-2237- if (err->apr_err
!= SVN_ERR_BAD_PROPERTY_VALUE)
~Neels
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by Neels J Hofmeyr <ne...@elego.de>.
Greg Stein wrote:
> On Thu, Mar 11, 2010 at 08:52, <ne...@apache.org> wrote:
>> ...
>> +++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Thu Mar 11 13:52:15 2010
>> ...
>> @@ -2219,9 +2229,32 @@ svn_wc__get_pristine_contents(svn_stream
>> return SVN_NO_ERROR;
>> }
>> }
>> + else
>> + if (status == svn_wc__db_status_base_deleted)
>
> Woah. This formatting is incorrect. We always do "else if (...". The
> code above almost makes it look like the "if" is totally separate
> since it is at the same indentation level, but it ISN'T. The else
> dramatically changes the meaning.
>
> The above style is used nowhere else in our code. Please fix the
> several uses in this function.
Hm, I've used this before, always have.
IMHO it looks much better this way, and is easier to edit around...
I do take care to have the 'else' in the line just above 'if'.
But whatever, if Greg is surprised by it, not many people will be using my
way. Will fix, but now it's high time for bed.
~Neels
Re: svn commit: r921848 - /subversion/trunk/subversion/libsvn_wc/adm_ops.c
Posted by Greg Stein <gs...@gmail.com>.
On Thu, Mar 11, 2010 at 08:52, <ne...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_wc/adm_ops.c Thu Mar 11 13:52:15 2010
>...
> @@ -2219,9 +2229,32 @@ svn_wc__get_pristine_contents(svn_stream
> return SVN_NO_ERROR;
> }
> }
> + else
> + if (status == svn_wc__db_status_base_deleted)
Woah. This formatting is incorrect. We always do "else if (...". The
code above almost makes it look like the "if" is totally separate
since it is at the same indentation level, but it ISN'T. The else
dramatically changes the meaning.
The above style is used nowhere else in our code. Please fix the
several uses in this function.
>...
Cheers,
-g