These patches all concern issue2447 and therefore should go into 2.10.1.

I hope splitting them up and adding a longer comment here and there makes it
a easier to review them.

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

patch 5a2d8fcf0496b0cacbc01fae5e10d1316f55e83a
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Jun 19 11:32:18 CEST 2015
  * added a test to show that show files is not affected by issue2447

patch 8f921d3129603e0e219b72374c90346b457a7599
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Thu Jun 18 00:06:10 CEST 2015
  * added test for issue2447

patch 6f48698abcf4c394090b8fc82704948370712c07
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Jun 19 20:44:44 CEST 2015
  * renamed no longer failing test for issue2447
  
  Also made the test more robust by explicitly adding files and slightly
  extended the tested functionality.

patch 73cdafc8a036b6dfa5c72752cefc7dfdb0e58682
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Jun 19 20:49:31 CEST 2015
  * haveNonrangeMatch: return False for index ranges 

patch 488c0803ea4c48b25415760b6b9394a277c035c1
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Jun 19 20:53:17 CEST 2015
  * pull the --index=N case into Darcs.Reporitory.Match.getNonrangeMatch
  
  This concentrates the case distinction between index and other non-range
  matches into a single place, allowing to simplify command implementations.

patch b16505a7ec68533306559fa24decdd2cc59d065b
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Jun 19 22:58:40 CEST 2015
  * expandTo must unconditionally expand tree along the given path
  
  This is a necessary (but not yet sufficient) part of resolving issue2447.
  The problem here is that H.S.M.fileExists returned False when the tree was
  not expanded, even if the expanded tree does contain the file. This caused
  'darcs show contents' to erroneously ignore the file.
  
  What remains for issue2447 to be resolved is to make 'darcs show contents'
  use the virtualTreeIO in all cases.

patch c9187119365d2e2f817cb5592250e150d0530cab
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Fri Jun 19 23:34:11 CEST 2015
  * resolve issue2447 by using getNonrangeMatch as in ShowFiles
  
  The crucial change here is that we don't use readRecorded if called with a
  match option. Instead we call getNonrangeMatch in an empty (temporary) tree,
  quite similar to how this is done in D.UI.Command.ShowFiles.
  
  This patch also refactors the code quite a bit and adds some comments to
  explain the tricky bits.

Attachments

• added-a-test-to-show-that-show-files-is-not-affected-by-issue2447.dpatch
• patch-preview.txt
• unnamed

The three test patches could be just a single one, with a single test
script instead of two.  And the renaming of that shell test should occur
in the patch that fix the bug. I think it's cleaner like that.

The tests scripts are OK anyway.

About the "expandTo must unconditionally expand tree along the given
path", could this cause some performance issue? For instance, with
commands like whatsnew or record?

The change to ShowContents are good (function is much easier to follow now).

Accepting it (also into 2.10).

I don't think this can cause performance problems. My rationale is that
either you need to know what's under a directory or not. If you need the
information you must expand possible stubs on the way before you make
any query, otherwise you get wrong information (such as happened with
issue2447). If you don't need to know, there is no reason to access this
part of the tree and in this case expansion does not happen and does not
cost anything.

I think you forgot to change the status and milestone so I'm doing that.

I've been looking at the 'expandTo' patch a bit and I don't 
understand how it makes a difference.

The old code used to expand when 'find' returns 'Nothing' or 'Just 
(Stub _)'. Don't the other cases imply that the tree is already 
expanded anyway? 'fileExists' itself seems to just call 'find' 
indirectly, so it seems the logic should be the same.

This might be worthy of a test case to demonstrate the difference, 
particularly if the test suite now passes reliably following your 
other patch :-)

You are right. Well spotted! And sorry for being dense.

I am certain that at some point I tested this and found it makes a
difference. However, when I now obliterate this single patch the test
for issue 2447 succeeds.

Besides I cannot find any fault in your reasoning and agree that the two
versions are functionally equivalent with regard to the returned value,
but *not* with regard to the effect on the tree, since the final item
now gets expanded whereas the old version expanded only SubTrees on way.
So I effectively just removed an optimization here just as Guillaume
suspected. Meh.

I wish I could just obliterate this patch and be done with it. But it is
now in reviewed and even in 2.10, so I guess a rollback is on order.

Should I send with the same patch number in the subject so it ends up here?

> Should I send with the same patch number in the subject so it ends up here?

Yes

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

patch a93f2a7007ed9ba49cc9beb91a0d064f87b26d9e
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Tue Jun 23 19:06:22 CEST 2015
  * rollback "expandTo must unconditionally expand tree along the given path"
  
  The patch merely removed an optimization. In case all you need to know is
  whether an item exists in the tree, only the SubTrees leading to the item
  have to be expanded, not the item itself. Otherwise both versions are
  functionally equivalent.

Attachments

• patch-preview.txt
• rollback-_expandto-must-unconditionally-expand-tree-along-the-given-path_.dpatch
• unnamed

On 23/06/2015 18:04, Ben Franksen wrote:
> 
> Ben Franksen <benjamin.franksen@helmholtz-berlin.de> added the comment:
> 
> You are right. Well spotted! And sorry for being dense.
> 
> I am certain that at some point I tested this and found it makes a
> difference. However, when I now obliterate this single patch the test
> for issue 2447 succeeds.
> 
> Besides I cannot find any fault in your reasoning and agree that the two
> versions are functionally equivalent with regard to the returned value,
> but *not* with regard to the effect on the tree, since the final item
> now gets expanded whereas the old version expanded only SubTrees on way.
> So I effectively just removed an optimization here just as Guillaume
> suspected. Meh.

I still don't see how there's even that difference, but perhaps it
doesn't matter :-)

With the original code, either find would return Just (File _) or Just
(SubTree _), implying it was already completely expanded including the
final item, or it would expand.

The main difference I see between the two variants is that the original
code did one pass to see if the relevant part was expanded, and then
possibly another pass to expand it, whereas the new code does a single
pass to expand. However expand is more expensive than find as it
rebuilds the SubTrees at each level, even if there was no change at the
bottom.

If that is the only difference, I'm not sure it's material and I'd be
fine just sticking with the new code which is a bit shorter/simpler. In
principle expandPath could be changed to return a Maybe to avoid even
that extra overhead.

Ganesh

> With the original code, either find would return Just (File _) or Just
> (SubTree _), implying it was already completely expanded including the
> final item, or it would expand.

Hmm, right. The optimization I thought my patch destroys is in fact
impossible with the way Trees are represented: In order to know whether
an item is a File or a SubTree you have to expand it.

I just noted that the Blob type contains a lazy ByteString for the
file's content. This allows us to avoid unnecessary work when all we
need to know is whether it is a File or a SubTree. There is a price, of
course, and it's that lazy IO is very hard to reason about. It also
raises concerns about using up many file descriptors when a large tree
with many items is traversed, since with lazy IO files are not closed in
a timely fashion. I'll open a another issue where we can discuss whether
and how to gradually move away from using lazy IO.

I'll let the two of you decide what to do (rollback or keep and perhaps
change expandTo to return Maybe).

Looking at the current code I agree with Ganesh's observations.

Looking at Petr's patches in http://repos.mornfall.net/hashed-storage/ :

* "Avoid calling expandPath when not needed, in Monad." basically is
the inverse of Ben's patch :-)  (July 2009)
* Then expandTo is eventually modified by "Fix withDirectory handling
in Monad" where the "Just (Stub _ _)" case is added. (Feb. 2010)

Hope that helps.

Guillaume

Sorry for not concluding in my previous mail: let us keep Ben's bundle.

Guillaume