On Wed, Aug 24, 2005 at 10:18:23PM -0700, Jason Dagit wrote:
> This implements the posthook option for all darcs commands and adds
> documentation as well. It does not add any test cases, I need help with
> that part.
Thanks for the contribution! I still have a few nits to pick and a few
suggestions for where the code could be cleaner...
> Here are examples of things we need to test:
> 1) Commands that don't run in the current working directory have the right
> path.
> Ex: remote repo has the lines "apply posthook echo `pwd`" and
> "apply run-posthook" defined in _darcs/prefs/defaults. Doing a
> push to this repo should give should show the remote path.
> 2) Make sure that the posthook command is only run once in all possible
> combinations of the command failing/passing.
> 3) Check to see if a posthook simply behaves backwards for any commands.
> This could be the case if trying to record when there are no patches to
> record. What should happen in this case?
> 4) Test this on windows.
> 5) Anything anyone else can think of.
>
> I'd implement some of these tests, but my perl skills are not up to the
> challenge. Perhaps if someone created some examples I could add a test or
> two...
If you're happier with shell, you could always implement some tests in
shell...
> I'm looking forward to this getting accepted and hopefully before 1.0.4 comes
> out, but who knows, maybe it needs more testing. But, I do know a lot of
> people will like having this hook. One of the cool things it can do is
> run a script to fix up permissions when people do a push. I found this was
> important when others in my group would push to my repo from their accounts
> because they might have some weird umask.
I think this is still a possibility, but I'd definitely prefer the code to
be cleaner before accepting (and I think there are still bugs in here). It
would also be nice to throw a --unified into your send defaults, to make
reading the patch a bit easier.
> hunk ./DarcsArguments.lhs 988
> +\begin{code}
> +posthook_cmd :: DarcsOption
> +posthook_cmd = DarcsMultipleChoiceOption
> + [DarcsArgOption [] ["posthook-command"] PosthookCmd
> + "COMMAND" "specify command to run after this darcs command.",
> + DarcsNoArgOption [] ["no-posthook"] NoPosthook
> + "Do not run posthook command.",
> + DarcsNoArgOption [] ["prompt-posthook"] AskPosthook
> + "Prompt before running posthook. [DEFAULT]",
> + DarcsNoArgOption [] ["run-posthook"] RunPosthook
> + "Run posthook command without prompting."]
I don't think this does what you want it to do... either that or there's a
bug in our flag handling. This *should* mean that one can't specify both
--posthook-command and --run-posthook, since the options in a
DarcsMultipleChoiceOption are supposed to be mutually exclusive.
I'm not terribly keen on the --prompt-posthook idea, and wouldn't mind
dropping it, in which case we'd just have two options, --no-posthook and
--posthook. It seems to me that --prompt-posthook is really only relevant
when we add support for reading (and obeying) remote defaults
configurations (e.g. the get posthook you were working on), and it would be
better wait until that is supported to add prompting support. As far as I
can tell, the default --prompt-posthook doesn't add any security, since
anyone who can modify your defaults file can as easily add --run-posthook
as --posthook.
The other option that comes to mind would be to have two
DarcsMultipleChoiceOptions, which would be (--posthook and --no-posthook)
and (--run-posthook and --prompt-posthook)... or one could stick
--no-posthook with --run-posthook and --prompt-posthook. In that case, one
could specify multiple posthooks (and have them all run), which might make
sense.
I haven't had time to check on how this actually behaves, so please correct
me if I'm wrong. If we keep --posthook and --no-posthook as a
DarcsMultipleChoiceOptions pair, the NoPosthook check below shouldn't be
necesary, as the two should naturally be mutually exclusive (although a
test should be added to verify this).
> [added run_posthook for actually running posthooks
> Jason Dagit <[EMAIL PROTECTED]>**20050803070156
> This adds the function run_posthook which should be used to run posthooks.
> The code was added to Test.lhs, but there may be a better place for this
> code.
Test seems fine to me, although this could be moved directly into
DarcsCommand perhaps...
> ] {
> hunk ./Test.lhs 17
> -module Test ( run_test, get_test, test_patch ) where
> +module Test ( run_test, get_test, test_patch, run_posthook ) where
> hunk ./Test.lhs 26
> -import DarcsArguments ( DarcsFlag( Quiet, LeaveTestDir ) )
> +import DarcsArguments ( DarcsFlag( Quiet, LeaveTestDir,
> + NoPosthook, RunPosthook ),
> + get_posthook_cmd )
> hunk ./Test.lhs 31
> +import DarcsUtils ( askUser )
> hunk ./Test.lhs 100
> +\begin{code}
> +run_posthook :: [DarcsFlag] -> FilePath -> IO ExitCode
> +run_posthook opts repodir
> + | NoPosthook `elem` opts = return ExitSuccess
> + | otherwise = do posthook <- get_posthook opts
> + withCurrentDirectory repodir posthook
> hunk ./Test.lhs 107
> +get_posthook :: [DarcsFlag] -> IO (IO ExitCode)
You're better off with one less level of IO here, instead call get_posthook
something like
actually_run_posthook :: [DarcsFlag] -> IO ExitCode
and then run_posthook just becomes
run_posthook opts _ | NoPosthook `elem` opts = return ExitSuccess
run_posthook opts repodir =
withCurrentDirectory repodir (actually_run_posthook opts)
> +get_posthook opts =
> + let putInfo s = when (not $ Quiet `elem` opts) $ putStr s
> + in do
> + posthookline <- return $ get_posthook_cmd opts
> + return $
> + case posthookline of
With the above redefinition (eliminating one of the IO's), this could be
more nicely written as
actually_run_posthook opts =
let putInfo = ...
in case get_posthook_cmd opts of
with the rest of the function being unmodified:
> + Nothing -> return ExitSuccess
> + Just command -> do
> + yorn <- maybeAskUser ("\nThe following command is set to execute.\n"++
> + "Execute the following command now (yes or
> no)?\n"++
> + command++"\n")
> + case yorn of ('y':_) -> do putInfo "Running posthook...\n"
> + ec <- system command
> + if ec == ExitSuccess
> + then putInfo "Posthook ran successfully.\n"
> + else putInfo "Posthook failed!\n"
> + return ec
> + _ -> return ExitSuccess
...
> hunk ./DarcsCommands.lhs 294
> - then (command_command cmd) (FixFilePath fix_path:specops) extra
> + then runWithPostHook specops extra
> hunk ./DarcsCommands.lhs 307
> + posthook (ExitException ExitSuccess) fs p = run_posthook fs p
> + posthook e _ _ = throwIO e
> + runWithPostHook os ex = do
> + here <- getCurrentDirectory
> + (command_command cmd) (FixFilePath fix_path:os) ex
> + `Control.Exception.catch` (\e -> do ec <- posthook e os here
> + throwIO (ExitException
> ec))
> + ec <- posthook (ExitException ExitSuccess) os here
> + throwIO (ExitException ec)
It's more than a bit ugly that there's duplicate code here. I think it'd
be a bit nicer as
runWithPostHook os ex = do
here <- getCurrentDirectory
(command_command cmd) ...
`Control.ExitException.catch`
(\e -> case e of
(ExitCode ExitSuccess) -> return ()
_ -> throwIO e)
run_posthook os here >>= exitWith
--
David Roundy
http://www.darcs.net
_______________________________________________
darcs-devel mailing list
[email protected]
http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
