[darcs-users] [patch345] Accept issue1898: set-default notificati... (and 5 more)

[Eric Kow]

On Sun, Aug 15, 2010 at 16:38:37 +0000, Reinier Lamers wrote:
> Here's a review. I have two questions below that I'd like to see answered
> before I apply it. It doesn't look like they are problems with the code
> though.

Thanks!  Aside from the benefits to Darcs, getting my code reviewed is
always teaches me something (in this case, about the if-then-else)...

> > +rm -rf S
> > +
> > +darcs init --repo S
> > +cd S
> > +darcs push ../R --dry-run
> > +not grep '/R$' _darcs/prefs/defaultrepo
> > +darcs push ../R
> > +cd ..
> 
> What are you testing with that last "darcs push ../R" there?

I think it was unintentional, but come to think of it, this may be
useful as a test that the --no-set-default is still respected; we
don't really need the dry-run.

Actually, come to think of it, I have a better idea to improve that

darcs push ../R --dry-run --set-default
not grep '/R$' _darcs/prefs/defaultrepo # dry-run
darcs push ../R
not grep '/R$' _darcs/prefs/defaultrepo # no-set-default

Will either send a follow up or amendment

> > hunk ./src/Darcs/Repository/Prefs.lhs 450
> >  setDefaultrepo :: String -> [DarcsFlag] -> IO ()
> > 
> > -setDefaultrepo r opts =  do doit <- if (NoSetDefault `notElem` opts && DryRun `notElem` opts && r_is_not_tmp)
> > -                                    then return True
> > -                                    else do olddef <-
> > -                                                getPreflist "defaultrepo"
> > -                                            return (olddef == [])
> > +setDefaultrepo r opts =  do olddef <- getPreflist "defaultrepo"
> > +                            let doit = NoSetDefault `notElem` opts
> > +                                       && wetRun
> > +                                       && not rIsTmp
> > +                                       && (olddef /= [r] || olddef == [])
> > 
> >                              when doit
> >                              
> >                                  (setPreflist "defaultrepo" [r])
> 
> How does this fix setting the default on "darcs get --no-set-default"? The
> strange thing is that I can check that it really does, but I can't find the
> cause in the code. In both versions, doit cannot be true if NoSetDefault is
> in opts. And besides the definition of doit, nothing appears to have
> changed.

The old version allows you to set default no matter what the conditional
above says (see the else case).  Part of the motivation for my change
was to avoid confusion caused by the if-then-else expression being of
type Bool.


> > hunk ./src/Darcs/Arguments.lhs 476
> > +fixUrlFlag :: [DarcsFlag] -> DarcsFlag -> IO DarcsFlag
> > +fixUrlFlag opts (RemoteRepo f) = RemoteRepo `fmap` fixUrl opts f
> > +fixUrlFlag _ f = return f

> 
> So we make a function that fixes the path in flags that may contain an URL.
> The function "fixUrl" that fixes a string only if it is a file already
> exists. Now the function "isFile" that is uses checks that its argument is a
> filename by checking that it doesn't contain a ':', which appears a bit
> low-tech to me. But that's not what this review is about.

Indeed, the whole FilePath and remotepath stuff really needs work.
Note also that this was a sort of stopgap.  I think we may need to
look into extending fixUrlFlag to other flags that take URLs, including
WorkRepo (or whatever it's called).  But there could be a better fix in
the bigger picture.  I was just afraid of doing it because of some
unexpected knock-on consequence in the UI

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey at jabber.fr (Jabber or Google Talk only)