darcs

Patch 2305 change (and fix) conflict markup for RepoPatchV3

Title change (and fix) conflict markup for RepoPatchV3
Superseder Nosy List bfrk
Related Issues
Status accepted Assigned To
Milestone

Created on 2023-06-05.20:53:24 by bfrk, last changed 2023-07-29.10:54:32 by ganesh.

Files
File name Status Uploaded Type Edit Remove
change-_and-fix_-conflict-markup-for-repopatchv3.dpatch bfrk, 2023-06-18.16:40:57 application/x-darcs-patch
patch-preview.txt bfrk, 2023-06-18.16:40:57 text/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg23315 (view) Author: bfrk Date: 2023-06-05.20:53:19
I will NOT screen this one immediately. PLEASE take a look at the semantic
changes (described in the patch comment as well as in the haddocs) and tell
me whether you think they are agreeable.

I originally pulled this one from my development branch after fixing
permutationsRL which in turn uncovered bugs (now) detected by
propResolutionsOrderIndependent, in both the old and the new version. Since
the new version is much simpler, I fixed that first, and since it plays much
better with adding patch names to the markup, I decided not to bother with
the bugs in the old version and instead to send the new (fixed) version as a
single patch.

The added regression test is a tricky (but not too verbose) example of what
can go wrong, exactly as found by quickcheck. I also added detailed
discussion of the subtleties in the docs for findComponents. Which, BTW, now
take up more space than the code, once again demonstrating how concise and
expressive Haskell is. It is the only practical and efficient language that
allows me to consequently follow the DRY principle.

1 patch for repository http://darcs.net/screened:

patch 24b0cab8841e962e322cf304c93ebad1a8928227
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 30 09:38:40 CET 2022
  * change (and also fix) conflict markup for RepoPatchV3

  We no longer calculate the maximal non-conflicting sets and merge them.
  Instead, the markup now displays each of the conflicting alternatives
  directly. These alternatives now also include those that others depend on.

  While the previous conflict markup method is (I believe) theoretically valid
  (and a beautiful application of graph theory), in the end it is less useful
  than explicitly listing all conflicting alternatives exactly as they arise
  in the project's history. This is especially true when the markup also
  contains the corresponding prim patch IDs (which is now implemented but not
  yet sent).

  On the implementation side, the conflict graph is no longer constructed
  explicitly. Instead, we take advantage of the information contained in the
  conflictors in order to construct the components of the conflict graph (i.e.
  the transitive conflicts) directly after commuting a conflicted patch to the
  head. This simplifies the algorithm substantially.

  Raising the lower bound on the containers dependency has been done so we can
  use 'Data.Set.disjoint'.
Attachments
msg23317 (view) Author: bfrk Date: 2023-06-06.10:15:25
Amended with a fix to tests/order_of_resolutions.sh for case-insensitive
file systems.

1 patch for repository http://darcs.net/screened:

patch ba51e86f86c258cdbdfe338888b692afe32cf113
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 30 09:38:40 CET 2022
  * change (and also fix) conflict markup for RepoPatchV3

  We no longer calculate the maximal non-conflicting sets and merge them.
  Instead, the markup now displays each of the conflicting alternatives
  directly. These alternatives now also include those that others depend on.

  While the previous conflict markup method is (I believe) theoretically valid
  (and a beautiful application of graph theory), in the end it is less useful
  than explicitly listing all conflicting alternatives exactly as they arise
  in the project's history. This is especially true when the markup also
  contains the corresponding prim patch IDs (which is now implemented but not
  yet sent).

  On the implementation side, the conflict graph is no longer constructed
  explicitly. Instead, we take advantage of the information contained in the
  conflictors in order to construct the components of the conflict graph (i.e.
  the transitive conflicts) directly after commuting a conflicted patch to the
  head. This simplifies the algorithm substantially.

  Raising the lower bound on the containers dependency has been done so we can
  use 'Data.Set.disjoint'.
Attachments
msg23318 (view) Author: ganesh Date: 2023-06-07.23:48:11
I chatted to Ben about this on IRC and after some discussion I
suggested the following principles for an "ideal" conflict markup:

1. See each patch in precisely one alternative.

This is quite a big deal in my experience. I generally try
to resolve conflicts by looking at the differences of one
alternative to the base and then manually applying them to another 
alternative. It substantially increases the mental load if two
alternatives have some of the same differences.

2. probably have as few alternatives as possible

I don't feel as strongly about this one, but I think it probably
reduces the work for resolving conflicts.
msg23319 (view) Author: bfrk Date: 2023-06-11.20:01:45
In the chat I suggested that the second requirement more or less 
implies that we retain the part of the previous behavior that refers to 
alternatives that depend on each other. So if we merge A;B with C, 
where B depends on A, and C conflicts with A and thus also with B, then 
we do not list A as a separate alternative. I agree that this makes 
sense, at least as the default behavior.

A typical scenario is when we make a (single) change in a clone and 
(much) later pull from upstream. If upstream has a patch that conflicts 
with ours, then there is a high probability that other changes in 
upstream depend on it. We don't want all those changes to be listed as 
separate alternatives in our markup. Instead we want only two 
alternatives: our single patch and the conflicting upstream change 
together with everything that depends on it (this assumes upstream 
itself has no unresolved conflicts).

I also suggested that when we annotate alternatives with patch IDs 
(hashes), we will should list all patch IDs that make up the 
alternative.

We could also make the exact behavior of conflict markup configurable, 
i.e. define markup strategies and let the user select them with a 
suitable option.
msg23320 (view) Author: bfrk Date: 2023-06-17.08:26:05
Will re-send amended patch after discussion.
msg23336 (view) Author: bfrk Date: 2023-06-18.16:40:57
Second version, with re-added suppression of alternatives that others depend
on, as discussed. This is done in a modular way (cf. purgeDeps) which means
we can easily disable it in case we later decide to make this part of the
behavior configurable.

Will screen this version unless there are complaints.

1 patch for repository http://darcs.net/screened:

patch a5e8c764c8aff56aa940e2bdddb1189a1b667fee
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Dec 30 09:38:40 CET 2022
  * change (and fix) conflict markup for RepoPatchV3

  The main semantic change is that we don't merge the maximal non-conflicting
  sets of the conflict graph. Instead, we display each of the conflicting
  alternatives exactly as they arise in the project's history. While merging
  of non-conflicting alternatives is theoretically valid and typically results
  in fewer alternatives, it makes manual conflict resolution more difficult.
  The comments explain the rationale in more detail.

  The previously failing test for propResolutionsOrderIndependent now pass.
  This is due to a few subtle fixes in the algorithm. The details are
  explained in the docs for findComponents.

  On the implementation side, we no longer calculate from scratch which
  alternatives conflict with each other, since the conflictor representation
  already contains that information; we just have to accumulate it properly
  during the traversal.

  Raising the lower bound on the containers dependency has been done so we can
  use 'Data.Set.disjoint'.
Attachments
msg23337 (view) Author: bfrk Date: 2023-06-18.16:48:34
Removed the obsolete bundles. Reason: I regularly become confused when 
I accidentally download a bundle that was merely marked as "dead" 
because you see the status only in the Files section, not in the 
Attachments section of the message.
msg23598 (view) Author: ganesh Date: 2023-07-16.20:15:08
Accepted - I've read through the text but consciously chosen not to
fully understand and double-check it against the code, as that would
take me quite a while.
History
Date User Action Args
2023-06-05 20:53:24bfrkcreate
2023-06-06 10:15:25bfrksetfiles: + patch-preview.txt, change-_and-also-fix_-conflict-markup-for-repopatchv3.dpatch
messages: + msg23317
2023-06-06 10:15:54bfrksetfiles: - change-_and-also-fix_-conflict-markup-for-repopatchv3.dpatch
2023-06-06 10:16:27bfrksetfiles: - patch-preview.txt
2023-06-07 23:48:12ganeshsetmessages: + msg23318
2023-06-11 20:01:47bfrksetmessages: + msg23319
2023-06-17 08:26:09bfrksetstatus: needs-screening -> rejected
messages: + msg23320
2023-06-17 08:27:17bfrksetstatus: rejected -> in-discussion
2023-06-18 16:40:57bfrksetfiles: + patch-preview.txt, change-_and-fix_-conflict-markup-for-repopatchv3.dpatch
messages: + msg23336
title: change (and also fix) conflict markup for RepoPatchV3 -> change (and fix) conflict markup for RepoPatchV3
2023-06-18 16:46:59bfrksetfiles: - change-_and-also-fix_-conflict-markup-for-repopatchv3.dpatch
2023-06-18 16:47:06bfrksetfiles: - patch-preview.txt
2023-06-18 16:48:35bfrksetmessages: + msg23337
2023-06-19 06:20:39bfrksetstatus: in-discussion -> needs-review
2023-07-16 20:15:10ganeshsetstatus: needs-review -> accepted-pending-tests
messages: + msg23598
2023-07-29 10:54:32ganeshsetstatus: accepted-pending-tests -> accepted