On Sun, Aug 17, 2008 at 07:45:39PM -0400, Karl Fogel wrote:
> firemeteor_at_tigris.org writes:
> > --- branches/issue-2843-dev/subversion/libsvn_wc/update_editor.c
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/update_editor.c
> > @@ -496,19 +496,22 @@ complete_directory(struct edit_baton *eb
> > if (entry && entry->depth == svn_depth_exclude)
> > {
> > char * full_target;
> > - entry->depth = svn_depth_infinity;
> > - SVN_ERR(svn_wc__entries_write(entries, adm_access, pool));
> > /* There is a small chance that the target is gone in the
> > repository. We'd better get rid of the exclude flag now. */
> > full_target = svn_path_join(eb->anchor, eb->target, pool);
> > SVN_ERR(svn_wc__adm_retrieve_internal
> > (&target_access, eb->adm_access, full_target, pool));
> > - if (!target_access)
> > + if (!target_access && entry->kind == svn_node_dir)
> > {
> > int log_number = 0;
> > SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target,
> > &log_number, pool));
> > }
> > + else
> > + {
> > + entry->depth = svn_depth_infinity;
> > + SVN_ERR(svn_wc__entries_write(entries, adm_access, pool));
> > + }
> > }
> > }
> > return SVN_NO_ERROR;
>
> I don't quite understand the conditional. Why does it matter if the
> entry is a dir? If we got no target_access, then the entry is excluded,
> and we should delete the entry, right? But in the current code, we
> could get no target_access and still end up writing out an entry with
> depth_infinity, if the entry is a file.
>
> (There's probably something I'm not understanding here.)
After a second check on this, I think this code is weird and there may be many
corner cases. The dir test possibly because we only exclude directory. But the
very first test should filter out all files. And I even doubt whether the path
(and thus the entry) will be the same as eb->target here. Neverthless, I'll
check this code carefully again.
>
> Regarding the "log_number = 0" thing:
>
> The log_number parameter to do_entry_deletion() was not documented (in
> fact, the entire function was not documented). I made an attempt to
> document it in r32516. Then I looked more carefully at the log message
> for r9868, where that parameter was introduced, and got a better idea of
> what log_number is for; now I'm not sure it's safe for us to just write
> "log_number = 0" there, unless we're sure that no other log files are in
> use right now. Are we sure of that?
I admit that I copied this line from somewhere else and did not dig into the
meanning of 'log_number'. Do you have any suggestion on this potential
problem?
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-28 18:58:12 CEST