Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C

From: Stefan Beller <sbeller@google.com>
Date: Jun 18, 2018, 22:01 UTC
Message-ID: <CAGZ79kaLSCXSDHN8=L-FCBvQ9jRJqit6VFNK3VmHm9H07ThEsg@mail.gmail.com>
In-Reply-To: <20180618131844.13408-3-alban.gruin@gmail.com>

On Mon, Jun 18, 2018 at 6:19 AM Alban Gruin <alban.gruin@gmail.com> wrote:

Show 44 quoted lines

>
> This rewrites setup_reflog_action() from shell to C.
>
> A new command is added to rebase--helper.c, “setup-reflog”, as such as a
> new flag, “verbose”, to silence the output of the checkout operation
> called by setup_reflog_action().
>
> The shell version is then stripped in favour of a call to the helper. As
> $GIT_REFLOG_ACTION is not longer set at the first call of
> checkout_onto(), a call to comment_for_reflog() is added at the
> beginning of this function.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>  builtin/rebase--helper.c   |  9 +++++++--
>  git-rebase--interactive.sh | 16 ++--------------
>  sequencer.c                | 31 +++++++++++++++++++++++++++++++
>  sequencer.h                |  3 +++
>  4 files changed, 43 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index d2990b210..d677fb663 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
>  int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>  {
>         struct replay_opts opts = REPLAY_OPTS_INIT;
> -       unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> +       unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
>         int abbreviate_commands = 0, rebase_cousins = -1;
>         enum {
>                 CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
>                 CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> -               ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
> +               ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG
>         } command = 0;
>         struct option options[] = {
>                 OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
>                 OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
>                 OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
>                          N_("keep original branch points of cousins")),
> +               OPT_BOOL(0, "verbose", &verbose, N_("verbose")),

verbose is quite a popular flag name, such that the option parsing dedicated it its own macro OPT__VERBOSE.

Show 6 quoted lines

> +int setup_reflog_action(struct replay_opts *opts, const char *commit,
> +                       int verbose)
> +{
> +       const char *action;
> +
> +       if (commit && *commit) {

While this is defensive programming (checking the pointer before dereferencing it, the first condition (commit == NULL) should never be false here, as the caller checks for argc == 2 ? Maybe we could move the logic of the whole condition there

       if (command == SETUP_REFLOG && argc == 2 && *argv[1])
               return !!setup_reflog_action(&opts, argv[1], verbose);

as then we could loose the outer conditional here.