On Mon, 2008-11-17 at 14:18 +0100, Stephen Butler wrote:
> Quoting Greg Stein <gstein_at_gmail.com>:
>
> > You need to make it loggy. Writing conflict data immediately will put
> > the directory in an inconsistent state. Consider the metadata at the
> > precise time you write it. You will have a recorded conflict on a file
> > that might not even exist, or on the wrong file. Or wrong revision.
> >
> > The tests happen to work because no test kills the processes randomly
> > to see if it can leave the WC in an inconsistent state.
> >
> > Please revert your change, and in the future keep in mind the integrity
> > of the metadata at each point in time.
>
> Reverted r34235 in r34238.
>
> > On Nov 16, 2008, at 19:48, "Neels J. Hofmeyr" <neels_at_elego.de> wrote:
> >
> >>
> >> Stephen Butler wrote:
> >>> Instead of keeping conflict structs in memory until the parent dir is
> >>> closed, why don't we write each tree conflict immediately? That would
> >>> eliminate the possibility of lost conflicts on cancel.
>
> Whoops, bad idea!
>
> >> I wonder though if there could be any problem with the tree-conflicts being
> >> recorded immediately and all the other node changes stored in the log being
> >> made only later on.
>
> Too true.
>
> >>
> >> I think the "best" fix of this would be to have a new log command, as in
> >> <entry-add-tree-conflict data="B:dir:..."/>. Are there any interface reasons
> >> preventing that?
>
> I think that's kosher. The log execution functions are static, so I'm
> creating log_do_add_tree_conflict() to implement this new command.
(And ..._del_... to complement it, of course.)
But, Steve, hold on a sec. I think that's a distraction and not a
solution. Let's take a look at why it sounds like that change would be
helpful.
The problem is that cached entries get out of sync with entries on disk
if we're not careful when we write and then run a log file.
This mis-match breaks anything that subsequently reads from the
adm_access, including svn_wc__[loggy_]add_tree_conflict().
However, the breakage happens because we write and run a log file and
then carry on using the adm_access without updating the cache. (I guess
we're supposed to close the corresponding adm_access to destroy the
cache, or something.)
This change would not solve that the problem, except within the local
context of svn_wc__add/del_tree_conflict(). This change would still
leave the adm_access cache out of sync. Callers would still have the
same problem, except that one instance of where they need to
read-then-write would have gone away.
I think the problem is caused by having functions such as
svn_wc_add_tree_conflict() that claim to update an entry by writing and
running a log file. That doesn't achieve what callers expect. A
(semi-)public function should not use an already-open adm_access and
return with it out of sync.
So, instead of this, if we get rid of svn_wc_add/del_tree_conflict() and
make the callers use the loggy versions of those instead, that gets rid
of one site where we make that mistake. The other sites are in the merge
code, and in the update editor's do_entry_deletion().
The merge code may well want to write and run a log file like
svn_wc_add_tree_conflict(), but it will be in control of creating,
using, and closing the adm_access, and so in control of its own
resynchronisation needs. Or else it could call a new function that does
something like svn_wc__add_tree_conflict() but that takes care of
opening and closing the adm_access so the caller can't see any
unsynchronized data.
I can't quite see how the log handling in do_entry_deletion() is
supposed to work. Will email separately.
There might (should?) be a way of re-synchronizing, which would be
perhaps a better solution. I'll solicit WC experts' help.
Does that sound right?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-17 15:14:40 CET