tree-conflicts: update conflicted, skip vs. error

[Neels Janosch Hofmeyr <ne...@elego.de>]

Hi tree-conflict folks,

I'm looking at the problem of what to say when an update is attempted of a)
a tree conflicted item or of b) an item somewhere within a tree-conflicted
directory.

The current stance is that conflicted nodes should be 'Skipped'. This sort
of applies to a). We reach a tree-conflicted node by recursion:

$ svn st
 M   dir
   C dir/sub/node

$ svn up dir
Skipped 'dir/sub/node'


Now we've got case b), where an *ancestor* dir is in conflict.

$ svn st
   C dir
M    dir/sub/node

$ svn up dir/sub/node
Skipped dir/sub/node

That's how it is currently specified to look. But, for example, if `dir' was
removed in the repository, that's pretty much failing to let the user know
why the update skipped everything.


Then, there's case c). First cd into the conflicted subtree and then update
something. Suddenly, when I look through the code, there's no place to
"skip" such an update. The failure information is available before any
update stuff even kicked off. When we let the update stuff kick off, it
issues another failure, namely that we're sort of not in a proper working
copy...

$ svn st
   C dir
M    dir/sub/node

$ cd dir/sub
$ svn up
svn: Target path '/.../dir/sub' does not exist
(depending on the type of tree-conflict?)


In case a), where we reach a tree-conflicted item by recursion, it's obvious
to do the 'Skipped' thing as usual.

But in case c), where we're *in* a directory that is *completely*
tree-conflicted by an ancestor, it makes sense to me to issue an error
instead of a 'Skipped':	
svn: Aborting update: '/.../dir' remains in tree-conflict.

In case b), I'm undecided really. We're currently still *outside* a
tree-conflicted subtree, but some of the *explicit* targets reach into a
tree-conflicted subtree. Intuitively, I want to issue an error on targets
within a conflicted subtree, like c). But we probably want to carry out the
updates on any other, non-conflicted targets issued on the same commandline.

Maybe an "augmented skip" notification would do for both b) and c), saying
"Skipped target 'dir/sub/node' because 'dir' remains in tree-conflict".

A code implication: The place to catch both b) and c) is in an entirely
different code layer than all other skip notifications. It feels like
fabricating a skip and having problems with not running whole chunks of code
although we exit with "success" (haven't tried that yet).
If c) were implemented to bail, that would also bail on b) cases, and it
would be some complex fiddling to allow any other targets to go through.

Do you guys have an opinion on this? That might shed some light.
Thanks!
~Neels

P.S.:
The place to catch c) would be for example here:
subversion/libsvn_client/update.c:210, in svn_client__update_internal().
or here, where the currently seen error is produced:
subversion/libsvn_repos/reporter.c:1162 in drive().
Attached is a trial patch that catches b) and c) by issuing an error in
svn_client__update_internal(). The patch also re-enables update_tests.py 50
to show this case.
Alternatively, find it here:
http://hofmeyr.de/svn/update-in-conflicted-1.patch

The place where tree-conflicted ancestry is currently detected, i.e. cases
a) and b), is deeper within the onion, here:
subversion/libsvn_wc/update_editor.c:1600, by already_in_a_tree_conflict()

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

Re: tree-conflicts: update conflicted, skip vs. error

[Julian Foad <ju...@btopenworld.com>]

Desired behaviour, according to Julian...

* Update and Switch should skip a target if it is a tree conflict victim
itself, or is inside a tree-conflicted directory in the WC.

* Behaviour should not depend on whether the target is explicit or
implicit, nor on whether it is immediate or encountered by recursion.

* Behaviour should not depend on the current directory. (If
implementation difficulties necessitate it, so be it.)


The example WC:

$ svn st
M   dir
   C dir/sub
M    dir/sub/node


Desired behaviour:

$ svn up dir
Skipped 'dir/sub' because it is tree-conflicted

$ svn up dir/sub
Skipped 'dir/sub' because it is tree-conflicted

$ svn up dir/sub/node
Skipped 'dir/sub/node' because 'dir/sub' is tree-conflicted


Demonstrating independence of CWD:

$ (cd dir && svn up)
Skipped 'sub' because it is tree-conflicted

$ (cd dir && svn up sub)
Skipped 'sub' because it is tree-conflicted

$ (cd dir && svn up sub/node)
Skipped 'sub/node' because 'sub' is tree-conflicted


$ (cd dir/sub && svn up ..)
Skipped '../sub' because it is tree-conflicted

$ (cd dir/sub && svn up)
Skipped '.' because it is tree-conflicted

$ (cd dir/sub && svn up node)
Skipped 'node' because '.' is tree-conflicted


$ (cd dir/sub/node && svn up ../..)
Skipped '../../sub' because it is tree-conflicted

$ (cd dir/sub/node && svn up ..)
Skipped '..' because it is tree-conflicted

$ (cd dir/sub/node && svn up)
Skipped '.' because '..' is tree-conflicted



On Fri, 2008-12-12 at 05:57 +0100, Neels J Hofmeyr wrote: 
> Hi tree-conflict folks,
> 
> I'm looking at the problem of what to say when an update is attempted of a)
> a tree conflicted item or of b) an item somewhere within a tree-conflicted
> directory.
> 
> The current stance is that conflicted nodes should be 'Skipped'. This sort
> of applies to a). We reach a tree-conflicted node by recursion:
> 
> $ svn st
>  M   dir
>    C dir/sub/node
> 
> $ svn up dir
> Skipped 'dir/sub/node'
> 
> 
> Now we've got case b), where an *ancestor* dir is in conflict.
> 
> $ svn st
>    C dir
> M    dir/sub/node
> 
> $ svn up dir/sub/node
> Skipped dir/sub/node
> 
> That's how it is currently specified to look. But, for example, if `dir' was
> removed in the repository, that's pretty much failing to let the user know
> why the update skipped everything.

Yes, but that's a general problem of the message "Skipped X" being a
rather poor indication for the user. In all Subversion's "Skipped"
messages we should aim to indicate why the thing was skipped.

I think it is right to skip in this case.


> Then, there's case c). First cd into the conflicted subtree and then update
> something. Suddenly, when I look through the code, there's no place to
> "skip" such an update. The failure information is available before any
> update stuff even kicked off. When we let the update stuff kick off, it
> issues another failure, namely that we're sort of not in a proper working
> copy...
> 
> $ svn st
>    C dir
> M    dir/sub/node
> 
> $ cd dir/sub
> $ svn up
> svn: Target path '/.../dir/sub' does not exist
> (depending on the type of tree-conflict?)
> 
> 
> In case a), where we reach a tree-conflicted item by recursion, it's obvious
> to do the 'Skipped' thing as usual.
> 
> But in case c), where we're *in* a directory that is *completely*
> tree-conflicted by an ancestor, it makes sense to me to issue an error
> instead of a 'Skipped':	
> svn: Aborting update: '/.../dir' remains in tree-conflict.

That sounds OK for a single-target command... but as you then point out,
for a multi-target update command, it would be better to be consistent
with other cases so. So I'd say this should still say "Skipped".

> In case b), I'm undecided really. We're currently still *outside* a
> tree-conflicted subtree, but some of the *explicit* targets reach into a
> tree-conflicted subtree. Intuitively, I want to issue an error on targets
> within a conflicted subtree, like c). But we probably want to carry out the
> updates on any other, non-conflicted targets issued on the same commandline.
> 
> Maybe an "augmented skip" notification would do for both b) and c), saying
> "Skipped target 'dir/sub/node' because 'dir' remains in tree-conflict".

Yes, definitely. But let's treat this (better message) as a separate
issue and not necessarily try to do this at the same time as ensuring
that targets are actually skipped when there's a conflict.

> A code implication: The place to catch both b) and c) is in an entirely
> different code layer than all other skip notifications. It feels like
> fabricating a skip and having problems with not running whole chunks of code
> although we exit with "success" (haven't tried that yet).

Hmm. Unless you've seen something in particular, I wouldn't expect to
run into major problems.

- Julian


> If c) were implemented to bail, that would also bail on b) cases, and it
> would be some complex fiddling to allow any other targets to go through.
> 
> Do you guys have an opinion on this? That might shed some light.
> Thanks!
> ~Neels
> 
> P.S.:
> The place to catch c) would be for example here:
> subversion/libsvn_client/update.c:210, in svn_client__update_internal().
> or here, where the currently seen error is produced:
> subversion/libsvn_repos/reporter.c:1162 in drive().
> Attached is a trial patch that catches b) and c) by issuing an error in
> svn_client__update_internal(). The patch also re-enables update_tests.py 50
> to show this case.
> Alternatively, find it here:
> http://hofmeyr.de/svn/update-in-conflicted-1.patch
> 
> The place where tree-conflicted ancestry is currently detected, i.e. cases
> a) and b), is deeper within the onion, here:
> subversion/libsvn_wc/update_editor.c:1600, by already_in_a_tree_conflict()
>

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