[darcs-users] [patch254] resolve issue1371: darcs assumes -p for SSH_PORT; but putty takes -P

Eric Kow kowey at darcs.net
Fri Jun 18 12:01:15 UTC 2010


Thanks, Guillaume,

Comments on the review:

IMHO hlint style comments are surprisingly helpful, not so much
because they improve the code but because they improve the coder.
(That is, if the style bug comes from a subtle misunderstanding
of something deeper and not just a brain fart)
 
The do-I-understand-this-patch is what I call first level review,
(which is the level I'm at personally).  The sort of second-level review
which I aspire to is the kind where you start thinking harder about how
the patch interacts with the rest of the code (yay functional?).
Perhaps further up the ladder is to understand what the patch omits!
What is it failing to anticipate?  And further up the ladder are things
like fundamental design, maintainability, etc.  Phew.
   
This is why I think highly of public code-review as a sort of mental
exercise for programmers.  I guess the only way to get better at it is
to do a lot of reviewing (and read a lot of reviews).

On Thu, Jun 17, 2010 at 09:34:58 +0000, gh wrote:
>>hunk ./src/Ssh.hs 224
>> getSSHOnly :: SSHCmd -> IO (String, [String])
>> getSSHOnly cmd =
>>  do ssh_command <- getEnv (evar cmd) `catchall` return (show cmd)
>>-    -- port
>>-    port <- (portFlag cmd `fmap` getEnv "SSH_PORT") `catchall` return []
>>     let (ssh, ssh_args) = breakCommand ssh_command
>>hunk ./src/Ssh.hs 225
>>+    opts <- getSSHVerOpts cmd ssh
> 
> This is the call to the most important function which given some
> command, tests which version is runnable, and gets the port flag
> accordingly. I would have called the returned variable "sshOpts"
> instead of just "opts".

When is getSSHOnly called?

>>+-- | Contains specific values for SSH implementations
>>+--   like ssh, plink
>>+data SSHCmdOpt = SSHCmdOpt { sshVerName    :: String                -- Information purpose
>>+                           , sshPortOpt    :: String -> [String]
>>+                           , sshBatchFlag  :: [String]
>>+                           , sshQuietFlag  :: [String] }

Would it simplify the code any to put the SSHVersionDetect information
in SSHCmdOpt, or would that just be confusing design?

 , sshVersionInfoFlag :: [String]
 , sshMatches :: String -> Bool

>>+-- | Various implementations for `ssh` command
>>+sshVersions :: [ (SSHVersionDetect,SSHCmdOpt) ]
>>+sshVersions =  [ (plinkMatch       , SSHCmdOpt "plink"   (\p -> ["-P", p]) ["-batch"] [])
>>+               , (sshMatch         , SSHCmdOpt "openssh" (\p -> ["-p", p]) []         ["-q"])
>>+               , (defaultMatchAll  , SSHCmdOpt "default" (\p -> ["-p", p]) []         ["-q"]) ] -- Fallback to default
>>+    where
>>+      sshMatch        = (["-V"], (\s -> "OpenSSH" `isPrefixOf` s))
>>+      plinkMatch      = (["-V"], (\s -> "plink:"  `isPrefixOf` s))
>>+      defaultMatchAll = (["-V"], (\_ -> True))

>>+-- | Detect the type of the ssh command (ssh,plink,etc)
>>+getSSHVerOpts :: SSHCmd -> String -> IO SSHCmdOpt
>>+getSSHVerOpts cmdType cmd = do
>>+  v <- (foldM (tryVersion) Nothing (cmdVersions cmdType))
>>+  return (fromJust v)
>>+    where
>>+      cmdVersions SSH  = sshVersions
>>+      cmdVersions SFTP = sftpVersions
>>+      cmdVersions SCP  = scpVersions
>>+      tryVersion (Just o) _     = return (Just o)
>>+      tryVersion Nothing  (d,o) = do
>>+                          ret <- challengeCMD cmd (fst d)
>>+                          if (snd d) ret then return (Just o) else return Nothing
> 
> OK I see what it does, trying to match the version string/prefix until it works.

I wonder if Darcs.Util.firstJustIO would be useful here:

  http://www.darcs.net/api-doc/Darcs-Utils.html#v%3AfirstJustIO
 (which could perhaps be rewritten with foldM?)

I wish there was something we could do to make this a bit more self
evident. We have the three ideas:

 - stop on first success (firstJustIO?)
 - run ssh -V
 - apply the test on its output

which seems to be slightly obscured by the interplay between the tuple
unwrapping and the Maybe plumbing.

[I don't this this is a big problem, by the way... just wishing I knew
how to write clearer code!]

>>+-- | Private function which takes command and arguments, execute and provides the
>>+--   output (stderr+stdout) as the string
>>+challengeCMD :: String -> [String] -> IO String
>>+challengeCMD cmd args = do
>>+  (_,outp,errp,pid) <- runInteractiveProcess cmd args Nothing Nothing
>>+  outstr <- hGetContents outp
>>+  errstr <- hGetContents errp
>>+  waitForProcess pid
>>+  let resp = (errstr++outstr)
>>+  return resp

Do we have a deadlock problem?
http://www.haskell.org/pipermail/haskell-cafe/2008-May/042994.html
-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100618/4ee7de9a/attachment.pgp>


More information about the darcs-users mailing list