darcs

Patch 1872 regard explicit dependencies as resolving conflicts

Title regard explicit dependencies as resolving conflicts
Superseder Nosy List bf
Related Issues
Status accepted Assigned To bf
Milestone

Created on 2019-08-15.07:20:41 by bf, last changed 2019-09-16.15:49:25 by ganesh.

Files
File name Status Uploaded Type Edit Remove
extend-tests_resolve_conflicts_explicitly_sh.dpatch bf, 2019-09-15.10:19:17 application/x-darcs-patch
fix-and-document-instance-conflict-named.dpatch bf, 2019-09-15.09:16:54 application/x-darcs-patch
patch-preview.txt bf, 2019-08-15.07:20:41 text/x-darcs-patch
patch-preview.txt bf, 2019-09-15.09:16:54 text/x-darcs-patch
patch-preview.txt bf, 2019-09-15.10:19:17 text/x-darcs-patch
regard-explicit-dependencies-as-resolving-conflicts.dpatch bf, 2019-08-15.07:20:41 application/x-darcs-patch
unnamed bf, 2019-08-15.07:20:41 text/plain
unnamed bf, 2019-09-15.09:16:54 text/plain
unnamed bf, 2019-09-15.10:19:17 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21103 (view) Author: bf Date: 2019-08-15.07:20:41
We have discussed this change on the list. I think this is the only viable
solution we can currently implement without a change in the semantics of
prim patches. It also lines up with (and gives an official justification
for) not looking beyond the latest clean tag when searching for conflicts
(see patchsetConflictResolutions).

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

patch 6f7195b9d5fd43169850bc85f2ebd28bbd52abb9
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Jul 20 13:51:58 CEST 2019
  * regard explicit dependencies as resolving conflicts
  
  This is necessary to allow users to resolve a conlfict in favour of what
  darcs does by default (i.e. apply neither of the conflicting changes). The
  implementation is mostly in Darcs.Patch.Named which now has an instance
  Conflict. The PatchInfoAnd instance only delegates to the contained Named 
  patch. A subtlety here is that for efficiency and correctness in the face of
  lazy repos, we must make sure that hopefully is lazy in its patch argument.
Attachments
msg21285 (view) Author: ganesh Date: 2019-08-30.20:50:54
Would you mind adding a comment to the Conflict (Named p) instance to
explain the rationale for the behaviour?

Otherwise looks fine.
msg21287 (view) Author: bf Date: 2019-08-30.21:49:41
> Would you mind adding a comment to the Conflict (Named p) instance to
> explain the rationale for the behaviour?

Good idea. I just started to write down a description and justification.
This made me see a flaw in the algorithm, which I think can lead to an
unresolved conflict not being shown because it erroneously regards it as
resolved. Can you spot the problem?

I am still thinking about how to fix it...

Cheers
Ben
msg21293 (view) Author: ganesh Date: 2019-08-31.01:34:32
> This made me see a flaw in the algorithm, which I think can lead to an
> unresolved conflict not being shown because it erroneously regards it as
> resolved. Can you spot the problem?

Is it to do with the fact that even content-based dependencies at the
Named level are less fine-grained than at the level of the contents of
those Nameds, since a Named contains an FL? So if p depends on q by
name, q depends on r by content, then r will end up in ctx. But if some
element of r that *isn't* depended on by something in q has a conflict,
we should perhaps regard it as unresolved.
msg21294 (view) Author: bf Date: 2019-08-31.08:22:03
>> This made me see a flaw in the algorithm, which I think can lead to an
>> unresolved conflict not being shown because it erroneously regards it as
>> resolved. Can you spot the problem?
> 
> Is it to do with the fact that even content-based dependencies at the
> Named level are less fine-grained than at the level of the contents of
> those Nameds, since a Named contains an FL? So if p depends on q by
> name, q depends on r by content, then r will end up in ctx. But if some
> element of r that *isn't* depended on by something in q has a conflict,
> we should perhaps regard it as unresolved.

Exactly. So if A1 and A2 are unrelated conflicted changes, B1 depends on
A1, but not A2, we have named patches A=(A1;A2) and B=(B1) and finally C
which depends on B by name. Then even A2 will count as resolved, which I
think is wrong.

The difficulty in fixing this is that we now need a way to pass the
information about which patches we /do/ know are resolved (all those
which are directly depended on by name) down to the resolveConflicts of
the lower layer. I think this requires another change to the API.

Or we pursue the idea of Dependors that capture explicit dependencies at
the RepoPatch level (that wouldn't work for V1 and V2, but perhaps this
is okay).
msg21422 (view) Author: bf Date: 2019-09-15.09:16:54
As discussed, the algorithm used before was too simple: it could lead to
conflicts being hidden by indirect dependencies between Named patches. It
turned out that this can be fixed without changing the API.

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

patch 7ae6089abf4eee4b583a9f9724bb49d90def4e28
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Aug 31 17:26:55 CEST 2019
  * fix and document instance Conflict Named
Attachments
msg21423 (view) Author: bf Date: 2019-09-15.10:19:17
1 patch for repository http://darcs.net/screened:

patch 3ee34d503c4a79de22531f261fa251e700d87c2a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Sep 15 12:17:52 CEST 2019
  * extend tests/resolve-conflicts-explicitly.sh
  
  This adds a test that explicitly depending on a partial resolution does not
  inadvertantly turn it into a full resolution. It also cleans up the existing
  test with better names for the patches and defines a common baseline for the
  conflicts.
Attachments
msg21427 (view) Author: bf Date: 2019-09-15.11:49:59
This is the last bundle that still stands in the way of pulling the
2.15.1 tag to reviewed, so if you could finish the review this would
make it easier for me to catch up with reviewing your patches in turn...

It really was a bad idea to tag screened. We should never do that again.
And we should make a 2.15.2 tag that includes all the follow-up patches,
this time directly on the reviewed repo.
msg21432 (view) Author: ganesh Date: 2019-09-16.14:29:52
Looks ok. The whole setup is now complicated enough that I can't completely
convince myself it's right. But I certainly can't spot any problems and the
overall situation seems better than before this patch.

I'll run the tests and push all the 'accepted-pending-tests' patches to 
reviewed now.
History
Date User Action Args
2019-08-15 07:20:41bfcreate
2019-08-15 07:21:30bfsetstatus: needs-screening -> needs-review
2019-08-30 20:50:54ganeshsetstatus: needs-review -> followup-requested
assignedto: bf
messages: + msg21285
2019-08-30 21:49:42bfsetmessages: + msg21287
2019-08-31 01:34:32ganeshsetmessages: + msg21293
2019-08-31 08:22:04bfsetmessages: + msg21294
2019-09-15 09:16:55bfsetfiles: + patch-preview.txt, fix-and-document-instance-conflict-named.dpatch, unnamed
messages: + msg21422
2019-09-15 10:19:17bfsetfiles: + patch-preview.txt, extend-tests_resolve_conflicts_explicitly_sh.dpatch, unnamed
messages: + msg21423
2019-09-15 11:49:59bfsetmessages: + msg21427
2019-09-16 14:03:54ganeshsetstatus: followup-requested -> review-in-progress
2019-09-16 14:29:53ganeshsetstatus: review-in-progress -> accepted-pending-tests
messages: + msg21432
2019-09-16 15:49:25ganeshsetstatus: accepted-pending-tests -> accepted