On Nov 28, 2007 8:44 PM, Stefan Sperling <stsp@elego.de> wrote:
> Hi folks,
>
> Steve and I have spent some time trying to get a patch done
> that is suitable to kick off the tree conflicts branch.
>
> The patch is supposed to show the ideas behind what we are going
> to do on the branch. We don't want to start the branch off by
> committing a diff that is so large that no one will be bothered
> to review it, or will have a hard time doing so.
>
> So we tried to keep implementation details out of it for now (mostly),
> and left a few other changes we've already made here and there for later.
>
> The comments in the treeconflicts.h header give an overview of the
> current design ideas. That file is right at the top of the patch,
> so if you don't want to review the whole thing you can still comfortably
> comment on the design notes if you want to.
>
> Most functions that we are currently planning to add on the branch
> are declared in this patch, and their bodies are either empty
> (if they are obvious) or contain some comments that we plan to change
> into code over time as we continue working on the branch.
>
> It would be great if especially people with skills in either
> or both the working copy and conflict areas could take a look
> and let us know if we are on the right track.
Hi Stefan,
I'm reading the patch and typing comments here. I think it's great
someone finally picks up this issue.
I do have some comments and will just put them here in the order I
encounter them in the patch:
- The description of svn_wc__write_tree_conflict_descs() looks
incomplete (see the line before the last)
- svn_wc__is_tree_conflict_victim() has an output argument
(*tree_conflict_victim), but it's placed in the 'wrong' place: the
convention in our code is to put pools last and output arguments first
in the argument list.
- Since svn_wc__write_tree_conflict_descs() adds information to the
directory entry, how do you envision this to be idempotent? The
comment seems to suggest the loggy system has to do with that, but the
loggy system is there for atomicity (not idempotenticity).
- The patch picks up a comment line in libsvn_wc/wc.h about there
being no #define for DEPTH because it's only relevant to the THIS_DIR
entry. From your description, I get the feeling the same applies to
the TREE_CONFLICT_DATA and TREE_CONFLICT_DESCRIPTION items. Is that
correct? And do you think it's like DEPTH enough to do without the
defines?
Well, that's what I got from cursory review. I'll keep an eye on the
branch too, but if necessary, I'll send more comments on this patch
later.
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 28 21:31:25 2007