[darcs-users] darcs patch: extend get_sendmail_cmd to search for 'sendmail' pref ...

Christian Kellermann Christian.Kellermann at nefkom.net
Wed Oct 22 14:44:41 UTC 2008


* David Roundy <droundy at darcs.net> [081022 16:37]:
> On Tue, Oct 21, 2008 at 08:54:17PM +0200, Chistian Kellermann wrote:
> > Tue Oct 21 17:49:35 CEST 2008  Chistian Kellermann <Christian.Kellermann at nefkom.net>
> >   * extend get_sendmail_cmd to search for 'sendmail' pref and SENDMAIL env similar to get_author
> >   
> >   With this I can store my sendmail command setting in
> >   _darcs/prefs/defaults or in $EMAIL.  For this the get_sendmail_cmd
> >   function is unfortunately no longer pure, and the apply as well as
> >   the send commands had to be changed accordingly.
> >   
> >   What I don't like about this patch is the code duplication in
> >   apply.lhs' redirect_output.  Maybe someone with a better haskell
> >   knowledge can help me out here?
> 
> I'll wait on docs for this, as every environment variable we use
> should be documented in the section of the manual... which you can
> grep for, that talks about such things.  This is important for
> enabling debugging of darcs.
> 
OK!

> In the meantime, if anyone thinks there is a better name for this
> environment variable (or that we shouldn't have it), they'd better
> speak up!
> 
> ... Actually, it turns out there are problems with this patch (see
> below) so it requires more than just documentation fixes, although it
> still requires that.
> 
> > -get_sendmail_cmd :: [DarcsFlag] -> String
> > -get_sendmail_cmd (SendmailCmd a:_) = a
> > +get_sendmail_cmd :: [DarcsFlag] -> IO(String)
> 
> It's more standard Haskell style to write -> IO String
OK!

> 
> > +get_sendmail_cmd (SendmailCmd a:_) = return a
> >  get_sendmail_cmd (_:flags) = get_sendmail_cmd flags
> > hunk ./src/Darcs/Arguments.lhs 1333
> > -get_sendmail_cmd [] = ""
> > +get_sendmail_cmd [] = do
> > +                      easy_sendmail <- get_easy_sendmail_cmd
> > +                      case easy_sendmail of
> > +                        Just a -> return a
> > +                        Nothing -> return ""
> > +
> > +-- | 'get_easy_sendmail' tries to get the sendmail command first from the repository preferences,
> > +-- then from global preferences, then from environment variables. Returns 'Nothing' if it
> > +-- could not get it.
> > +get_easy_sendmail_cmd :: IO (Maybe String)
> > +get_easy_sendmail_cmd = firstJustIO [ get_preflist "sendmail" >>= return . firstNotBlank,
> > +                                get_global "sendmail" >>= return .firstNotBlank,
> > +                                maybeGetEnv "SENDMAIL" ]
> 
> I would prefer to avoid introducing a separate helper function here.
> It's particularly helpful *not* to introduce top-level functions that
> are only used once, as it immediately informs the code reader (and
> reviewer) the purpose of your function.  You can nicely define such
> helper functions in a where block (which can be a bit confusing before
> you're use to the indentation rules).
> 
> In this case, I wouldn't write a helper at all, but would write
> 
> get_sendmail_cmd [] = do sm <- firstJustIO [ ...,
>                                              ... ]
>                          case sm of
>                            Just a -> ...
> 
> 
> However, there's another issue that I just noticed, which is your use
> of get_preflist and get_global.  I'd rather not use that function, as
> it means that we're adding two more config files, which would also
> need to be documented, which would be _darcs/prefs/sendmail and
> .darcs/sendmail.  Too many ways to configure the same feature is just
> confusing, as it means that a user whose sendmail is broken has that
> many more places to look for something that's perhaps affecting the
> behavior.  It's also more to document.  So your code will be both
> simpler and nicer if you just use maybeGetEnv alone...

I have got that idea from the get_author function, maybe that one
should get an overhaul as well then?

> 
> >          where sendit tempf e@(ExitException ExitSuccess) =
> > -                do body <- sanitizeFile tempf
> > +                do scmd <- get_sendmail_cmd opts
> > +                   body <- sanitizeFile tempf
> >                     sendEmail f to "Patch applied" cc scmd body
> >                     throwIO e
> 
> I think a nice way to rewrite these to reduce duplication would be to
> change the sanitizeFile function to a sendSanitizedEmail function,
> which would do what sanitizeFile does, and call get_sendmail_cmd and
> do sendEmail. This would be a net improvement over the existing code.
> :)

Alright, thanks for your insights, I will come up with a cleaner patch.

Christian

-- 
You may use my gpg key for replies:
pub  1024D/47F79788 2005/02/02 Christian Kellermann (C-Keen)
-------------- 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/20081022/b478ac01/attachment.pgp 


More information about the darcs-users mailing list