Re: [PATCH 6/6] sequencer: factor code out of revert builtin

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Expose the cherry-picking machinery through a public
> sequencer_pick_revisions() (renamed from pick_revisions() in
> builtin/revert.c), so that cherry-picking and reverting are special
> cases of a general sequencer operation.  The cherry-pick builtin is
> now a thin wrapper that does command-line argument parsing before
> calling into sequencer_pick_revisions().  In the future, we can write
> a new "foo" builtin that calls into the sequencer like:

Nice.

I don't think the plan is actually a "foo" builtin.

I guess I would say "So now your 'foo' builtin can use the sequencer
machinery by implementing a 'parse_args_populate_opts' function and
then running the following:" so as to leave the reader more excited
and not to imply a promise we are not going to keep. :)

>   memset(&opts, 0, sizeof(opts));
>   opts.command = REPLAY_CMD_FOO;
>   opts.revisions = xmalloc(sizeof(*opts.revs));
>   parse_args_populate_opts(argc, argv, &opts);
>   init_revisions(opts.revs);
>   sequencer_pick_revisions(&opts);

Hm, I wonder if opts.command should be a string so each new caller
does not have to add to the enum and switch statements...

> This patch does not intend to make any functional changes.  Check
> with:
>
>   $ git blame -s -C HEAD^..HEAD -- sequencer.c

Neat.

[...]
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -24,6 +24,29 @@ enum replay_subcommand {
>  	REPLAY_ROLLBACK
>  };
>  
> +struct replay_opts {
[...]
> +};
[...]
> @@ -33,4 +56,6 @@ struct replay_insn_list {
[...]
> +int sequencer_pick_revisions(struct replay_opts *opts);

Looks sensible.  Maybe it would be useful to include a
Documentation/technical/api-sequencer.txt explaining how this is meant
to be used.  (Not much detail needed, just an overview.  See the
surrounding files for some examples.)

Haven't checked the code movement yet, since it is sitting on top of a
slushy base.  Next time, hopefully.

Thanks,
Jonathan