Re: [PATCH 08/12] merge-ort: provide a merge_get_conflicted_files() helper function

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,

On Fri, 25 Feb 2022, Elijah Newren wrote:

> On Fri, Feb 25, 2022 at 8:31 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Tue, 22 Feb 2022, Elijah Newren wrote:
> >
> > > On Mon, Feb 21, 2022 at 2:46 AM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > >
> > > > Concretely: while I am not currently aware of any web UI that allows
> > > > to resolve simple rename/rename conflicts, it is easily conceivable
> > > > how to implement such a thing. When that happens, we will need to be
> > > > able to teach the server-side code to discern between the cases that
> > > > can be handled in the web UI (trivial merge conflicts, trivial
> > > > rename/rename conflicts) as compared to scenarios where the conflicts
> > > > are just too complex.
> > >
> > > Um, I'm really worried about attempting to make the conflict notices
> > > machine parseable.  I don't like that idea at all, and I even tried to
> > > rule that out already with my wording:
> > > """
> > > In all cases, the
> > > <Informational messages> section has the necessary info, though it is
> > > not designed to be machine parseable.
> > > """
> > > though maybe I should have been even more explicit.  The restrictions
> > > that those messages be stable is too rigid, I think.  I also think
> > > they're a poor way to communicate information to a higher level tool.
> > > I would much rather us add some kind of additional return data
> > > structures from merge ort and use them if we want extra info.
> >
> > Okay.
> >
> > I thought that we could keep the `CONFLICT (<type>)` constant enough to
> > serve as such a machine-parseable thing.
>
> That _probably_ is, but I thought you wanted to parse all N paths
> embedded in the message after that part as well?

Actually, no, sorry for being unclear. My main aim with the
machine-parseable messages was to detect whether a given failed merge
contains _only_ conflicts of the sort that a particular UI can handle.

In that respect, I would not even need to parse the file names, I'd just
require them not to contain line feed characters ;-)

> > And then presenting
> > `<path>NUL<message>NUL` could have served my use case well...
>
> Would it?  Wouldn't you need something more like
>
> <number-of-paths>NUL<path1>NUL<path2>NUL<pathN>NUL<stable-short-type-description>NUL<message>NUL
>  ?

Probably, you're right.

> I mean, if rename/rename is what you want to handle, there are three
> paths in that message.  And you need to know all three paths in order
> to combine the relevant parts of the <Conflicted File Info> section
> together.
>
> (Also, while we're at it, I decided to throw a stable short-type
> description string (e.g. "CONFLICT (rename/rename)") in there, which
> will _probably_ be the first part of the message string but still
> allow us to change the message string later if we want.)

Yes, I very much like that idea to keep the prefix in a format that we can
guarantee to be stable enough for applications (or server backends) to
rely on.

> Also, we'd want those parsing this information to keep in mind that:
>   * Any given conflict can affect multiple paths
>   * Any path can be part of multiple conflicts
>   * (The above two items imply a potentially many-to-many relationship
> between paths and conflicts)
>   * Paths listed in these logical conflicts may not correspond to a
> file in the index (they could be a directory, or file that was in a
> previous version)
>   * Some of these "logical conflicts" are not actually conflicts but
> just notices (e.g. "auto-merging" or "submodule updated" or "WARNING"
> or "<submodules are weird>" messages)
>
> and we'd have to do some work to make sure the paths in the given
> messages lined up with the files actually recorded in the index (e.g.
> with distinct types we rename both files to avoid the collision, but
> print the conflict notice for the original path rather than the new
> paths)
>
> [...]
> > > But I'm going to reserve the right in merge-ort to modify, add, or
> > > delete any of those messages passed to path_msg(), which might wreak
> > > havoc on your attempts to parse those strings.  I think they're a bad
> > > form for communicating information to a script or program, and trying
> > > to transform them into such risks making them suboptimal at
> > > communicating info to humans.  These messages should optimize the
> > > latter, and if we want something for the former, it should probably be
> > > a new independent bit of info.
> >
> > Makes sense.
> >
> > So we need something in addition to those messages.
>
> Yes.  Does the proposal above sound like it'd cover your needs?  If
> so, we'd probably need to go through all the callers to path_msg() and
> either add an immediate call to another function immediately
> afterwards that stores this additional information or somehow change
> the path_msg() call itself to somehow take an additional arbitrary
> list of arguments representing the paths and short-desc we want to
> store somewhere.

Yes, you're right, the proper thing to do is to go through the callers to
`path_msg()` so that we can provide that stable output you proposed. A
couple of thoughts about this:

* These are not informal messages, i.e. I think we would need another flag
  that would then trigger another double-`NUL`-separated section to be
  shown, probably between the conflicted file info and the informational
  messages.

* Instead of _adding_ the calls, we could look into refactoring the
  existing `path_msg()` calls, introducing yet another function that would
  call `path_msg()` but also optionally generate that machine-parseable
  conflict info.

* None of this needs to hold up `en/merge-tree`. I am sorry that I am the
  blocker (unintentionally so, I guarantee you that!).

Ciao,
Dscho