Bug in update_editor.c:change_dir_prop?

[Philip Martin <ph...@codematters.co.uk>]

Hello

I'm looking at change_dir_prop in the delta_editor in
update_editor.c.  The logic is

   if propname is wc prop (e.g svn:wc:ra_dav:version-url)
      store immediately in .svn/dir-wcprops
   else if propname is entry prop (e.g. committed-rev)
      store immediately in  .svn/entries
   else
      cache in the access baton and store during close_directory

Is it correct to store entry props in the entries file immediately?
What if the update is interrupted?  If that happens the directory will
retain its original revision, but will have entry props for some other
revision.  Does this matter?  Should entry props be cached and only
stored in close_directory?

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Bug in update_editor.c:change_dir_prop?

[Philip Martin <ph...@codematters.co.uk>]

Karl Fogel <kf...@newton.ch.collab.net> writes:

> As far as I can tell, simply deferring both entry props and wcprops
> until close_directory() would remove the potential damage window
> here.
> 
> Does that sound right to you too?

Yes it does.  I see a benefit beyond simply fixing the bug, namely
that writing to the disk (the entries file and the dir-wcprops file)
will happen less often.

-- 
Philip Martin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: Bug in update_editor.c:change_dir_prop?

[Karl Fogel <kf...@newton.ch.collab.net>]

Philip Martin <ph...@codematters.co.uk> writes:
> I'm looking at change_dir_prop in the delta_editor in
> update_editor.c.  The logic is
> 
>    if propname is wc prop (e.g svn:wc:ra_dav:version-url)
>       store immediately in .svn/dir-wcprops
>    else if propname is entry prop (e.g. committed-rev)
>       store immediately in  .svn/entries
>    else
>       cache in the access baton and store during close_directory
> 
> Is it correct to store entry props in the entries file immediately?
> What if the update is interrupted?  If that happens the directory will
> retain its original revision, but will have entry props for some other
> revision.  Does this matter?  Should entry props be cached and only
> stored in close_directory?

Hmmm.  The current code sounds like a bug, yeah, nice catch.  Looks
like it has always been this way (since revision 652, when the file
was still named get_editor.c).  The reason I checked the history was
to see if we had deliberately done this in response to some bug,
though we would hopefully have left a comment explaining ourselves in
that case.

I guess there are two different problems here: first, the directory's
entry props could get updated before its regular props (since the
regular prop changes are deferred until close_directory).  Second, the
entry props can also be out of sync w.r.t. its wcprops, during an
update.  After all, both kinds of props are written out immediately,
yet by definition they arrive in separate calls, so one or the other
must happen first, leaving windows of time during which they are out
of sync.

(Note that the directory's "text" -- that is, its entries -- is not an
issue, since the entries hold their own revisions.)

With files, we have to time entry prop updates carefully w.r.t. the
text changes, since keyword expansion depends on the entry being up to
date.  Otherwise things like "$Rev$" and "$Author$" get expanded
incorrectly.  But with directories, we don't have this concern.  As
far as I can tell, simply deferring both entry props and wcprops until
close_directory() would remove the potential damage window here.

Does that sound right to you too?

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org