Here is the latest version of the function including Petr's tweak from 'Fix the path restriction versus pending renames in unrecordedChanges'.
I can now say that I've made an honest attempt at reading this code and following what it does, so I'll be ready to apply this once the blockers are fixed. If something still slips through between the three of us (R, E, P), we can only shrug our shoulders and say we've tried our best. Email in three parts: overview, slow reading, and reply to Petr. Overview -------- - boring file (Reinier, Petr: regression?) - pending renames (Petr: regression?) - Darcs.Gorsvet witnesses (Eric: blocker for application) - pending conflicts check (Petr) - darcs whatsnew subset (Eric, Petr [fixed by restrict?]) I'm going on the assumption that my patch manager job is to hold off on applying these patches until we fix the agreed blockers and regressions. Slow reading ------------ I regret that I was not diligent enough to carefully read get_unrecorded and compare notes, but it would be a useful thing to do. From a cursory glace at Darcs.Repository.Internal.get_unrecorded_private, I get the impression that there's a bit more stuff going on there, so I hope we're not leaving out anything important :-) It may be good to walk through get_unrecorded_private and justify everything we're taking/leaving, perhaps with tests. | unrecordedChanges :: (RepoPatch p) => [DarcsFlag] -> Repository p | -> (Tree -> Tree) -> IO (FL Prim) | unrecordedChanges opts repo restrict_ = do I'm just going to go sportscaster on each line | checkIndex repo Update the index as necessary | slurp_pending repo -- XXX: only here to get us the "pending conflicts" check | -- that I don't know yet how to implement properly | pristine <- readDarcsPristine "." | Sealed pending_patches <- read_pending repo | (res, current') <- virtualTreeIO (apply [] pending_patches) pristine Read pristine + pending and combine the two in memory. Return the resulting post-pending "current" tree. It looks like we discard the result of apply (isn't that () anyway?). This may be a bit surprising as my mental model of darcs has the pending patch as "new stuff" (and not 'pristine'), I think we do this because our treeDiff function needs filenames to match so that we know what file to compare to what. In other words, we only use treeDiff to obtain a list of hunks. [Update: actually now with Petr's pending renames comments below, I'm still a bit confused] I also wonder if this is why Petr thinks that darcs remove adding hunks to pending is problematic. | let current = restrict_ current' Narrow our combined pristine+pending to the subset that our user asked for. We need to do the same thing for working | working <- case (LookForAdds `elem` opts, IgnoreTimes `elem` opts) of | (False, False) -> (restrict_ `fmap` readIndex) >>= unfold I'll echo Reinier's comment that having API haddocks in hashed-storage was really helpful! This is the normal case: since we're not looking for adds, we can grab the list of working files from the index. We apply the same restrict_ that we did to pristine. | (False, True) -> do guide <- unfold current | restrict guide `fmap` readPlainTree "." If we get just --ignore-times, we have to manually inspect each requested file. This means that the shortcut of getting the file list from 'readIndex' is not going to be adequate (the index may tell us to ignore a file since the modification times match), so we have @readPlainTree "."@ instead. I'm assuming here that @readIndex >>= unfold@ and @readPlainTree "."@ are expected to give the same result if the index is up to date. I'm not sure why we can't just restrict_ the result instead of doing @restrict guide@, though. My guess is that it's because we would need to tack a @filter nodarcs@ onto that and that it was simpler just to @restrict guide@ and kill two birds with one stone. | (True, _) -> filter nodarcs `fmap` readPlainTree "." I guess --look-for-adds implies --ignore-times in that if we want to check if a new working file exists in pristine, we're going to have to read the whole tree anyway. Note that we need to have the effect of @filter nodarcs@ in all cases. The reason we don't see it in the @(False, False)@ case is that it's implied by 'readIndex' (presumably), and likewise for the second case is a logical consequence of intersecting with the pristine. | ft <- filetype_function | diff <- treeDiff ft current working Perform the diff. We use @ft@ to determine what kind of diff to perform for each file. Darcs's 'filetype_function' ought to be documented. It seems to read the binaries file (which is why it needs to be IO) for a list of regexps, and return a function telling you if any given file is matched by that list. | return $ sort_coalesceFL (pending_patches +>+ diff) Return the pending changes and the hunks on top of that. We'll have to watch for the usual pending bugs, e.g remove foo, something else, add foo sandwiches. | where nodarcs (AnchoredPath (Name x:_)) _ | x == BS.pack "_darcs" = False | nodarcs _ _ = True On Sun, May 31, 2009 at 17:10:43 +0200, Petr Rockai wrote: > This suffers from one complicated issue though, that is pending renames. I so > far didn't figure out where and how to apply the renames without breaking > something, somewhere. Breaking what exactly? Is it just UI level stuff you're talking about, like the user asks you to show whatsnew on a given file and darcs says nothing is new, but only because the file moved? > Current darcs behaviour is to show file's changes > whenever either the pre-rename or post-rename filename given. Could you demonstrate what you mean with a couple of example darcs commands? It sounds like what you're saying is good behaviour from a UI standpoint, but I could be mistaken :-) > I think the right > solution would be to extend the list of restrictions like this: > > let paths' = paths `union` apply_to_filepaths (inverse pending) paths > or such, and then feeding it to restrict_subpaths. However, I have layering > problem, since the client code shouldn't need to know anything about > pending. I'm also not sure I understand the layering problem. What's the client code in this context? -- Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow> PGP Key ID: 08AC04F9
signature.asc
Description: Digital signature
_______________________________________________ darcs-users mailing list [email protected] http://lists.osuosl.org/mailman/listinfo/darcs-users
