This bundle takes into account the discussion at http://bugs.darcs.net/issue2042
but only adresses the topic of reorganizing darcs' subcommands with regards to
repair, check and test.

The effect of this patch bundle is that:

* all code testing functionalities of darcs now live inside the
  command `darcs test':
    * `darcs test'             was `darcs check --test'
                               (test current state of the repo)
    * `darcs test --trackdown' was `darcs trackdown'
    * `darcs test --bisect'    was `darcs trackdown --bisect'
* flags --trackdown and --bisect are mutually exclusive
* `darcs check' is now an alias for `darcs repair --dry-run'

I did not try to rewrite the merged commands into something well
factorized, on the contrary I basically plugged the functions together
with a few if-then-else's and guards and fixed the help messages.

The point is to have a code that passes the regression tests. I added
one regression test that verify that the user sees a warning message
in the case they call `darcs test INIT_COMMAND TEST_COMMAND', since in
that case, INIT_COMMAND is useless. (I'm maintaining the former behaviour
of check --test here, but maybe in the future we can say INIT_COMMAND
should be executed also in that case.)

We will see if there is a need to introduce aliases for trackdown, but
I'm not sure it's a good thing given that we won't be able to alias
`darcs trackdown --bisect' to `darcs test --bisect' (given the current
codebase).



6 patches for repository http://darcs.net:

Wed Mar 21 11:57:20 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * introduce darcs test, doing the same as did check --test without repo check

Wed Mar 21 13:26:18 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * adapt test suite to switch from darcs check to darcs test

Wed Mar 21 13:37:46 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * adapt testsuite to test --trackdown and test --bisect

Wed Mar 21 15:49:53 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * switch to test --trackdown and test --bisect

Wed Mar 21 18:19:15 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * add regression test to check warning message presence for darcs test

Wed Mar 21 18:41:10 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * make check an alias for repair --dry-run

Attachments

• introduce-darcs-test_-doing-the-same-as-did-check-__test-without-repo-check.dpatch
• patch-preview.txt
• unnamed

7 patches for repository http://darcs.net:

Wed Mar 21 11:57:20 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * introduce darcs test, doing the same as did check --test without repo check

Wed Mar 21 13:26:18 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * adapt test suite to switch from darcs check to darcs test

Wed Mar 21 13:37:46 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * adapt testsuite to test --trackdown and test --bisect

Wed Mar 21 15:49:53 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * switch to test --trackdown and test --bisect

Wed Mar 21 18:19:15 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * add regression test to check warning message presence for darcs test

Wed Mar 21 18:41:10 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * make check an alias for repair --dry-run

Wed Mar 21 18:56:25 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * update manual for darcs test and repair --dry-run

Attachments

• introduce-darcs-test_-doing-the-same-as-did-check-__test-without-repo-check.dpatch
• patch-preview.txt
• unnamed

Hi Guillaume.  Thanks for patches.  Since I said I thought this was a good idea, it 
seems only fair that I do some code review too :)

> hunk ./src/Darcs/Commands/Test.hs
> +test = DarcsCommand  {commandProgramName = "darcs",
> +                      commandName = "test",
> +                      commandHelp = testHelp,
> +                      commandDescription = testDescription,
> +                      commandExtraArgs = 0,
> +                      commandExtraArgHelp = [],
> +                      commandCommand = testCmd,
> +                      commandPrereq = amInHashedRepository,
> +                      commandGetArgPossibilities = return [],
> +                      commandArgdefaults = nodefaults,
> +                      commandAdvancedOptions = [],
> +                      commandBasicOptions = [ leaveTestDir,
> +                                              workingRepoDir
> +                                             ]}

A good opportunity to rearrange following the style guide.

> hunk ./tests/oldfashioned_refusal.sh 56
> -not darcs trackdown

Should this add a line "not darcs test"?

> hunk ./tests/trackdown-bisect.sh 35
> -# You can remove --bisect for compare with linear trackdown
> -trackdown_args='--bisect' $
> +# You can replace --bisect by --trackdown for compare with linear trackdown
> +test_args='--bisect' $

A good chance to remove the trailing whitespace on these lines.

> hunk ./tests/trackdown-bisect.sh 131
> -trackdown_args='--bisect' $
> +test_args='--bisect' $

Trailing whitespace.

> hunk ./src/Darcs/Arguments.hs
> +__trackdown, __bisect :: DarcsAtomicOption
> +__trackdown = DarcsNoArgOption [] ["trackdown"] TrackDown
> +                "locate the most recent version lacking an error"
> +__bisect = DarcsNoArgOption [] ["bisect"] Bisect
> +             "binary instead of linear search"

I'd really like to see --trackdown renamed to --linear and a new option --once (for 
the default behavior).  Having --once lets one override on the command line "test -
-bisect" in a defaults file.  This naming convention makes it clearer, to my eyes, 
that one is specifying a search strategy.  --strategy={once,linear,bisect} would be 
even better, but maybe that's too different from the rest of darcs options.

I've been working on a trackdown strategy that starts at head, backing off 
exponentially until the test passes, then bisecting the skipped over patches.  
While working on that patch, the absence of `trackdown --linear` stood out as a UI 
flaw.

> hunk ./src/Darcs/Commands/Test.hs 97
> - "If a regression test is defined (see `darcs setpref') it will be run.\n"
> + unlines
> + [ "Run test on the current recorded state of the repository.  Given no"

Great to see unlines here.

> +  ,"is an initialization command with is run only once, and the second is"

s/with is/which is/

> +  if once
> +   then withRepository (Test:opts) $ RepoJob $ oneTest opts
> +   else withRecorded repository (withTempDir "trackingdown") $ \_ -> do

I'd personally like to see `darcs test --once` perform its test in a clean 
checkout, like --linear and --bisect do.  Although that's a departure from `darcs 
check` behavior, so probably out of line for a code review anyway.

> +      currProg = (1, 1 + round ((logBase 2 $ fromIntegral $ lengthRL ps) :: 
Double)) :: (Int,Int)

More precisely, ":: BisectState"

> +# ensure no warning message is displayed when not giving an initialization
> +# command to darcs test without --trackdown nor --bisect flags
> +darcs test --trackdown true true | grep -v WARNING
> +
> +# ensure no warning message is displayed when not giving an initialization
> +# command to darcs test without --trackdown nor --bisect flags
> +darcs test --bisect true | grep -v WARNING

It took me quite a while to understand the comments because of all the negatives.  
Is the following clearer to anyone else?

  ensure no warning message is displayed when giving an initialization
  command to darcs test with --trackdown or --bisect flags

>> hunk ./tests/oldfashioned_refusal.sh 56
>> -not darcs trackdown
>
> Should this add a line "not darcs test"?

It's done in the patch "adapt test suite to switch from darcs check to
darcs test".

>> hunk ./src/Darcs/Arguments.hs
>> +__trackdown, __bisect :: DarcsAtomicOption
>> +__trackdown = DarcsNoArgOption [] ["trackdown"] TrackDown
>> +                "locate the most recent version lacking an error"
>> +__bisect = DarcsNoArgOption [] ["bisect"] Bisect
>> +             "binary instead of linear search"
>
> I'd really like to see --trackdown renamed to --linear and a new option --once (for
> the default behavior).  Having --once lets one override on the command line "test -
> -bisect" in a defaults file.  This naming convention makes it clearer, to my eyes,
> that one is specifying a search strategy.  --strategy={once,linear,bisect} would be
> even better, but maybe that's too different from the rest of darcs options.
>
> I've been working on a trackdown strategy that starts at head, backing off
> exponentially until the test passes, then bisecting the skipped over patches.
> While working on that patch, the absence of `trackdown --linear` stood out as a UI
> flaw.

OK I agree that "no flag", --trackdown and --bisect are mutually
exclusive. I'm doing a followup patch that introduces '--once",
mutually exclusive with --trackdown and --bisect.

>> +  ,"is an initialization command with is run only once, and the second is"
>
> s/with is/which is/

thanks!

>> +  if once
>> +   then withRepository (Test:opts) $ RepoJob $ oneTest opts
>> +   else withRecorded repository (withTempDir "trackingdown") $ \_ -> do
>
> I'd personally like to see `darcs test --once` perform its test in a clean
> checkout, like --linear and --bisect do.  Although that's a departure from `darcs
> check` behavior, so probably out of line for a code review anyway.

Well in fact I realised that "darcs test (--once)" never uses the test
command passed as parameter since the piece of code just looks at
_darcs/prefs/prefs . So for that reason I'm going to use withRecorded
also for the --once case.

This makes a few functions of Darcs.Repository.Internal now unused
(testRecord / withRecorded) and move some of the flag logic (eg: has
the "--test" flag been passed ?) out of this module, which IMO is a
good thing.

>> +      currProg = (1, 1 + round ((logBase 2 $ fromIntegral $ lengthRL ps) ::
> Double)) :: (Int,Int)
>
> More precisely, ":: BisectState"

Good one.

>> +# ensure no warning message is displayed when not giving an initialization
>> +# command to darcs test without --trackdown nor --bisect flags
>> +darcs test --trackdown true true | grep -v WARNING
>> +
>> +# ensure no warning message is displayed when not giving an initialization
>> +# command to darcs test without --trackdown nor --bisect flags
>> +darcs test --bisect true | grep -v WARNING
>
> It took me quite a while to understand the comments because of all the negatives.
> Is the following clearer to anyone else?
>
>  ensure no warning message is displayed when giving an initialization
>  command to darcs test with --trackdown or --bisect flags

Ok. Comments about my English writing style are always welcome!

g.

Followup patch.

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

Fri Mar 23 21:11:12 ART 2012  Guillaume Hoffmann <guillaumh@gmail.com>
  * make darcs test more consistent
   * darcs test INIT TEST uses INIT and TEST
   * --leave-test-directory works for all strategies of darcs test
   * rename --trackdown to --linear and adapt test scripts

Attachments

• make-darcs-test-more-consistent.dpatch
• patch-preview.txt
• unnamed

Follow up patch looks good.  Consistent UI, good documentation, clean code 
and passing tests.  Committers, please push these patches.