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 <[email protected]> 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 _______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
