On Wed, 2009-11-04, Neels J Hofmeyr wrote:
> Julian Foad wrote:
> > Neels Janosch Hofmeyr wrote:
> >> Hi Hyrum, Bert or other wc-ng pros,
> >>
> >> could someone please glance over this patch and point out stupid use of
> >> wc-ng, if any?
> >
> > Hi Neels. Some comments on the doc strings.
>
> Hey Julian,
>
> thanks for your quality review. I'd like to note apologetically that I
> concentrated on making the behavior exactly the same as it was with ENTRY,
> not quite looking at the design, and it shows in the log messages.
No problem. Sorry it's taken me a couple of days to respond.
> So, this one is supposed to be the step where behavior matches exactly, but
> I think we should also change some behavior. Previously, we had the ENTRY
> and tried to get all information from that. Now, we have more fine-grained
> and distinct information on BASE and WORKING readily available. So we should
> roll it out from that side. And I think that should be done in a separate
> commit.
>
> >
> >> +/* Helper for check_tree_conflict(): query a node's local status.
> >> + * Use svn_wc__db_read_info() and others to return selected bits of info
> >> + * useful for check_tree_conflict().
> >
> > I don't think there's any benefit in saying what functions it calls.
> > (The impl. is likely to change soonish, I guess.)
>
> Agreed. Right now it isn't anything more than a wrapper around the
> __db_read_info(), so I guess that's what I wrote in the comment (vs. just
> writing it out inline in check_tree_conflict() and no comment at all).
Oh, but it was already more than a wrapper around __db_read_info: it
used __db_base_get_info as well.
[...]
> > Please could you describe KIND without reference to the implementation?
> > "Set KIND to the node kind of the (working node? base node? something
> > else?)."
>
> Agreed.
>
> Note that we don't have any tests on the kind/path/peg rev shown in svn info
> with a tree conflict, so we apparently have no clear definition of what it
> should say.
In my mind it's clear. There is an incoming change to this node, and the
change is the difference between two repository locations, PATH1_at_REV1
and PATH2_at_REV2. REV1 and REV2 both exist. PATH1_at_REV1 and/or PATH2_at_REV2
might not exist. The node kind is 'none' if a node doesn't exist.
[...]
> >> +/* Helper for check_tree_conflict() which finds the repository and node
> >> + * paths for recording a tree-conflict.
> >> + *
> >> + * REVISON, REPOS_ROOT_URL and REPOS_RELPATH are return values, and
> >> + * DB, LOCAL_ABSPATH, RESULT_POOL and SCRATCH_POOL are input parameters, all
> >> + * of which are the same as in svn_wc__db_read_info().
> >> + */
> >> +static svn_error_t*
> >> +get_node_uri(svn_revnum_t *revision,
> >> + const char **repos_relpath,
> >> + const char **repos_root_url,
> >> + svn_wc__db_t *db,
> >> + const char *local_abspath,
> >> + apr_pool_t *result_pool,
> >> + apr_pool_t *scratch_pool)
[...]
> > How about,
> >
> > [[[
> > Set *REVISION, *REPOS_RELPATH and *REPOS_ROOT_URL to the revision,
> > path-in-repository and repository root URL (respectively) of the
> > base node implied by LOCAL_ABSPATH from the local filesystem,
> > looked up in DB.
> >
> > Allocate the result strings in RESULT_POOL.
> > ]]]
>
> That's not enough, because in case of an added node, there is no BASE, yet
> it returns stuff. This is svn_wc__get_entry() code stripped down.
> check_tree_conflict()'s inline comments say that it reflects both
> source-left and source-right for all except the REVISION, because they will
> be the same for all cases... something in that line.
>
> We better come from the other side: what *should* it say?
Yes, it's better to approach from that side.
> <design-mode>
>
> This code determines the tree-conflict information shown, i.e.:
> [[[
> Source left: () ^/foo_at_1
> Source right: (file) ^/foo_at_2
> ]]]
> But, what is this "Source" anyway? AFAIK, it's the incoming change; in the
> case of an update to HEAD:
> - "Source left" should be BASE,
> - "Source right" should be HEAD, and
> - The difference between "Source left" and "Source right" is the "action".
Yes, exactly.
> Furthermore,
> - "Target" should be WORKING.
> - The difference between BASE and WORKING (think "Target left" and "Target
> right", respectively) is the "schedule", or the "conflict reason".
Yes.
> (NOTE: If updating to a specific revision like 'update -r5', replace HEAD
> with that revision number.)
>
> What are the implications during an update? I think:
>
> - In the case action=="edit", "Source left" and "Source right" can only
> differ in REVISION. "edit" does not change the kind.
> --> left: PATH_at_BASE with KIND == kind-in-BASE ( == kind-in-HEAD)
> right: PATH_at_HEAD with KIND == kind-in-BASE ( == kind-in-HEAD)
>
> - In the case action=="replace", "Source left" and "Source right" can only
> differ in REVISION and KIND.
> --> left: PATH_at_BASE with KIND == kind-in-BASE
> right: PATH_at_HEAD with KIND == kind-in-HEAD
>
> - In action=="simple add", technically, "Source left" would be <nothing>.
> However, it is more accurate to state the non-existent PATH with the
> revision at which it did not exist, i.e. the revision of its nearest parent
> folder in BASE. The "left" "kind" should be svn_node_none.
> --> left: PATH_at_BASE with KIND == svn_node_none.
> right: PATH_at_HEAD with KIND == kind-in-HEAD
Nearly. Where you said "i.e. the revision of its nearest parent folder
in BASE", normally you can't assume that the parent folder is at the
same BASE revision as one of its children. In this case, where the
schedule is "add", the parent's BASE is the relevant revision UNLESS the
child has metadata recording the special state "I'm updated to rX at
which I'm deleted".
> - In action=="simple delete", "Source *right*" would be <nothing>. Again, it
> is more accurate to say:
> --> left: PATH_at_BASE with KIND == kind-in-BASE
> right: PATH_at_HEAD with KIND == svn_node_none.
Yes.
> - The "add" part of a "copied/moved here" could be seen as a "simple add",
> so that "Source left" is <nothing> and more accurately PATH_at_BASE.
Yes.
> On the
> other hand, it might make sense to state the PATH_at_REV of the copied_from
> location of that copy/move operation. ...?
No. When we update a non-existent child so that it comes into existence,
the incoming change is adding a whole node. The copied_from information
is also available, separately, but is not the left-hand side of the
incoming change.
> - The "delete" part of a "moved away" could be seen as a "simple delete", so
> that "Source right" is <nothing> and more accurately PATH_at_HEAD.
Yes.
> On the other
> hand, it might make sense to state the PATH_at_REV of the moved_to location of
> that move operation. ...?
No. Similar to the "copied/moved here" case above.
> I know that the current code does return KIND == svn_node_unknown in certain
> cases, but I can't think of any justification for that. Either it is 'none'
> or 'file'/'dir', there shouldn't be an 'unknown' case, right?
Correct.
> Right, what about 'switch', then?
> 'switch' is the same as 'update', but now "Source left", being BASE,
> reflects the path where the user's working copy is at, and "Source right"
> reflects the path of the switch target at HEAD. So while the REPOS_ROOT_URL
> will stay the same (cross-repos switch not supported), the REPOS_RELPATH
> should be different between "Source left" and "Source right".
> --> left: CURRENT_WC_PATH_at_BASE with KIND == kind-in-BASE
> right: SWITCH_TO_PATH_at_HEAD with KIND == kind-in-SWITCH_TO_PATH_at_HEAD.
Yes.
> </design-mode>
>
> ...but with this patch, I'd like to not even consider the design yet, only
> cut out the ENTRY bits. It would be nice to have a design refactoring ready
> and commit it right after this one, but I'd rather not wait and commit soon.
>
> So I'll see if I can improve the comments...
OK.
> > p.s. <peeve kind="pet"> We shouldn't need to be told what calls it, and
> > never mind that it's a "helper": every function except "main()" is a
> > helper. Redundant info. </peeve>
>
> "Helper", to me, means: this function is only called by that other function,
> and that is how it is meant to be. The reason why it is a separate function
> is merely cosmetic. No consideration has gone into code re-usability. So if
> someone came along and wanted to use it for something different, she should
> take a close look and possibly refactor / change the comment.
Sure, I understand, but I think that style of programming leads to
unmaintainable code: when the next person comes along and could
potentially re-use some functionality, it's too hard to untangle the
re-usable part from the caller-specific details and so they just create
another caller-specific "helper" function and don't bother to improve
the old one.
Whenever some code is factored out into a separate function, that's
usually a good sign that it's worth making it a clearly defined
re-usable chunk of code, even if it isn't currently used in more than
one place.
> Not good? I'll
> cut it out.
>
> How about this one?
I'm going to send this reply now because I'm not sure how long before I
review the new patch you attached.
Thanks for the detailed response and designing.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415071
Received on 2009-11-06 12:43:34 CET