Hi tree-conflicts folks,
I'm intrigued by the tree-conflicts code in update_editor.c that checks for
TCs involving add. Using 'update' and 'switch', I can't find any way to
produce a tree-conflict.
No-one asked for tests about it, so we don't have any.
Does anyone remember what our aim was?
In case anyone is interested, my test scripts for all file|dir combinations
of a simple add vs. add during update and during switch are attached. None
report a tree-conflict.
edit|delete vs. add don't make sense anyway.
It feels like we can simply remove all current 'add' TC code from
update_editor.c...
So what *should* it be like? At least the 'switch' tests of 'add vs. add'
don't look very good, especially when they try to switch back again.
So... I guess... we need to fix those TCs...?
~Neels
Neels Janosch Hofmeyr wrote:
> 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
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415633
#!/bin/bash
## CONFIGURE THIS:
## The next line is the only line you should need to adjust.
## (and uncomment all four lines)
#SVNDIR=/my/svn/trunk
#alias svn=${SVNDIR}/subversion/svn/svn
#alias svnserve=${SVNDIR}/subversion/svnserve/svnserve
#alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin
svn --version
REPOS="`pwd`/repos"
URL="file://$REPOS"
rm -rf repos wc
svnadmin create repos
svn co -q ${URL} wc
set -x
## ACTUAL TEST
svn mkdir -m boo $URL/a
echo a > wc/a
svn add wc/a
svn up wc
svn st wc
svn info wc/a
#!/bin/bash
## CONFIGURE THIS:
## The next line is the only line you should need to adjust.
## (and uncomment all four lines)
#SVNDIR=/my/svn/trunk
#alias svn=${SVNDIR}/subversion/svn/svn
#alias svnserve=${SVNDIR}/subversion/svnserve/svnserve
#alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin
svn --version
REPOS="`pwd`/repos"
URL="file://$REPOS"
rm -rf repos wc wc2
svnadmin create repos
set -x
## ACTUAL TEST
svn co -q ${URL} wc
echo a > wc/a
svn add wc/a
svn co -q ${URL} wc2
echo b > wc2/a
svn add wc2/a
svn ci -mm wc2
svn up --accept=postpone wc
svn st wc
svn info wc/a
#!/bin/bash
## CONFIGURE THIS:
## The next line is the only line you should need to adjust.
## (and uncomment all four lines)
#SVNDIR=/my/svn/trunk
#alias svn=${SVNDIR}/subversion/svn/svn
#alias svnserve=${SVNDIR}/subversion/svnserve/svnserve
#alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin
svn --version
REPOS="`pwd`/repos"
URL="file://$REPOS"
rm -rf repos wc wc2
svnadmin create repos
set -x
## ACTUAL TEST
svn co -q ${URL} wc
mkdir wc/a
svn add wc/a
svn co -q ${URL} wc2
mkdir wc2/a
svn add wc2/a
svn ci -mm wc2
svn up wc
svn st wc
svn info wc/a
#!/bin/bash
## CONFIGURE THIS:
## The next line is the only line you should need to adjust.
## (and uncomment all four lines)
#SVNDIR=/my/svn/trunk
#alias svn=${SVNDIR}/subversion/svn/svn
#alias svnserve=${SVNDIR}/subversion/svnserve/svnserve
#alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin
svn --version
REPOS="`pwd`/repos"
URL="file://$REPOS"
rm -rf repos wc
svnadmin create repos
svn co -q ${URL} wc
set -x
cd wc
## ACTUAL TEST
mkdir trunk
echo a > trunk/a
svn add trunk
svn ci -mm
svn cp -mm ^/trunk ^/branch
svn up
svn mkdir branch/b
svn ci -mm
svn up
echo b > trunk/b
svn add trunk/b
cd trunk
svn st
svn switch ^/branch
svn info b
#!/bin/bash
## CONFIGURE THIS:
## The next line is the only line you should need to adjust.
## (and uncomment all four lines)
#SVNDIR=/my/svn/trunk
#alias svn=${SVNDIR}/subversion/svn/svn
#alias svnserve=${SVNDIR}/subversion/svnserve/svnserve
#alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin
svn --version
REPOS="`pwd`/repos"
URL="file://$REPOS"
rm -rf repos wc
svnadmin create repos
svn co -q ${URL} wc
set -x
cd wc
## ACTUAL TEST
mkdir trunk
svn add trunk
svn ci -mm
svn cp -mm ^/trunk ^/branch
svn up
svn mkdir branch/b
svn ci -mm
svn up
mkdir trunk/b
echo important data > trunk/b/c
svn add trunk/b
cd trunk
svn st
svn switch ^/branch
svn st
svn info b
cat b/c
#echo "just for interest, what if we switch back?"
#svn switch ^/trunk
#svn st
#svn info b
#ls
#!/bin/bash
## CONFIGURE THIS:
## The next line is the only line you should need to adjust.
## (and uncomment all four lines)
#SVNDIR=/my/svn/trunk
#alias svn=${SVNDIR}/subversion/svn/svn
#alias svnserve=${SVNDIR}/subversion/svnserve/svnserve
#alias svnadmin=${SVNDIR}/subversion/svnadmin/svnadmin
svn --version
REPOS="`pwd`/repos"
URL="file://$REPOS"
rm -rf repos wc
svnadmin create repos
svn co -q ${URL} wc
set -x
cd wc
## ACTUAL TEST
mkdir trunk
svn add trunk
svn ci -mm
svn cp -mm ^/trunk ^/branch
svn up
echo a > branch/a
svn add branch/a
svn ci -mm
svn up wc
echo important data > trunk/a
svn add trunk/a
cd trunk
svn st
svn switch --accept=postpone ^/branch
svn st
svn info a
cat a
#echo "just for interest, what if we switch back?"
#svn switch ^/trunk
#svn st
#svn info a
#cat a
Received on 2009-11-09 00:32:31 CET