After going through all the effort of rebuilding the pristine tree, darcs check
and darcs repair do not bother looking for missing files, which is unfortunate!

The attached patch solves this by making checkPristineAgainstCwd run smart_diff
twice (there were objections to using LookForAdds on performance grounds).  It
updates "darcs check" appropriately.  Also in the bundle is a separate patch
which updates some stale docs in Darcs.Diff.

Attachments

• check-and-repair-look-both-ways

I'm curious:  why would we want to do this?

David

On Sat, Aug 16, 2008 at 12:09 AM, Nathaniel Filardo <bugs@darcs.net> wrote:
> After going through all the effort of rebuilding the pristine tree, darcs check
> and darcs repair do not bother looking for missing files, which is unfortunate!
>
> The attached patch solves this by making checkPristineAgainstCwd run smart_diff
> twice (there were objections to using LookForAdds on performance grounds).  It
> updates "darcs check" appropriately.  Also in the bundle is a separate patch
> which updates some stale docs in Darcs.Diff.

On Sat, Aug 16, 2008 at 08:41:13PM -0000, David Roundy wrote:
> 
> David Roundy <daveroundy@gmail.com> added the comment:
> 
> I'm curious:  why would we want to do this?

In my case, I have a corrupted repository where there are only addfile and
hunk operations on a given file but that file does not exist in the pristine
tree and so "darcs get" vs "darcs init --darcs-2 && darcs pull --all" give
different behavior.  "darcs whatsnew" does not consider the deletion to be a
change, either.  Further, patches which, e.g., move everything extant out of
the directory and then remove the directory are inapplicable on pull but
work fine locally thanks to pristine caching, leading to very delayed
detection of the problem.

This at least makes darcs check and repair spot and fix the problem, so I am
hoping that I can more easily track down exactly what happened.  (I'm
suspicious that un-something isn't rebuilding the pristines correctly, but
I'm not entirely sure.)

Thanks.
--nwf;

P.S. In case it wasn't clear from the description, by "run smart_diff twice"
I mean switching left and right repositories, so that such removed files
show up as deletions in the second pass (they are ignored as adds in the
first direction).

> David
> 
> On Sat, Aug 16, 2008 at 12:09 AM, Nathaniel Filardo <bugs@darcs.net> wrote:
> > After going through all the effort of rebuilding the pristine tree, darcs check
> > and darcs repair do not bother looking for missing files, which is unfortunate!
> >
> > The attached patch solves this by making checkPristineAgainstCwd run smart_diff
> > twice (there were objections to using LookForAdds on performance grounds).  It
> > updates "darcs check" appropriately.  Also in the bundle is a separate patch
> > which updates some stale docs in Darcs.Diff.
> 
> ----------
> nosy: +daveroundy
> 
> __________________________________
> Darcs bug tracker <bugs@darcs.net>
> <http://bugs.darcs.net/issue1006>
> __________________________________

On Sat, Aug 16, 2008 at 2:14 PM, Nathaniel Filardo <bugs@darcs.net> wrote:
> On Sat, Aug 16, 2008 at 08:41:13PM -0000, David Roundy wrote:
>> I'm curious:  why would we want to do this?
>
> In my case, I have a corrupted repository where there are only addfile and
> hunk operations on a given file but that file does not exist in the pristine
> tree and so "darcs get" vs "darcs init --darcs-2 && darcs pull --all" give
> different behavior.  "darcs whatsnew" does not consider the deletion to be a
> change, either.  Further, patches which, e.g., move everything extant out of
> the directory and then remove the directory are inapplicable on pull but
> work fine locally thanks to pristine caching, leading to very delayed
> detection of the problem.
>
> This at least makes darcs check and repair spot and fix the problem, so I am
> hoping that I can more easily track down exactly what happened.  (I'm
> suspicious that un-something isn't rebuilding the pristines correctly, but
> I'm not entirely sure.)
>
> Thanks.
> --nwf;
>
> P.S. In case it wasn't clear from the description, by "run smart_diff twice"
> I mean switching left and right repositories, so that such removed files
> show up as deletions in the second pass (they are ignored as adds in the
> first direction).

Ah, that clears this up.  Somehow I had the impression that you were
diffing with the working directory, which didn't make sense.  I think
a better option might actually be to make smart_diff never ignore
adds.  The ignoring of adds, I believe, dates from before the addition
of "co_slurp" to the code, and with co_slurp (which improves
efficiency as well), we really ought to look at all the differences!

Any chance you'd be willing to look into fixing up smart_diff? It's
some pretty hideous code, so if you aren't interested, I'll take a
look at this patch (and presumably apply it) when I've got time.

It would also be nice to have a test for this bug.  It would probably
need to be written in an ugly repository-format-specific way, since
you'd need to manually corrupt a repository (in the test script), but
it'd definitely be valuable, as I don't think we currently have any
real tests of darcs check at all.

David

P.S. Thanks for the clear explanation, after my rather brusque query.

On Sun, Aug 17, 2008 at 02:07:01PM -0000, David Roundy wrote:
> 
> David Roundy <daveroundy@gmail.com> added the comment:
> 
> On Sat, Aug 16, 2008 at 2:14 PM, Nathaniel Filardo <bugs@darcs.net> wrote:
> > On Sat, Aug 16, 2008 at 08:41:13PM -0000, David Roundy wrote:
> >> I'm curious:  why would we want to do this?
> >
> > In my case, I have a corrupted repository where there are only addfile and
> > hunk operations on a given file but that file does not exist in the pristine
> > tree and so "darcs get" vs "darcs init --darcs-2 && darcs pull --all" give
> > different behavior.  "darcs whatsnew" does not consider the deletion to be a
> > change, either.  Further, patches which, e.g., move everything extant out of
> > the directory and then remove the directory are inapplicable on pull but
> > work fine locally thanks to pristine caching, leading to very delayed
> > detection of the problem.
> >
> > This at least makes darcs check and repair spot and fix the problem, so I am
> > hoping that I can more easily track down exactly what happened.  (I'm
> > suspicious that un-something isn't rebuilding the pristines correctly, but
> > I'm not entirely sure.)
> >
> > Thanks.
> > --nwf;
> >
> > P.S. In case it wasn't clear from the description, by "run smart_diff twice"
> > I mean switching left and right repositories, so that such removed files
> > show up as deletions in the second pass (they are ignored as adds in the
> > first direction).
> 
> Ah, that clears this up.  Somehow I had the impression that you were
> diffing with the working directory, which didn't make sense.  I think
> a better option might actually be to make smart_diff never ignore
> adds.  The ignoring of adds, I believe, dates from before the addition
> of "co_slurp" to the code, and with co_slurp (which improves
> efficiency as well), we really ought to look at all the differences!

So adding "LookForAdds" to the smart_diff call in the comparison function
won't dramatically hurt performance?  There was some concern in the IRC
channel, which is why I opted for the "run smart_diff twice and have darcs
check report appropriately" as taken in this patch.  The first pass did use
LookForAdds and didn't have to make the change in darcs check; I liked it
better.

> Any chance you'd be willing to look into fixing up smart_diff? It's
> some pretty hideous code, so if you aren't interested, I'll take a
> look at this patch (and presumably apply it) when I've got time.

I can look at it, but my day job is likely only going to be amused by my
work on darcs to a point... if it's unlikely to be a contenteded area and
you don't mind a slow worker on it...

> It would also be nice to have a test for this bug.  It would probably
> need to be written in an ugly repository-format-specific way, since
> you'd need to manually corrupt a repository (in the test script), but
> it'd definitely be valuable, as I don't think we currently have any
> real tests of darcs check at all.

I have been secretly lusting for a way to munge repositories without being a
part of the darcs executable proper (i.e. just using Darcs.Repository as a
library); such a tool would make this and similar tests easier to write.
Would also have made debugging this interestingly broken repo a bit easier
if I'd had something that spat out the hash-tree of pristines rather than
using zless.  (Similarly, a way to use Darcs.Patch as a library would be
kinda neat.) Thoughts?

In the interim, I can manually break the repository using shell scripting.
I'll hack something up when I'm back at my terminal.

> David
> 
> P.S. Thanks for the clear explanation, after my rather brusque query.

No problem. :)

--nwf;

I have sent a message requesting that Nathaniel darcs send these patches.

I presume this is fixed, since both check and repair now use this:

checkPristineAgainstSlurpy repository@(Repo _ opts _ _) s2 =
    do s1 <- slurp_recorded repository
       ftf <- filetype_function
       return $ nullFL $ unsafeDiff (LookForAdds:IgnoreTimes:opts) ftf s1 s2

Notice the "LookForAdds" in the options, which makes unsafeDiff look both ways.