Bunch of mostly unrelated fixes and improvements to test scripts.

6 patches for repository http://darcs.net/screened:

patch 724d1532b60f56c3859f46289e66dfb1fbc157b3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jul  9 18:33:58 CEST 2019
  * slightly extend and cleanup test for issue154
  
  The test did not test that the previously added file d/moo can still be
  found somewhere after we pull 'rmdir ./d'.
  
  BTW, I wonder why the file d/moo gets deleted at all by the pull.

patch 91a9966d596ac9866bcc91736e465591168b355b
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 19 23:50:06 CEST 2019
  * tests: make ssh test more robust
  
  This means we set the remote PATH so that it uses the darcs we are testing,
  and also set --remote-darcs to that when pushing.
  Also replace use -a option instead of piping ys.

patch ab8d4748e854e9ee8bd16c3797921446824b5d74
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul  7 21:22:50 CEST 2019
  * add tests/dependent-conflict-resolution.sh

patch 8ccb471056fea3a2ff082489ae0d9aaa810eb4b8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug  1 23:23:44 CEST 2019
  * tests: conflict-fight-failure now tests that we scale reasonably
  
  We take the time of the first test run and then check that subsequent runs
  take no more than the quadratic amount of time. So the test again fails
  miserably for darcs-1 (after about 11 test runs) and for darcs-2 (after
  about 6 runs). The new darcs-3 format succeeds with at least 200 runs.

patch af9ca9e86dbafcf4b2ead44b4f156f5beccf36a8
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Aug  2 00:26:45 CEST 2019
  * disable tests/conflict-fight-failure.sh for darcs-1/2

patch 48f9dbad72020035e441c8acb76899582128bd69
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Aug  8 10:30:27 CEST 2019
  * tests/look_for_moves_and_replaces.sh: remove repo dir before creating it

Attachments

• patch-preview.txt
• slightly-extend-and-cleanup-test-for-issue154.dpatch
• unnamed

>  * slightly extend and cleanup test for issue154

> # ideally we would preserve the pending 'addfile ./d/moo',
> # but we currently do not
> #darcs whatsnew | grep 'addfile ./d/moo'

I guess this is a general question about what conflict resolution
should do to pending. We can't just leave 'addfile ./d/moo' in
pending because recorded no longer contains './d'. So we'd have
to include both "adddir ./d" and "addfile ./d/moo".

Logically there's a stronger case for actually just including
"adddir ./d" because that's the (conflicted) effect of merging
the new patch and pending. But it doesn't seem very helpful in
practice.

>  BTW, I wonder why the file d/moo gets deleted at all by the pull.

Connected to the treatment of pending, I guess.

> * add tests/dependent-conflict-resolution.sh

> +# Test for conflict resolution in the case of
> +# merging C with of A;B, where B depends on A
> +# and C conflicts with either A or B or both of them.

> +# The point here is that A should not appear
> +# as an alternative in the resolution, unless C
> +# conflicts only with A and not with B.

What does it mean for C to conflict with A and not B? If
B depends on A and C conflicts with A then C can never
be in a context to merge with B. It also looks the conflict
markup doesn't contain A as an alternative even in this case.

Did you mean to say conflicts with B but not A? I do see A in
the conflict markup in that case. But either way it doesn't
make sense to me to distinguish "C conflicts with A and B" from
"C conflicts with A but not B".

>  * tests: make ssh test more robust
>  * tests: conflict-fight-failure now tests that we scale reasonably
>  * tests: conflict-fight-failure now tests that we scale reasonably

All fine.

Also fine (forgot to include it in the last comment):

>  * tests/look_for_moves_and_replaces.sh: remove repo dir before 
creating it

> +  /usr/bin/time -f %e -o ../elapsed darcs pull ../RA --quiet --all --
patch "^A$i\$" --allow-conflicts
[...]
> +    (( $(echo "$elapsed < $i * $i * $start" | bc) ))

FWIW I had to install 'time' and 'bc' in msys to run this test. I guess
none of our other test scripts already depended on them. Luckily pacman
makes it quite easy to install things.

Sorry for being late to reply. This is actually an interesting problem.

>> # ideally we would preserve the pending 'addfile ./d/moo',
>> # but we currently do not
>> #darcs whatsnew | grep 'addfile ./d/moo'
> 
> I guess this is a general question about what conflict resolution
> should do to pending. We can't just leave 'addfile ./d/moo' in
> pending because recorded no longer contains './d'. So we'd have
> to include both "adddir ./d" and "addfile ./d/moo".
> 
> Logically there's a stronger case for actually just including
> "adddir ./d" because that's the (conflicted) effect of merging
> the new patch and pending. But it doesn't seem very helpful in
> practice.

IME the /only/ thing that helps in practice is to be logically consistent.

Let's look at the example in detail.

So we have the common c=(adddir d) and then a pending p=(addfile d/f).
We pull in b=(rmdir d), which conflicts with p. We get

  a;p+b=a;p;b'

with

  effect b'=p^=(rmfile d/f)

But we can't do that because pending must always be on top. Effectively,
there is an implicit --reorder-patches regarding p. So indeed we should
logically expect

  a;b;p'

with

  effect p'=b^=(adddir ./d)

But this would mean our new pending p' is conflicted! We can't deal with
that either, since the pending patch is only a list of prims.

So the question is not what conflict /resolution/ should do to pending,
but rather what darcs should do if we get a conflict with pending.

I think the above considerations clearly show that there is /no/
consistent solution. This means we must /fail/ with an error message
saying that pulling a patch that conflicts with pending changes is not
supported; to make the pull succeed, the user can either revert or
record the conflicting pending changes. The former means they avoid the
conflict, whereas the latter means they accept the conflict. We should
list the conflicting changes, as we would do when resolving, but we
should not merge the remote patches.

>> * add tests/dependent-conflict-resolution.sh

You are right that there is something fishy about the descriptions of
these tests. I need to think more about what I actually wanted to test
here. At the moment I have difficulties to explain that precisely.

> But we can't do that because pending must always be on top. Effectively,
> there is an implicit --reorder-patches regarding p. So indeed we should
> logically expect
> 
>   a;b;p'
> 
> with
> 
>   effect p'=b^=(adddir ./d)
> 
> But this would mean our new pending p' is conflicted! We can't deal with
> that either, since the pending patch is only a list of prims.

Well, we can implement that specification with

  p'=adddir ./d

But it does seem a bit odd.

> So the question is not what conflict /resolution/ should do to pending,
> but rather what darcs should do if we get a conflict with pending.

Yeah, I was being loose in my language. I also meant the more general
question about how to proceed when there's a conflict with pending.
Though it's possible the answer could include running the conflict
markup algorithm.

> I think the above considerations clearly show that there is /no/
> consistent solution. This means we must /fail/ with an error message
> saying that pulling a patch that conflicts with pending changes is not
> supported; to make the pull succeed, the user can either revert or
> record the conflicting pending changes.

So it's ok to conflict with unrecorded, but not with pending? That feels
awkward to me, but I'm not sure there are any good solutions so we'll
have to compromise somewhere.

Just thinking out loud now, when darcs puts something in pending, is it
always also present in unrecorded? We certainly can't make the stronger
claim that pending is always a subset of unrecorded because the user can
always break that invariant.

If so, then maybe the starting point is to think about what should land
in unrecorded when there's a conflict with a patch we're pulling, and
then try to deduce what that implies for pending.

>> I think the above considerations clearly show that there is /no/
>> consistent solution. This means we must /fail/ with an error message
>> saying that pulling a patch that conflicts with pending changes is not
>> supported; to make the pull succeed, the user can either revert or
>> record the conflicting pending changes.
> 
> So it's ok to conflict with unrecorded, but not with pending? That feels
> awkward to me, but I'm not sure there are any good solutions so we'll
> have to compromise somewhere.

Ah, no. I meant pending +>+ working. Sorry for being unclear. We throw
them together into a single anonymous Named patch during merge. The
wording also makes sense as these really are all "pending changes" i.e.
unrecorded changes. I'll try to be precise and say "unrecorded" from now on.

> Just thinking out loud now, when darcs puts something in pending, is it
> always also present in unrecorded?

By definition, yes. D.R.State.unrecordedChanges reads the pending patch,
applies it to the recorded state, then calculates the diff between that
and the working state. Then the two sequences are appended (but there
are other calls that return them separately).

BTW. Apart form the single use of addPendingDiffToPending (what a name!)
in the move command, we usually add things to pending by making a patch
that applies to the working state, then commute it backwards so we can
add it to the end of the pending patch. Oh, I forgot: we also have
tentativelyRemoveFromPending and tentativelyRemoveFromPW. these are
there to adapt pending when we amend, record, obliterate etc. They add
things to pending from the left. For those commands we must manually
ensure we have a consistent working state.

> We certainly can't make the stronger
> claim that pending is always a subset of unrecorded because the user can
> always break that invariant.

I think the way we read working simply makes it so. You can of course
make changes to the working state that annihilate parts of pending. This
just means our list of unrecorded changes gets longer.

> If so, then maybe the starting point is to think about what should land
> in unrecorded when there's a conflict with a patch we're pulling, and
> then try to deduce what that implies for pending.

This is exactly what I tried to do with my analysis. Replace "pending"
with "unrecorded changes" in my previous message and re-read.

The point is that we have no way to represent a conflict with unrecorded
changes. Applying the conflict markup is not a good solution. First of
all, we can only mark hunk/hunk conflicts. What about other conflicts?
Next, there is no way to restore the conflict markup if we change the
working state. This is the same problem I have with rebase unsuspend: a
simple revert -a eradicates all the information about the conflict. We
should not do something automatic that is neither sound nor safe.
Telling the user that they must either record or revert before the merge
can succeed is the only sensible solution.

On 29/08/2019 20:35, Ben Franksen wrote:

> The point is that we have no way to represent a conflict with unrecorded
> changes. Applying the conflict markup is not a good solution. First of
> all, we can only mark hunk/hunk conflicts. What about other conflicts?
> Next, there is no way to restore the conflict markup if we change the
> working state. This is the same problem I have with rebase unsuspend: a
> simple revert -a eradicates all the information about the conflict. We
> should not do something automatic that is neither sound nor safe.
> Telling the user that they must either record or revert before the merge
> can succeed is the only sensible solution.

Agreed it's the same problem as rebase unsuspend - I was actually
already working on a reply to your unsuspend proposal from last month
and saying the same thing :-)

But the possibility you suggest for rebase unsuspend could also apply to
this situation, so I don't think we're forced to go for "record or
revert" (though it's an option). Even applying the conflict markup,
flawed as it is, isn't that bad a solution in many cases.

Perhaps the standard options already roughly expose the valid choices
(I'm not sure to what extent they actually implement this correctly):

 --no-allow-conflicts: refuse the pull, i.e. user has to record or revert
 --mark-conflicts: use the markup, flawed though it is
 --allow-conflicts: quite dangerous for unrecorded changes

The problem is that these have different levels of danger when the
conflict is with recorded changes versus unrecorded changes.

And perhaps we should actually consider allowing pending to have conflicts?

>> The point is that we have no way to represent a conflict with unrecorded
>> changes. Applying the conflict markup is not a good solution. First of
>> all, we can only mark hunk/hunk conflicts. What about other conflicts?
>> Next, there is no way to restore the conflict markup if we change the
>> working state. This is the same problem I have with rebase unsuspend: a
>> simple revert -a eradicates all the information about the conflict. We
>> should not do something automatic that is neither sound nor safe.
>> Telling the user that they must either record or revert before the merge
>> can succeed is the only sensible solution.
> 
> Agreed it's the same problem as rebase unsuspend - I was actually
> already working on a reply to your unsuspend proposal from last month
> and saying the same thing :-)

Ha, good we are converging on the essentials.

> But the possibility you suggest for rebase unsuspend could also apply to
> this situation, so I don't think we're forced to go for "record or
> revert" (though it's an option).

I don't think we can do that. The situation is different in that when we
unsuspend-reify we are creating a new named patch for the fixups that
the user could, in principle, keep in their repo. But we can't just
record all the user's unrecorded changes! That would be preposterous.

> Even applying the conflict markup,
> flawed as it is, isn't that bad a solution in many cases.

Sure, but that doesn't help. It must be the right solution in all cases.
Suppose the situation is complex with many unrecorded changes and
several conflicts, some of them with unrecorded changes. This is a
desaster waiting to happen; the users unrecorded changes may become
completely messed up in the process and darcs will get the blame for it.

> Perhaps the standard options already roughly expose the valid choices
> (I'm not sure to what extent they actually implement this correctly):
> 
>  --no-allow-conflicts: refuse the pull, i.e. user has to record or revert
>  --mark-conflicts: use the markup, flawed though it is
>  --allow-conflicts: quite dangerous for unrecorded changes
> 
> The problem is that these have different levels of danger when the
> conflict is with recorded changes versus unrecorded changes.

You can look at D.R.Merge, it contains all the gory details.
--no-allow-conflicts also covers unrecorded changes, since before we
merge we "record" (only in memory) all of them into a single anonymous
PatchInfoAnd. In the other two cases, we ask the user whether they want
to proceed. This is done even if --all is in effect, I think. I guess
--mark-conflicts will not always behave reasonably and --allow-conflicts
will just delete conflicting unrecorded changes, which is not nice
unless you know exactly what you're doing. I personally wouldn't dream
of using --allow-conflicts when there are /any/ unrecorded changes.

> And perhaps we should actually consider allowing pending to have conflicts?

What about unrecorded changes that are not in the pending patch? Also,
how do you suppose we could ever resolve a conflict in pending, given
that pending must always stay on top?

With this rollback I think the bundle is now accepted?

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

patch b3fd594b2b2c58c22631909a3f305a98239025e2
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sat Sep 14 23:00:52 CEST 2019
  * remove tests/dependent-conflict-resolution.sh
  
  I was confused when I wrote this test. The descriptions are wrong and what
  it tests is already covered by tests/conflict-chain-resolution.sh.

Attachments

• patch-preview.txt
• remove-tests_dependent_conflict_resolution_sh.dpatch
• unnamed

> With this rollback I think the bundle is now accepted?

Yep