Hi,

this patch separates test running (when 'darcs set test' is set) from
comitting changes to the repository.

When run in interactive mode, if test fails, the user is asked to
confirm the abort.  When run in non-interactive mode, the old behaviour
is kept (i.e. abort without asking).

This patch changes finalizeRepositoryChanges so that it does not run the
test anymore.  It also changes the type of testTentative/testRecorded to
return the exit code of the test.

Uses of finalizeRepositoryChanges in darcs code have been updated to
perform the test when needed (for instance, no change have been made to
Convert.lhs since this command does not have a --test/no-test option).
Two uses of this function in Repository.hs have been left unmodified
because I believe no test is ever run when performing a full copy of a
repository.  In case I am wrong, please tell me and I'll fix that too.

Finally, this patch updates one test in the testsuite to take into
account the new confirmation.

Best,
Gabriel

1 patch for repository http://darcs.net:

Sun Nov  7 20:19:55 CET 2010  Gabriel Kerneis <kerneis@pps.jussieu.fr>
  * resolve issue332: ask if test fails

Attachments

• resolve-issue332_-ask-if-test-fails.dpatch
• unnamed

This looks good. I think the new feature has been discussed enough in 
the associated bug that it's ok to add without further discussion.

Some comments justifying the changes to myself and also some style 
points below.

OK, so finalizeRepositoryChanges is called by AmendRecord, Apply, Get, 
Pull, Tag, Unrecord, Unrevert, Rollback, Record, Optimize.

So the things you haven't updated are Unrecord, Unrevert, Tag, Optimize 
and Get. I agree that makes sense.

Minor style points:

 - it would have been better to separate the refactoring and the new 
functionality into separate patches, to make it easier to later 
understand what changed and undo things if necessary.

 - could the similar code in Record and AmendRecord for asking whether 
to go ahead be factored out?

I'm happy to accept this as is, but I'll wait a bit in case you want to 
address the above points with new patch(es).

Actually, the similar code for asking whether to go ahead anyway is in 
more than two places; so I definitely think it should be factored out but 
this could be done in a followup at some point.

Addressing your concerns:
- I try as much as possible to separate refactoring and new
functionality, but I couldn't manage to get a refactoring-only patch
which would make sense (both aspects are intimately tied together).  If
you have any idea on a sensible split, do not hesitate to unrecord my
patch and re-record it as two separate patches.
- I am willing to factor out prompting, but I think it should be part of
a larger work on refactoring how prompting is done in darcs.  It has
been discussed a bit on IRC, but I think it deserves a separate issue to
decide how it should be done.

Gabriel Kerneis wrote:
> Gabriel Kerneis <kerneis@pps.jussieu.fr> added the comment:
> 
> Addressing your concerns:
> - I try as much as possible to separate refactoring and new
> functionality, but I couldn't manage to get a refactoring-only patch
> which would make sense (both aspects are intimately tied together). 
> If you have any idea on a sensible split, do not hesitate to unrecord
> my patch and re-record it as two separate patches.

My naïve expectation was that moving the testing calls out of finalizeRepositoryChanges could be separated from prompting on failure, but I could well be wrong. It's certainly not worth a huge amount of effort.
 
> - I am willing to factor out prompting, but I think it should be part
> of a larger work on refactoring how prompting is done in darcs.  It
> has been discussed a bit on IRC, but I think it deserves a separate
> issue to decide how it should be done.   

Fair enough.

Cheers,

Ganesh

=============================================================================== 
Please access the attached hyperlink for an important electronic communications disclaimer: 
http://www.credit-suisse.com/legal/en/disclaimer_email_ib.html 
===============================================================================

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-9de38ad50f86a6dde7150774cc38aee02a9ce180

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-9de38ad50f86a6dde7150774cc38aee02a9ce180