Ramkumar Ramachandra wrote:
> Start building the generalized sequencer by moving code from revert.c
> into sequencer.c and sequencer.h. Make the builtin responsible only
> for command-line parsing, and expose a new sequencer_pick_revisions()
> to do the actual work of sequencing commits.
>
> This is intended to be almost a pure code movement patch with no
> functional changes. Check with:
Do I understand correctly that the purpose of this patch is to expose
some functions through the "sequencer.h" API, which patches later in
the series will use? Which functions? What is this generalized
sequencer which we are starting to build? Why should I be happy about
(or care about, for that matter) code having moved from one source
file to another?
Rule of thumb for commit messages: after reading a commit message, I
should be able to predict what the patch will do, without reading the
patch.
I am guessing the above description started sane and then went through
a few revisions without a person reading it all the way through again.
Please consider just rewriting it.
[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -1,19 +1,9 @@
> #include "cache.h"
> #include "builtin.h"
> -#include "object.h"
> -#include "commit.h"
> -#include "tag.h"
> -#include "run-command.h"
> -#include "exec_cmd.h"
> -#include "utf8.h"
> #include "parse-options.h"
> -#include "cache-tree.h"
> #include "diff.h"
> #include "revision.h"
> #include "rerere.h"
> -#include "merge-recursive.h"
> -#include "refs.h"
> -#include "dir.h"
> #include "sequencer.h"
Hoorah!
[snipping lots of deletion of code from builtin/revert.c]
> @@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
> opts.action = REPLAY_REVERT;
> git_config(git_default_config, NULL);
> parse_args(argc, argv, &opts);
> - res = pick_revisions(&opts);
> + res = sequencer_pick_revisions(&opts);
The new sequencer_pick_revisions is just a new name for the old
pick_revisions. Sane, but probably worth mentioning in the log
message.
[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1,7 +1,27 @@
> #include "cache.h"
> +#include "object.h"
> +#include "commit.h"
> +#include "tag.h"
> +#include "run-command.h"
> +#include "exec_cmd.h"
> +#include "utf8.h"
> +#include "cache-tree.h"
> +#include "diff.h"
> +#include "revision.h"
> +#include "rerere.h"
> +#include "merge-recursive.h"
> +#include "refs.h"
> -#include "sequencer.h"
> -#include "strbuf.h"
> #include "dir.h"
> +#include "sequencer.h"
Why did sequencer.h move to after dir.h? Wow, we use a lot of headers
here --- I wonder if there are some pieces that could be split out
(that's not due to your patch, though).
[...]
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -8,6 +8,30 @@
> #define SEQ_OPTS_FILE "sequencer/opts"
>
> enum replay_action { REPLAY_REVERT, REPLAY_PICK };
> +enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
> +
> +struct replay_opts {
[...]
> @@ -25,4 +49,6 @@ struct replay_insn_list {
> */
> void remove_sequencer_state(int aggressive);
>
> +int sequencer_pick_revisions(struct replay_opts *opts);
Ah, so this moves most of the logic of "git cherry-pick" to the sequencer
but the only new API that needs to be exposed is pick_revisions(). The
calling sequence looks like this:
memset(&opts, o, sizeof(opts));
opts.action = REPLAY_PICK;
opts.revs = xmalloc(sizeof(*opts.revs));
init_revisions(opts.revs);
add_pending_object / setup_revisions / etc
sequencer_pick_revisions(&opts);
The small exposed interface makes this a relatively uninvasive patch,
and the immediate advantage is that we plan to reuse some of the
functionality used in pick_revisions() in other, new APIs to be used
by commands other than cherry-pick. No functional change yet intended.
Except for the commit message, looks reasonable (though I haven't
tried the "git blame" magic to check the code movement part). Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html