[darcs-users] [patch283] Accept issue1227: darcs repository forma... (and 2 more)

Eric Kow kowey at darcs.net
Sun Jun 20 20:06:23 UTC 2010


Hi,

Thanks for the review and for catching oversights!  I'll be sending
my amendments, hopefully after tonight's meeting with Adolfo

On Sun, Jun 20, 2010 at 18:25:27 +0000, Reinier Lamers wrote:
> > hunk ./tests/failing-issue1277-repo-format.sh 1
> > +. lib                           # Load some portability helpers.
> > +rm -rf R                        # Another script may have left a mess.
> > +darcs init      --repo R        # Create our test repos.
> > +cd R
> > + darcs init --repo R2            # Protect the darcs darcs repo with R
> > + cd R2
> > + echo impossible >> _darcs/format
> > + echo 'Example content.' > f
> > + not darcs add f > log
> > + grep "Can't understand repository format" log
> > + cd ..
> > +cd ..

Acknowledged on the redirection in your previous mail.  I wonder why I
missed it this, though when running the  test The 

> > -    air <- currentDirIsRepository
> > -    if air
> > -       then return (Right ())
> > -       else do cd <- toFilePath `fmap` getCurrentDirectory
> > +    status <- maybeIdentifyRepository [] "."
> > +    case status of
> > +      Right _ -> return (Right ())
> > +      Left e | "Can't understand repository format" `isPrefixOf` e -> return (Left e)
> > +      Left _ ->
> > +            do cd <- toFilePath `fmap` getCurrentDirectory
> > 
> >                 setCurrentDirectory ".."
> >                 cd' <- toFilePath `fmap` getCurrentDirectory
> >                 if cd' /= cd
> >
> 
> So here we distinguish a new case, the one where our error is one about the
> repository format. This is the fix for the issue.

> But doesn't calling code expect to get either Right () or the given onFail
> from this function? At least that was the behavior of the old function. In
> findRepository, I see a call "seekRepo (Right ())". Doesn't that expect
> never to get a Left? But it looks like findRepository is usually used as a
> commandPrereq, in which case a Left will be handled.

Yow, this was indeed an oversight.  Unfortunately, I went chasing after
the code before I read your paragraph correctly only to come to the same
conclusions.

I guess the only reason why we have both amInRepository and
findRepository is that the latter handles the --repo flag (which unlike
its cousin --repodir, handles remote repositories).  I think we should
think about making findRepository make some sort of remark, something
similar to amInRepository but not identical (because you don't need to
be *in* a repository to run darcs changes --repo foo).  Also perhaps the
amInRepository error for the --repodir flag should say something like
"'d' needs to be in a repository" 

> So this is in the writePatchSet function, which writes a PatchSet to a
> repository, discarding what's already in the repo. Could I ask you to remove
> the quotes around "bad"? ;-)

Yep! On my way.

> Spurious newline added? Looking at it with hexedit, the file does not
> currently have a newline at the end. I'm willing to let this pass.

Sorry about that.  I'll see if I remember to fix this too (this be
addressed in a separate whitespace patch, to keep patches as easy to
review as possible)

> Now we're inside the maybeIdentifyRepository function, in a pattern match
> alternative that special-cases it for trying to identify the current
> directory ("."). This hunk handles the case that identifyRepoFormat returns
> Left, in which case there is no _darcs/format or _darcs/inventory file in
> the directory. It's safe to guess it's not a repo then.

Note that I think of the "not a repo" guess as being the conservative
one in the sense that this will make Darcs keep trying (by seeking
upwards), but I think that's just perspective.
> 
> > hunk ./src/Darcs/Repository/Internal.hs 210
> > -                                         return $ Right $ Repo here opts rf (DarcsRepository pris cs)
> > -                                 else return (Left "Not a repository")
> > +                                         return $ GoodRepository $ Repo here opts rf (DarcsRepository pris cs)
> > +                                 else return (NonRepository "Not a repository")
> 
> And finally we handle the case that the _darcs directory does not exist: in
> that case it's not a repo. But what sense does it make to check if _darcs
> exist if we already found an inventory or a format file, which is what
> identifyRepoFormat seems to check for?

Nice not taking preexisting code for granted.  One reason I'm a fan of
(conspicuous) review is that it forces is to revisit and sometimes
question the surrounding code.

I think the reason we do this is because old fashioned darcs-1 repos
will have no 'format' file (I suspect the mechanism was only introduced
after darcs release, but not sure).

> Shouldn't we also change the amInRepository function above seekRepo? It will
> still say "You need to be a repository directory to run this command." even
> when it should already know that it *is* in a repo but just doesn't
> understand the format. At least if it gets an argument of the "WorkRepoDir"
> constructor. But I don't even know what that is, so I'll shut up.

That sounds like a good idea.  I bet we could extend the test case to
show that darcs whatsnew --repodir X does the wrong thing when it does
not understand the format in X

-- 
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: 195 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100620/ad66f7b7/attachment.pgp>


More information about the darcs-users mailing list