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 > <Johannes.Schindelin@xxxxxx> wrote: > > > > On Mon, 10 Jan 2022, Elijah Newren wrote: > > > > > 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. > > > 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. > > 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
