[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: Q: editor callback order?

From: Neels J. Hofmeyr <neels_at_elego.de>
Date: Fri, 07 Nov 2008 04:12:05 +0100

Julian Foad wrote:
> OK, I'll attempt a response.
> On Thu, 2008-11-06 at 06:14 +0100, Neels J. Hofmeyr wrote:
>> I'm busy writing code around svn_wc_diff_callbacks3_t that relies on the
>> "Editor" to send *all* prop changes on a directory *before* it starts
>> sending stuff on the children of that directory.
>> More specifically, I'm wedging in a function that gets called as soon as
>> anything is sent on any child node of a directory for the first time. The
>> function wants to claim that it is called after all prop changes on the
>> directory have come in, and before anything is done to the children of that
>> directory. A first SVN_ERR_ASSERT trial of mine suggests that it actually
>> works. But, generally:
>> Is This Ok?
> You're talking about receiving Editor calls and converting them to
> svn_wc_diff_callbacks3_t calls, which is done in the client diff code.
> One of the svn_delta_editor_t promises is:
> * 4. A producer must call @c change_dir_prop on a directory either
> * before opening any of the directory's subdirs or after closing
> * them, but not in the middle.
> So you can't rely on them coming before. I don't know whether you feel
> you've exercised the parts of our code that generate diffs enough to be
> confident that they always do it in this order. I would feel much better
> if you could cope with any order. If you rely on the order you're
> finding now, I think this would only be acceptable as a temporary
> implementation.

Yes, I feared that. I have so far only seen the receiving side of the editor

>> A concern is that the current diff driving code tends to handle dir prop
>> changes on close_dir(), meaning *after* all children. If that has a good and
>> fix reason, I'm screwed. Unless all "late" props are things like mergeinfo.
> Huh, what do you mean by "it works" above, then? This paragraph seems to
> contradict that. Are they referring to different things?

I mean I put an SVN_ERR_ASSERT(<no children seen>) in repos-diff.c's
change_dir_prop() editor function and ran the merge tests without assertion

> But I wonder if this is really such a problem...
>> Why do I want to do this?
>> Both tree-conflicts notification and summarizing diff need to know all prop
>> changes on a directory before iterating the children of that directory:
>> - Tree-conflicts may want to skip all children,
> We're obviously talking about a merge here (as update/switch don't use
> diff_callbacks). But in which scenario is this relevant? I can't picture
> it.

Yes, I was also asking myself whether it was. I am getting a little
entangled in the multitude of possibilities again. Taking a step back and
trying to clarify...

1) This looks like a desirable flow:

 dir_opened() Is there a file on disk instead? -> TC
                    Is this dir missing or unversioned? -> TC
 dir_props_canged() Just apply props.
 IF not TC: Process children.

 dir_added() Is there a file or dir on disk already? -> TC
 dir_props_canged() Just apply props.
 IF not TC: Process children.

Props can come in late, they don't affect tree-conflicts, e.g.:
 IF not TC: Process children.

2) In diff --summarize, this poses a problem for ADD:

diff --summarize
 dir_opened() Silence.
 Process children.
 dir_props_canged() Notify ` M dir'
(this is ok)

diff --summarize
 dir_added() Notify `A dir'
 Process children.
 dir_props_canged() Notify ` M dir'

Dir add and dir prop mod get split into two lines.
This could be handled by a dir_closed() callback in
svn_wc_diff_callbacks3_t, I admit. Now that I get your approval about
sending diff notification on parent directories only after its children. I
still think it's not ideal, but I see why it's preferable anyway.

diff --summarize
 dir_added() Silence.
 Process children.
 dir_props_canged() Silence.
 dir_closed() Notify `AM dir'

3) The current implementation does something like this:

 dir_opened() Is there a file on disk instead? -> TC
                    Is this dir missing or unversioned? -> TC
 IF not TC: Process children.
 dir_props_canged() Did it error with NOT_FOUND or UNVERSIONED? -> TC

 dir_added() Is there a file or dir on disk already? -> TC
 IF not TC: Process children.
 dir_props_canged() Did it error with NOT_FOUND or UNVERSIONED? -> TC

So tree-conflicts on dir prop changes don't have any effect on processing of
the children of that dir. I see now that the dir_prop_changed() callback is
also used for files, and that this tree-conflict detection is intended to
catch files, not really directories. I should have made this sketch a long
time ago... :P
For directories, a NOT_FOUND or UNVERSIONED error in dir_props_changed() is
always really an error, because the same thing was verified to be perfectly
fine upon dir_opened() or dir_added().

Conclusion: My whole point about tree-conflicts and property changes isn't
really valid.

I would still like to illuminate one case, so I completely understand:

Say I run a merge that only wants to change one prop on one directory. Say
there is nothing versioned at that path in the target, and thus, a prop
change is causing a tree-conflict. But how is this caught?

 dir_opened() Is there a file on disk instead? No.
                    Is this dir missing or unversioned? Yes! TC!
 <skip the rest>

How do we ever know the conflict was due to a property change? Do we never
need to know that?

Info currently says "The merge attempted to edit 'modified_dir'". This same
message will show for all of a dir prop change, a child node modification,
or presence of both.

Ok, I think I got it straight this time. I guess it's all fine.

And I guess there is actually no room for improvement, either! We can't pull
dir_opened/dir_added and dir_props_changed together, because opened/added
needs to happen at the beginning[1] and props_changed should be allowed to
happen whenever. And, we also need to re-introduce the dir_closed() callback
to satisfy the needs diff --summarize has.

Sigh. It's firmly wedged this way. I don't like it, because it means the
diff_callbacks3 behaviour stays as complex as it is. But there's nothing I
can do about it. How depressing, I "wasted" a lot of time on this. :P

Comments welcome.

[1] Merge needs to be notified about a directory add() before the children,
to be able to create the directory before its children are created.
Tree-conflict detection needs to be notified about both directory open() and
add() before the children, to be able to skip the children on a
tree-conflict; nevermind prop changes.

>> - Summarizing diff wants to
>> (a) print directories above their children while
> It would be OK for it to print the directory below the children. It
> doesn't have to be above. Maybe you can collect them all in a baton and
> print them on dir_close() (heh, you'd have to resurrect it). You could

Heh, yeah, that's what it's going to come down to now.
And I need to ensure that dir_opened, dir_added and dir_closed callbacks are
actually *called* everywhere, which they aren't in libsvn_client/diff.c.

> store this data in the single diff_callbacks baton; the callbacks don't
> need to support per-directory or per-file batons to do this.

Like, store stuff in a hash or something? To me it seems far simpler to
provide a *dir_baton for the directory diff_callbacks3, in terms of code
complexity and execution time.

>> BTW, the callbacks already rely on a node deletion to come in before a node
>> addition in order to detect a replace. (If it's related at all.)
> That is related in the sense that it's a case where we had to decide to
> impose an ordering where initially we hadn't imposed it. We could
> probably do the same, if we have to, in other respects: we could say
> that from v1.6 onwards the order of calls is restricted to XYZ. But then
> we'd need to consider what would happen when a new client connects to an
> old server.

Right. Blimey.
Do you think we need the diff callbacks simplified that badly? Probably not.


Received on 2008-11-07 04:12:39 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.