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

patch 5913eea4ed4c7bcb92bee2987d19a8b36f60d17a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jan 24 17:11:19 CET 2020
  * resolve issue2639: darcs diff crashes with --last=1 and file name

Attachments

• patch-preview.txt
• resolve-issue2639_-darcs-diff-crashes-with-__last_1-and-file-name.dpatch
• unnamed

Can you explain the fix? I've stared at it a bit but couldn't see
what was wrong with the old logic and right with the new.

Please see the explanation to issue2639 (msg21697) which I added before
I sent the patch. I think this should clear things up, if not please
yell and i will add more details.

Attachments

• pEpkey.asc

Thanks, I missed that (sorry!)

So (in the old code), unrecorded' contains the unrecorded changes
filtered by the files the user selected, but base contains the
unrecorded changes unfiltered.

Is the problem that we then try to remove todiff+>+tounapply from base to
make oldtree, and this fails because tounapply doesn't contain the changes
to unselected files (g in your test) so if todiff also contains changes to
g then unapplying it fails?

No, the problem is not 'base', but the files inside 'pdirpath'.

pdirpath is used in the first call to hashedTreeIO when we create
oldtree. It is the result of copying our (hashed) pristine tree (the 4
lines after the comment

  -- make a temporary copy of pristineDirPath where we have write access

). But if we have no secondMatch this is wrong: pdirpath should be (a
hashed version of) the working tree, not the pristine tree.

My fix returns the same value for base as before (i.e. as
unrecordedChanges does), but /as a side effect/ the unrecorded' changes
are applied to the pristine tree on disk inside pdirpath; which is then
in the correct state, i.e. we can now unapply 'todiff+>+tounapply'
without crashing.

In principle this bug has nothing to do with path filtering. I must
admit that I am not 100% sure why we need path filtering to actually get
the crash. It may have to do with the fact that the directory on disk is
used only as 'backup storage' for the Tree, i.e. we consult it only if
we hit an unrealized Blob inside the Tree (assuming it is already
expanded, but i think it is). This may mask the bug in many common
cases. I know this is not really a satisfying explanation...

Attachments

• pEpkey.asc

I see a bit better now. The pre-condition for the arguments to
hashedTreeIO is being violated. I think 'tounapply' is a bit of
a red herring as it'll be empty when there's no second match, 
because we'll have match=all.

But if hashedTreeIO has a side-effect on pdirpath, why is it ok to 
call it twice in succession to make 'oldtree' and 'newtree'?

> I see a bit better now. The pre-condition for the arguments to
> hashedTreeIO is being violated. I think 'tounapply' is a bit of
> a red herring as it'll be empty when there's no second match, 
> because we'll have match=all.

Ah, correct. I should have said 'todiff+>+tounapply', not just
'tounapply'. In the failing test scenario unrecorded' is the last patch
in todiff.

> But if hashedTreeIO has a side-effect on pdirpath, why is it ok to 
> call it twice in succession to make 'oldtree' and 'newtree'?

Yes this is strange, isn't it? I tried for some time to come up with a
test case to show this is unsafe, but to no avail.

Then I thought about it some more and came to the conclusion that the
code actually works, because hashed files are never removed (until
explicitly garbage collected). So all items and subtrees of the Tree
'base' can still be found in 'pdirpath'. The first two hashedTreeIO
calls merely added some more hashed files and these are just ignored.

So this is not (another) bug, strictly speaking. I still think the code
would be more obvious if it re-applied todiff and passed oldtree to the
second call:

  oldtree <- filt `fmap` hashedTreeIO
    (unapply $ unsafeCoercePEnd todiff +>+ tounapply) base pdirpath
  newtree <- filt `fmap` hashedTreeIO
    (apply todiff) oldtree pdirpath

or, better yet, to avoid unnecessary patch application:

  newtree <- filt `fmap` hashedTreeIO
    (unapply tounapply) base pdirpath
  oldtree <- filt `fmap` hashedTreeIO
    (unapply todiff) newtree pdirpath

What do you think?

Attachments

• pEpkey.asc

>   newtree <- filt `fmap` hashedTreeIO
>     (unapply tounapply) base pdirpath
>   oldtree <- filt `fmap` hashedTreeIO
>     (unapply todiff) newtree pdirpath

Ahem. I found that this isn't good when there is path filtering taking
place. We should postpone the filtering to when we write out the plain
trees. Will send a patch.

Attachments

• pEpkey.asc

Following up on my recent comments.

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

patch c10013a29a6341db6ed596378539162dd4d75391
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jan 26 20:35:15 CET 2020
  * diff command: make construction of trees more obviously correct

Attachments

• diff-command_-make-construction-of-trees-more-obviously-correct.dpatch
• patch-preview.txt
• unnamed

Ganesh: does this explanation satisfy you? I have a few follow-up
patches and would like to move on...

Quoting myself:
> So your initial assumption that this is all about path filtering was
> correct. The underlying problem is that we treat recorded and unrecorded
> changes differently with respect to path filtering. The former are taken
> as is, but the latter are filtered. Unapplying filtered unrecorded
> changes from the full working tree retains filtered-out files as they
> are (i.e. changed, relative to pristine), which means we may get an
> error when we unapply recorded changes (since these are not filtered).
> 
> The reason my patch fixes the problem is that it does not call
> readUnrecorded, but rather re-constructs a "filtered working tree" from
> the (filtered) unrecorded changes.
> 
> A less invasive fix would be to pass Nothing (instead of mpaths) to
> unrecordedChanges, which means we don't filter the unrecorded changes.
> Instead we only filter the Trees that we pass to the external diff
> command. This would perhaps be less efficient in cases where there are
> large amounts of unrecorded changes but only a small subset affect the
> files we are interested in.

Attachments

• pEpkey.asc

Apologies, I got distracted by breaking out my testing changes and
forgot about reviews for a while. I think your current fix is fine
but it'd be good to have some comments documenting what we now know
about how things work and what could potentially be changed.