I think we definitely want the first patch; it allows users to silence
the intrusive set-default hint message by setting a default.

The second patch is much more speculative as it makes the hint message
even more verbose. Also it ignores the fact that the user can 
also set the default for the one repo in _darcs/prefs/prefs.

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

Thu Jan  6 23:03:07 GMT 2011  Ganesh Sittampalam <ganesh@earth.li>
  * resolve issue2003: no set-default hint if user explicitly chose no-set-default

Thu Jan  6 23:03:42 GMT 2011  Ganesh Sittampalam <ganesh@earth.li>
  * add instruction on suppressing set-default message

Attachments

• resolve-issue2003_-no-set_default-hint-if-user-explicitly-chose-no_set_default.dpatch
• unnamed

My first version broke --set-default. This version comes with real
live tests.

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

Fri Jan  7 20:36:17 GMT 2011  Ganesh Sittampalam <ganesh@earth.li>
  * resolve issue2003: no set-default hint if user explicitly chose no-set-default

Fri Jan  7 20:36:19 GMT 2011  Ganesh Sittampalam <ganesh@earth.li>
  * add instruction on suppressing set-default message

Fri Jan  7 20:36:30 GMT 2011  Ganesh Sittampalam <ganesh@earth.li>
  * add test of set-default hint messages

Attachments

• resolve-issue2003_-no-set_default-hint-if-user-explicitly-chose-no_set_default.dpatch
• unnamed

Should have pricked my ears when yo said you wanted to get that
branch out the door for HP.

Generally +1 idea, +1 implementation with minor comments about
the UI stuff and some implementation niggles.

I'm going to see if I can push the first and third patch to
screened/unstable.  Second patch at your discretion.

On Fri, Jan 07, 2011 at 21:58:00 +0000, Ganesh Sittampalam wrote:
> Fri Jan  7 20:36:17 GMT 2011  Ganesh Sittampalam <ganesh@earth.li>
>   * resolve issue2003: no set-default hint if user explicitly chose no-set-default
 
> Fri Jan  7 20:36:19 GMT 2011  Ganesh Sittampalam <ganesh@earth.li>
>   * add instruction on suppressing set-default message
> 
> Fri Jan  7 20:36:30 GMT 2011  Ganesh Sittampalam <ganesh@earth.li>
>   * add test of set-default hint messages


resolve issue2003: no set-default hint if user explicitly chose no-set-default
------------------------------------------------------------------------------
I couldn't figure out how to do this because I wasn't thinking out of
the pre-existing flag box. :-)

My only real comment is that I'd probably have an easier time reading
the code if we replaced the Bool with something self-explanatory like
ExplicitlySet.

This is a nice hack for expediency.  In the long term, we need to
overhaul this flags stuff, hopefully extending the notion of implicitly
and explicitly set options accordingly.  Sort of a useful thing for Neil
and Petr (cmdargs and cmdlib) to know about.

> +(DarcsInternalOption f)             `isin` fs = f `elem` fs

I didn't check if there are any other pattern matches

> +    | DarcsInternalOption DarcsFlag
> +    -- ^ @DarcsInternalOption@
> +    -- An option just for internal use (e.g. defaulting), not directly available to the user.

We introduce this because we don't want to display the help for

> -optionFromDarcsAtomicOption :: AbsolutePath -> DarcsAtomicOption -> OptDescr DarcsFlag
> +optionFromDarcsAtomicOption :: AbsolutePath -> DarcsAtomicOption -> Maybe (OptDescr DarcsFlag)
> +optionFromDarcsAtomicOption wd (DarcsInternalOption _) = Nothing

This is used when talking to GetOpt.
GetOpt doesn't need to know about the implicit options, so Nothing.

There's a stray 'wd' that GHC will warn us about likely

>  setDefault :: Bool -> DarcsOption
>  setDefault wantYes
> -  | wantYes   = mkMutuallyExclusive [] yes [no]
> -  | otherwise = mkMutuallyExclusive [yes] no []
> +  | wantYes   = mkMutuallyExclusive [yes,no] defaultyes []
> +  | otherwise = mkMutuallyExclusive [yes,no] defaultno  []
>   where
> hunk ./src/Darcs/Arguments.lhs 1112
> -  yes = ( DarcsNoArgOption [] ["set-default"], SetDefault
> -        , "set default repository" )
> -  no  = ( DarcsNoArgOption [] ["no-set-default"], NoSetDefault
> -        , "don't set default repository" )
> +  yes = ( DarcsNoArgOption [] ["set-default"], SetDefault True
> +        , "set default repository" ++ defaultText wantYes )
> +  no  = ( DarcsNoArgOption [] ["no-set-default"], NoSetDefault True
> +        , "don't set default repository" ++ defaultText (not wantYes) )
> +  defaultyes = ( \f _ -> DarcsInternalOption f, SetDefault False, "" )
> +  defaultno = ( \f _ -> DarcsInternalOption f, NoSetDefault False, "" )
> +  defaultText True = " [DEFAULT]"
> +  defaultText False = ""

The default option is now an implicit one.

Unfortunately, we lose the factorisation on the defaultText setting in
the process.  Oh well.  We could work harder on this if this wasn't an
expedient hack... but of course we also want to conserve our effort and
focus on getting the overhaul done instead.


> +               -- The Bool parameters are a bit of a hack so that we can tell
> +               -- whether the user explicitly set the option or not.
> +               -- A more general mechanism would be better.
> +               -- True = explicitly set by user (on command-line or in prefs/defaults),
> +               -- False = defaulted by darcs
> +               | SetDefault Bool | NoSetDefault Bool

COMMENT: maybe something like ExplicitlySet and ImplicitlySet would be
better, especially since you start running into double negations like
NoSetDefault False (speaking as the kind of person who's really prone
to confusion)

I was going to ask if we could have some kind of catch-all case
ImplicitDefault Flag, but thinking about it more, that wouldn't
work for code that has to test for existence of flag or otherwise

add instruction on suppressing set-default message
--------------------------------------------------
From a UI standpoint, I'd wonder if maybe just telling the user to
set ALL no-set-default would be better.

I figure it'd be surprising if you went through some procedure to
make darcs shut up (darcs get...) and then it doesn't (darcs pull).
Ah, the world of totally informal making-things-up-as-we-go-along
UI design without the backing of proper research.  But even if we 
were a well-resourced totally UI-obsessed team, there'd be limits
on what we can apply research on.

> -  setDefaultrepo repodir opts
> +  setDefaultrepo "get" repodir opts

Can't we just use commandName?

> -setDefaultrepo :: String -> [DarcsFlag] -> IO ()
> -setDefaultrepo r opts =  do olddef <- getPreflist "defaultrepo"
> +setDefaultrepo :: String -> String -> [DarcsFlag] -> IO ()
> +setDefaultrepo command r opts =  do
> +                            olddef <- getPreflist "defaultrepo"
>                              let doit = null noSetDefault && greenLight
>                                  greenLight = wetRun
>                                             && not rIsTmp
> hunk ./src/Darcs/Repository/Prefs.lhs 458
>                                             && (olddef /= [r] || olddef == [])
>                              if doit
>                                 then setPreflist "defaultrepo" [r]
> -                               else when (True `notElem` noSetDefault && greenLight) $ putStr . unlines $
> +                               else
> +                                 when (True `notElem` noSetDefault && greenLight) $ do
> +                                   mdir <- globalPrefsDir
> +                                   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
> hunk ./src/Darcs/Repository/Prefs.lhs 468
>                                        [ "Note: if you want to change the default remote repository to"
>                                        , r ++ ","
>                                        , "quit now and issue the same command with the --set-default flag."
> -                                      ]
> +                                      ] ++
> +                                      maybe [] (\dir -> ["      To suppress this message globally, add \"" ++ command ++ " no-set-default\""
> +                                                        ,"      to " ++ dir </> "defaults"])
> +                                               mdir
>                              addToPreflist "repos" r
>                           `catchall` return () -- we don't care if this fails!
>   where


add test of set-default hint messages
-------------------------------------
Nice, very clear and readable test that checks both our expectation
(message triggers) and also for the unexpected (message does not
trigger when we're not expecting it).  Hard to train ourselves to
check for the letter.  Could maybe be part of the set-default test
script.

-- 
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)

Following up on myself...

1. There are conflicts with screened which I won't deal with.

2. I think about Ganesh's concerns about the message itself.
   More below.
   
3. Should the test check for interactions between no-set-default
   in prefs and set-default in the command line?  I guess we can
   assume that the rest of argument handling stuff takes care of
   it?

On Thu, Jan 06, 2011 at 23:16:12 +0000, Ganesh Sittampalam wrote:
> The second patch is much more speculative as it makes the hint message
> even more verbose. Also it ignores the fact that the user can 
> also set the default for the one repo in _darcs/prefs/prefs.

The message now looks like:

      Note: if you want to change the default remote repository to
      /tmp/darcs/unstable,
      quit now and issue the same command with the --set-default flag.
            To suppress this message globally, add "pull no-set-default"
            to /home/eykk10/.darcs/defaults
      
I think the indentation is to account for screened.

I'd agree that our increasing verbosity as of late is not a good thing
(that'd be personal taste talking, not UI principles or research alas).

I wonder if we should could shoot for very terse messages, maybe with
increasing use of online documentation.

The very last bit of input you should see is something like:

  $ darcs pull foo@example.com:/quux/branch/blub
  ...
  Finished pulling and applying.
  HINT: Default repository is still foo@example.com:/foo/bar
  See 'darcs help defaultrepo' to change it or suppress this message.

(OK, we don't have to give a text length hint...)

-- 
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)

On Fri, 21 Jan 2011, Eric Kow wrote:

>> +               -- True = explicitly set by user (on command-line or in prefs/defaults),
>> +               -- False = defaulted by darcs
>> +               | SetDefault Bool | NoSetDefault Bool
>
> COMMENT: maybe something like ExplicitlySet and ImplicitlySet would be
> better, especially since you start running into double negations like
> NoSetDefault False (speaking as the kind of person who's really prone
> to confusion)

I agree. I considered it but then got lazy.

> add instruction on suppressing set-default message
> --------------------------------------------------
> From a UI standpoint, I'd wonder if maybe just telling the user to
> set ALL no-set-default would be better.

I don't think so, because some commands do, and should, set the 
default by default, e.g. get.

>> -  setDefaultrepo repodir opts
>> +  setDefaultrepo "get" repodir opts
>
> Can't we just use commandName?

I did consider that, but I'd still have to refer to a global 
(the code would be "commandName get") so there'd be no particular 
guarantee of the two staying connected properly. Also, we already used the 
string name in other similar contexts - see Send - so I decided to just 
keep it simple.

Ganesh

On Fri, 21 Jan 2011, Eric Kow wrote:

> On Thu, Jan 06, 2011 at 23:16:12 +0000, Ganesh Sittampalam wrote:
>> The second patch is much more speculative as it makes the hint message
>> even more verbose. Also it ignores the fact that the user can
>> also set the default for the one repo in _darcs/prefs/prefs.
>
> The message now looks like:
>
>      Note: if you want to change the default remote repository to
>      /tmp/darcs/unstable,
>      quit now and issue the same command with the --set-default flag.
>            To suppress this message globally, add "pull no-set-default"
>            to /home/eykk10/.darcs/defaults
>
> I think the indentation is to account for screened.

Yes, there's a patch that reformats the overall message a bit and makes 
that text look sensible again. I was planning on putting that in 2.5.1 
(and have now done so).

But in any case I'm not going to put the "To suppress this message 
globally" patch into 2.5.1 (or HEAD) unless anyone argues for it - for 
myself, I think it's just a little too much.

> I'd agree that our increasing verbosity as of late is not a good thing
> (that'd be personal taste talking, not UI principles or research alas).
>
> I wonder if we should could shoot for very terse messages, maybe with
> increasing use of online documentation.
>
> The very last bit of input you should see is something like:
>
>  $ darcs pull foo@example.com:/quux/branch/blub
>  ...
>  Finished pulling and applying.
>  HINT: Default repository is still foo@example.com:/foo/bar
>  See 'darcs help defaultrepo' to change it or suppress this message.

I like this approach.

Ganesh