Hi Elijah,

to save every reader some time, I snipped the parts that were addressed
elsewhere already.

On Tue, 11 Jan 2022, Elijah Newren wrote:

On Tue, Jan 11, 2022 at 2:30 PM Johannes Schindelin
[off-list ref] wrote:

quoted

On Mon, 10 Jan 2022, Elijah Newren wrote:

quoted

You have a couple problematic assumptions here:

  * What if the user's resolution required fixing a semantic
  conflict, meaning they needed to modify a file that had no
  conflicts?  Your code here would ignore those, because the clean
  case is handled above.

  * What if the user's resolution required adding a totally new file
  (either because a rename is handled as a separate add/delete, or
  they just needed a new file)?  The loop above isn't over items in
  pre_resolved_conflicts, it just assumes that items in
  pre_resolved_conflicts are also in the plist.items being looped
  over.

I can see how these assumptions may look ludicrous when coming from
the command-line. But again, we are talking about the realities of a
conflict resolution via a web UI.

So I think that it is out of the question to address non-textual
conflicts in this scenario. And then the assumptions make much more
sense.

Waving your hands and saying it came from a web UI doesn't reduce my
worries or concerns at all.  I could easily imagine a web UI that
allows users to specify other modifications they want to make, even
limited to textual ones, to include in the merge.  What happens when
they modify some file that had otherwise merged cleanly (e.g. a file
that gains a new call to some function, which after the merge needs an
extra parameter passed to it)?  Your solution doesn't handle it; it'd
send that user-specified change to /dev/null.

To be fair, you mentioned below that this is just a proof of concept,
and that certainly reduces worries/concerns.  It's totally fine to
have such things.  Maybe you intend to keep this patch internal
indefinitely.  That's fine too.  My commentary is just feedback on if
we want merge-ort/merge-tree extended more in this kind of fashion
(and it might also serve as useful pointers on how to extend your
internal patch if you get requests to extend your web UI a bit to
handle more cases).  :-)

Fair enough. I think I'll keep this patch internal-only for now, and
iterate with real scenarios to figure out what we need/not need.

quoted

quoted

Could you clarify what you mean here by OIDs and modes?  For a given
path, are you just looking for a (pathname, OID, mode) tuple?  (In
which case, you'd get the OID of a file that potentially has embedded
conflict markers)

Or are you looking for multiple OIDs and modes for each file?

This. In case of a conflict, I am looking for (mode,OID) for the merge
base (which _can_ be a virtual one in case of recursive merges) as well as
for the two divergent revisions that were supposed to be merged.

I do realize that other types of conflicts can occur, such as a
rename/rename conflict, and we would need a way to represent these in the
output, too. But in such an instance, where no clear (mode,OID) triplet
can be provided that represents the merge conflicts for this file, the
current web UI cannot offer a way to resolve the conflict, so I am a bit
less interested in that scenario.

Okay, this is helping explain a bit better.

Out of curiosity, does this mean the web UI only can resolve cases
where all three modes & OIDs are present, and the files are text
rather than binary?  For example, does this mean the web UI cannot
handle cases like modify/delete or add/add?

Right now, the UI is based on the assumption that the underlying merge
machinery does not even try to detect renames. Which simplifies the range
of supported scenarios somewhat.

I have not looked at the underlying code, but
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github
clearly says:

	You can resolve simple merge conflicts that involve competing line
	changes on GitHub, using the conflict editor.

So yes, the UI does not even try to support resolving modify/delete
conflicts at this time.

quoted

But again, you made me think of rename/rename conflicts and friends, and
we would need a way to represent those, too. Even if my current use case
would need to only detect their presence in order to say "nope, can't
resolve that on the web".

But now you're making me worry whether I can satisfy your requests
again, or at minimum, whether I need to do a lot more work in
merge-ort to answer them.  Do you need a representation other than the
list of (stage, mode, oid) triplets?

I guess we will have to accept that the (stage, mode, OID) list is the
best form, for now.

We will have to see what is possible to do on the UI side, and then extend
the `merge-ort` side as needed.

I'm a little worried I might come across sounding a little picky since I
tend to dive into details and really fixate on them.  I apologize in
advance if it sounds that way at all; you provide lots of good points to
think about and I can't help but dive into the quirks surrounding each.
I really appreciate all the awesome feedback and food for thought.

Personally, I did not find your comments picky at all, but rather
constructive. I find this conversation thoroughly enjoyable and productive
even at times when you prove me wrong. You set a really high bar as far as
collaboration style goes. Thank you very much for that!

Ciao,
Dscho