Re: [PATCH 2/7] merge-ort: add ability to record conflict messages in a file

From: Jeff King <peff@peff.net>
Date: Sep 30, 2021, 07:58 UTC
Message-ID: <YVVuP8ReqaPi/Z5E@coredump.intra.peff.net>
In-Reply-To: <CABPp-BEioOUJRd6FSsmsDtYHhUy7xhr4YabdEmVKzkduo4g9TQ@mail.gmail.com>

On Tue, Sep 28, 2021 at 11:25:20PM -0700, Elijah Newren wrote:

Show 13 quoted lines

> > Just brainstorming some alternatives:
> >
> >  - we have diff.orderFile, etc. Could we stuff this data into a less
> >    confusing name (even just "$filename.conflict_msg"), and then provide
> >    a custom ordering to the diff code? I think it could be done by
> >    generating a static ordering ahead of time, but it might even just be
> >    possible to tell diffcore_order() to take the ".conflict_msg"
> >    extension into account in its comparison function.
> 
> I can't just go on the ".conflict_msg" extension.  As you noted above,
> this scheme is not sufficient for avoiding collisions.  So I need to
> append extra "cruft" to the name in the case of collisions -- meaning
> we can't special case on just that extension.

Sure, but we can call it filename.conflict_msg.1, etc, and the sort code can pattern-match. It can never be fully robust (if you really did have a foo.conflict_msg, we'd sort it differently), but I think it's OK if the worst-case is that pathological trees get ordered slightly sub-optimally).

That said, I think it could also make sense to annotate the conflict files by giving them an unusual set of mode bits. That would be easier to recognize (and while real trees _could_ have silly modes, we do complain about them in fsck, so it shouldn't happen in practice).

Show 11 quoted lines

> I also don't like how diff.orderFile provides a global ordering of the
> files listed, rather than providing some scheme for relative
> orderings.  That'd either force me to precompute the diff to determine
> all the files that were different so I can list _all_ of them, or put
> up with the fact that the files with non-content conflicts will be
> listed very first in the output, even if their name is
> 'zee-last-file.c' -- surprising users at the output ordering.
> 
> This also means that if the user had their own ordering defined, then
> I'm overriding it and messing up their ordering, which might be
> problematic.

Agreed. I do think it may be too horrible unless you teach diffcore_order() to actually understand your annotations in code. But that wouldn't be too hard if it's done in the mode bits.

But I do think anything that avoids these pseudo-files is going to be a lot cleaner overall.

Show 11 quoted lines

> >  - there can be other non-diff data between the individual segments. For
> >    example, "patch" will skip over non-diff lines. And certainly in Git
> >    we have our own custom headers. I'm wondering if we could attach
> >    these annotations to the diff-pair somehow, and then show something
> >    like:
> >
> >      diff --git a/foo.c b/foo.c
> >      index 1234abcd..5678cdef 100644
> >      conflict modify/delete foo.c
> 
> A couple things here...

I think Junio already indicated that we can pretty much make this look like whatever we want. As soon as any reader sees "conflict", it should happily ignore the rest as something it doesn't know about. And my short example here was just meant to be illustrative. I agree it probably needs more details (and the whole CONFLICT line that usually goes to stderr is probably the best thing).

-Peff