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
pgpro4Qc1Q87l.pgp
Description: PGP signature
_______________________________________________ darcs-users mailing list darcs-users@darcs.net http://lists.osuosl.org/mailman/listinfo/darcs-users
