You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2008/09/02 20:18:08 UTC
Tree-conflicts branch - log message / review
Hi, tree conflicts fans.
I have written a complete log message of what is on the tree-conflicts
branch compared with trunk. This is attached and also stored in
README.branch in the root of the branch.
The diff is available for a while at
<http://filebin.ca/ajqeez/tc-r32853.patch>.
Updates to the branch since last time include: resolve a bunch of
non-essential differences such as changes that were better made on the
trunk; catch up with recent changes from trunk; Neels' work on improving
the tests.
Anyone able to pick a small part of it to review, or help with, or just
ping me about, is very welcome.
Thanks.
- Julian
Re: Tree-conflicts branch - log message / review
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> * subversion/include/svn_wc.h
> (svn_wc_conflict_description_t): Add fields representing the
> operation
> resulting in a conflict, and the victim of a tree conflict.
[...]
> ### Can we make the new 'operation' field a universal enhancement,
> rather than
> specific to tree conflicts?
We already answered that. We could do, but there is no good reason to do
it that way around, versus introducing it for tree conflicts first and
then expanding its use into other conflicts later. So ignore this
question.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Tree-conflicts branch - log message / review
Posted by Stephen Butler <sb...@elego.de>.
Quoting Julian Foad <ju...@btopenworld.com>:
> The most helpful thing I/we can produce right now is, I think, a summary
> of what behaviours the branch improves vs. where it fails, so that we
> can see at a glance if there are any regressions and how much fixing is
> required to make it a definite win for putting on trunk.
>
> I think that's the only thing needed before merging to trunk. (I also
> need to check how much it meets its goals, but that is not holding it
> off the trunk.)
>
> If you (anyone) can help evaluate it - perhaps by running the tests and
> looking at the verbose output - I'd very much appreciate it.
>
> Tests (on the branch) that are directly testing tree conflicts:
> libsvn_wc/tree-conflict-data-test.c
> cmdline/info_tests.py 1
> cmdline/update_tests.py 46 47 48 49 50
> cmdline/switch_tests.py 31 32 33 34 35
> cmdline/merge_tests.py 107 108 109 110 111 112 113 114 115 116
> cmdline/tree_conflict_tests.py # 1 through 16
>
> Tests (on the branch) that are failing for related reasons:
Here's some annotations on two themes:
1. What test-patch will make the test pass on the tree-conflicts branch?
As a stopgap, we can patch tests so they pass. We've done this
in the past with quite a few tests that inadvertently create tree
conflicts. At the least, this is an instructive exercise. For
some tests, the "innocent victims", this is good enough.
2. What must we change in the test design to accommodate the new
tree-conflicts design?
On trunk, I understand we want XFAIL tests that describe the
correct behavior:
a. Detect "deep" tree conflicts.
b. Skip the operation (checkout/update/switch/merge) on a tree
that is conflicted, even if the tree is a single file.
Yes, I added another operation to the list. Doh!! When faced
with obstructions, checkout acts like update, and (already)
reports tree conflicts.
Are there any other features to take into account? Soon we'll need
some new tests for new features:
* Resolve tree conflicts interactively.
* Handle tree conflicts involving properties.
* Report and resolve tree conflicts directly, not via the parent
directory (where the implementation just happens to store the
data).
I don't think we need them before merging to trunk, since we haven't
started on those features yet.
Here are my notes on the failing tests:
> cmdline/checkout_tests.py
13 co_with_obstructing_local_adds
Patching:
Needs 'C ' conflict markers in output and status.
Also, the client no longer aborts, so the "ensure checkout fails"
subtests need to become normal run_and_verify_* function calls. I
haven't sorted out yet what the exact output, etc ought to be.
Design:
Need to skip the checkout of a tree upon detecting a tree conflict.
> cmdline/update_tests.py 14 15 31 33 34
cmdline/update_tests.py
14 update_deleted_missing_dir
15 another_hudson_problem
This is a known crash (when updating or checking out an item, and the
item itself turns out to be tree-conflicted). I was working on a fix
when I decided to take a step back and write this email. No need to
change the tests yet.
cmdline/update_tests.py
31 forced_update_failures
33 update_wc_with_replaced_file
34 update_with_obstructing_additions
Patching:
Add 'C ' and stir, to fix the output and status. Some subtests
expecting failure need to be rewritten with run_and_verify_* function
calls.
Design:
Need to skip the update of a tree upon detecting a tree conflict.
> cmdline/switch_tests.py
21 forced_switch_failures
24 switch_with_obstructing_local_adds
Same as update tests.py 31 33 34.
> cmdline/merge_tests.py 19 20 39 68 106
A side note: 19, 20, 39 and 68 are all marked XFail on the
tree-conflicts branch, but not on trunk. I assume we want to patch
them so they pass.
cmdline/merge_tests.py 19 merge_skips_obstructions
Patching:
The usual 'C ' added to output & status of the fourth and fifth
subtests, plus the resolution of a tree conflict in '/A/B' revealed by
the fourth subtest.
Design:
Individual skipped items should not appear in the output of 'svn
merge'. Instead, each tree conflict path should be printed with a
warning that the conflicted path has been skipped. Of course,
eventually the user will have a chance to resolve the conflict
interactively.
In this test, the merge commands are applied to the whole wc_dir. The
client output should report the tree conflict at the root of wc_dir,
not at the /A/B subdir.
(Here are the remaining merge tests. I'll annotate them tomorrow if
nobody else takes them.)
cmdline/merge_tests.py 20 merge_into_missing
cmdline/merge_tests.py 34 merge_dir_and_file_replace
cmdline/merge_tests.py 39 merge_add_over_versioned_file_conflicts
cmdline/merge_tests.py 68 mergeinfo_recording_in_skipped_merge
cmdline/merge_tests.py 103 del_differing_file
(fails for me)
cmdline/merge_tests.py 106 tree_conflicts_and_obstructions
Regards,
Steve
--
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Tree-conflicts branch - log message / review
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-09-02 at 16:03 -0500, Hyrum K. Wright wrote:
> Julian Foad wrote:
> > Hi, tree conflicts fans.
> >
> > I have written a complete log message of what is on the tree-conflicts
> > branch compared with trunk. This is attached and also stored in
> > README.branch in the root of the branch.
> >
> > The diff is available for a while at
> > <http://filebin.ca/ajqeez/tc-r32853.patch>.
> >
> > Updates to the branch since last time include: resolve a bunch of
> > non-essential differences such as changes that were better made on the
> > trunk; catch up with recent changes from trunk; Neels' work on improving
> > the tests.
> >
> > Anyone able to pick a small part of it to review, or help with, or just
> > ping me about, is very welcome.
>
> I haven't reviewed the branch, but I was wondering what the time table for
> merging back to trunk is. I feel that the sooner we can get it to trunk, the
> better, just from a testing/review perspective.
Yes, exactly. The timetable is "yesterday of last month", or something
like that.
Seriously, I'm trying to do stuff to help make it mergeable: both in
terms of coding, but more particularly in terms of making it reviewable
and digestible. This log message is a part of that.
The most helpful thing I/we can produce right now is, I think, a summary
of what behaviours the branch improves vs. where it fails, so that we
can see at a glance if there are any regressions and how much fixing is
required to make it a definite win for putting on trunk.
I think that's the only thing needed before merging to trunk. (I also
need to check how much it meets its goals, but that is not holding it
off the trunk.)
If you (anyone) can help evaluate it - perhaps by running the tests and
looking at the verbose output - I'd very much appreciate it.
Tests (on the branch) that are directly testing tree conflicts:
libsvn_wc/tree-conflict-data-test.c
cmdline/info_tests.py 1
cmdline/update_tests.py 46 47 48 49 50
cmdline/switch_tests.py 31 32 33 34 35
cmdline/merge_tests.py 107 108 109 110 111 112 113 114 115 116
cmdline/tree_conflict_tests.py # 1 through 16
Tests (on the branch) that are failing for related reasons:
cmdline/checkout_tests.py 13
cmdline/update_tests.py 14 15 31 33 34
cmdline/switch_tests.py 21 24
cmdline/merge_tests.py 19 20 39 68 106
Thanks,
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Tree-conflicts branch - log message / review
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
Julian Foad wrote:
> Hi, tree conflicts fans.
>
> I have written a complete log message of what is on the tree-conflicts
> branch compared with trunk. This is attached and also stored in
> README.branch in the root of the branch.
>
> The diff is available for a while at
> <http://filebin.ca/ajqeez/tc-r32853.patch>.
>
> Updates to the branch since last time include: resolve a bunch of
> non-essential differences such as changes that were better made on the
> trunk; catch up with recent changes from trunk; Neels' work on improving
> the tests.
>
> Anyone able to pick a small part of it to review, or help with, or just
> ping me about, is very welcome.
I haven't reviewed the branch, but I was wondering what the time table for
merging back to trunk is. I feel that the sooner we can get it to trunk, the
better, just from a testing/review perspective.
-Hyrum
Re: Tree-conflicts branch - log message / review
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-09-02 at 21:18 +0100, Julian Foad wrote:
> I have written a complete log message of what is on the tree-conflicts
> branch compared with trunk. This is attached and also stored in
> README.branch in the root of the branch.
>
> The diff is available for a while at
> <http://filebin.ca/ajqeez/tc-r32853.patch>.
[...]
> * subversion/include/svn_wc.h
[...]
> (svn_wc_conflict_description_t): Add fields representing the
> operation
> resulting in a conflict, and the victim of a tree conflict.
[...]
> ### The 'victim_path' field is redundant. It should be removed and
> its use
> replaced by use of the basename of 'path'.
I have now done that, in r32866.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User Interface)
Posted by Danil Shopyrin <da...@gmail.com>.
> Write access to some siblings of skipped path can be restricted. In that case:
> A) user will be disappointed because commit will be prohibited;
>
> B) inconsistent mergeinfo will be committed even if user will manually
> remove mergeinfo on write-restricted paths.
>
> Write-restriction rarely occurs in the open-source development but
> it's a common practice in "the industry". And write-restricted users
> are usually not-a-very-experienced programmers. And they can be not
> very familiar with advanced Subversion topics.
It's fair to say that this problem can be avoided if we will not write
mergeinfo for unaffected paths. Am I understand properly, that we are
not requested to write explicit mergeinfo to unaffected siblings of
the skipped paths in that case?
E.g. if not-writing-explicit-mergeinfo-for-unaffected-paths discussed
at http://svn.haxx.se/dev/archive-2008-09/0443.shtml will be
implemented, can we get the following mergeinfo with your approach?
Properties on 'branch':
svn:mergeinfo
/trunk:2,4* <--- Non-inheritable mergeinfo.
Properties on 'branch/bar.c':
svn:mergeinfo
/trunk/bar.c:2,4
Properties on 'branch/subdir':
svn:mergeinfo
<null> <-- subdir isn't somehow affected by the merge.
--
Danil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User Interface)
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Sep 17, 2008 at 8:13 AM, Stefan Sperling <st...@elego.de> wrote:
> On Tue, Sep 16, 2008 at 08:22:51PM -0400, Paul Burba wrote:
>> Hi Stefan,
>>
>> Let's assume your target branch looks like this (these other paths
>> under trunk help better illustrate what I'm going to propose):
>>
>> /branch
>> /branch/bar.c
>> /branch/subdir
>>
>> We tweak the merge logic to set non-inheritable mergeinfo on the merge
>> target describing the merge, and normal mergeinfo on all the target's
>> present children, like this:
>>
>> Properties on 'branch':
>> svn:mergeinfo
>> /trunk:2,4* <--- Non-inheritable mergeinfo.
>> Properties on 'branch/bar.c':
>> svn:mergeinfo
>> /trunk/bar.c:2,4
>> Properties on 'branch/subdir':
>> svn:mergeinfo
>> /trunk/subdir:2,4
>
> Sounds like a plan!
>
>> Of course if the skipped path was a deeper subtree and not an
>> immediate child of our merge target we would adjust accordingly and
>> set the non-inheritable mergeinfo on the skipped path's immediate
>> parent and the normal mergeinfo on the skipped path's siblings.
>
> Yes.
>
>> If we commit the above merge and then merge -c3 into branch now foo.c
>> is added but it only has mergeinfo for r2-3:
>>
>> Properties on 'branch':
>> svn:mergeinfo
>> /trunk:2-3,4*
>> Properties on 'branch/foo.c':
>> svn:mergeinfo
>> /trunk/foo.c:2-3
>> Properties on 'branch/bar.c':
>> svn:mergeinfo
>> /trunk/bar.c:2-4
>> Properties on 'branch/subdir':
>> svn:mergeinfo
>> /trunk/subdir:2-4
>>
>> Note: In my ad hoc testing foo.c isn't getting r2 in it's mergeinfo,
>> it should, but that can be fixed. Assuming that we fix this, if we
>> re-merge -r4 to branch then foo.c gets that change and all the
>> mergeinfo elides* up to branch and we are left with is this:
>>
>> Properties on 'branch':
>> svn:mergeinfo
>> /trunk:2-4
>>
>> I think this change is fairly simple, we already catch similar cases -
>> see libsvn_client/merge.c:get_mergeinfo_paths(). The only difference
>> is that we can't identify these paths at the start of the merge but
>> must catch them once we realize they are skipped. The
>> notification_receiver_baton_t struct keeps track of these already in
>> its skipped_paths member so this shouldn't be too hard.
>
> It's nice to know that you have a simple solution.
> I was afraid of the solution being really complicated,
> but your proposal sounds very straightforward.
...and I hope it is straightforward. I'm sure there will be some
complications...there always are with merge tracking.
>> I can crank out a patch that does what I propose in fairly short order
>> if this idea solves your problem.
>
> Great. I'll gladly test anything you throw at me.
I'll start work on a branch for this sometime this week if no
objections come up.
>> * In the interests of full disclosure, in another thread I am
>> proposing the elimination of automatic mergeinfo elision as a fairly
>> useless feature...ironically here is a case where it is nice to have
>> :-\
>
> So we keep it? :)
I think we should keep elision, but not have it happen automatically
but rather upon request. Rather than going into it too much here just
see this thread: http://svn.haxx.se/dev/archive-2008-09/0595.shtml
Paul
>> > XXX big ugly hack idea: Could we extend the mergeinfo
>> > format so that parent directories can record "negative"
>> > mergeinfo for direct children which were skipped?
>>
>> Befitting a "big ugly hack" there's a problem :-) What if, in your
>> example, r4 also affected trunk directly (e.g. we added a svn:ignore
>> property). It could get a bit ugly if we want a way to record that r4
>> affected trunk, but not one of its (currently) non-existent children.
>
> Oh, I was thinking about extending the mergeinfo format to include
> names for skipped child paths, something like
>
> Properties on 'branch':
> svn:mergeinfo
> /trunk:2-4,foo.c:!3
> ^^^^^
> But your solution is much better, so let's forget about this :)
>
> Thanks!
> Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User
Interface)
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Sep 16, 2008 at 08:22:51PM -0400, Paul Burba wrote:
> Hi Stefan,
>
> Let's assume your target branch looks like this (these other paths
> under trunk help better illustrate what I'm going to propose):
>
> /branch
> /branch/bar.c
> /branch/subdir
>
> We tweak the merge logic to set non-inheritable mergeinfo on the merge
> target describing the merge, and normal mergeinfo on all the target's
> present children, like this:
>
> Properties on 'branch':
> svn:mergeinfo
> /trunk:2,4* <--- Non-inheritable mergeinfo.
> Properties on 'branch/bar.c':
> svn:mergeinfo
> /trunk/bar.c:2,4
> Properties on 'branch/subdir':
> svn:mergeinfo
> /trunk/subdir:2,4
Sounds like a plan!
> Of course if the skipped path was a deeper subtree and not an
> immediate child of our merge target we would adjust accordingly and
> set the non-inheritable mergeinfo on the skipped path's immediate
> parent and the normal mergeinfo on the skipped path's siblings.
Yes.
> If we commit the above merge and then merge -c3 into branch now foo.c
> is added but it only has mergeinfo for r2-3:
>
> Properties on 'branch':
> svn:mergeinfo
> /trunk:2-3,4*
> Properties on 'branch/foo.c':
> svn:mergeinfo
> /trunk/foo.c:2-3
> Properties on 'branch/bar.c':
> svn:mergeinfo
> /trunk/bar.c:2-4
> Properties on 'branch/subdir':
> svn:mergeinfo
> /trunk/subdir:2-4
>
> Note: In my ad hoc testing foo.c isn't getting r2 in it's mergeinfo,
> it should, but that can be fixed. Assuming that we fix this, if we
> re-merge -r4 to branch then foo.c gets that change and all the
> mergeinfo elides* up to branch and we are left with is this:
>
> Properties on 'branch':
> svn:mergeinfo
> /trunk:2-4
>
> I think this change is fairly simple, we already catch similar cases -
> see libsvn_client/merge.c:get_mergeinfo_paths(). The only difference
> is that we can't identify these paths at the start of the merge but
> must catch them once we realize they are skipped. The
> notification_receiver_baton_t struct keeps track of these already in
> its skipped_paths member so this shouldn't be too hard.
It's nice to know that you have a simple solution.
I was afraid of the solution being really complicated,
but your proposal sounds very straightforward.
> I can crank out a patch that does what I propose in fairly short order
> if this idea solves your problem.
Great. I'll gladly test anything you throw at me.
> * In the interests of full disclosure, in another thread I am
> proposing the elimination of automatic mergeinfo elision as a fairly
> useless feature...ironically here is a case where it is nice to have
> :-\
So we keep it? :)
> > XXX big ugly hack idea: Could we extend the mergeinfo
> > format so that parent directories can record "negative"
> > mergeinfo for direct children which were skipped?
>
> Befitting a "big ugly hack" there's a problem :-) What if, in your
> example, r4 also affected trunk directly (e.g. we added a svn:ignore
> property). It could get a bit ugly if we want a way to record that r4
> affected trunk, but not one of its (currently) non-existent children.
Oh, I was thinking about extending the mergeinfo format to include
names for skipped child paths, something like
Properties on 'branch':
svn:mergeinfo
/trunk:2-4,foo.c:!3
^^^^^
But your solution is much better, so let's forget about this :)
Thanks!
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User Interface)
Posted by Danil Shopyrin <da...@gmail.com>.
> There is one serious problem with this solution: It won't work if r4
> affects 'trunk' directly (e.g. a prop change on 'trunk').
>
> Because in that case the prop change in r4 will be merged into
> 'branch' so we need to record it in 'branch's mergeinfo. But if we
> add '/trunk:4' to 'branch's mergeinfo that implies that
> '/branch/foo.c' got r4, which of course it didn't since 'foo.c' isn't
> present yet. I mentioned this problem earlier in this thread - see
> "Befitting a "big ugly hack" there's a problem".
>
> Do you understand why this is a problem? If not let's resolve that first!
We've missed this problem. We understand it and the proposed solution
isn't viable because of it. Sorry.
>> Only part of this revision is merged. It's better
>> than write that r4 is merged "non-inheritable", because
>> 'non-inheritable' merged revisions make no sense for an average user.
>
> So normal 'inheritable' mergeinfo makes sense to users, but
> non-inheritable mergeinfo doesn't? Heck, I'm not very fond of
> mergeinfo as an inheritable property, but it's what we have at the
> moment and I don't see how non-inheritable properties are
> fundamentally more complex.
We're not trying to say that users understand inheritable and don't
understand non-inheritable mergeinfo records. The problem is in the
level of abstraction (keep in mind that we're talking about "very
average Subversion user").
* What's happened: revision r4 is merged incompletely.
* What's recorded: non-inheritable mergeinfo for target and
inheritable mergeinfo for all siblings of the skipped path.
It's not so easy to figure out "what's happened" from "what's
recorded". It will be far more easy for users if "incomplete merge"
will be recorded explicitly.
That's what I'm trying to say.
> Also, am I to take it that you are proposing removing non-inheritable
> mergeinfo?
I'm not proposing to completely remove non-inheritable mergeinfo. I'm
arguing to find a better solution for this problem.
> Unless we are planning on disabling merge-tracking when
> performing shallow merges (i.e. with depths other than infinity), when
> merging into shallow working copies, working copies with switched
> subtrees, or WCs with subtrees missing due to authorization
> restrictions, then we need non-inheritable mergeinfo. These problems
> and the problem we are discussing in this thread are all subsets of
> the same basic issue: How do we account for subtrees that are
> (potentially) affected by a merge but that are missing from the
> working copy. The solution to this class of problems is currently
> non-inheritable mergeinfo. I'd like whatever solution is settled on
> for this problem to the solution we use for this whole class of
> problems.
Am I understand correctly, that with the current approach explicit
mergeinfo will be written to all siblings of skipped (/switched/etc.)
path? There is a problem with access rights management if it is.
Write access to some siblings of skipped path can be restricted. In that case:
A) user will be disappointed because commit will be prohibited;
B) inconsistent mergeinfo will be committed even if user will manually
remove mergeinfo on write-restricted paths.
Write-restriction rarely occurs in the open-source development but
it's a common practice in "the industry". And write-restricted users
are usually not-a-very-experienced programmers. And they can be not
very familiar with advanced Subversion topics.
> Note: This example assumes the ideas proposed in
> http://svn.haxx.se/dev/archive-2008-09/0443.shtml have been
> implemented (but that is hardly a done deal). Specifically, r3
> doesn't affect 'bar.c' so that revision is not recorded in 'bar.c's
> mergeinfo. Not a big deal, but worth noting.
Ok.
>> Now user can find out that r4 is still not merged completely
>
> Seriously? Can you state the general steps one would use to determine
> this? Before you do, keep in mind what "real" mergeinfo looks like;
> go run svn pg svn:mergeinfo -vR on any of the Subversion branches to
> see what I mean. Then tell me how easy it will be to spot a single
> revision or revision range that exists on one subtree but not on the
> others.
As you pointed, the whole proposed approach is not viable (because of
possible target's properties change). But the initial idea will be as
follows:
* user sees that mergeinfo is recorded to some sub-paths but not
recorded to the target;
* this means that the revision isn't merged completely;
* user is not requested to clearly understand the difference between
inheritable and non-inheritable mergeinfo.
What do you think about following solution? In the discussed example
we can do the following:
1) record on target that r4 is merged incompletely
2) record r4 on all affected (and not skipped) paths
Generated mergeinfo can look like this:
Properties on 'branch':
svn:mergeinfo
/trunk:4! (r4 is merged incompletely)
Properties on 'branch/bar.c':
svn:mergeinfo
/trunk/bar.c:4
Properties on 'branch/subdir':
svn:mergeinfo
<null>
I understand that this approach requires introduction of additional
semantics. But I just want to investigate, is it viable or not.
> I'd go as far as to say that the non-inheritable marker '*' makes it
> somewhat easier to spot what revisions haven't been merged completely.
Yes, it can be. But there are following problems:
1) write-restriction problem that's described above;
2) it's better to see 'what's changed' than 'what's not changed'.
Usually, the count of changed paths is smaller than count of unchanged
ones.
>> on branch and merge it once again.
>>
>> We will get the following mergeinfo:
>>
>> Properties on 'branch':
>> svn:mergeinfo
>> /trunk:3,4
>> Properties on 'branch/bar.c':
>> svn:mergeinfo
>> <empty>
>> Properties on 'branch/foo.c':
>> svn:mergeinfo
>> <empty>
>> Properties on 'branch/subdir':
>> svn:mergeinfo
>> <empty>
>>
>> Note than now we can elide mergeinfo on 'branch/bar.c'.
>
> Actually no, until we rewrite the elision code this won't work.
> Because when you perform the merge above, at the end of the merge,
> before elision is attempted you will have mergeinfo that looks like
> this:
I agree. But I think it's not a good idea to discuss elision in
details in this thread.
--
Danil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User Interface)
Posted by Paul Burba <pt...@gmail.com>.
On Mon, Sep 22, 2008 at 8:04 AM, Danil Shopyrin
<da...@gmail.com> wrote:
>> Let's assume your target branch looks like this (these other paths
>> under trunk help better illustrate what I'm going to propose):
>>
>> /branch
>> /branch/bar.c
>> /branch/subdir
>>
>> We tweak the merge logic to set non-inheritable mergeinfo on the merge
>> target describing the merge, and normal mergeinfo on all the target's
>> present children, like this:
>>
>> Properties on 'branch':
>> svn:mergeinfo
>> /trunk:2,4* <--- Non-inheritable mergeinfo.
>> Properties on 'branch/bar.c':
>> svn:mergeinfo
>> /trunk/bar.c:2,4
>> Properties on 'branch/subdir':
>> svn:mergeinfo
>> /trunk/subdir:2,4
>>
>> Of course if the skipped path was a deeper subtree and not an
>> immediate child of our merge target we would adjust accordingly and
>> set the non-inheritable mergeinfo on the skipped path's immediate
>> parent and the normal mergeinfo on the skipped path's siblings.
>>
>> If we commit the above merge and then merge -c3 into branch now foo.c
>> is added but it only has mergeinfo for r2-3:
>>
>> Properties on 'branch':
>> svn:mergeinfo
>> /trunk:2-3,4*
>> Properties on 'branch/foo.c':
>> svn:mergeinfo
>> /trunk/foo.c:2-3
>> Properties on 'branch/bar.c':
>> svn:mergeinfo
>> /trunk/bar.c:2-4
>> Properties on 'branch/subdir':
>> svn:mergeinfo
>> /trunk/subdir:2-4
>>
>> Note: In my ad hoc testing foo.c isn't getting r2 in it's mergeinfo,
>> it should, but that can be fixed. Assuming that we fix this, if we
>> re-merge -r4 to branch then foo.c gets that change and all the
>> mergeinfo elides* up to branch and we are left with is this:
>>
>> Properties on 'branch':
>> svn:mergeinfo
>> /trunk:2-4
>>
>> I think this change is fairly simple, we already catch similar cases -
>> see libsvn_client/merge.c:get_mergeinfo_paths(). The only difference
>> is that we can't identify these paths at the start of the merge but
>> must catch them once we realize they are skipped. The
>> notification_receiver_baton_t struct keeps track of these already in
>> its skipped_paths member so this shouldn't be too hard.
>>
>> I can crank out a patch that does what I propose in fairly short order
>> if this idea solves your problem.
>>
>> * In the interests of full disclosure, in another thread I am
>> proposing the elimination of automatic mergeinfo elision as a fairly
>> useless feature...ironically here is a case where it is nice to have
>> :-\
>>
>>> XXX big ugly hack idea: Could we extend the mergeinfo
>>> format so that parent directories can record "negative"
>>> mergeinfo for direct children which were skipped?
>>
>> Befitting a "big ugly hack" there's a problem :-) What if, in your
>> example, r4 also affected trunk directly (e.g. we added a svn:ignore
>> property). It could get a bit ugly if we want a way to record that r4
>> affected trunk, but not one of its (currently) non-existent children.
>
> We've discussed this informally at VisualSVN and have found
> alternative solution.
>
> Let's assume that our target branch looks like this:
> /branch
> /branch/bar.c
> /branch/subdir
>
> And we have /trunk/foo.c added in r3. Files /trunk/bar.c and
> /trunk/foo.c are both changed in r4.
>
> We are merging r4 to the branch. Since r4 contains changes for an
> absent file (i.e. foo.c will be skipped) this revision can't be
> treated as regularly merged into branch. So the merge should produce
> the following mergeinfo in the trunk:
>
> Properties on 'branch':
> svn:mergeinfo
> <empty> //r4 isn't merged completely so we can't claim that it's merged!
> Properties on 'branch/bar.c':
> svn:mergeinfo
> /trunk/bar.c:4
> Properties on 'branch/subdir':
> svn:mergeinfo
> <empty>
>
> From our point of view, it's far more consistent, because r4 isn't
> actually merged.
There is one serious problem with this solution: It won't work if r4
affects 'trunk' directly (e.g. a prop change on 'trunk').
Because in that case the prop change in r4 will be merged into
'branch' so we need to record it in 'branch's mergeinfo. But if we
add '/trunk:4' to 'branch's mergeinfo that implies that
'/branch/foo.c' got r4, which of course it didn't since 'foo.c' isn't
present yet. I mentioned this problem earlier in this thread - see
"Befitting a "big ugly hack" there's a problem".
Do you understand why this is a problem? If not let's resolve that first!
> Only part of this revision is merged. It's better
> than write that r4 is merged "non-inheritable", because
> 'non-inheritable' merged revisions make no sense for an average user.
So normal 'inheritable' mergeinfo makes sense to users, but
non-inheritable mergeinfo doesn't? Heck, I'm not very fond of
mergeinfo as an inheritable property, but it's what we have at the
moment and I don't see how non-inheritable properties are
fundamentally more complex.
Also, am I to take it that you are proposing removing non-inheritable
mergeinfo? Unless we are planning on disabling merge-tracking when
performing shallow merges (i.e. with depths other than infinity), when
merging into shallow working copies, working copies with switched
subtrees, or WCs with subtrees missing due to authorization
restrictions, then we need non-inheritable mergeinfo. These problems
and the problem we are discussing in this thread are all subsets of
the same basic issue: How do we account for subtrees that are
(potentially) affected by a merge but that are missing from the
working copy. The solution to this class of problems is currently
non-inheritable mergeinfo. I'd like whatever solution is settled on
for this problem to the solution we use for this whole class of
problems.
> Note that we record that /trunk/bar.c has merged changes from r4. So
> there will be no conflicts in the future.
>
> Then we are merging r3 and get the following mergeinfo:
> Properties on 'branch':
> svn:mergeinfo
> /trunk:3
> Properties on 'branch/bar.c':
> svn:mergeinfo
> /trunk/bar.c:4
> Properties on 'branch/foo.c':
> svn:mergeinfo
> <empty>
> Properties on 'branch/subdir':
> svn:mergeinfo
> <empty>
Note: This example assumes the ideas proposed in
http://svn.haxx.se/dev/archive-2008-09/0443.shtml have been
implemented (but that is hardly a done deal). Specifically, r3
doesn't affect 'bar.c' so that revision is not recorded in 'bar.c's
mergeinfo. Not a big deal, but worth noting.
> Now user can find out that r4 is still not merged completely
Seriously? Can you state the general steps one would use to determine
this? Before you do, keep in mind what "real" mergeinfo looks like;
go run svn pg svn:mergeinfo -vR on any of the Subversion branches to
see what I mean. Then tell me how easy it will be to spot a single
revision or revision range that exists on one subtree but not on the
others.
I'd go as far as to say that the non-inheritable marker '*' makes it
somewhat easier to spot what revisions haven't been merged completely.
> on branch and merge it once again.
>
> We will get the following mergeinfo:
>
> Properties on 'branch':
> svn:mergeinfo
> /trunk:3,4
> Properties on 'branch/bar.c':
> svn:mergeinfo
> <empty>
> Properties on 'branch/foo.c':
> svn:mergeinfo
> <empty>
> Properties on 'branch/subdir':
> svn:mergeinfo
> <empty>
>
> Note than now we can elide mergeinfo on 'branch/bar.c'.
Actually no, until we rewrite the elision code this won't work.
Because when you perform the merge above, at the end of the merge,
before elision is attempted you will have mergeinfo that looks like
this:
Properties on 'branch':
svn:mergeinfo
/trunk:3,4
Properties on 'branch/bar.c':
svn:mergeinfo
/trunk/bar.c:4
The mergeinfo on '/branch/bar.c' will not elide to 'branch'. The
elision code needs to be rewritten to contact the server to check that
r3 doesn't impact '/branch/bar.c' and can thus be ignored for purposes
of eliding. Remember that "real" world mergeinfo is not going to be
this simple and thus we might contact the repository a *lot* for this
purpose and pay steep performance penalty for doing so.
> Advantages:
> * user understand that some revisions are merged incompletely;
As stated above I couldn't disagree more with this statement.
> * less mergeinfo records generated;
Agreed, less explicit mergeinfo may be generated. It's not correct in
all situations, but there is less of it.
> * user can easily figure our why r4 mergeinfo is written on /branch/bar.c
I don't want to sound like a broken record, but this behavior doesn't
seem any clearer than what I am proposing.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User
Interface)
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Sep 15, 2008 at 03:23:36PM +0200, Stefan Sperling wrote:
> Say there was a textdelta for file foo.c in revision 4,
> and the file foo.c was created in revision 3.
>
> I am merging revision 4 into a branch.
>
> In the current implementation, revision 4 (the textdelta)
> will be skipped, but mergeinfo will claim that revision 4
> was merged! (See attached recipe script for reference.)
>
> /trunk:2,4
>
> Instead, I'd expect explicit mergeinfo on foo.c with
> revision 4 omitted:
>
> /trunk/foo.c:2
>
> or even:
>
> /trunk/foo.c:2,!4
>
> if such "negative mergeinfo" was feasible -- it may be easier
> to record "We failed to apply revision X to path Y" rather
> than determining the answer to the question "Which revisions
> in the merge range could not be applied to path Y?" after
> a merge of some arbitrary revision range.
Stephen Butler pointed out to me that the client-side merge
editor does not know which revisions individual changes come from.
The merge only knows the merge-left and merge-right revisions, and
gets the resulting diff without knowing how the diff was composed.
So the client may not even know which particular revision fails
to apply.
Thoughts?
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User Interface)
Posted by Danil Shopyrin <da...@gmail.com>.
> Properties on 'branch':
> svn:mergeinfo
> <empty> //r4 isn't merged completely so we can't claim that it's merged!
> Properties on 'branch/bar.c':
> svn:mergeinfo
> /trunk/bar.c:4
> Properties on 'branch/subdir':
> svn:mergeinfo
> <empty>
A little remark. <empty> means <null> or <absent> everywhere in the my
previous post. I forget that there are special <empty> mergeinfos :)
--
Danil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User Interface)
Posted by Danil Shopyrin <da...@gmail.com>.
> Let's assume your target branch looks like this (these other paths
> under trunk help better illustrate what I'm going to propose):
>
> /branch
> /branch/bar.c
> /branch/subdir
>
> We tweak the merge logic to set non-inheritable mergeinfo on the merge
> target describing the merge, and normal mergeinfo on all the target's
> present children, like this:
>
> Properties on 'branch':
> svn:mergeinfo
> /trunk:2,4* <--- Non-inheritable mergeinfo.
> Properties on 'branch/bar.c':
> svn:mergeinfo
> /trunk/bar.c:2,4
> Properties on 'branch/subdir':
> svn:mergeinfo
> /trunk/subdir:2,4
>
> Of course if the skipped path was a deeper subtree and not an
> immediate child of our merge target we would adjust accordingly and
> set the non-inheritable mergeinfo on the skipped path's immediate
> parent and the normal mergeinfo on the skipped path's siblings.
>
> If we commit the above merge and then merge -c3 into branch now foo.c
> is added but it only has mergeinfo for r2-3:
>
> Properties on 'branch':
> svn:mergeinfo
> /trunk:2-3,4*
> Properties on 'branch/foo.c':
> svn:mergeinfo
> /trunk/foo.c:2-3
> Properties on 'branch/bar.c':
> svn:mergeinfo
> /trunk/bar.c:2-4
> Properties on 'branch/subdir':
> svn:mergeinfo
> /trunk/subdir:2-4
>
> Note: In my ad hoc testing foo.c isn't getting r2 in it's mergeinfo,
> it should, but that can be fixed. Assuming that we fix this, if we
> re-merge -r4 to branch then foo.c gets that change and all the
> mergeinfo elides* up to branch and we are left with is this:
>
> Properties on 'branch':
> svn:mergeinfo
> /trunk:2-4
>
> I think this change is fairly simple, we already catch similar cases -
> see libsvn_client/merge.c:get_mergeinfo_paths(). The only difference
> is that we can't identify these paths at the start of the merge but
> must catch them once we realize they are skipped. The
> notification_receiver_baton_t struct keeps track of these already in
> its skipped_paths member so this shouldn't be too hard.
>
> I can crank out a patch that does what I propose in fairly short order
> if this idea solves your problem.
>
> * In the interests of full disclosure, in another thread I am
> proposing the elimination of automatic mergeinfo elision as a fairly
> useless feature...ironically here is a case where it is nice to have
> :-\
>
>> XXX big ugly hack idea: Could we extend the mergeinfo
>> format so that parent directories can record "negative"
>> mergeinfo for direct children which were skipped?
>
> Befitting a "big ugly hack" there's a problem :-) What if, in your
> example, r4 also affected trunk directly (e.g. we added a svn:ignore
> property). It could get a bit ugly if we want a way to record that r4
> affected trunk, but not one of its (currently) non-existent children.
We've discussed this informally at VisualSVN and have found
alternative solution.
Let's assume that our target branch looks like this:
/branch
/branch/bar.c
/branch/subdir
And we have /trunk/foo.c added in r3. Files /trunk/bar.c and
/trunk/foo.c are both changed in r4.
We are merging r4 to the branch. Since r4 contains changes for an
absent file (i.e. foo.c will be skipped) this revision can't be
treated as regularly merged into branch. So the merge should produce
the following mergeinfo in the trunk:
Properties on 'branch':
svn:mergeinfo
<empty> //r4 isn't merged completely so we can't claim that it's merged!
Properties on 'branch/bar.c':
svn:mergeinfo
/trunk/bar.c:4
Properties on 'branch/subdir':
svn:mergeinfo
<empty>
Re: Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User Interface)
Posted by Paul Burba <pt...@gmail.com>.
2008/9/15 Stefan Sperling <st...@elego.de>:
> On Fri, Sep 12, 2008 at 02:34:32PM +0200, Stefan Sperling wrote:
>> On Fri, Sep 12, 2008 at 01:57:53PM +0200, Stefan Sperling wrote:
>> > There are certain types of conflicts that require an additional
>> > merge to be resolved, such as when a textdelta is applied to a file
>> > which is not yet present in the target branch -- the fix is to
>> > *repeat* the merge with a wider revision range, this time including
>> > the revision in which the file was created on the source branch.
>>
>> I just realised that this is wrong.
>> The merge cannot simply be repeated.
>>
>> Because of merge tracking, the file addition would be merged,
>> but the textdelta would not be merged again.
>> So the final merge result would miss the textdelta,
>> unless the user reapplied it manually.
>
> On second thought, this is actually a problem with the way
> we're currently recording mergeinfo. It cannot account
> for failed merges due to missing paths.
>
> Say there was a textdelta for file foo.c in revision 4,
> and the file foo.c was created in revision 3.
>
> I am merging revision 4 into a branch.
>
> In the current implementation, revision 4 (the textdelta)
> will be skipped, but mergeinfo will claim that revision 4
> was merged! (See attached recipe script for reference.)
>
> /trunk:2,4
>
> Instead, I'd expect explicit mergeinfo on foo.c with
> revision 4 omitted:
>
> /trunk/foo.c:2
>
> or even:
>
> /trunk/foo.c:2,!4
>
> if such "negative mergeinfo" was feasible -- it may be easier
> to record "We failed to apply revision X to path Y" rather
> than determining the answer to the question "Which revisions
> in the merge range could not be applied to path Y?" after
> a merge of some arbitrary revision range.
>
> A problem of course is that we can't create create mergeinfo
> for foo.c if it does not yet exist in the branch's working copy :(
>
> So before we can solve this problem, we'd probably need to store
> mergeinfo somewhere else than properties...
>
> Any idea on how we could record mergeinfo for skipped paths
> in the current implementation?
Hi Stefan,
Let's assume your target branch looks like this (these other paths
under trunk help better illustrate what I'm going to propose):
/branch
/branch/bar.c
/branch/subdir
We tweak the merge logic to set non-inheritable mergeinfo on the merge
target describing the merge, and normal mergeinfo on all the target's
present children, like this:
Properties on 'branch':
svn:mergeinfo
/trunk:2,4* <--- Non-inheritable mergeinfo.
Properties on 'branch/bar.c':
svn:mergeinfo
/trunk/bar.c:2,4
Properties on 'branch/subdir':
svn:mergeinfo
/trunk/subdir:2,4
Of course if the skipped path was a deeper subtree and not an
immediate child of our merge target we would adjust accordingly and
set the non-inheritable mergeinfo on the skipped path's immediate
parent and the normal mergeinfo on the skipped path's siblings.
If we commit the above merge and then merge -c3 into branch now foo.c
is added but it only has mergeinfo for r2-3:
Properties on 'branch':
svn:mergeinfo
/trunk:2-3,4*
Properties on 'branch/foo.c':
svn:mergeinfo
/trunk/foo.c:2-3
Properties on 'branch/bar.c':
svn:mergeinfo
/trunk/bar.c:2-4
Properties on 'branch/subdir':
svn:mergeinfo
/trunk/subdir:2-4
Note: In my ad hoc testing foo.c isn't getting r2 in it's mergeinfo,
it should, but that can be fixed. Assuming that we fix this, if we
re-merge -r4 to branch then foo.c gets that change and all the
mergeinfo elides* up to branch and we are left with is this:
Properties on 'branch':
svn:mergeinfo
/trunk:2-4
I think this change is fairly simple, we already catch similar cases -
see libsvn_client/merge.c:get_mergeinfo_paths(). The only difference
is that we can't identify these paths at the start of the merge but
must catch them once we realize they are skipped. The
notification_receiver_baton_t struct keeps track of these already in
its skipped_paths member so this shouldn't be too hard.
I can crank out a patch that does what I propose in fairly short order
if this idea solves your problem.
* In the interests of full disclosure, in another thread I am
proposing the elimination of automatic mergeinfo elision as a fairly
useless feature...ironically here is a case where it is nice to have
:-\
> XXX big ugly hack idea: Could we extend the mergeinfo
> format so that parent directories can record "negative"
> mergeinfo for direct children which were skipped?
Befitting a "big ugly hack" there's a problem :-) What if, in your
example, r4 also affected trunk directly (e.g. we added a svn:ignore
property). It could get a bit ugly if we want a way to record that r4
affected trunk, but not one of its (currently) non-existent children.
Paul
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Mergeinfo for skipped paths? (was: Re: Tree Conflicts and User
Interface)
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 12, 2008 at 02:34:32PM +0200, Stefan Sperling wrote:
> On Fri, Sep 12, 2008 at 01:57:53PM +0200, Stefan Sperling wrote:
> > There are certain types of conflicts that require an additional
> > merge to be resolved, such as when a textdelta is applied to a file
> > which is not yet present in the target branch -- the fix is to
> > *repeat* the merge with a wider revision range, this time including
> > the revision in which the file was created on the source branch.
>
> I just realised that this is wrong.
> The merge cannot simply be repeated.
>
> Because of merge tracking, the file addition would be merged,
> but the textdelta would not be merged again.
> So the final merge result would miss the textdelta,
> unless the user reapplied it manually.
On second thought, this is actually a problem with the way
we're currently recording mergeinfo. It cannot account
for failed merges due to missing paths.
Say there was a textdelta for file foo.c in revision 4,
and the file foo.c was created in revision 3.
I am merging revision 4 into a branch.
In the current implementation, revision 4 (the textdelta)
will be skipped, but mergeinfo will claim that revision 4
was merged! (See attached recipe script for reference.)
/trunk:2,4
Instead, I'd expect explicit mergeinfo on foo.c with
revision 4 omitted:
/trunk/foo.c:2
or even:
/trunk/foo.c:2,!4
if such "negative mergeinfo" was feasible -- it may be easier
to record "We failed to apply revision X to path Y" rather
than determining the answer to the question "Which revisions
in the merge range could not be applied to path Y?" after
a merge of some arbitrary revision range.
A problem of course is that we can't create create mergeinfo
for foo.c if it does not yet exist in the branch's working copy :(
So before we can solve this problem, we'd probably need to store
mergeinfo somewhere else than properties...
Any idea on how we could record mergeinfo for skipped paths
in the current implementation?
XXX big ugly hack idea: Could we extend the mergeinfo
format so that parent directories can record "negative"
mergeinfo for direct children which were skipped?
Stefan
Re: Tree Conflicts and User Interface (was: Re: [PATCH]
Tree-conflicts: do_entry_deletion segfault)
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 12, 2008 at 01:57:53PM +0200, Stefan Sperling wrote:
> There are certain types of conflicts that require an additional
> merge to be resolved, such as when a textdelta is applied to a file
> which is not yet present in the target branch -- the fix is to
> *repeat* the merge with a wider revision range, this time including
> the revision in which the file was created on the source branch.
I just realised that this is wrong.
The merge cannot simply be repeated.
Because of merge tracking, the file addition would be merged,
but the textdelta would not be merged again.
So the final merge result would miss the textdelta,
unless the user reapplied it manually.
Damn, this is really complicated stuff we're dealing with :/
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Tree Conflicts and User Interface (was: Re: [PATCH]
Tree-conflicts: do_entry_deletion segfault)
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 12, 2008 at 12:31:29PM +0100, Julian Foad wrote:
> We won't attempt to apply another update or merge once the
> node is already in some sort of conflict.
Are you really sure about that?
I think we talked about this in the past, and came to the
conclusion that we'd have to allow further merges into
tree-conflicted directories.
There are certain types of conflicts that require an additional
merge to be resolved, such as when a textdelta is applied to a file
which is not yet present in the target branch -- the fix is to
*repeat* the merge with a wider revision range, this time including
the revision in which the file was created on the source branch.
The problem, of course, is what happens when yet another tree
conflict is recorded for the same victim. We currently only
support a one-to-one mapping between victims and conflicts.
But that is more of an implementation detail, right?
Or is there another important reason for this?
If we didn't allow merges into tree-conflicted directories,
people would have to revert -R their working copies, and
then repeat the merge. But if we do that, we're basically
treating the tree conflict as unresolvable.
Unresolvable conflicts would be a totally new concept,
and in a good UI the user would be informed at commit
time that the conflict is not resolvable.
Which implies that we'd need to make a list of unresolvable
tree conflict situations to compare what we find in the
working copy against. Big can of worms :(
> > $ svn update
> > CM foo.c (a file with text conflict and property mod)
> > C dir (a directory containing tree conflict victims)
> > V dir/bar.c (a tree conflict victim)
> > Updated to revision 42.
> > There was 1 text conflict and 1 tree conflict.
>
> Why did you want the notification that 'dir' contains tree conflicts? If
> it's so that you can do a non-recursive status on it and still see
> there's something wrong with it, then wouldn't you want the indication
> to bubble all the way up to the WC root?
>
> I suggest there's no need for reporting 'this contains tree conflict
> victims'.
Yes, agreed. It's redundant and can be omitted.
> > But there is a problem. What if dir/bar.c was text conflicted,
> > had property conflicts, and are also the victim of a tree conflict?
> > Note that such a condition may be the result of multiple updates
> > or merges without conflict resolution done in between.
>
> Let's not allow that. The design we were working to said we would not
> allow that, as I recall. It should bail out if we try to update or merge
> into something that's already conflicted. If that's not done, we'll fix
> it.
I don't think we can simply say "let's not allow that". See above.
> (There's a complication in that the new merge-tracking merges are
> multi-phase and it's not ideal if they bail out after raising a conflict
> on an early phase, but that problem is already present outside of tree
> conflict handling.)
So that's yet another problem with this approach.
> > If we only had 'C' and 'V' and two columns, we could encode
> > this situation as:
>
> I'll follow up with my own proposal.
>
> - Julian
>
I'm waiting in suspense :)
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Tree Conflicts and User Interface
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Neels J. Hofmeyr wrote:
> $ svn status
> M beta
> D epsilon
> M V gamma
> M zeta
btw, this example is slightly stupid, I admit. More like:
M beta
D epsilon
M V gamma
M zeta
, so my argument about indenting tree-conflicts only applies to update.
Either way, I'm pretty sure my vote is on an added column.
~Neels
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: Tree Conflicts and User Interface
Posted by "Neels J. Hofmeyr" <ne...@elego.de>.
Julian Foad wrote:
> A node is either tree-conflicted (as a victim), or can have (text and/or
> prop) conflicts: one of:
>
> - not in conflict
> - tree conflict victim
> - prop conflicts
> - text conflict
> - text and prop conflicts
I'd like to draw attention to D, M: files and directories, when victims of
tree-conflicts, are mostly scheduled for deletion or locally modified. That
is valuable information!
working copy 1: working copy 2: `svn update' in wc 2:
M alpha D alpha D V alpha
D beta M beta M V beta
(committed) (locally) ...or something.
Looking around, I realized this:
$ svn --version
svn, version 1.5.1 (r32289)
compiled Sep 4 2008, 18:42:55
[...]
$ svn help status
[...]
The first six columns in the output are each one character wide:
First column: Says if item was added, deleted, or otherwise changed
' ' no modifications
'A' Added
'C' Conflicted
'D' Deleted
'I' Ignored
'M' Modified
'R' Replaced
'X' item is unversioned, but is used by an externals definition
'?' item is not under version control
'!' item is missing (removed by non-svn command) or incomplete
'~' versioned item obstructed by some item of a different kind
Second column: Modifications of a file's or directory's properties
' ' no modifications
'C' Conflicted
'M' Modified
Third column: Whether the working copy directory is locked
' ' not locked
'L' locked
Fourth column: Scheduled commit will contain addition-with-history
' ' no history scheduled with commit
'+' history scheduled with commit
Fifth column: Whether the item is switched relative to its parent
' ' normal
'S' switched
Sixth column: Repository lock token
(without -u)
' ' no lock token
'K' lock token present
(with -u)
' ' not locked in repository, no lock token
'K' locked in repository, lock toKen present
'O' locked in repository, lock token in some Other working copy
'T' locked in repository, lock token present but sTolen
'B' not locked in repository, lock token present but Broken
[...]
So, there already *IS* a multitude of columns? So what's the big deal with
adding another column for tree-conflicts -- isn't that the obvious choice?
Even update has a third column:
$ svn help update
[...]
A 'B' in the third column signifies that the lock for the file has
been broken or stolen.
[...]
So why not report tree-conflicts in an added column *only* when
tree-conflicts are to be reported? I know that stsp wouldn't like it to look
like this:
$ svn up
A alpha
G beta
D V gamma
D delta
$ svn status
M beta
D epsilon
M V gamma
M zeta
But I myself wouldn't mind it at all: I think it's even an added bonus,
because it visually sets tree-conflicted items apart from "normal" ones.
In a "third" column, couldn't we also actually use a 'T', because it's an
all new column? (Have these things been considered in another thread??)
~Neels
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: Tree Conflicts and User Interface (was: Re: [PATCH]
Tree-conflicts: do_entry_deletion segfault)
Posted by Julian Foad <ju...@btopenworld.com>.
On Fri, 2008-09-12 at 13:10 +0200, Stefan Sperling wrote:
> On Fri, Sep 12, 2008 at 12:44:11AM +0200, Neels Hofmeyr wrote:
> >
> >
> > Julian Foad wrote:
> > > Bear in mind that "reported as" and "stored as" are two completely
> > > different things. I'm not intending to change the storage, only the
> > > reporting. The storage can stay in the parent directory.
> >
> > Oh, I see. In that case...:
>
> This discussion is important!
>
> Please keep in mind that the current way we're reporting tree
> conflicts is nearly useless. I don't want it to end up in a
> release like this. This is the most urgent problem on the
> tree-conflicts branch IMO. We really need to fix this!
>
> > One day, stsp and I were discussing the _reporting_ of tree conflicts, and
> > thought that maybe victims and storage holder (i.e. parent directory) should
> > have distinct markings; C for parent dir, as in "conflicted", and V for an
> > actual victim node where the conflict manifested.
> >
> > We also thought it would be nice to have an additional column for tree
> > conflicts, to be able to still see the other markings on the
> > tree-conflict-marked files.
>
> I'm still not entirely convinced about that third column.
>
> There is a bit of a problem with the way Subversion currently
> displays information during update/status, and how to encode
> tree conflict information in a compatible way.
>
> To recap, what we currently have (in 1.5 and trunk) is two columns.
> One column for displaying information about files/directories,
> and one for displaying information about properties.
>
> Files and properties can be text conflicted (signalled by 'C'),
> directories cannot be conflicted.
>
> Enter tree conflicts:
>
> * Files can now be text conflicted, or be victims of a tree conflict,
> or both.
No, not both. We won't attempt to apply another update or merge once the
node is already in some sort of conflict.
> * Properties can be text conflicted.
Yes, but not if their node is tree-conflicted.
A node is either tree-conflicted (as a victim), or can have (text and/or
prop) conflicts: one of:
- not in conflict
- tree conflict victim
- prop conflicts
- text conflict
- text and prop conflicts
> * Directories can now contain tree conflict victims, or be tree
> conflict victims themselves, or both.
Yes.
> Currently, on the tree-conflicts branch we're only displaying
> information about directories which contain tree conflicts.
> This works sort-of well, by showing a 'C' in the first column
> of the status output for a directory.
>
> For example:
>
> CM foo.c (a file with text conflict and property mod)
> C dir (a directory containing tree conflict victims)
>
> To get information on individual victims inside a directory,
> the user has to run 'svn info' on that directory, and will
> then get a display like this:
>
> Tree conflicts:
> The update edited the file 'bar.c'.
> You have deleted 'bar.c' locally.
> Maybe you renamed it?
>
> This UI is less than ideal. It's fine for use during development
> to see whether victims have been detected correctly, but it is
> not something I'd like to use when actually working with Subversion.
>
> I'd rather have something like this:
>
> $ svn update
> CM foo.c (a file with text conflict and property mod)
> C dir (a directory containing tree conflict victims)
> V dir/bar.c (a tree conflict victim)
> Updated to revision 42.
> There was 1 text conflict and 1 tree conflict.
Why did you want the notification that 'dir' contains tree conflicts? If
it's so that you can do a non-recursive status on it and still see
there's something wrong with it, then wouldn't you want the indication
to bubble all the way up to the WC root?
I suggest there's no need for reporting 'this contains tree conflict
victims'.
> Note the conflict summary display which should be rather handy
> once those numbers get big. Imagine not getting a summary when
> you actually run into something like this:
> There were 33 text conflicts and 20 tree conflicts.
Yes. Or, more crucially, when 300 files were updated, there was just 1
conflict.
> But there is a problem. What if dir/bar.c was text conflicted,
> had property conflicts, and are also the victim of a tree conflict?
> Note that such a condition may be the result of multiple updates
> or merges without conflict resolution done in between.
Let's not allow that. The design we were working to said we would not
allow that, as I recall. It should bail out if we try to update or merge
into something that's already conflicted. If that's not done, we'll fix
it.
(There's a complication in that the new merge-tracking merges are
multi-phase and it's not ideal if they bail out after raising a conflict
on an early phase, but that problem is already present outside of tree
conflict handling.)
> If we only had 'C' and 'V' and two columns, we could encode
> this situation as:
I'll follow up with my own proposal.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Tree Conflicts and User Interface (was: Re: [PATCH]
Tree-conflicts: do_entry_deletion segfault)
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 12, 2008 at 12:44:11AM +0200, Neels Hofmeyr wrote:
>
>
> Julian Foad wrote:
> > Bear in mind that "reported as" and "stored as" are two completely
> > different things. I'm not intending to change the storage, only the
> > reporting. The storage can stay in the parent directory.
>
> Oh, I see. In that case...:
This discussion is important!
Please keep in mind that the current way we're reporting tree
conflicts is nearly useless. I don't want it to end up in a
release like this. This is the most urgent problem on the
tree-conflicts branch IMO. We really need to fix this!
> One day, stsp and I were discussing the _reporting_ of tree conflicts, and
> thought that maybe victims and storage holder (i.e. parent directory) should
> have distinct markings; C for parent dir, as in "conflicted", and V for an
> actual victim node where the conflict manifested.
>
> We also thought it would be nice to have an additional column for tree
> conflicts, to be able to still see the other markings on the
> tree-conflict-marked files.
I'm still not entirely convinced about that third column.
There is a bit of a problem with the way Subversion currently
displays information during update/status, and how to encode
tree conflict information in a compatible way.
To recap, what we currently have (in 1.5 and trunk) is two columns.
One column for displaying information about files/directories,
and one for displaying information about properties.
Files and properties can be text conflicted (signalled by 'C'),
directories cannot be conflicted.
Enter tree conflicts:
* Files can now be text conflicted, or be victims of a tree conflict,
or both.
* Properties can be text conflicted.
* Directories can now contain tree conflict victims, or be tree
conflict victims themselves, or both.
Currently, on the tree-conflicts branch we're only displaying
information about directories which contain tree conflicts.
This works sort-of well, by showing a 'C' in the first column
of the status output for a directory.
For example:
CM foo.c (a file with text conflict and property mod)
C dir (a directory containing tree conflict victims)
To get information on individual victims inside a directory,
the user has to run 'svn info' on that directory, and will
then get a display like this:
Tree conflicts:
The update edited the file 'bar.c'.
You have deleted 'bar.c' locally.
Maybe you renamed it?
This UI is less than ideal. It's fine for use during development
to see whether victims have been detected correctly, but it is
not something I'd like to use when actually working with Subversion.
I'd rather have something like this:
$ svn update
CM foo.c (a file with text conflict and property mod)
C dir (a directory containing tree conflict victims)
V dir/bar.c (a tree conflict victim)
Updated to revision 42.
There was 1 text conflict and 1 tree conflict.
$
Note the conflict summary display which should be rather handy
once those numbers get big. Imagine not getting a summary when
you actually run into something like this:
There were 33 text conflicts and 20 tree conflicts.
But there is a problem. What if dir/bar.c was text conflicted,
had property conflicts, and are also the victim of a tree conflict?
Note that such a condition may be the result of multiple updates
or merges without conflict resolution done in between.
If we only had 'C' and 'V' and two columns, we could encode
this situation as:
CC dir/bar.c (hides the tree conflict)
VC dir/bar.c (hides the text conflict)
CV dir/bar.c (ambiguous -- does the 'C' in the first column indicate
text or property conflict? And inconsistent: the second
column is used for something other than information
about properties -- or the first column is used for
something other than files, depending on what 'CV'
should mean)
VV dir/bar.c (Bah, no one will grok this.)
So imagine we had a third column to show tree conflict status:
$ svn update
CM foo.c (a file with text conflict and property mod)
C dir (a directory containing tree conflict victims)
V dir/bar.c (a tree conflict victim)
Updated to revision 42.
There was 1 text conflict and 1 tree conflict.
$
We can now encode every conflict individually and unambiguously.
Encoding all three types of conflict at once is easy:
CCV dir/bar.c (a file that is text conflicted, has property
conflicts, and is the victim of a tree conflict)
CV dir/dir2 (a directory which has property conflicts, and is
the victim of a tree conflict)
CC dir/dir2 (a directory which has property conflicts, and contains
tree conflict victims)
However, I see two drawbacks:
a) For easy parsing, the 3rd column would always have to be present
in the output, even if no tree conflicts are reported. This wastes
a single charater and possibly breaks some wrapper scripts, but
it's not that big of a problem.
b) The 3rd column would only ever show status about conflicts.
The only letters that could appear there would be 'C' and 'V'.
No 'U', not 'M', etc. As such, adding this column breaks with
the current design of the UI (one column for *anything* about
files, one column for *anything* about properties).
And by the way, I think that a proper description of all conflicts
should be printed when the user tries to commit:
$ svn commit
Commit failed (reasons follow):
foo.c is text-conflicted.
dir/bar.c is a tree conflict victim:
dir/bar.c has been edited in the repository.
You have deleted dir/bar.c locally.
Maybe you renamed it?
$
This is only a rough idea, though. We could also sort output by type
of conflict, change the wording, etc. The point is that svn should
become more verbose, to accurately explain complex conflict situations
to the user. Just printing "'foo.c' remains in conflict" as we do now
is not gonna help people much. This is a big chance to improve the
user interface wrt conflicts, we should use it.
Also, an additional way of getting at this information without trying
to commit would be nice. I don't really think that using 'svn info'
for this is right, but I don't have any better ideas either:
$ svn conflicts (new subcommand, name sounds odd)
$ svn show-conflicts (new subcommand, too long to type)
$ svn status --verbose or svn status -v (important information hidden
behind some option switch,
not good)
$ svn resolve --show-conflicts=tree (too long to type, user might not
want to resolve yet)
Hmmm... :-/
> What do you (all) think?
>
> ~Neels
What he asked :)
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Tree-conflicts: do_entry_deletion segfault
Posted by Neels Hofmeyr <ne...@elego.de>.
Julian Foad wrote:
> Bear in mind that "reported as" and "stored as" are two completely
> different things. I'm not intending to change the storage, only the
> reporting. The storage can stay in the parent directory.
Oh, I see. In that case...:
One day, stsp and I were discussing the _reporting_ of tree conflicts, and
thought that maybe victims and storage holder (i.e. parent directory) should
have distinct markings; C for parent dir, as in "conflicted", and V for an
actual victim node where the conflict manifested.
We also thought it would be nice to have an additional column for tree
conflicts, to be able to still see the other markings on the
tree-conflict-marked files.
What do you (all) think?
~Neels
BTW, my girls and I are leaving on monday for a three-months' stay in
Seville, so I'm not getting much work done these days. It should get better
after Monday.
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: [PATCH] Tree-conflicts: do_entry_deletion segfault
Posted by Julian Foad <ju...@btopenworld.com>.
Neels Hofmeyr wrote:
> > Heh. It was me who added that. It wasn't by accident. I was trying to
> > solve the problem of when the target is the root of the WC and therefore
> > there is no parent WC directory. However, I didn't properly solve that
> > problem, and I forgot to mention it in the log message, so we might very
> > well call it an accident.
>
> lol, sorry and thanks :)
>
> But, about when the target is the working copy root: that got me thinking.
> There is no parent directory to report conflicts in! And what will
> adm_retrieve return in such a case?
>
> This is the exact case:
> 1. Directory X has been issued on the svn commandline (e.g. `svn update X')
> 2. This directory X is a tree-conflict victim, and
> 3. X also happens to be the root of the local working copy.
>
> (It doesn't make sense to think of the repository root being a tree
> conflicts victim. Here, though, X has been checked out explicitly, and is
> the working copy root, as one would check out `trunk')
>
> There has been word that tree conflicts should be reported at the actual
> victim nodes, not in their respective parent directories. That would
> sufficiently prevent this problem altogether.
Bear in mind that "reported as" and "stored as" are two completely
different things. I'm not intending to change the storage, only the
reporting. The storage can stay in the parent directory.
> But for now, I tried to create conflicts in a working copy's root dir. It
> seems that at least update is waterproof, in a way. I've attached scripts
> that run the cases listed here, numbers corresponding:
>
>
> 1) Try to update the working copy root to a deleted revision (no conflicts):
> "svn: Target path does not exist"
>
> 2) Prop conflicts are handled as they should.
>
> 3) Run `svn rm .' in the working copy root and try to commit:
> States that the commit succeeded but warns about ugly problems.
> Status complains about a "missing" file removed by svn itself.
>
> 4) First run `svn rm .' in the working copy root, then try to update it to a
> deleted revision: same as 1): "svn: Target path does not exist".
> Also, `svn rm .' fails to lock a subdir, whatever that means.
>
> 5) First run `svn rm .' in the working copy root, then try to update it to a
> modified revision: same as 3): commit succeeded but warns about ugly
> problems. Also, the update "restores" a file removed by svn itself.
>
>
> It is pretty obvious that these cases are not considered valid. At least no
> user out there will run into the problem I am poking for here, because it
> doesn't even run that far, for other reasons.
>
> Have I missed a case? Otherwise everything is fair enough, at least from the
> tree-conflicts point of view ... switch and merge remain to be checked, but
> I expect the same stuff happening.
That sounds fair enough. Thanks for the scripts to demonstrate these
cases.
> > I made just one tweak: There was a point where you wrote "if
> > (svn_io_check_path(...) != SVN_NO_ERROR ..." which would leak the error
> > object if it threw an error. Instead, we have to call the function on
> > its own, as "SVN_ERR(svn_io_check_path(...));". Any error that it might
> > return is one that we don't know how to deal with, so it is OK to just
> > return the error to our caller.
>
> Ah yes, true. (Hasn't this been said before somewhere, too?)
Yes, Stefan said it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Tree-conflicts: do_entry_deletion segfault
Posted by Neels Hofmeyr <ne...@elego.de>.
Well, I attached the files renamed to *.script and it apparently worked.
Seems like *.sh endings are rejected, solely by name.
Karl Fogel wrote:
> Neels Hofmeyr <ne...@elego.de> writes:
>> And the mail server swallows *.sh files. Great. Trying again.
>
> I noticed several block messages from you (I'm one of the moderators),
> with names like "root_conflict3.sh" and such. I have no idea why they
> got blocked, unless the system actually bases it on the name of the
> file, which would seem silly, but is, I suppose, possible.
>
> If you can't attach them, putting them inline is the way to go, I
> guess. Seems you did that. Sorry for the trouble.
>
> -Karl
>
>> Neels Hofmeyr wrote:
>>> Julian Foad wrote:
>>>> On Fri, 2008-09-05 at 04:25 +0200, Neels Hofmeyr wrote:
>>>>> Quoting:
>>>>>
>>>>> * subversion/libsvn_wc/update_editor.c
>>>>> (bump_dir_info): Update a doc-string to allow for a directory to have tree
>>>>> conflicts.
>>>>> (entry_has_local_mods, check_tree_conflict): New functions.
>>>>> (do_entry_deletion): Have the parent's admin access baton passed in by the
>>>>> caller. Check for tree conflicts.
>>>>> ### Broken when parent_adm_access is NULL.
>>>>>
>>>>>
>>>>> I've already posted a fix for this one almost two weeks ago. It wasn't being
>>>>> heard much.
>>>> That's my fault, Neels. I read your message then, and the earlier ones,
>>>> and started to reply, but lost my way. Sorry.
>>>>
>>>>> So I'm posting an update, using today's tree-conflicts branch, compacting my
>>>>> explanations. You may refer to the following mail for more (confusing) detail:
>>>> Excellent. Thanks.
>>>>
>>>>> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142154
>>>>> Date: Sat, 23 Aug 2008 22:43:11 +0200
>>>>> From: Neels Hofmeyr <ne...@elego.de>
>>>>> Subject: Re: Tree-conflicts branch - log message / review
>>>>>
>>>>>
>>>>>
>>>>> Two problems coincide:
>>>>>
>>>>> 1. Tree conflict detection is skipped when examining the path that was
>>>>> specifically given as target. E.g. `svn update A/D/G' means that while G's
>>>>> contents are checked for tree conflicts, G itself will not be checked.
>>>>>
>>>>> 2. Tree conflict detection segfaults if run in that situation given in point
>>>>> 1 above.
>>>>>
>>>>>
>>>>> This patch fixes both, plus it tweaks two "unrelated" cmdline test to ignore
>>>>> a 'C' marker caused by the fix. The fix itself consists of three parts:
>>>>>
>>>>> i) A change made in the tree-conflicts branch is reverted to trunk:
>>>>> do_entry_deletion() does not need a PARENT_ADM_ACCESS parameter. It was
>>>>> being used to indicate the specific case mentioned in point 1 above, so as
>>>>> to be able to skip tree-conflicts detection in that case, oddly enough. Stsp
>>>>> has pointed out that this change must have been committed by accident.
>>>> Heh. It was me who added that. It wasn't by accident. I was trying to
>>>> solve the problem of when the target is the root of the WC and therefore
>>>> there is no parent WC directory. However, I didn't properly solve that
>>>> problem, and I forgot to mention it in the log message, so we might very
>>>> well call it an accident.
>>> lol, sorry and thanks :)
>>>
>>> But, about when the target is the working copy root: that got me thinking.
>>> There is no parent directory to report conflicts in! And what will
>>> adm_retrieve return in such a case?
>>>
>>> This is the exact case:
>>> 1. Directory X has been issued on the svn commandline (e.g. `svn update X')
>>> 2. This directory X is a tree-conflict victim, and
>>> 3. X also happens to be the root of the local working copy.
>>>
>>> (It doesn't make sense to think of the repository root being a tree
>>> conflicts victim. Here, though, X has been checked out explicitly, and is
>>> the working copy root, as one would check out `trunk')
>>>
>>> There has been word that tree conflicts should be reported at the actual
>>> victim nodes, not in their respective parent directories. That would
>>> sufficiently prevent this problem altogether.
>>>
>>> But for now, I tried to create conflicts in a working copy's root dir. It
>>> seems that at least update is waterproof, in a way. I've attached scripts
>>> that run the cases listed here, numbers corresponding:
>>>
>>>
>>> 1) Try to update the working copy root to a deleted revision (no conflicts):
>>> "svn: Target path does not exist"
>>>
>>> 2) Prop conflicts are handled as they should.
>>>
>>> 3) Run `svn rm .' in the working copy root and try to commit:
>>> States that the commit succeeded but warns about ugly problems.
>>> Status complains about a "missing" file removed by svn itself.
>>>
>>> 4) First run `svn rm .' in the working copy root, then try to update it to a
>>> deleted revision: same as 1): "svn: Target path does not exist".
>>> Also, `svn rm .' fails to lock a subdir, whatever that means.
>>>
>>> 5) First run `svn rm .' in the working copy root, then try to update it to a
>>> modified revision: same as 3): commit succeeded but warns about ugly
>>> problems. Also, the update "restores" a file removed by svn itself.
>>>
>>>
>>> It is pretty obvious that these cases are not considered valid. At least no
>>> user out there will run into the problem I am poking for here, because it
>>> doesn't even run that far, for other reasons.
>>>
>>> Have I missed a case? Otherwise everything is fair enough, at least from the
>>> tree-conflicts point of view ... switch and merge remain to be checked, but
>>> I expect the same stuff happening.
>>>
>>>
>>>
>>>> I made just one tweak: There was a point where you wrote "if
>>>> (svn_io_check_path(...) != SVN_NO_ERROR ..." which would leak the error
>>>> object if it threw an error. Instead, we have to call the function on
>>>> its own, as "SVN_ERR(svn_io_check_path(...));". Any error that it might
>>>> return is one that we don't know how to deal with, so it is OK to just
>>>> return the error to our caller.
>>> Ah yes, true. (Hasn't this been said before somewhere, too?)
>>> I checked and yours is better. I had assumed that the SVN_ERR indicates a
>>> missing directory, but duh.
>>>
>>>
>>>> Committed in r32932.
>>> Thanks!
>>>
>>> ~Neels
>>>
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: [PATCH] Tree-conflicts: do_entry_deletion segfault
Posted by Karl Fogel <kf...@red-bean.com>.
Neels Hofmeyr <ne...@elego.de> writes:
> And the mail server swallows *.sh files. Great. Trying again.
I noticed several block messages from you (I'm one of the moderators),
with names like "root_conflict3.sh" and such. I have no idea why they
got blocked, unless the system actually bases it on the name of the
file, which would seem silly, but is, I suppose, possible.
If you can't attach them, putting them inline is the way to go, I
guess. Seems you did that. Sorry for the trouble.
-Karl
> Neels Hofmeyr wrote:
>>
>> Julian Foad wrote:
>>> On Fri, 2008-09-05 at 04:25 +0200, Neels Hofmeyr wrote:
>>>> Quoting:
>>>>
>>>> * subversion/libsvn_wc/update_editor.c
>>>> (bump_dir_info): Update a doc-string to allow for a directory to have tree
>>>> conflicts.
>>>> (entry_has_local_mods, check_tree_conflict): New functions.
>>>> (do_entry_deletion): Have the parent's admin access baton passed in by the
>>>> caller. Check for tree conflicts.
>>>> ### Broken when parent_adm_access is NULL.
>>>>
>>>>
>>>> I've already posted a fix for this one almost two weeks ago. It wasn't being
>>>> heard much.
>>> That's my fault, Neels. I read your message then, and the earlier ones,
>>> and started to reply, but lost my way. Sorry.
>>>
>>>> So I'm posting an update, using today's tree-conflicts branch, compacting my
>>>> explanations. You may refer to the following mail for more (confusing) detail:
>>> Excellent. Thanks.
>>>
>>>> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142154
>>>> Date: Sat, 23 Aug 2008 22:43:11 +0200
>>>> From: Neels Hofmeyr <ne...@elego.de>
>>>> Subject: Re: Tree-conflicts branch - log message / review
>>>>
>>>>
>>>>
>>>> Two problems coincide:
>>>>
>>>> 1. Tree conflict detection is skipped when examining the path that was
>>>> specifically given as target. E.g. `svn update A/D/G' means that while G's
>>>> contents are checked for tree conflicts, G itself will not be checked.
>>>>
>>>> 2. Tree conflict detection segfaults if run in that situation given in point
>>>> 1 above.
>>>>
>>>>
>>>> This patch fixes both, plus it tweaks two "unrelated" cmdline test to ignore
>>>> a 'C' marker caused by the fix. The fix itself consists of three parts:
>>>>
>>>> i) A change made in the tree-conflicts branch is reverted to trunk:
>>>> do_entry_deletion() does not need a PARENT_ADM_ACCESS parameter. It was
>>>> being used to indicate the specific case mentioned in point 1 above, so as
>>>> to be able to skip tree-conflicts detection in that case, oddly enough. Stsp
>>>> has pointed out that this change must have been committed by accident.
>>> Heh. It was me who added that. It wasn't by accident. I was trying to
>>> solve the problem of when the target is the root of the WC and therefore
>>> there is no parent WC directory. However, I didn't properly solve that
>>> problem, and I forgot to mention it in the log message, so we might very
>>> well call it an accident.
>>
>> lol, sorry and thanks :)
>>
>> But, about when the target is the working copy root: that got me thinking.
>> There is no parent directory to report conflicts in! And what will
>> adm_retrieve return in such a case?
>>
>> This is the exact case:
>> 1. Directory X has been issued on the svn commandline (e.g. `svn update X')
>> 2. This directory X is a tree-conflict victim, and
>> 3. X also happens to be the root of the local working copy.
>>
>> (It doesn't make sense to think of the repository root being a tree
>> conflicts victim. Here, though, X has been checked out explicitly, and is
>> the working copy root, as one would check out `trunk')
>>
>> There has been word that tree conflicts should be reported at the actual
>> victim nodes, not in their respective parent directories. That would
>> sufficiently prevent this problem altogether.
>>
>> But for now, I tried to create conflicts in a working copy's root dir. It
>> seems that at least update is waterproof, in a way. I've attached scripts
>> that run the cases listed here, numbers corresponding:
>>
>>
>> 1) Try to update the working copy root to a deleted revision (no conflicts):
>> "svn: Target path does not exist"
>>
>> 2) Prop conflicts are handled as they should.
>>
>> 3) Run `svn rm .' in the working copy root and try to commit:
>> States that the commit succeeded but warns about ugly problems.
>> Status complains about a "missing" file removed by svn itself.
>>
>> 4) First run `svn rm .' in the working copy root, then try to update it to a
>> deleted revision: same as 1): "svn: Target path does not exist".
>> Also, `svn rm .' fails to lock a subdir, whatever that means.
>>
>> 5) First run `svn rm .' in the working copy root, then try to update it to a
>> modified revision: same as 3): commit succeeded but warns about ugly
>> problems. Also, the update "restores" a file removed by svn itself.
>>
>>
>> It is pretty obvious that these cases are not considered valid. At least no
>> user out there will run into the problem I am poking for here, because it
>> doesn't even run that far, for other reasons.
>>
>> Have I missed a case? Otherwise everything is fair enough, at least from the
>> tree-conflicts point of view ... switch and merge remain to be checked, but
>> I expect the same stuff happening.
>>
>>
>>
>>> I made just one tweak: There was a point where you wrote "if
>>> (svn_io_check_path(...) != SVN_NO_ERROR ..." which would leak the error
>>> object if it threw an error. Instead, we have to call the function on
>>> its own, as "SVN_ERR(svn_io_check_path(...));". Any error that it might
>>> return is one that we don't know how to deal with, so it is OK to just
>>> return the error to our caller.
>>
>> Ah yes, true. (Hasn't this been said before somewhere, too?)
>> I checked and yours is better. I had assumed that the SVN_ERR indicates a
>> missing directory, but duh.
>>
>>
>>> Committed in r32932.
>> Thanks!
>>
>> ~Neels
>>
>>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Tree-conflicts: do_entry_deletion segfault
Posted by Neels Hofmeyr <ne...@elego.de>.
And the mail server swallows *.sh files. Great. Trying again.
Neels Hofmeyr wrote:
>
> Julian Foad wrote:
>> On Fri, 2008-09-05 at 04:25 +0200, Neels Hofmeyr wrote:
>>> Quoting:
>>>
>>> * subversion/libsvn_wc/update_editor.c
>>> (bump_dir_info): Update a doc-string to allow for a directory to have tree
>>> conflicts.
>>> (entry_has_local_mods, check_tree_conflict): New functions.
>>> (do_entry_deletion): Have the parent's admin access baton passed in by the
>>> caller. Check for tree conflicts.
>>> ### Broken when parent_adm_access is NULL.
>>>
>>>
>>> I've already posted a fix for this one almost two weeks ago. It wasn't being
>>> heard much.
>> That's my fault, Neels. I read your message then, and the earlier ones,
>> and started to reply, but lost my way. Sorry.
>>
>>> So I'm posting an update, using today's tree-conflicts branch, compacting my
>>> explanations. You may refer to the following mail for more (confusing) detail:
>> Excellent. Thanks.
>>
>>> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142154
>>> Date: Sat, 23 Aug 2008 22:43:11 +0200
>>> From: Neels Hofmeyr <ne...@elego.de>
>>> Subject: Re: Tree-conflicts branch - log message / review
>>>
>>>
>>>
>>> Two problems coincide:
>>>
>>> 1. Tree conflict detection is skipped when examining the path that was
>>> specifically given as target. E.g. `svn update A/D/G' means that while G's
>>> contents are checked for tree conflicts, G itself will not be checked.
>>>
>>> 2. Tree conflict detection segfaults if run in that situation given in point
>>> 1 above.
>>>
>>>
>>> This patch fixes both, plus it tweaks two "unrelated" cmdline test to ignore
>>> a 'C' marker caused by the fix. The fix itself consists of three parts:
>>>
>>> i) A change made in the tree-conflicts branch is reverted to trunk:
>>> do_entry_deletion() does not need a PARENT_ADM_ACCESS parameter. It was
>>> being used to indicate the specific case mentioned in point 1 above, so as
>>> to be able to skip tree-conflicts detection in that case, oddly enough. Stsp
>>> has pointed out that this change must have been committed by accident.
>> Heh. It was me who added that. It wasn't by accident. I was trying to
>> solve the problem of when the target is the root of the WC and therefore
>> there is no parent WC directory. However, I didn't properly solve that
>> problem, and I forgot to mention it in the log message, so we might very
>> well call it an accident.
>
> lol, sorry and thanks :)
>
> But, about when the target is the working copy root: that got me thinking.
> There is no parent directory to report conflicts in! And what will
> adm_retrieve return in such a case?
>
> This is the exact case:
> 1. Directory X has been issued on the svn commandline (e.g. `svn update X')
> 2. This directory X is a tree-conflict victim, and
> 3. X also happens to be the root of the local working copy.
>
> (It doesn't make sense to think of the repository root being a tree
> conflicts victim. Here, though, X has been checked out explicitly, and is
> the working copy root, as one would check out `trunk')
>
> There has been word that tree conflicts should be reported at the actual
> victim nodes, not in their respective parent directories. That would
> sufficiently prevent this problem altogether.
>
> But for now, I tried to create conflicts in a working copy's root dir. It
> seems that at least update is waterproof, in a way. I've attached scripts
> that run the cases listed here, numbers corresponding:
>
>
> 1) Try to update the working copy root to a deleted revision (no conflicts):
> "svn: Target path does not exist"
>
> 2) Prop conflicts are handled as they should.
>
> 3) Run `svn rm .' in the working copy root and try to commit:
> States that the commit succeeded but warns about ugly problems.
> Status complains about a "missing" file removed by svn itself.
>
> 4) First run `svn rm .' in the working copy root, then try to update it to a
> deleted revision: same as 1): "svn: Target path does not exist".
> Also, `svn rm .' fails to lock a subdir, whatever that means.
>
> 5) First run `svn rm .' in the working copy root, then try to update it to a
> modified revision: same as 3): commit succeeded but warns about ugly
> problems. Also, the update "restores" a file removed by svn itself.
>
>
> It is pretty obvious that these cases are not considered valid. At least no
> user out there will run into the problem I am poking for here, because it
> doesn't even run that far, for other reasons.
>
> Have I missed a case? Otherwise everything is fair enough, at least from the
> tree-conflicts point of view ... switch and merge remain to be checked, but
> I expect the same stuff happening.
>
>
>
>> I made just one tweak: There was a point where you wrote "if
>> (svn_io_check_path(...) != SVN_NO_ERROR ..." which would leak the error
>> object if it threw an error. Instead, we have to call the function on
>> its own, as "SVN_ERR(svn_io_check_path(...));". Any error that it might
>> return is one that we don't know how to deal with, so it is OK to just
>> return the error to our caller.
>
> Ah yes, true. (Hasn't this been said before somewhere, too?)
> I checked and yours is better. I had assumed that the SVN_ERR indicates a
> missing directory, but duh.
>
>
>> Committed in r32932.
> Thanks!
>
> ~Neels
>
>
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: [PATCH] Tree-conflicts: do_entry_deletion segfault
Posted by Neels Hofmeyr <ne...@elego.de>.
Julian Foad wrote:
> On Fri, 2008-09-05 at 04:25 +0200, Neels Hofmeyr wrote:
>> Quoting:
>>
>> * subversion/libsvn_wc/update_editor.c
>> (bump_dir_info): Update a doc-string to allow for a directory to have tree
>> conflicts.
>> (entry_has_local_mods, check_tree_conflict): New functions.
>> (do_entry_deletion): Have the parent's admin access baton passed in by the
>> caller. Check for tree conflicts.
>> ### Broken when parent_adm_access is NULL.
>>
>>
>> I've already posted a fix for this one almost two weeks ago. It wasn't being
>> heard much.
>
> That's my fault, Neels. I read your message then, and the earlier ones,
> and started to reply, but lost my way. Sorry.
>
>> So I'm posting an update, using today's tree-conflicts branch, compacting my
>> explanations. You may refer to the following mail for more (confusing) detail:
>
> Excellent. Thanks.
>
>> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142154
>> Date: Sat, 23 Aug 2008 22:43:11 +0200
>> From: Neels Hofmeyr <ne...@elego.de>
>> Subject: Re: Tree-conflicts branch - log message / review
>>
>>
>>
>> Two problems coincide:
>>
>> 1. Tree conflict detection is skipped when examining the path that was
>> specifically given as target. E.g. `svn update A/D/G' means that while G's
>> contents are checked for tree conflicts, G itself will not be checked.
>>
>> 2. Tree conflict detection segfaults if run in that situation given in point
>> 1 above.
>>
>>
>> This patch fixes both, plus it tweaks two "unrelated" cmdline test to ignore
>> a 'C' marker caused by the fix. The fix itself consists of three parts:
>>
>> i) A change made in the tree-conflicts branch is reverted to trunk:
>> do_entry_deletion() does not need a PARENT_ADM_ACCESS parameter. It was
>> being used to indicate the specific case mentioned in point 1 above, so as
>> to be able to skip tree-conflicts detection in that case, oddly enough. Stsp
>> has pointed out that this change must have been committed by accident.
>
> Heh. It was me who added that. It wasn't by accident. I was trying to
> solve the problem of when the target is the root of the WC and therefore
> there is no parent WC directory. However, I didn't properly solve that
> problem, and I forgot to mention it in the log message, so we might very
> well call it an accident.
lol, sorry and thanks :)
But, about when the target is the working copy root: that got me thinking.
There is no parent directory to report conflicts in! And what will
adm_retrieve return in such a case?
This is the exact case:
1. Directory X has been issued on the svn commandline (e.g. `svn update X')
2. This directory X is a tree-conflict victim, and
3. X also happens to be the root of the local working copy.
(It doesn't make sense to think of the repository root being a tree
conflicts victim. Here, though, X has been checked out explicitly, and is
the working copy root, as one would check out `trunk')
There has been word that tree conflicts should be reported at the actual
victim nodes, not in their respective parent directories. That would
sufficiently prevent this problem altogether.
But for now, I tried to create conflicts in a working copy's root dir. It
seems that at least update is waterproof, in a way. I've attached scripts
that run the cases listed here, numbers corresponding:
1) Try to update the working copy root to a deleted revision (no conflicts):
"svn: Target path does not exist"
2) Prop conflicts are handled as they should.
3) Run `svn rm .' in the working copy root and try to commit:
States that the commit succeeded but warns about ugly problems.
Status complains about a "missing" file removed by svn itself.
4) First run `svn rm .' in the working copy root, then try to update it to a
deleted revision: same as 1): "svn: Target path does not exist".
Also, `svn rm .' fails to lock a subdir, whatever that means.
5) First run `svn rm .' in the working copy root, then try to update it to a
modified revision: same as 3): commit succeeded but warns about ugly
problems. Also, the update "restores" a file removed by svn itself.
It is pretty obvious that these cases are not considered valid. At least no
user out there will run into the problem I am poking for here, because it
doesn't even run that far, for other reasons.
Have I missed a case? Otherwise everything is fair enough, at least from the
tree-conflicts point of view ... switch and merge remain to be checked, but
I expect the same stuff happening.
> I made just one tweak: There was a point where you wrote "if
> (svn_io_check_path(...) != SVN_NO_ERROR ..." which would leak the error
> object if it threw an error. Instead, we have to call the function on
> its own, as "SVN_ERR(svn_io_check_path(...));". Any error that it might
> return is one that we don't know how to deal with, so it is OK to just
> return the error to our caller.
Ah yes, true. (Hasn't this been said before somewhere, too?)
I checked and yours is better. I had assumed that the SVN_ERR indicates a
missing directory, but duh.
> Committed in r32932.
Thanks!
~Neels
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Re: [PATCH] Tree-conflicts: do_entry_deletion segfault
Posted by Julian Foad <ju...@btopenworld.com>.
On Fri, 2008-09-05 at 04:25 +0200, Neels Hofmeyr wrote:
> Quoting:
>
> * subversion/libsvn_wc/update_editor.c
> (bump_dir_info): Update a doc-string to allow for a directory to have tree
> conflicts.
> (entry_has_local_mods, check_tree_conflict): New functions.
> (do_entry_deletion): Have the parent's admin access baton passed in by the
> caller. Check for tree conflicts.
> ### Broken when parent_adm_access is NULL.
>
>
> I've already posted a fix for this one almost two weeks ago. It wasn't being
> heard much.
That's my fault, Neels. I read your message then, and the earlier ones,
and started to reply, but lost my way. Sorry.
> So I'm posting an update, using today's tree-conflicts branch, compacting my
> explanations. You may refer to the following mail for more (confusing) detail:
Excellent. Thanks.
> http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142154
> Date: Sat, 23 Aug 2008 22:43:11 +0200
> From: Neels Hofmeyr <ne...@elego.de>
> Subject: Re: Tree-conflicts branch - log message / review
>
>
>
> Two problems coincide:
>
> 1. Tree conflict detection is skipped when examining the path that was
> specifically given as target. E.g. `svn update A/D/G' means that while G's
> contents are checked for tree conflicts, G itself will not be checked.
>
> 2. Tree conflict detection segfaults if run in that situation given in point
> 1 above.
>
>
> This patch fixes both, plus it tweaks two "unrelated" cmdline test to ignore
> a 'C' marker caused by the fix. The fix itself consists of three parts:
>
> i) A change made in the tree-conflicts branch is reverted to trunk:
> do_entry_deletion() does not need a PARENT_ADM_ACCESS parameter. It was
> being used to indicate the specific case mentioned in point 1 above, so as
> to be able to skip tree-conflicts detection in that case, oddly enough. Stsp
> has pointed out that this change must have been committed by accident.
Heh. It was me who added that. It wasn't by accident. I was trying to
solve the problem of when the target is the root of the WC and therefore
there is no parent WC directory. However, I didn't properly solve that
problem, and I forgot to mention it in the log message, so we might very
well call it an accident.
> ii) Enable tree-conflicts detection for the case in point 1 above.
Great.
> iii) Make check_tree_conflict() handle a missing directory properly, so that
> it reports a reason_missing instead of segfaulting. (The diff looks a little
> confusing there because of indent changes.)
Thanks for explaining each part of your changes.
Committed in r32932.
I made just one tweak: There was a point where you wrote "if
(svn_io_check_path(...) != SVN_NO_ERROR ..." which would leak the error
object if it threw an error. Instead, we have to call the function on
its own, as "SVN_ERR(svn_io_check_path(...));". Any error that it might
return is one that we don't know how to deal with, so it is OK to just
return the error to our caller.
Thanks ever so much for fixing this.
- Julian
> [[[
> Fix segfault when the update editor calls do_entry_deletion() on an explicit
> target of an update.
>
> Patch by: Neels Hofmeyr <ne...@elego.de>
>
> * subversion/libsvn_wc/update_editor.c
> (do_entry_deletion, delete_entry, close_edit): Remove parameter
> PARENT_ADM_ACCESS from do_entry_deletion(), reverting to trunk
> and allowing tree-conflicts detection for update targets.
> (check_tree_conflict): Properly handle a missing directory, reporting
> it as `svn_wc_conflict_reason_missing'.
>
> * subversion/tests/cmdline/update_tests.py
> (update_deleted_missing_dir, another_hudson_problem): Tweak these tests
> to expect and handle the tree-conflicts that inherently arise.
> ]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Tree-conflicts: do_entry_deletion segfault
Posted by Neels Hofmeyr <ne...@elego.de>.
Julian Foad wrote:
> Hi, tree conflicts fans.
>
> I have written a complete log message of what is on the tree-conflicts
> branch compared with trunk. This is attached and also stored in
> README.branch in the root of the branch.
>
> The diff is available for a while at
> <http://filebin.ca/ajqeez/tc-r32853.patch>.
>
> Updates to the branch since last time include: resolve a bunch of
> non-essential differences such as changes that were better made on the
> trunk; catch up with recent changes from trunk; Neels' work on improving
> the tests.
>
> Anyone able to pick a small part of it to review, or help with, or just
> ping me about, is very welcome.
>
> Thanks.
> - Julian
>
Quoting:
* subversion/libsvn_wc/update_editor.c
(bump_dir_info): Update a doc-string to allow for a directory to have tree
conflicts.
(entry_has_local_mods, check_tree_conflict): New functions.
(do_entry_deletion): Have the parent's admin access baton passed in by the
caller. Check for tree conflicts.
### Broken when parent_adm_access is NULL.
I've already posted a fix for this one almost two weeks ago. It wasn't being
heard much.
So I'm posting an update, using today's tree-conflicts branch, compacting my
explanations. You may refer to the following mail for more (confusing) detail:
http://subversion.tigris.org/servlets/ReadMsg?listName=dev&msgNo=142154
Date: Sat, 23 Aug 2008 22:43:11 +0200
From: Neels Hofmeyr <ne...@elego.de>
Subject: Re: Tree-conflicts branch - log message / review
Two problems coincide:
1. Tree conflict detection is skipped when examining the path that was
specifically given as target. E.g. `svn update A/D/G' means that while G's
contents are checked for tree conflicts, G itself will not be checked.
2. Tree conflict detection segfaults if run in that situation given in point
1 above.
This patch fixes both, plus it tweaks two "unrelated" cmdline test to ignore
a 'C' marker caused by the fix. The fix itself consists of three parts:
i) A change made in the tree-conflicts branch is reverted to trunk:
do_entry_deletion() does not need a PARENT_ADM_ACCESS parameter. It was
being used to indicate the specific case mentioned in point 1 above, so as
to be able to skip tree-conflicts detection in that case, oddly enough. Stsp
has pointed out that this change must have been committed by accident.
ii) Enable tree-conflicts detection for the case in point 1 above.
iii) Make check_tree_conflict() handle a missing directory properly, so that
it reports a reason_missing instead of segfaulting. (The diff looks a little
confusing there because of indent changes.)
~Neels
[[[
Fix segfault when the update editor calls do_entry_deletion() on an explicit
target of an update.
Patch by: Neels Hofmeyr <ne...@elego.de>
* subversion/libsvn_wc/update_editor.c
(do_entry_deletion, delete_entry, close_edit): Remove parameter
PARENT_ADM_ACCESS from do_entry_deletion(), reverting to trunk
and allowing tree-conflicts detection for update targets.
(check_tree_conflict): Properly handle a missing directory, reporting
it as `svn_wc_conflict_reason_missing'.
* subversion/tests/cmdline/update_tests.py
(update_deleted_missing_dir, another_hudson_problem): Tweak these tests
to expect and handle the tree-conflicts that inherently arise.
]]]
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194