6 patches for repository http://darcs.net/releases/branch-2.5:

All of these patches go together, I'm afraid. Ganesh, would you be available
to have a look?

This was a surprisingly tricky one (for me) because I had a lot of trouble
anticipating the little interactions between features (for example,
--remote-repo and the --set-default stuff).

Note that the fix remote-repo patch was motivated by Darcs not noticing that
../R1 and abspath/R1 were the same thing in the test. 

Wed Aug 11 15:19:03 BST 2010  Eric Kow <kowey@darcs.net>
  * Accept issue1898: set-default notification system.

Thu Aug 12 16:26:37 BST 2010  Eric Kow <kowey@darcs.net>
  * Accept issue1875: darcs does not honor no-set-default on fetch.

Thu Aug 12 16:48:27 BST 2010  Eric Kow <kowey@darcs.net>
  * Generalise issue1875 test on not setting default.

Thu Aug 12 16:48:47 BST 2010  Eric Kow <kowey@darcs.net>
  * Resolve issue1875: avoid accidentally setting default.
  Two cases fixed:
   - setting default on dry-run
   - setting default on darcs get --no-set-default

Thu Aug 12 16:09:20 BST 2010  Eric Kow <kowey@darcs.net>
  * Fix the remote-repo flag if it's not a URL.
  The word 'fix' here refers to the filepath canonicalisation mechanism
  that makes it easier to check filepath equality.

Thu Aug 12 16:59:01 BST 2010  Eric Kow <kowey@darcs.net>
  * Resolve issue1898: notify user when they can use set-default.

Chatty aside
------------
Here's a bug in one of my earlier attempts.  It's sort of in the category
of too-dumb-for-imperative-programming.  This code was always printing the
notification even with --set-default.  Why? Spoiler below.

shouldSetDefaultrepo :: String -> [DarcsFlag] -> IO Bool
shouldSetDefaultrepo r opts =
  do olddef <- getPreflist "defaultrepo"
     if (willSetDefault opts && r_is_not_tmp)
        then return (olddef /= [r])
        else return (olddef == [])
 where
  r_is_not_tmp = not $ r `elem` [x | RemoteRepo x <- opts]

remindAboutSetDefaultrepo :: String -> [DarcsFlag] -> IO ()
remindAboutSetDefaultrepo r opts =
  do shouldSet <- shouldSetDefaultrepo r opts
     unless shouldSet $ putStr . unlines $ [] -- blah blah blah

setDefaultrepo :: String -> [DarcsFlag] -> IO ()
setDefaultrepo r opts =  do doit <- (wetRun &&) `fmap` shouldSetDefaultrepo r opts
                            when doit (setPreflist "defaultrepo" [r])
                            addToPreflist "repos" r
                            remindAboutSetDefaultrepo r opts
                         `catchall` return () -- we don't care if this fails!
 where
  wetRun = DryRun `notElem` opts

-- SPOILER
--
-- I check the value of the default repo to determine if I should print the
-- notification.  Only, once I've set the default, I've changed the value.
-- Doh.


___________________________________________________________
This email has been scanned by MessageLabs' Email Security
System on behalf of the University of Brighton.
For more information see http://www.brighton.ac.uk/is/spam/
___________________________________________________________

Attachments

• accept-issue1898_-set_default-notification-system_.dpatch
• unnamed

I'm reviewing it now

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.

> [Accept issue1898: set-default notification system.
> Eric Kow <kowey@darcs.net>**20100811141903
> 
>  Ignore-this: d33212de428eaf5e2fd85aa4a6cc644a
> 
> ]
> [Accept issue1875: darcs does not honor no-set-default on fetch.
> Eric Kow <kowey@darcs.net>**20100812152637
> 
>  Ignore-this: 32573c47c25ec3e5ad187a5537f50c73
> 
> ] 

These look alright.

> [Generalise issue1875 test on not setting default.
> Eric Kow <kowey@darcs.net>**20100812154827
> hunk ./tests/failing-issue1875-honor-no-set-default.sh 32
> 
>  darcs init      --repo R        # Create our test repos.
>  darcs get R S  --no-set-default
>  not find S/_darcs/prefs/defaultrepo
> 
> +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?

> [Resolve issue1875: avoid accidentally setting default.
> Eric Kow <kowey@darcs.net>**20100812154847
> 
>  Ignore-this: d03cfc6111805515ae4f1ca467beab2c
>  
>  Two cases fixed:
>   - setting default on dry-run
>   - setting default on darcs get --no-set-default
> 
> ] move ./tests/failing-issue1875-honor-no-set-default.sh ./tests/issue1875-honor-no-set-default.sh
> 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.

> hunk ./src/Darcs/Repository/Prefs.lhs 460
>   where
> 
> -  r_is_not_tmp = not $ r `elem` [x | RemoteRepo x <- opts]
> +  wetRun = DryRun `notElem` opts
> +  rIsTmp = r `elem` [x | RemoteRepo x <- opts]

Looks cleaner :)

> [Fix the remote-repo flag if it's not a URL.
> Eric Kow <kowey@darcs.net>**20100812150920
> 
>  Ignore-this: 10082e2dc200ece25ece1519242962e2
>  The word 'fix' here refers to the filepath canonicalisation mechanism
>  that makes it easier to check filepath equality.
> 
> ]
> hunk ./src/Darcs/Arguments.lhs 476
> +fixUrlFlag :: [DarcsFlag] -> DarcsFlag -> IO DarcsFlag
> +fixUrlFlag opts (RemoteRepo f) = RemoteRepo `fmap` fixUrl opts f
> +fixUrlFlag _ f = return f
> +
> 
>  fixUrl :: [DarcsFlag] -> String -> IO String
>  fixUrl opts f = if isFile f
>  
>                  then toFilePath `fmap` fixFilePath opts 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.

>  import Darcs.ArgumentDefaults ( getDefaultFlags )
> 
> hunk ./src/Darcs/RunCommand.hs 144
>                    else do let fixFlag = FixFilePath here cwd
> 
> -                          (commandCommand cmd) (fixFlag : os) ex
> +                          fixedOs <- mapM (fixUrlFlag [fixFlag]) os
> +                          (commandCommand cmd) (fixFlag : fixedOs) ex

So here we fix any remote-repo options with a path in our arguments. It
seems that the fixFlag is an extra DarcsFlag that's added to communicate our
current directory to fixFlag and fixUrlFlag.

> [Resolve issue1898: notify user when they can use set-default.
> Eric Kow <kowey@darcs.net>**20100812155901
> 
>  Ignore-this: 638b575b32d700cfae9f057293cd5aa8
> 
> ] move ./tests/failing-issue1898-set-default.sh ./tests/issue1898-set-default-notification.sh
> hunk ./src/Darcs/Repository/Prefs.lhs 451
> 
>  setDefaultrepo :: String -> [DarcsFlag] -> IO ()
>  setDefaultrepo r opts =  do olddef <- getPreflist "defaultrepo"
> 
> -                            let doit = NoSetDefault `notElem` opts
> -                                       && wetRun
> -                                       && not rIsTmp
> -                                       && (olddef /= [r] || olddef == [])
> -                            when doit
> -                                (setPreflist "defaultrepo" [r])
> +                            let doit = NoSetDefault `notElem` opts && greenLight
> +                                greenLight = wetRun
> +                                           && not rIsTmp
> +                                           && (olddef /= [r] || olddef == [])
> +                            if doit
> +                               then setPreflist "defaultrepo" [r]
> +                               else when greenLight $ putStr . unlines $
> +                                      -- the nuance here is that we should only notify when the
> +                                      -- reason we're not setting default is the --no-set-default
> +                                      -- flag, not the various automatic show stoppers
> +                                      [ "Note: if you want to change the default remote repository to"
> +                                      , r ++ ","
> +                                      , "quit now and issue the same command with the --set-default flag."
> +                                      ]
> 
>                              addToPreflist "repos" r
>                           
>                           `catchall` return () -- we don't care if this fails!
>   
>   where

This looks beautifully self-explanatory. Notify the user only when the
default wasn't set because of the --no-set-default flag.

> Here's a review. I have two questions below that I'd like to see answered
> before I apply it

And not unimportantly: it makes the following tests fail:

issue1909-unrecord-O-misses-tag.sh
output.sh
pull.sh
pull_binary.sh
utf8.sh

Can you add a patch fixing these?

Reinier

8 patches for repository http://darcs.net/releases/branch-2.5:

Wed Aug 11 15:19:03 BST 2010  Eric Kow <kowey@darcs.net>
  * Accept issue1898: set-default notification system.

Thu Aug 12 16:26:37 BST 2010  Eric Kow <kowey@darcs.net>
  * Accept issue1875: darcs does not honor no-set-default on fetch.

Thu Aug 12 16:48:27 BST 2010  Eric Kow <kowey@darcs.net>
  * Generalise issue1875 test on not setting default.

Thu Aug 12 16:48:47 BST 2010  Eric Kow <kowey@darcs.net>
  * Resolve issue1875: avoid accidentally setting default.
  Two cases fixed:
   - setting default on dry-run
   - setting default on darcs get --no-set-default

Thu Aug 12 16:09:20 BST 2010  Eric Kow <kowey@darcs.net>
  * Fix the remote-repo flag if it's not a URL.
  The word 'fix' here refers to the filepath canonicalisation mechanism
  that makes it easier to check filepath equality.

Thu Aug 12 16:59:01 BST 2010  Eric Kow <kowey@darcs.net>
  * Resolve issue1898: notify user when they can use set-default.

Sun Aug 15 21:22:23 BST 2010  Eric Kow <kowey@darcs.net>
  * Fix tests that were broken by issue1875 fix.
  
  The tests were assuming --set-default.  As these are artifical repositories for
  test cases, we sometimes have cases where we pull/push from freshly created
  repositories with no defaultrepo.

Sun Aug 15 21:25:29 BST 2010  Eric Kow <kowey@darcs.net>
  * Fix tests that were broken by issue1898 fix.
  These tests were confused by Darcs UI messages.

Attachments

• accept-issue1898_-set_default-notification-system_.dpatch
• unnamed

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@jabber.fr (Jabber or Google Talk only)

This patch bundle (with 6 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-5b4627640b6f0ea28de411f6596bdcd3d1887905

This patch bundle (with 8 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-0d70110fa00a862851efff6601af0ce328047393

This patch bundle (with 6 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-5b4627640b6f0ea28de411f6596bdcd3d1887905

This patch bundle (with 8 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-0d70110fa00a862851efff6601af0ce328047393