[darcs-users] versioned show files

[Eric Kow]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 194 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20090930/e69c2d91/attachment.pgp>