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
callbacks.
>
>> 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
failure.
>
> 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:
merge
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.
merge
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.:
merge
dir_opened()
IF not TC: Process children.
dir_props_canged()
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:
merge
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
merge
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?
merge
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.
~Neels
Received on 2008-11-07 04:12:39 CET