Greg Stein <gstein@lyra.org> writes:
> > + if (! strcmp (incompletestr, "true"))
> > + entry->incomplete = TRUE;
> > + else if (! strcmp (incompletestr, "false"))
> > + entry->incomplete = FALSE;
> > + else if (! strcmp (incompletestr, ""))
> > + entry->incomplete = FALSE;
>
> These other two states are not possible. The flag can be present and set to
> "true", or it will not be present. There isn't a need to check for "false"
> or the empty string.
I agree. However, I was just copying & pasting the same code for all
our boolean xml entry atts ("deleted", "copied", etc.) I guess I'll
go in there and do a cleanup. :-)
> > + /* The refcount is zero, thus we remove the 'incomplete' flag. */
> > + SVN_ERR (svn_wc_adm_retrieve (&adm_access, eb->adm_access, bdi->path,
> > + pool));
> > + tmp_entry.incomplete = FALSE;
> > + SVN_ERR (svn_wc__entry_modify (adm_access, NULL /* this_dir */,
> > + &tmp_entry,
> > + SVN_WC__ENTRY_MODIFY_INCOMPLETE,
> > + TRUE /* immediate write */, pool));
>
> Will the incomplete flag always be set at this point in the code? IOW,
> should we maybe test and only run this when if 'incomplete' was set?
Opening or adding a directory always sets the 'incomplete' flag; thus
closing a directory will always remove it.
But I guess I could add a sanity-check. If the incomplete flag isn't
there, something is way wrong.
> > + tmp_entry.kind = svn_node_dir;
> > + tmp_entry.deleted = FALSE;
> > + SVN_ERR (svn_wc__entry_modify (adm_access, db->name, &tmp_entry,
> > + (SVN_WC__ENTRY_MODIFY_KIND
> > + | SVN_WC__ENTRY_MODIFY_DELETED),
> > + TRUE /* immediate write */, pool));
>
> Shouldn't that be the "incomplete" flag rather than "deleted" ? And set it
> to TRUE... IOW, the comments don't appear to match the code.
What the heck? Damn, what a stupid pasto! Thanks for catching that!
(I'm glad the crawler isn't searching for the incomplete flag yet.)
Will fix right away, phew.
> Why have the 'modify_flags' variable? Just insert the constants right into
> the call to __entry_modify. You init it to zero, then unconditionally mask
> in values to it. It can be simplified...
Agree, will fix.
> > + else if (! eb->target)
> > + {
> > + /* For an update with a NULL target, this is equivalent to open_dir(): */
>
> Then these two blocks of code should be factored out.
Aw, don't be silly. We're talking about 2 funtion calls -- just
fetching the adm_access baton and calling entry_modify. Wouldn't that
be just a bit of overabstraction? :-)
> Hmm. Further: the following check is to look for an obstructed update. I
> would think that should happen *before* you go and create a new entry.
> Especially because you might have a file sitting there and the above
> modification would force that entry to a directory(!).
Doh, of course! Makes sense, I'll fix that.
As usual, THANK YOU for the great review.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed May 14 02:47:55 2003