branch tree-conflicts-notify: how many columns? -- was: merge to trunk?

["Neels J. Hofmeyr" <ne...@elego.de>]

Stephen Butler wrote:
> Quoting "Neels J. Hofmeyr" <ne...@elego.de>:
> 
>> Hey Steve! (sbutler)
>>
>> That's so awesome! Somehow you've magically fixed almost everything on
>> the
>> branch! All I had to do was raise and notify a tree-conflict on file
>> addition in a conflicted directory.
> 
> Thanks!
> 
> BTW, I think we should (silently) skip a file-add in a tree-conflicted
> tree, rather than flag a new tree conflict.  And of course skip all
> other changes, too.  Otherwise the user might see a whole treeful of
> t.c. victims, and not be sure where the problem started.  A single
> tree conflict, raised at the root of the tree during open_directory(),
> should be enough.

Hey, of course. What was I thinking. If a directory is tree-conflicted,
everything inside is naturally going to be a tree-conflict and doesn't need
to be listed in detail.

Oh, but what about the case when a directory has a previously raised,
persisting tree-conflict, and say update wants to add a file in it.
Shouldn't that then be a new, separate tree-conflict?

> 
>> Should we add the M or lose the A and D? I'd say add the M and
>> whatever else
>> would have been shown without a tree-conflict, to be able to see what was
>> tried by the update. (Maybe there are more cases to this, though.)
> 
> I believe the first two columns tell the user what changes the update
> has made in their working copy.  If a path is skipped due to a tree
> conflict, we should leave these columns blank.
> 
> Of course, we plan to skip all tree conflict victims (very soon), so
> the 'x  C' will become '   C' in all cases.
> 
> If '   C' is too cryptic, we could add the '(M->!)' later.

Yes, I believe it should be visible what was being tried when encountering a
tree-conflict. Either way, 'D  C' or '   C(D->M)' but btw, I'm actually -1
on the brackets and even on the arrow. I'd prefer having '   CDM'. Much less
characters to read, and less unnecessary indentation for most of the cases
where there are no tree-conflicts. Here's a sort of scale of possibilities:

notify      nr of additional columns
==========  ===
   C         1
D  C         1
D  CM        2
   CDM       3   <-- neels likes this one best.
   CD>M      4
   C D>M     5
   C:D>M     5
   C D->M    6
   C:D->M    6
   C(D>M)    6
   C D->M    6
   C(D->M)   7
   ...
   Tree-Conflict: ((( Delete  --->  Modified )))  25  ;)

So I'd like to stay near the top of this list, because most of the time, the
additional columns will be empty anyway, making it harder to see which
normal notification (on the far left) goes with which path.

Also, the one I like best conforms to the way notification is used until
now: we're just adding two columns for conflict action and reason, no more
and no less. And naturally this should be similar across commands.

> 
>>
>>
>> Either way, I think this branch is ready for reintegration into trunk!!
>> What do you think, Steve?
>> (still in need of a complete `make check' though)
> 
> Yes, if the switch and merge tests aren't badly affected.  Looking
> into that now.

Well, actually, that was a bit quick by me. Let's bugfix some more.

>> The complete local tree was removed, so if you try to add a file a few
>> levels inside a removed dir, all the non-existing path elements are in
>> tree
>> conflict. Makes sense to me.
> 
> But the whole tree is already tree-conflicted, so raising new tree
> conflicts insided it is redundant.

Yes. Somehow I forgot to not want redundancy.


>> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
>> think: In the tests, I guess we should also try to run the same operation
>> that causes the conflicts again, to check that the persisting conflicts
>> aren't re-raised or missed altogether. Is this accounted for yet?
> 
> No, currently we test only fresh tree conflicts.  Good point.

I'll try that now if you haven't already.

> 
>>
>>
>> And to squeeze yet another topic into this mail: What's the deal with the
>> run_and_verify_unquiet_status tests -- is status not intended to show
>> tree-conflicts (on missing nodes) by default?
> 
> True.  The final status check in every run_and_verify_X() runs with the
> -q option, ignoring files and dirs that aren't under version control.

Ah, I see ... Nice catch!

> 
> I think I'll change our DeepTreesTestCase class (on trunk) to do the
> status check unquietly, because so many t.c. victims don't exist.

+1

~Neels

Re: branch tree-conflicts-notify: how many columns?

["Neels J. Hofmeyr" <ne...@elego.de>]

Stephen Butler wrote:
> Quoting Neels J Hofmeyr <ne...@elego.de>:
> 
>> Neels J. Hofmeyr wrote:
>>>
>>> Stephen Butler wrote:
>> [...]
>>>>> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
>>>>> think: In the tests, I guess we should also try to run the same
>>>>> operation
>>>>> that causes the conflicts again, to check that the persisting
>>>>> conflicts
>>>>> aren't re-raised or missed altogether. Is this accounted for yet?
>>>> No, currently we test only fresh tree conflicts.  Good point.
>>>
>>> I'll try that now if you haven't already.
>>
>> No, actually, got caught up in more pressing matters, as you can see
>> in the
>> commit log of r33891.
> 
> Hello tree-conflict fans,
> 
> Neels, I believe the changes in r33891 are actually a part of checking that
> persistent tree conflicts "aren't re-raised or missed altogether."  But I

No, they are not:
[[[
Skip files added into tree-conflicted dirs (and don't notify).
Skip deletion of tree-conflicted nodes.
...
]]]

Firstly, it fixes a notification that I added wrongly, but I left some code
in a comment because we will most certainly want to use it if encountering a
persistent conflict, once we want to deal with that stuff.

Secondly, it skips the actual deletion if the delete was discovered to cause
a tree-conflict, which has an effect on notification, which is the reason
why I included that.

> think we should solve that problem in one go rather than piecemeal.
> Otherwise we'll waste a lot of time massaging test expectations from one
> intermediate (bad) state to another.
+1
But on certain changes it is good to review most relevant test suites to
find errors or understand situations. It helped me today ;)

> 
> Attached is a diff to the tree-conflicts-notify branch, just some hacking
> I've done today to try to skip changes to the working copy in the presence
> of existing tree conflicts.  BTW I cribbed a few good things from Julian's
> skip-inside-conflicts-3.patch from a couple days ago.

So, you're not quite done with it yet, right? Let me know if you'd be happy
to continue on trunk (or whether you've completed it on the branch), see below.

> 
> But I think we should tidy up and merge-to-trunk the pre-r33891 state of
> our branch, because we've accomplished our first goal: per-victim tree
> conflict notification in update/switch output.  Skipping conflicted trees,
> handling merge output, UI improvements (e.g. 'CDM'), etc. can be done in
> future patches to trunk or in short-term branches as needed.
> 
> What do you (all) think?

There's just one case I found today that we'd break when merging as-is:
we've got per-victim conflict notification for update/switch, and we changed
the notification code to count them by the new tree_conflicted flag.
However, merge doesn't use that flag yet and displays tree-conflicts as a
"text-conflict" on the parent dir. The result can be seen in merge_tests.py
103 (on branch tree-conflicts-notify r33895): A tree conflict is counted as
a text conflict.

So do we want to tackle merge notification on trunk? I don't mind either
way. But we're slowly drifting farther and farther from trunk. And it's
slowly becoming a "let's do everything on the branch"-branch instead of an
"I just want to try this first"-branch. If no-one objects (or is quicker
than me), I'll reintegrate.

~Neels

Re: branch tree-conflicts-notify: how many columns?

[Stephen Butler <sb...@elego.de>]

Quoting Neels J Hofmeyr <ne...@elego.de>:

> Neels J. Hofmeyr wrote:
>>
>> Stephen Butler wrote:
> [...]
>>>> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
>>>> think: In the tests, I guess we should also try to run the same operation
>>>> that causes the conflicts again, to check that the persisting conflicts
>>>> aren't re-raised or missed altogether. Is this accounted for yet?
>>> No, currently we test only fresh tree conflicts.  Good point.
>>
>> I'll try that now if you haven't already.
>
> No, actually, got caught up in more pressing matters, as you can see in the
> commit log of r33891.

Hello tree-conflict fans,

Neels, I believe the changes in r33891 are actually a part of checking that
persistent tree conflicts "aren't re-raised or missed altogether."  But I
think we should solve that problem in one go rather than piecemeal.
Otherwise we'll waste a lot of time massaging test expectations from one
intermediate (bad) state to another.

Attached is a diff to the tree-conflicts-notify branch, just some hacking
I've done today to try to skip changes to the working copy in the presence
of existing tree conflicts.  BTW I cribbed a few good things from Julian's
skip-inside-conflicts-3.patch from a couple days ago.

But I think we should tidy up and merge-to-trunk the pre-r33891 state of
our branch, because we've accomplished our first goal: per-victim tree
conflict notification in update/switch output.  Skipping conflicted trees,
handling merge output, UI improvements (e.g. 'CDM'), etc. can be done in
future patches to trunk or in short-term branches as needed.

What do you (all) think?

Steve


Draft commit message for attached patch:

On the tree-conflicts-notify branch:  Try to skip tree-conflicted
items.  So far, I have edited the dir callbacks only.  The file
callbacks, test expectations, etc. remain.  Oh, and testing to see
if it works at all.

[[[

* subversion/libsvn_wc/update_editor.c
   (edit_baton): New boolean field inside_a_tree_conflict.
   (maybe_bump_dir_info): Don't call complete_directory() if inside a
    tree conflict.
   (check_tree_conflict): Rename to ...
   (maybe_raise_tree_conflict): ... and add a todo comment.
   (tree_has_local_mods,
    add_file,
    open_file): Track rename of check_tree_conflict().
   (do_entry_deletion,
    add_directory,
    open_directory,
    close_directory): Track rename of check_tree_conflict().  Skip
    almost everything if inside a tree conflict.  Otherwise, if the dir
    has an existing or freshly-raised tree conflict, notify the user
    and skip almost everything else.  Otherwise proceed normally.
   (change_dir_prop): Skip everything if inside a tree conflict.

]]]



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

Re: branch tree-conflicts-notify: how many columns?

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

Neels J. Hofmeyr wrote:
> 
> Stephen Butler wrote:
[...]
>>> The check_tree_conflict() vs. svn_wc_conflicted_p() discussion made me
>>> think: In the tests, I guess we should also try to run the same operation
>>> that causes the conflicts again, to check that the persisting conflicts
>>> aren't re-raised or missed altogether. Is this accounted for yet?
>> No, currently we test only fresh tree conflicts.  Good point.
> 
> I'll try that now if you haven't already.

No, actually, got caught up in more pressing matters, as you can see in the
commit log of r33891.

~Neels