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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 8, 2012, 20:38 UTC
Message-ID: <20120108203800.GM1942@burratino>
In-Reply-To: <1326025653-11922-7-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

Show 7 quoted lines

> 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. :)

Show 6 quoted lines

>   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.

[...]

Show 7 quoted lines

> --- 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