Sander Striker wrote:
> CMike told me you were doing mass patch review today.
> If you could drop a line when you glanced at the
> svn_wc_status_obstructed patch, I would appreciate
> it. I'm not quite sure how/if I should handle STRICT
> to be frank.
I see that Brane has looked it over also -- I assume you're waiting
for the conversation with him to play out before committing.
(Actually, I'd also like for Mike Pilato to evaluate it; it looks good
to me, as far as intent goes, but Mike always thinks of some weird
corner case, especially when we're changing the conditions under which
errors are returned. Mike?).
I have two comments on the patch:
> This patch adds a new status code to subversion: svn_wc_status_obstructed.
> An entry gets this status when:
>
> a) the entry's kind is different from the filesystem
> b) the entry should be a versioned directory, but the directory
> doesn't appear to be so on the filesystem.
When you're writing the log message, you might want to change phrase
(b). I got confused a couple of times, thinking you were saying that
the directory "doesn't appear to be on the filesystem", because it's
easy to miss the word "so". Of course, if the directory's not there
at all, `svn_wc_status_absent' is the code used.
So maybe:
b) the entry should be a versioned directory, but the directory by
that name is not versioned
or something.
My other thought is regarding your question about the STRICT flag to
svn_wc_statuses(). The doc string says:
If STRICT is non-zero, then if we encounter a path that is not in
the wc, we'll return an error. STRICT should be zero if we're
updating, as the update will catch any non wc path errors (and
properly deal with files that are in the repository but missing
from the wc for whatever reason).
At first I thought "not in the wc" meant the same as "unversioned",
but the following code in assemble_status() shows what's really going
on. This is the only place where `strict' is actually used, instead
of just passed along:
/* If we're in strict mode and we encounter a path that doesn't
exist in the wc, then we return an error*/
if (strict && (path_kind == svn_node_none))
return svn_error_createf (APR_ENOENT, 0, NULL, pool,
"assemble_status: "
"%s: No such file or directory",
path);
So STRICT is about returning an error instead of storing code
svn_wc_status_absent. Thus, strictly speaking, you don't have to pay
attention to STRICT. But in real life, I think it would be best to
see who is passing the STRICT flag to svn_wc_statuses(), and why, and
consider whether those cases will be affected by this new
svn_wc_status_obstructed code. And if you want to clarify the
description of STRICT semantics in the doc string for
svn_wc_statuses(), that'd be great :-).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 6 16:57:50 2002