[darcs-users] darcs patch: Import relevant bits of gorsvet, for now... (and 16 more)

[Eric Kow]

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