Re: [PATCH 11/22] sequencer: get rid of the subcommand field

[Johannes Schindelin]

Hi Kuba,

On Wed, 31 Aug 2016, Jakub Narębski wrote:

> W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
> 
> > While at it, ensure that the subcommands return an error code so that
> > they do not have to die() all over the place (bad practice for library
> > functions...).
> 
> This perhaps should be moved to a separate patch, but I guess
> there is a reason behind "while at it".

Yes. It seemed like the logical thing to do: I already introduce a new
function, why should I shlep over a paradigm I do not want in the end?

> Also subcommand functions no longer are local to sequencer.c

They never were. All you had to do was to set a field and run the global
function.

The real problem there was that the different local functions needed
different parameters, and the round-about way to set those parameters as
fields in a struct and then call a global function with that struct just
makes it impossible to have compile-time safety.

Ciao,
Dscho

Re: [PATCH 11/22] sequencer: get rid of the subcommand field

[Jakub Narębski]

W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:

> The subcommands are used exactly once, at the very beginning of
> sequencer_pick_revisions(), to determine what to do. This is an
> unnecessary level of indirection: we can simply call the correct
> function to begin with. So let's do that.

Looks good.  Parsing is moved from parse_args(), now unnecessary,
to the new run_sequencer().  Which also picked up dispatch from
sequencer_pick_revisions() - that sometimes didn't pick revisions :-o.

"All problems in computer science can be solved by another level
 of indirection, except of course for the problem of too many
 indirections." -- David John Wheeler

> 
> While at it, ensure that the subcommands return an error code so that
> they do not have to die() all over the place (bad practice for library
> functions...).

This perhaps should be moved to a separate patch, but I guess
there is a reason behind "while at it".

Also subcommand functions no longer are local to sequencer.c

> 
> Signed-off-by: Johannes Schindelin 
> ---
>  builtin/revert.c | 36 
>  sequencer.c  | 35 +++
>  sequencer.h  | 13 -
>  3 files changed, 31 insertions(+), 53 deletions(-)

Nice size reduction.

[PATCH 11/22] sequencer: get rid of the subcommand field

[Johannes Schindelin]

The subcommands are used exactly once, at the very beginning of
sequencer_pick_revisions(), to determine what to do. This is an
unnecessary level of indirection: we can simply call the correct
function to begin with. So let's do that.

While at it, ensure that the subcommands return an error code so that
they do not have to die() all over the place (bad practice for library
functions...).

Signed-off-by: Johannes Schindelin 
---
 builtin/revert.c | 36 
 sequencer.c  | 35 +++
 sequencer.h  | 13 -
 3 files changed, 31 insertions(+), 53 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7365559..c9ae4dc 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -71,7 +71,7 @@ static void verify_opt_compatible(const char *me, const char 
*base_opt, ...)
die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
 }
 
-static void parse_args(int argc, const char **argv, struct replay_opts *opts)
+static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
 {
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
@@ -115,25 +115,15 @@ static void parse_args(int argc, const char **argv, 
struct replay_opts *opts)
if (opts->keep_redundant_commits)
opts->allow_empty = 1;
 
-   /* Set the subcommand */
-   if (cmd == 'q')
-   opts->subcommand = REPLAY_REMOVE_STATE;
-   else if (cmd == 'c')
-   opts->subcommand = REPLAY_CONTINUE;
-   else if (cmd == 'a')
-   opts->subcommand = REPLAY_ROLLBACK;
-   else
-   opts->subcommand = REPLAY_NONE;
-
/* Check for incompatible command line arguments */
-   if (opts->subcommand != REPLAY_NONE) {
+   if (cmd) {
char *this_operation;
-   if (opts->subcommand == REPLAY_REMOVE_STATE)
+   if (cmd == 'q')
this_operation = "--quit";
-   else if (opts->subcommand == REPLAY_CONTINUE)
+   else if (cmd == 'c')
this_operation = "--continue";
else {
-   assert(opts->subcommand == REPLAY_ROLLBACK);
+   assert(cmd == 'a');
this_operation = "--abort";
}
 
@@ -156,7 +146,7 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
"--edit", opts->edit,
NULL);
 
-   if (opts->subcommand != REPLAY_NONE) {
+   if (cmd) {
opts->revs = NULL;
} else {
struct setup_revision_opt s_r_opt;
@@ -174,6 +164,14 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
 
if (argc > 1)
usage_with_options(usage_str, options);
+
+   if (cmd == 'q')
+   return sequencer_remove_state(opts);
+   if (cmd == 'c')
+   return sequencer_continue(opts);
+   if (cmd == 'a')
+   return sequencer_rollback(opts);
+   return sequencer_pick_revisions(opts);
 }
 
 int cmd_revert(int argc, const char **argv, const char *prefix)
@@ -185,8 +183,7 @@ int cmd_revert(int argc, const char **argv, const char 
*prefix)
opts.edit = 1;
opts.action = REPLAY_REVERT;
git_config(git_default_config, NULL);
-   parse_args(argc, argv, );
-   res = sequencer_pick_revisions();
+   res = run_sequencer(argc, argv, );
if (res < 0)
die(_("revert failed"));
return res;
@@ -199,8 +196,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char 
*prefix)
 
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
-   parse_args(argc, argv, );
-   res = sequencer_pick_revisions();
+   res = run_sequencer(argc, argv, );
if (res < 0)
die(_("cherry-pick failed"));
return res;
diff --git a/sequencer.c b/sequencer.c
index 1b65202..ba1fd05 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -127,7 +127,7 @@ void *sequencer_entrust(struct replay_opts *opts, void 
*set_me_free_after_use)
return set_me_free_after_use;
 }
 
-static void remove_sequencer_state(const struct replay_opts *opts)
+int sequencer_remove_state(struct replay_opts *opts)
 {
struct strbuf dir = STRBUF_INIT;
int i;
@@ -141,6 +141,8 @@ static void remove_sequencer_state(const struct replay_opts 
*opts)
strbuf_addf(, "%s", get_dir(opts));
remove_dir_recursively(, 0);
strbuf_release();
+
+   return 0;
 }
 
 static const char *action_name(const struct replay_opts *opts)
@@ -941,7 +943,7 @@ static int rollback_single_pick(void)
return reset_for_rollback(head_sha1);
 }
 
-static int sequencer_rollback(struct replay_opts *opts)