[darcs-users] darcs patch: extend get_sendmail_cmd to search for 'sendmail' pref ...
droundy at darcs.net
Wed Oct 22 14:35:10 UTC 2008
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.
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
... 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
> +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...
> 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.
More information about the darcs-users