Issue 2427 incremental darcs convert export is broken

Title incremental darcs convert export is broken
Priority bug Status resolved
Milestone 2.10.0 Resolved in 2.10.0
Superseder Nosy List bf
Assigned To

Created on 2015-01-22.23:40:53 by bf, last changed 2015-02-12.13:55:48 by gh.

msg17940 (view) Author: bf Date: 2015-01-22.23:40:51
I am trying to incrementally update a git mirror of a darcs repo. In the
darcs repo I do

darcs convert export --read-marks
/opt/repositories/controls/git/seq/branch-2-1/darcs.marks --write-marks
/opt/repositories/controls/git/seq/branch-2-1/darcs.marks | (cd
/opt/repositories/controls/git/seq/branch-2-1 && git fast-import
--import-marks=git.marks --export-marks=git.marks)

This works fine when I create the git repo for the first time. However,
when I apply or record a darcs patch and re-run, I get

darcs failed:  ### Error applying:
hunk ./src/seq/seq_ca.c 53
-       int             delay = 2.0;
+       int             delay = 2;
### to file ./src/seq/seq_ca.c:

There are no conflicts in the two patches that are new:

franksen@tiber: ...support/seq/branch-2-2 > tail
1318: f1df6822df42d94e02d5360e000f09d4dd3ccbef
1319: 9ed63c453680d4bee6efc9b706be580fceb56b40
1320: 517b0f7268b40127df1a7895ff878a56a313b4e2
1321: 3d72dbc523da7b1ef1a6045475f0df1dec63bf5f
1322: 702574fa45ced22bc8c8ab78580e2f759e3a5fef
1323: f0a741b39dd00abe0b5f0726906636565f66a236
1324: de0be0b5a1b77d3098a4b17d8a6fdc0cf42986a7
1325: 3a1a1668cac7fc6df439d1ff502c8cfbeb39410d
1326: 8a2588acd42472d66b0efd1b481acd915ef70f9b
1327: 30313f774f5706b2fe1da5ce39cd1000d9a93f12
franksen@tiber: ...support/seq/branch-2-2 > darcs log -is
patch f6ac9fc34b64cdc0dd082561d286aaf7d104151d
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Jan 23 00:23:53 CET 2015
  * test: replaced testSkip with testPass in pvGet.st

    M ./test/validate/pvGet.st -4 +4
Shall I view this patch? (1/?) [yN...], or ? for more options: n
patch 8cec8fae614ea084dc71aad8b0250aea16543b05
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Wed Jan 21 22:25:30 CET 2015
  * seq: fixed two MS C compiler warnings

    M ./src/seq/seq_ca.c -2 +2
Shall I view this patch? (2/?) [yN...], or ? for more options: n
patch 30313f774f5706b2fe1da5ce39cd1000d9a93f12
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Wed Jan 21 14:24:46 CET 2015
  * Makefile: add changelog target, upload_repo depends on mirror

    M ./Makefile -10 +14
Shall I view this patch? (3/?) [yN...], or ? for more options: n
patch 8a2588acd42472d66b0efd1b481acd915ef70f9b
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Wed Jan 21 20:24:49 CET 2015
  * seq: must not assume valid dbch in CA callbacks
  As a consequence of unlocking around pvVarDestroy inside seq_pvAssign,
  callbacks may be called with dbch being NULL, leading to assertion
  This patch removes the assertion and instead returns in this case.

    M ./src/seq/seq_ca.c -7 +9
Shall I view this patch? (4/?) [yN...], or ? for more options: q

I am using 2.9.9 (+ 213 patches) i.e. current screened.
msg17946 (view) Author: bf Date: 2015-01-24.16:08:42
This is strange: the error message comes from Darcs.Patch.Prim.V1.Apply.
But the repo is in darcs-2 format.
msg17947 (view) Author: ganesh Date: 2015-01-24.16:52:22
It's a bit confusing: darcs patches come in two layers, "primitives" 
and "conflict handling". darcs-1 and darcs-2 both use V1 primitives but 
differ in the conflict-handling layer.
msg17949 (view) Author: bf Date: 2015-01-24.18:56:05
Ok, thanks.

Another thing I noticed: When I do 'darcs convert export --write-marks
darcs.marks' in different copies of the same repo (same as far as darcs
is concerned, i.e. push and pull report no patches), the marks files can
differ. What is even more disconcerting is they still differ if I
removed the numbering (so they contain only the hashes) and sort them
afterwards. They don't even have the same number of lines (1333 vs.
1335). However, if I make a fresh copy of a repo with 'darcs get', then
the marks files are identical.

Looking at the code, it seems that patches are looked up in the marks
file by number, not by their hash key. I think this is wrong, since
darcs can re-order the patches in a repo. Instead, darcs should keep
track of which patches (identified by their hash) have already been
converted (by looking them up in the marks file by the hash value).
These patches should all be applied first, without outputting them to
the fast-export stream. Only then should all the other patches be
converted, added to the fast-export stream, and also recorded in the
marks file.
msg17951 (view) Author: bf Date: 2015-01-24.21:55:14
Should read the fine print:

In the case of incremental export, be careful to never amend, delete or
reorder patches in the source darcs repository.

Quoted from darcs convert export --help. I still think we should be able
to do better. This is a severe restriction and it is all too easy to
break it accidentally.
msg17971 (view) Author: bf Date: 2015-02-01.15:55:26
I am re-opening this as a bug: I have tried it with an intermediate repo
to which I am pushing only (so nothing should get reordered), and I
still get the same error. I have converted the repo with the new patch
from scratch, which worked fine. But on the next push I get the error again:

darcs failed:  ### Error applying:
hunk ./documentation/Installation.txt 53
-   `2.1.18`_ :ref:`Release_Notes_2.1.18` n/a
+   `2.1.18`_ :ref:`Release_Notes_2.1.18` :ref:`Known_Problems_2.1.18`
### to file ./documentation/Installation.txt:
msg17977 (view) Author: bf Date: 2015-02-02.20:41:30
Resolved by Patch1253, which also resolves issue2429, which had the same
reason, even though the symptoms were quite different.
msg17990 (view) Author: noreply Date: 2015-02-02.23:03:41
The following patch sent by Ben Franksen <benjamin.franksen@helmholtz-berlin.de> updated issue issue2427 with
status=resolved;resolvedin=2.10.0 HEAD

* resolve issue2427: start conversion from intermediate tree state 
Ignore-this: d1f5e117b0304dd0bf5f54a82f6546a3

The bug was caused by ignoring the tree created when replaying the patches
from the marks file (first call to hashedTreeIO) and then starting the
actual conversion from an empty tree (second call to hashedTreeIO). The
simple fix is to use the result from the first call as the starting point
for the second one.
msg18081 (view) Author: gh Date: 2015-02-12.13:54:26
Yes, your fix is good. The bug is my mistake, when porting the
fastconvert code to darcs, I ignored the intermediate tree state and
that's the problem. It was because initially I wanted that code to be as
minimal as possible and removed the possibility of doing incremental
exports (and imports). When enabling incremental imports again I forgot
to use that tree state.

Indeed, as you said, when convert --export is invoked with an existing
marksfile, the function "check" is called and builds the state of the
repository following the existing marksfile.

The patch hashes in the marksfile are present to check that the patches
and patch order have not been changed since the last export, ie that the
export is strictly incremental.
msg18083 (view) Author: gh Date: 2015-02-12.13:55:47
The fix goes into 2.10.
Date User Action Args
2015-01-22 23:40:53bfcreate
2015-01-24 16:08:43bfsetmessages: + msg17946
2015-01-24 16:52:23ganeshsetmessages: + msg17947
2015-01-24 18:56:07bfsetmessages: + msg17949
2015-01-24 21:55:16bfsetpriority: feature
messages: + msg17951
title: incremental darcs convert export fails -> incremental darcs convert export should not fail when patches are reordered
2015-02-01 15:55:28bfsetpriority: feature -> bug
messages: + msg17971
title: incremental darcs convert export should not fail when patches are reordered -> incremental darcs convert export is broken
2015-02-02 20:41:32bfsetstatus: unknown -> resolved
resolvedin: 2.10.0
messages: + msg17977
milestone: 2.10.0
2015-02-02 23:03:43noreplysetmessages: + msg17990
2015-02-12 13:54:28ghsetstatus: resolved -> unknown
messages: + msg18081
2015-02-12 13:55:48ghsetstatus: unknown -> resolved
messages: + msg18083