Re: wc-ng patch review

Committed the tweaked patch in r40424.
More changes/fixes welcome, as always.

~Neels

Julian Foad wrote:
> 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

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415617