darcs

Patch 1806 add an explicit type for the output of resolveConflicts

Title add an explicit type for the output of resolveConflicts
Superseder Nosy List ganesh
Related Issues
Status in-discussion Assigned To
Milestone

Created on 2019-06-09.11:57:03 by ganesh, last changed 2019-06-12.15:23:04 by bf.

Files
File name Status Uploaded Type Edit Remove
add-an-explicit-type-for-the-output-of-resolveconflicts.dpatch ganesh, 2019-06-09.11:57:02 application/x-darcs-patch
patch-preview.txt ganesh, 2019-06-09.11:57:02 text/x-darcs-patch
unnamed ganesh, 2019-06-09.11:57:02 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg20702 (view) Author: ganesh Date: 2019-06-09.11:57:02
I'm not going to screen this for a bit, to allow a chance
for comments. It's a followup to the discussion in patch1709.

I think it makes options for further refactorings clearer,
e.g. we could delay the mangling until we actually want to
mark the conflicts. But particularly the V1 code is a bit
involved so I'd rather take it slowly.

1 patch for repository darcs-unstable@darcs.net:screened:

patch c2e1a252c039522eb7d92b2a1c69f512e955db3b
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Jun  9 12:43:43 BST 2019
  * add an explicit type for the output of resolveConflicts
  
  This makes it clear that the output contains first the
  'mangled' view followed by all the conflicting parts.
  Currently the mangled view is the conflict markers if
  the conflict is all hunks, and one of the conflicting
  parts if not.
  
  There is a small behaviour change for the conflict parts
  for V1 patches: previously the mangled value would be
  removed from the parts if present, and now it's not. This
  doesn't make any difference at the use sites, and is
  consistent with V2 patches.
Attachments
msg20718 (view) Author: bf Date: 2019-06-11.16:53:09
> I'm not going to screen this for a bit, to allow a chance
> for comments. It's a followup to the discussion in patch1709.

Please don't. It will certainly conflict with the API changes I needed
to make to integrate RepoPatchV3. These changes remove one layer of
lists in the API and instead add a generic function combineConflicts to
Darcs.Patch.Conflicts that the existing (V1 and V2) RepoPatch
implementations are able to use to reduce the two-layer list to one
layer. The list-of-lists really is an internal implementation detail
here and should not be exposed to any user code (i.e. in
Darcs.Repository.Resolution). I'll add some explanations when I send
this patch.

> I think it makes options for further refactorings clearer,
> e.g. we could delay the mangling until we actually want to
> mark the conflicts. But particularly the V1 code is a bit
> involved so I'd rather take it slowly.

I have done some refactoring there, too, in order to make the changes
mentioned above feasible. Please postpone until you have taken a look at
them, I think the code is a lot clearer now and also uses much less
witness casting.
msg20738 (view) Author: ganesh Date: 2019-06-12.05:45:15
> Please don't. It will certainly conflict with the API changes I 
> needed to make to integrate RepoPatchV3.

Sure. I would just comment that my change is purely a representation
change + a single tiny behaviour change to V1 patches, so would be
relatively easy to merge with and might make the intent of further
refactorings clearer. But I'm happy to just review other changes
directly based on my understanding of the current representation
anyway.
msg20758 (view) Author: bf Date: 2019-06-12.15:23:03
Please take a look at the patch named "change the type of 
resolveConflicts" in the latest bunch of patches I attached to 
patch1730. I think this already clears up the confusion regarding the 
nested list. If not, I am fully open to introduce a new type as an 
additional refactor.
History
Date User Action Args
2019-06-09 11:57:03ganeshcreate
2019-06-09 11:58:41ganeshsetstatus: needs-screening -> in-discussion
2019-06-11 16:53:09bfsetmessages: + msg20718
2019-06-12 05:45:15ganeshsetmessages: + msg20738
2019-06-12 15:23:04bfsetmessages: + msg20758