Re: [Monotone-devel] non-content conflict messages

[Derek Scherger]

Zack Weinberg wrote:
> Cool!  Your revisions are definitely better than what we had, but I
> think they could be even better still.

What? Impossible! ;)

> The term 'node' is only clear if you're familiar with monotone
> internals.  I'd suggest checking what kind of node it is and
> substituting 'file' or 'directory' as appropriate.  (Yes,

Agreed. I'd had this thought as well, but hadn't got to it.

> It's not clear to me if the names printed are leaf names or full
> file_paths.  IMO they should be full file_paths.

They are the full paths from the rosters on both sides of the merge.

> I don't think we should print the node IDs; to user's they're
> mysterious numbers that bear no relation to anything they know about,
> and mysterious numbers are one of the chief evils of computer

Sure, that's easy.

> diagnostic messages.  What would be good to have, though, is ancestral
> names, whenever that makes sense.

Possibly. I'm a bit confused by "names" though. Will there ever be more
than one ancestral "name"? Or do you just mean listing the name it was
when things diverged and what the names have been changed to on both sides.

> As a tangential nit, these are _not_ warnings, we're just using W()
> for them because we don't have a better way to do it.  Can we have D()
> [stands for "detailed error"] whose semantics are: it prints a
> diagnostic which is a hard error, but then it returns to its caller
> rather than terminating the process; furthermore, the "error: " leader
> appears only the first time D() is called, and _not_ subsequently when
> E() is called; and finally, it sets a flag in the ui object which
> causes an invariant failure if D() has been used and then the program
> attempts to exit successfully.  [Or some more elegant way of ensuring
> that code that uses D() always follows it up with an E().]

Agreed, but I'm already time strapped so I'll leave this for now. I'm
just following the current idiom for complaining about several things
before dying, which I think I may have started long ago with manifest
restrictions. I'd be happy to have a better way to report several
problems and ensure an exit afterwards.

> Finally, your proposed diagnostics don't make it sufficiently clear
> which side of the merge is which.  This will be especially important

Yeah, I've been confused myself as to which is which and some
consistency would be good. Although I think that "left" and "right"
sometimes refer to argument order rather than alphabetic order of
revision ids so this may be a bit more invasive than it first appears.

> with workspace merge, because the primitive there is 'merge this head
> into my current workspace', so you really want the pathnames that
> refer to your current workspace to appear in the same place always.
> To deal with this, I suggest we should print a header whenever we're
> about to report conflicts, reading something like this:
> 
> mtn: error: non-content conflicts found while merging:
> mtn:   left revision <LEFTID>
> mtn:  right revision <RIGHTID>
> mtn: common ancestor <LCAID>
> mtn: details follow:

The only change I'd make to this would be to list the specific conflict
type rather than the generic "non-content conflict" using something like
the terminology I currently have.

Hrm, actually, the output of merge already lists left and right:
mtn: 2 heads on branch 'attribute'
mtn: [left]  2cdb958165c446bbccc7746f1d7d5bc48dd83167
mtn: [right] b1e8012e14bb8c65502a6e873ec82d9ef87d3f0d

Do we always have a single common ancestor in *-merge?

> and then consistently refer to LEFTID as left and RIGHTID as right
> throughout.

Sure.

> mtn: name split: ancestral (file|directory) 'LCA_FILE'
> mtn: renamed to 'LEFT_FILE' on the left
> mtn: renamed to 'RIGHT_FILE' on the right

Good.

> This case needs to distinguish four subcases:
> 
> mtn: name collision: (file|directory) 'TARGET_FILE'
> mtn: renamed from 'LEFT_LCA_FILE' on the left
> mtn: renamed from 'RIGHT_LCA_FILE' on the right
> 
> mtn: name collision: (file|directory) 'TARGET_FILE'
> mtn: renamed from 'LEFT_LCA_FILE' on the left
> mtn: created as 'TARGET_FILE' on the left
> 
> mtn: name collision: (file|directory) 'TARGET_FILE'
> mtn: created as 'TARGET_FILE' on the left
> mtn: renamed from 'RIGHT_LCA_FILE' on the right
> 
> mtn: name collision: (file|directory) 'TARGET_FILE'
> mtn: created independently in left and right revs

We also may have a file on one side and a directory on the other so this
 might need a bit more tweaking.

> If this happens the way I think it happens,

Two directories, foo and bar. move foo into bar on one side, move bar
into foo on the other.

> mtn: attempt to create a directory loop:
> mtn: 'foo' renamed to 'bar/foo' on the left
> mtn: and 'foo/bar' renamed to 'bar' on the right
> 
>> [OLD] mtn: warning: orphaned node conflict on node 15, dead parent 13,
>> name bar
>> [NEW] mtn: warning: orphaned node conflict: parent of 'foo/bar' was
>> removed (node 15 parent 13)
> 
> mtn: orphaned (file|directory):
> mtn: directory 'bar' was deleted in (left|right) rev
> mtn: (file|directory) 'bar/foo' was (created|renamed from 'baz') in
> (right|left) rev
> mtn: [... repeat the third line for all immediate children of 'bar';
> do *not* spam
>           the user with messages about descendants of those children]

It appears that this is what we do currently.

>> [OLD] mtn: warning: illegal name conflict: node 16, wanted parent 13,
>> name _MTN
>> [NEW] mtn: warning: illegal name conflict: between 'foo/_MTN' (node 16)
>> and '' (node 13)
> 
> (Can we please avoid the term 'illegal' in user diagnostics?  Nobody
> broke any laws.)

Sure... I'm just using the current terminology. Would "invalid" be any
better?

> This is another case that I'm not sure how to produce; guessing
> something like
> 
> mtn: name collision with the bookkeeping directory:
> mtn: directory 'foo/_MTN' created on the left
> mtn: directory 'foo' pivoted to root on the right

Yeah, and this leaves _MTN as a *versioned* file but its path is not
allowed. (first component can't be "_MTN')

> with variations, as above, for all the different actual operations
> that could do this.  I don't think it's possible to put a versioned
> file inside the bookkeeping directory without also causing a versioned
> directory to have the same name as the bookkeeping directory itself,
> so we should not spam the user for children of 'foo/_MTN' in the

Seems like we do this too.

> example.  (Also, my brain insists, contrary to Ispell, that 'pivoted'
> is misspelt - but doesn't like 'pivotted' either.  How the heck do you
> spell that word?)

Thunderbird likes "pivoted" here?

> Guessing again:
> 
> mtn: merged revision has no root directory:
> mtn: directory 'foo' deleted on the left
> mtn: directory 'foo' pivoted to root on the right

Yup.

> mtn: attribute conflict: 'test-attr' on 'foo'
> mtn: given value 'left-value' on the left
> mtn: given value 'right-value' on the right
> 
> (do we get an attribute conflict when an attr is changed in one rev
> and deleted in another?  If so, that case should be distinguished.)

Apparently, yes.

>> The other thing we should do is add a more detailed section on conflicts
>> to the manual that lists all of these with some hints on how they occur
>> and what to do about them. I'll probably make a first pass at that as I
>> clean this stuff up.
> 
> This would definitely be good, but I don't think it substitutes for
> the diagnostics themselves making clear how the situation occurred.

You're right and I like pretty much all of your suggestions. I'll try
and improve things further and post a follow up.

Cheers,
Derek