OK, I think I have a fix for being able to view versioned show files, fixing

http://bugs.darcs.net/issue1499

Attached is a redone patch, mostly the same in spirit as the first
patch I contributed but better organized.

The main substantitve change, and the change that seems to have solved
the issue I was having with the weird at_exit messages, is I switched
to using

withDelayedDir rather than withTempDir.

I hoped this can be applied for 2.4!

On Wed, Sep 30, 2009 at 12:13 PM, Eric Kow <kowey@darcs.net> wrote:
> Hi Thomas,
>
> Thanks again for this patch.  I just noticed it (again) while going
> through some old emails.  I think it fell through the cracks :-(
>
> Do you have any more time to work on it?
>
> It looks all we have to (a) solve the with atexit mystery and (b)
> write a regression test
>
> On Tue, May 12, 2009 at 03:25:11 -0500, Thomas Hartman wrote:
>> It seemed to do the right thing when I tried it out on happstack, but when I
>> tried it on a large repository
>
> Great! Now if you have some time to continue working on this, let's try
> to get it from 'seems to do the right thing' to something more
> confident.
>
> Could you submit some regression tests for this, for example?
>
> See http://wiki.darcs.net/RegressionTests
>
>> Exception thrown by an atexit registered action:
>> ./tests/lib/perl/Test/Tutorial.pod-0: removeLink: inappropriate type (Not a
>> directory)
>>
>> what are those atexit errors?
>
> I'm a bit puzzled by this too.
>
> It doesn't come from withTempDir because that's supposed to kick in
> after the slurping and showing is done, whereas what you're reporting is
> an action registered via atexit (which runs things as Darcs is about to
> quit).
>
> I recursively grepped for atexit but did not notice anything that makes
> sense as a candidate.  This makes me wonder if we should tweak atexit
> to also register a String saying what bit of code made the request.
>
> Any Darcs hackers have a clue?
>
>> Can this be applied, since it seems to at least work a lot of the time? Can
>> this be improved?
>
> If you'd bear with us a little bit, I'd like to see this amended
> before we apply it.
>
> See http://wiki.darcs.net/Development/GettingStarted
>
> There may be a few cycles of amend and resend ahead, but hopefully
> with us getting closer to done each time!
>
> first draft for revisioned query show files
> -------------------------------------------
> It's probably not a good idea to call a patch 'draft for...' unless you
> mean it.
>
> In fact, this was meant to resolve an issue on our tracker so you could
> consider naming it something like
>
>  Resolve issueNNN: darcs show files --match
>
> I don't mean to be nitpicky; just trying to make sure the history is
> easy to search through in the long run :-)
>
> Darcs amend --edit may be helpful here
>
>> +-- move this to Darcs.Repository, like slurp_pending and slurp_recorded?
>> +slurp_revisioned r opts = do withTempDir "revisioned.showfiles" $ \_ -> do get_nonrange_match r opts
>> +                                                                           slurp =<< getCurrentDirectory
>
> Reading the Darcs.Match code again, I see that they handle pretty much
> all the work, that is slurping recorded into the current working
> directory and applying inverse patches to them.  So this seems like the
> right thing.
>
> COMMENT: I notice that you're calling getCurrentDirectory instead of
> just using the parameter provided by withTempDir.  Any reason why?
>
>> -manifest_cmd :: ([DarcsFlag] -> Slurpy -> [FilePath]) -> [DarcsFlag] -> [String] -> IO ()
>> +manifest_cmd :: ([DarcsFlag] -> Slurpy -> [FilePath]) -> [DarcsFlag] -> [FilePath] -> IO ()
>
> Given that the parameter corresponding to this change isn't used anyway,
> I'm not sure why you changed the type signature.
>
> On the other hand, I suppose in the future, it'd be nice do have
>  darcs show files foo
>
> Which would be equivalent to
>  darcs show files | grep '^foo/'
>
>
>> -manifest_cmd to_list opts _ = do
>> -    list <- (to_list opts) `fmap` withRepository opts slurp
>> +manifest_cmd to_list opts _ = withRepository opts $- \r -> do
>> +    sl <- EYK: slurp choice snipped
>> +    let list = to_list opts sl
>
> This mostly tosses an extra point in (list `fmap` action to sl <-
> action; list sl).  I'm not particularly bothered either way.
>
> COMMENT: What's the justification for moving withRepository out from
> just the local action of slurping to cover the whole command?  It may
> be a good thing; I just wanted to check
>
>> +    let isPending = NoPending `notElem` opts
>> +        isRevisioned = have_nonrange_match opts
>> +    sl <- case (isPending, isRevisioned) of
>> +            (True,False) -> slurp_pending r
>> +            (False,True) -> slurp_revisioned r opts
>> +            (False, False) -> slurp_recorded r
>> +            (True,True) -> error "Can't mix pending option with revisioned show files"
>> hunk ./src/Darcs/Commands/ShowFiles.lhs 114
>> -    where slurp :: RepoPatch p => Repository p C(r u r) -> IO Slurpy
>> -          slurp = if NoPending `notElem` opts
>> -                  then slurp_pending else slurp_recorded
>
> Extended logic to choose a slurp function based not just on the pending
> flag but the presence of a matcher.  As Thomas points out, it doesn't
> make sense to have both matchers and pending.  I wonder if our matcher
> logic would be cleaner if it also had this pending stuff baked in, ie.
> if there are other commands where we wanted to offer a choice to look
> in the pending or not.
>
>> hunk ./src/Darcs/Commands/ShowFiles.lhs 116
>> +
>> +
>> +-- slurp_recorded :: RepoPatch p => Repository p C(r u t) -> IO Slurpy
>> +-- get_partial_nonrange_match :: RepoPatch p => Repository p C(r u t) -> [DarcsFlag] -> [FileName] -> IO ()
>> +-- createPristineDirectoryTree :: RepoPatch p => Repository p C(r u t) -> FilePath -> IO ()
>> +-- get_nonrange_match :: RepoPatch p => Repository p C(r u t) -> [DarcsFlag] -> IO ()
>> +-- sp2fn :: SubPath -> PatchFileName.FileName
>>  \end{code}
>> hunk ./src/Darcs/Commands/ShowFiles.lhs 124
>> +
>> +
>> +
>
> Looks like you have some debugging leftovers in this patch bundle.
> Perhaps you could unrecord and re-record without them?
>
> --
> Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
> PGP Key ID: 08AC04F9
>



-- 
Need somewhere to put your code? http://patch-tag.com
Want to build a webapp? http://happstack.com

The patch I have attached is the only one I could find (in the issue tracker),
is it the right one?

Sun Nov 15 17:32:06 PST 2009  thomashartman1@gmail.com
  * add versioned show files functionality (darcs show files -p 'some
patch')

Thomas, can you post a follow-up patch to make --no-pending the default
when invoked with -p? An option that does not work by default is not
very convenient.

The rest is ok.

New patches:

[add versioned show files functionality (darcs show files -p 'some
patch')
>> This is needed for convenience and flagset consistency.
 
 manifest_cmd :: ([DarcsFlag] -> Slurpy -> [FilePath]) -> [DarcsFlag] ->
[String] -> IO ()
 manifest_cmd to_list opts _ = do
-    list <- (to_list opts) `fmap` withRepository opts slurp
+    list <- (to_list opts) `fmap` withRepository opts myslurp
     mapM_ output list
hunk ./src/Darcs/Commands/ShowFiles.lhs 97
-    where slurp :: RepoPatch p => Repository p C(r u r) -> IO Slurpy
-          slurp = if NoPending `notElem` opts
-                  then slurp_pending else slurp_recorded
+    where myslurp :: RepoPatch p => Repository p C(r u r) -> IO Slurpy
+          myslurp = do let isPending = NoPending `notElem` opts
+                           isRevisioned = have_nonrange_match opts
+                       case (isPending, isRevisioned) of
+                          (True,False) -> slurp_pending 
+                          (False,True) -> slurp_revision opts
+                          (False,False) -> slurp_recorded
+                          (True,True) -> error $ unlines ["Can't mix
pending option (default) with revisioned show files; ",
+                                                          "perhaps you
want darcs <your cmd> --no-pending ?"]
           output_null name = do { putStr name ; putChar '\0' }
           output = if NullFlag `elem` opts then output_null else
putStrLn
>> The argument-handling logic could be externalized to the Arguments
and Flags modules. Either way, -p should imply --no-pending with that
command (and with all commands since it is only ever used here).

hunk ./src/Darcs/Commands/ShowFiles.lhs 108
+
+slurp_revision :: RepoPatch p => [DarcsFlag] -> Repository p C(r u r)
-> IO Slurpy
+slurp_revision opts r = withDelayedDir "revisioned.showfiles" $ \_ ->
do 
+  get_nonrange_match r opts 
+  slurp =<< getCurrentDirectory
+
>> The logic is good: create a snapshot of the repository at a given >>
patch, then slurp at that date. Why not use virtualTreeIO like in
>> showContents? I think this would be more performant than slurping
>> (Petr, can you comment on that?)

Okay, a patch is attached.

For now the only change I have made from your suggestions is to make
--pending the default if not in a versioned repo, and --no-pending the
default if versioned, otherwise complain.

darcs show files --pending | grep testp # goo
./testpending

darcs show files --pending -p 'replace commutex with commute in
elegant_merge' # fine
darcs: Can't mix pending option (default) with revisioned show files;

Some replies to your other suggestions.

>>> The logic is good: create a snapshot of the repository at a given >>
> patch, then slurp at that date. Why not use virtualTreeIO like in
>>> showContents? I think this would be more performant than slurping
>>> (Petr, can you comment on that?)

I chatted with petr on darcs and he confirmed that darcs is
transitioning from Slurpy to Storage.Hashed, however he was
pessimistic that we would see a significant performance improvement.

<mornfall> patch-tag: Slurpies are being phased out -- hashed-storage
provides better mechanisms to
achieve similar ends.                   [10:51]
<mornfall> patch-tag: Nevertheless, versioned show files is going to be slow
with current repository format, and I don't see that properly
addressed anytime soon. A year or two at least, I'm afraid. Lots of
work needs to go into a new, robust and performant format...

I did look into replacing the slurpy logic in Storage.Hashed with
something similar to what we have in Command.ShowContents, which uses
hash storage. However, I couldn't figure out how to distinguish
between files and directories when doing a dump from Tree IO, which is
necessary for show files
(--files,--no-files,--directories,--no-directories).

If someone can point me in the right direction with regards to this I
would be interested in helping to migrate everything to Hashed
Storage, if that helps.

The current versioned show files using slurpies is very slow for old
revisions, but relatively snappy for recent revisions, which I think
is a much more common use, and probably the best we can do for now.

thartman@ubuntu:~/darcs.net>time darcs show files -p 'Create patch
sequence type' | head -n3 # very slow, becaus this is a way old patch
./Add.lhs
real	0m22.289s
thartman@ubuntu:~/darcs.net>time darcs show files -p 'replace commutex
with commute in elegant_merge' | head -n3 # quite snappy on a more
recent revision
real	0m0.347s

>>> The argument-handling logic could be externalized to the Arguments
> and Flags modules.

Not sure if I understand you here. I did get the feeling looking at
this code that something seems off with the way repos/patchsets are
selected from the arguments passed in. And this bugs me now just in
Showfiles but also ShowIndex, ShowContents, and probably other places.

Seems like the way this should work is there is a function

data RepoThingie = PendingRepo | RecordedRepo | PointInTimeRepo Patch
getRepo :: Monad m => [DarcsFlag] -> m RepoThingie

then you could handle the logic for what kind of repo/patchset we will
be operating in directly from the DarcsFlags, rather than mixed with
other bits of code. I think this would make things a lot more
understandable. Monadic because would throw error if can't match darcs
args to a particular repo.

Currently we are almost there with

Darcs.Match.nonrange_matcher :: Patchy p => [DarcsFlag] -> Maybe (Matcher p)

but this misses the case for --index=n (a single patch a few
"versions" back) and I don't see how to fix this. Basicaly I don't
know how to go from index n to the right patch. Incidentally, this is
also the root of broken darcs show contents --index=1 which I
submitted a bug on yesteday.

http://bugs.darcs.net/issue1705

Cheers, and happy haciking.

thomas.

On Thu, Nov 19, 2009 at 2:00 AM, Florent Becker <bugs@darcs.net> wrote:
>
> Florent Becker <florent.becker@univ-orleans.fr> added the comment:
>
> Sun Nov 15 17:32:06 PST 2009  thomashartman1@gmail.com
>  * add versioned show files functionality (darcs show files -p 'some
> patch')
>
> Thomas, can you post a follow-up patch to make --no-pending the default
> when invoked with -p? An option that does not work by default is not
> very convenient.
>
> The rest is ok.
>
> New patches:
>
> [add versioned show files functionality (darcs show files -p 'some
> patch')
>>> This is needed for convenience and flagset consistency.
>
>  manifest_cmd :: ([DarcsFlag] -> Slurpy -> [FilePath]) -> [DarcsFlag] ->
> [String] -> IO ()
>  manifest_cmd to_list opts _ = do
> -    list <- (to_list opts) `fmap` withRepository opts slurp
> +    list <- (to_list opts) `fmap` withRepository opts myslurp
>     mapM_ output list
> hunk ./src/Darcs/Commands/ShowFiles.lhs 97
> -    where slurp :: RepoPatch p => Repository p C(r u r) -> IO Slurpy
> -          slurp = if NoPending `notElem` opts
> -                  then slurp_pending else slurp_recorded
> +    where myslurp :: RepoPatch p => Repository p C(r u r) -> IO Slurpy
> +          myslurp = do let isPending = NoPending `notElem` opts
> +                           isRevisioned = have_nonrange_match opts
> +                       case (isPending, isRevisioned) of
> +                          (True,False) -> slurp_pending
> +                          (False,True) -> slurp_revision opts
> +                          (False,False) -> slurp_recorded
> +                          (True,True) -> error $ unlines ["Can't mix
> pending option (default) with revisioned show files; ",
> +                                                          "perhaps you
> want darcs <your cmd> --no-pending ?"]
>           output_null name = do { putStr name ; putChar '\0' }
>           output = if NullFlag `elem` opts then output_null else
> putStrLn
>>> The argument-handling logic could be externalized to the Arguments
> and Flags modules. Either way, -p should imply --no-pending with that
> command (and with all commands since it is only ever used here).
>
> hunk ./src/Darcs/Commands/ShowFiles.lhs 108
> +
> +slurp_revision :: RepoPatch p => [DarcsFlag] -> Repository p C(r u r)
> -> IO Slurpy
> +slurp_revision opts r = withDelayedDir "revisioned.showfiles" $ \_ ->
> do
> +  get_nonrange_match r opts
> +  slurp =<< getCurrentDirectory
> +
>>> The logic is good: create a snapshot of the repository at a given >>
> patch, then slurp at that date. Why not use virtualTreeIO like in
>>> showContents? I think this would be more performant than slurping
>>> (Petr, can you comment on that?)
>
> ----------
> nosy: +florent.becker
> status: needs-review -> amend-requested
> title: patch for -- Re:  versioned show files -> patch for -- Re: versioned show files
>
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/patch88>
> __________________________________
>



-- 
Need somewhere to put your code? http://patch-tag.com
Want to build a webapp? http://happstack.com

Attachments

• add-versioned-show-files-functionality-_darcs-show-files-_p-_some-patch__.dpatch

OK, it's been over a week since Thomas sent his amendment.
Florent: may I nudge you for your review?  If you're not available, could we
bump this to somebody else?

Thanks!

Le Wed, 09 Dec 2009 15:42:30 +0000,
Eric Kow <bugs@darcs.net> a écrit :

> 
> 
> Eric Kow <kowey@darcs.net> added the comment:
> 
> OK, it's been over a week since Thomas sent his amendment.
> Florent: may I nudge you for your review?  If you're not available,
> could we bump this to somebody else?
> 
> Thanks!
I should be available again from tomorrow, expect the review then.

Sorry for the delay.

Florent

This should go in, with the following companion patch to make it compatible with
recent CamelCasings. If/when flags get overhauled, it will be a good idea to
make the last given flag take precedence over the others instead of complaining
about inconsistencies.

Many thanks to Florent for the review and to Thomas for the patch.
This will go in once my staging repo is happy.

Thomas, protip for the future: if you had started your patch name with 'Resolve
issue1499', our bugtracker integration script would have automatically noticed
and closed off the ticket.  The more we automate, the fewer mistakes we make and
the more time we spend doing useful things :-)

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-9beb83cbef2ce4fc6c4644ecbde0387a1c9c6ce7

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-9beb83cbef2ce4fc6c4644ecbde0387a1c9c6ce7