On Fri, Aug 26, 2005 at 10:25:15AM -0700, [EMAIL PROTECTED] wrote:
> Quoting David Roundy <[EMAIL PROTECTED]>:
>
> > 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...
>
> Okay, it's annoying from my perspective, but in the long run we all thank
> you for not accepting crap code :) And I do appreciate the comments on my
> coding style. I'm always looking for ways to improve and it helps.
Attach: /tmp/posthookpatch test script
Also, keep in mind that when I give suggestions for improvement they're
often better than what I would have coded if I had written the same
functionality from scratch, because I see what you've done and seeing
mistakes is easier than avoiding making mistakes (at least for me). The
back-and-forth interaction is slow, but (I believe) leads to better code
than either of us would have written alone.
> > If you're happier with shell, you could always implement some tests in
> > shell...
>
> Hmm...Shell might be easier for me, but unless I used
> haskell/lisp/c/c++/java I'd be learning a new language. I find perl to
> be really hard to grok, but shell might be a bit easier to understand.
Mostly shell is just what you'd do on the command-line to test your code,
so it's really pretty easy. You copy one of the existing shell scripts,
which has a bit in the header to make $DARCS point to the right darcs, and
has some cleanup code, and has "set -e" which make the script exit with
error when it executes a command that exits with an error.
Then it's helpful to read the man page of test (or grep test from the
existing shell tests), which allows you to do things like check if files
exist or don't exist. And keep in mind that diff returns an error when the
two files differ.
I'm attaching a patch that adds a pretty simple test script for the
--posthook option. It currently fails, for reasons I'll discuss below.
Shell is also easier for me, although it doesn't allow you to do some of
the tricky things like creating "TODO" tests. And I actually have written
quite a bit of perl code in my day...
> > 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.
>
> My main concern here is that outside of this weekend, I don't know when
> I'll be able to find time between now and the end of November. The
> problem is that my summer job is finishing up, then I move home and start
> my school routine again. During school I historically have very limited
> free time.
Okay, I can definitely polish this off if you don't get it finished this
weekend. I prefer having other contributors do the work while I do the
reviewing, as it generally results in both better code and better
interfaces.
> > 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.
>
> Okay, I hadn't realized it was meant to be mutually exculsive. I have
> tested the way this code works (I use it on a repo with a few other
> contributors) it does actually work as I expected it would. But, since
> it violates a darcs assumption it should be changed.
Ah. After a bit of testing (which you can see in my test script), I see
that DarcsMultipleChoiceOptions are mutually exclusive when they come from
different origins. A DarcsMultipleChoiceOption on the command line
overrides all options from that set that are in _darcs/prefs/defaults, and
it is intended that _darcs/prefs/defaults will override options in
~/.darcs/defaults.
I'm thinking that DarcsMultipleChoiceOptions *should* also be mutually
exclusive within a single source, but I'm not sure how one should go about
this. On the command line we could just exit with an error if a user
specifies
darcs wh --summary --no-summary
but I'd rather not exit with an error when there are contradictory flags in
defaults. But on the other hand, having confusing behavior (*I* don't know
what happens if --summary and --no-summary are both in defaults) isn't
really any better.
> > 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.
>
> I think prompting is more inline with how darcs handles other operations.
> Take record, push and pull as examples. The default is to interact with
> the user. I don't think we lose anything here, and advanced users can
> put a line in ~/.darcs/defaults to never be prompted. One of the things
> that brought me to darcs was the level of user interaction. I feel
> pretty strongly about having the prompting :) If others on the list think
> I'm nuts they should speak up.
Okay, he who writes the code gets to decide, as long as he doesn't do
anything stupid (which this is not). We can add it as you like, and if
users agree with me, they'll complain and we can discuss it and maybe
change it.
> > 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 think having prompt-posthook and run-posthook at the granularity of
> individual commands is best, and there is already a way to make all must
> posthooks run without prompting.
I'm not sure you understood me. If we create --posthook as a
non-multiple-choice flag, then it can be specified any number of times (I
think), which means the user could specify multiple posthooks per command.
This could be handy if users wanted a universal posthook. Imagine a
defaults file (after we implement some environment variable passing) of
ALL --posthook echo $DARCS_COMMAND_LINE >> log_of_darcs_actions
apply --posthook echo hello world | mail droundy
We might want both posthooks to be run when the darcs apply is run. On the
other hand, perhaps this is just being silly..
> > Test seems fine to me, although this could be moved directly into
> > DarcsCommand perhaps...
>
> I had tossed around the idea of putting some of this in a new file,
> Hooks.lhs or similar, but, I wasn't sure if I had enough code to justify
> it. I figured that would be best left to people like Ian or yourself.
I agree, adding a new file for so little code seems silly. We can break it
out, if it starts getting big.
> > It's more than a bit ugly that there's duplicate code here. I think it'd
> > be a bit nicer as
>
> Heh, I was following some pseudo code you had sent me at one point, and I
> didn't think of the simplification that you suggest next. It's nice by
> the way. I shouldn't have been so literal and should have thought more
> for myself ;)
Definitely... When I send pseudo code (or even actual code, if it's not a
darcs patch--which means I haven't compiled it) I'm definitely hoping to
inspire thinking! ;)
--
David Roundy
http://www.darcs.net
New patches:
[add skeleton posthook test.
David Roundy <[EMAIL PROTECTED]>**20050827123744]
<
> {
addfile ./tests/posthook.sh
hunk ./tests/posthook.sh 1
+#!/bin/sh
+
+
+set -ev
+
+test $DARCS || DARCS=$PWD/../darcs
+
+rm -rf temp1
+mkdir temp1
+cd temp1
+$DARCS init
+touch foo
+$DARCS add foo
+
+# Check that prompting works as expected when answering yes...
+echo yes | $DARCS whatsnew -s --posthook 'touch posthook-ran'
+test -f posthook-ran
+rm posthook-ran
+
+# Check that prompting works as expected when answering no...
+echo no | $DARCS whatsnew -s --posthook 'touch posthook-ran'
+test ! -e posthook-ran
+
+# Check that prompting works as expected with defaults (yes)...
+echo ALL --posthook touch posthook-ran > _darcs/prefs/defaults
+echo yes | $DARCS whatsnew -s
+test -f posthook-ran
+rm posthook-ran
+
+# Check that prompting works as expected with defaults (yes)...
+echo no | $DARCS whatsnew -s
+test ! -e posthook-ran
+
+# Check that --run-posthook works in defaults
+echo ALL --run-posthook >> _darcs/prefs/defaults
+$DARCS whatsnew -s
+test -f posthook-ran
+rm posthook-ran
+
+# Check that --run-posthook works when specified both in defaults and on
+# command line
+$DARCS whatsnew --run-posthook -s
+test -f posthook-ran
+rm posthook-ran
+
+# Check that --posthook works when --run-posthook is in defaults
+echo ALL --run-posthook > _darcs/prefs/defaults
+$DARCS whatsnew --posthook 'touch posthook-ran' -s
+test -f posthook-ran
+rm posthook-ran
+
+echo Successful.
+
+cd ..
+rm -rf temp1
}
Context:
[remove hideous malloc hack.
David Roundy <[EMAIL PROTECTED]>**20050818161411]
[change my AUTHORS email to [EMAIL PROTECTED]
David Roundy <[EMAIL PROTECTED]>**20050808124703]
[fix mkstemp implementation for win32
Peter Strand <[EMAIL PROTECTED]>**20050810211303]
[Implement parts of System.Posix.(IO|Files) for win32
[EMAIL PROTECTED]
[implement RawMode with library functions instead of ffi
[EMAIL PROTECTED]
[call hsc2hs without output filename argument
[EMAIL PROTECTED]
[Rename compat.c to c_compat.c to avoid object filename conflict with Compat.hs
[EMAIL PROTECTED]
[Move atomic_create/sloppy_atomic_create to Compat
Ian Lynagh <[EMAIL PROTECTED]>**20050730141703]
[Split the raw mode stuff out into its own .hsc file. Windows needs some TLC
Ian Lynagh <[EMAIL PROTECTED]>**20050730134030]
[Move maybe_relink out of compat.c
Ian Lynagh <[EMAIL PROTECTED]>**20050730131205]
[Remove is_symlink
Ian Lynagh <[EMAIL PROTECTED]>**20050730122255]
[Move mkstemp to Compat.hs
Ian Lynagh <[EMAIL PROTECTED]>**20050730020918]
[Remove unused function
Ian Lynagh <[EMAIL PROTECTED]>**20050730010118]
[Start Compat.hs, and move stdout_is_a_pipe from compat.c
Ian Lynagh <[EMAIL PROTECTED]>**20050730004829]
[fix compilation errors with ghc-6.2.2 on win32
Peter Strand <[EMAIL PROTECTED]>**20050809192759]
[Retain both Git's author and committer.
Juliusz Chroboczek <[EMAIL PROTECTED]>**20050810000820]
[Move slurping into syncPristine.
Juliusz Chroboczek <[EMAIL PROTECTED]>**20050809232101
Avoids creating a useless pristine tree when there is none. Thanks to
Ian for pointing this out.
]
[Split --relink into --relink and --relink-pristine.
Juliusz Chroboczek <[EMAIL PROTECTED]>**20050809230951
Relinking the pristine tree breaks handling of timestamps, which causes
Darcs to compare file contents. It should not be used unless you know
what you are doing.
]
[fix for bug Ian found in apply.
David Roundy <[EMAIL PROTECTED]>**20050811162558
This is the bug manifested in the cabal repository.
]
[Cleanup --verbose handling in repair command
Matt Lavin <[EMAIL PROTECTED]>**20050805020630]
[clean up Printer.wrap_text.
David Roundy <[EMAIL PROTECTED]>**20050808114844]
[add several changelog entries.
David Roundy <[EMAIL PROTECTED]>**20050808114800]
[improve EOD message a tad.
David Roundy <[EMAIL PROTECTED]>**20050807112644
This change also introduces a "wrapped_text" function in Printer, so we
won't have to worry so often about manually wrapping lines.
]
[changed ***DARCS*** to ***END OF DESCRIPTION***
Jason Dagit <[EMAIL PROTECTED]>**20050729032543]
[remove unused opts argument from apply_patches and apply_patches_with_feedback
Matt Lavin <[EMAIL PROTECTED]>**20050807031038]
[add code to read patch bundles with added CRs.
David Roundy <[EMAIL PROTECTED]>**20050806222631
I think this'll address bug #291.
]
[accept command-line flags in any order.
David Roundy <[EMAIL PROTECTED]>**20050806211828
In particular, we no longer require that --flags precede filename and
repository arguments.
]
[add obliterate command as alias for unpull.
David Roundy <[EMAIL PROTECTED]>**20050804104929]
[Do not ask confirmation for revert -a
[EMAIL PROTECTED]
Giving -a as a parameter means the user expects all changes to be reverted.
Just like for unrevert and record go ahead with it do not ask for confirmation.
]
[clarify help text for 'd' in SelectPatches.
David Roundy <[EMAIL PROTECTED]>**20050806231117]
[Add --with-static-libs configure flag for linking static versions of libraries.
[EMAIL PROTECTED]
[add changelog entry for bug #477.
David Roundy <[EMAIL PROTECTED]>**20050806212148]
[add description of how to add changelog entries to ChangeLog.README.
David Roundy <[EMAIL PROTECTED]>**20050806225901]
[Explain the missing ChangeLog
Mark Stosberg <[EMAIL PROTECTED]>**20050526135421
It should be easy for casual users and contributors to view and update the
ChangeLog.
Providing a README file in the place where people are most likely to look
provides a very useful clue.
However, it's still not clear to me exactly how the system works, so I have
left a stub to complete that documentation.
Mark
]
[make repair work on partial repositories.
David Roundy <[EMAIL PROTECTED]>**20050805113001]
[Use apply_patch_with_feedback from check and repair commands
Matt Lavin <[EMAIL PROTECTED]>**20050805020830]
[show patch numbers instead of dots on get
Matt Lavin <[EMAIL PROTECTED]>**20050804013649]
[fix obsolete error explanation in get_extra bug.
David Roundy <[EMAIL PROTECTED]>**20050804130610]
[simplify fix for bug 463; reuse /// from FilePathUtils
Matt Lavin <[EMAIL PROTECTED]>**20050804021130]
[Make curl exit with error on failed downloads
[EMAIL PROTECTED]
[Bump up AC_PREREQ version to 2.59.
[EMAIL PROTECTED]
[fix for bug 463 (with new test)
Matt Lavin <[EMAIL PROTECTED]>**20050802002116]
[bump version number, since I just made a release.
David Roundy <[EMAIL PROTECTED]>**20050731190756]
[Use simpler curl_version() function to get version string.
Kannan Goundan <[EMAIL PROTECTED]>**20050322221027]
[fix documentation on --reorder-patches.
David Roundy <[EMAIL PROTECTED]>**20050731185406]
[add changelog entry for bug #224.
David Roundy <[EMAIL PROTECTED]>**20050731133942]
[fix bug when editing long comment leaves empty file.
David Roundy <[EMAIL PROTECTED]>**20050731133612]
[changelog entry for bug #189.
David Roundy <[EMAIL PROTECTED]>**20050731132624]
[TAG 1.0.4pre2
David Roundy <[EMAIL PROTECTED]>**20050731121029]
Patch bundle hash:
04757e7b2464dc54a0da87ad4392cee342c3b30f
_______________________________________________
darcs-devel mailing list
[email protected]
http://www.abridgegame.org/cgi-bin/mailman/listinfo/darcs-devel
