[darcs-users] patch for -- Re: versioned show files

Thomas Hartman thomashartman1 at googlemail.com
Mon Nov 16 01:42:21 UTC 2009


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 at 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


More information about the darcs-users mailing list