On 18/06/18 14:18, Alban Gruin wrote:
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().

I'm having difficulty understanding what that means, surely the verbose flag is to stop the output from being silenced.

I'm not that keen on the naming in this patch, if it's only a staging post and will be removed it probably doesn't matter too much. I can see why you've based the function and flag names on the shell version, but the C version is not setting up the reflog it is checking out the branch we're rebasing. --checkout-base or something like that would be clearer.

Also the name of the function that does the checkout is tied to checking out the base revision, but then reused in the next patch to checkout the 'onto' commit. As such I think it would be clearer if it was called run_git_checkout() or something like that.

One further thought - how easy would it be to refactor the code in do_reset() to handle the checkouts in this patch series, rather than having to fork 'git checkout'

Best wishes

Phillip

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.gr...@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")),
                OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
                                CONTINUE),
                OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
                OPT_CMDMODE(0, "edit-todo", &command,
                            N_("edit the todo list during an interactive 
rebase"),
                            EDIT_TODO),
+               OPT_CMDMODE(0, "setup-reflog", &command,
+                           N_("setup the reflog action"), SETUP_REFLOG),
                OPT_END()
        };
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
                return !!append_todo_help(0, keep_empty);
        if (command == EDIT_TODO && argc == 1)
                return !!edit_todo_list(flags);
+       if (command == SETUP_REFLOG && argc == 2)
+               return !!setup_reflog_action(&opts, argv[1], verbose);
        usage_with_options(builtin_rebase_helper_usage, options);
  }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..048bbf041 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
# Switch to the branch in $into and notify it in the reflog
  checkout_onto () {
+       comment_for_reflog start
        GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
        output git checkout $onto || die_abort "$(gettext "could not detach 
HEAD")"
        git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
        esac
  }
-setup_reflog_action () {
-       comment_for_reflog start
-
-       if test ! -z "$switch_to"
-       then
-               GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-               output git checkout "$switch_to" -- ||
-                       die "$(eval_gettext "Could not checkout \$switch_to")"
-
-               comment_for_reflog start
-       fi
-}
-
  init_basic_state () {
        orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
        mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary 
\$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
                return 0
        fi
- setup_reflog_action
+       git rebase--helper --setup-reflog "$switch_to" ${verbose:+--verbose}
        init_basic_state
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 3437673d2..4bfe29c7b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3131,6 +3131,37 @@ static const char *reflog_message(struct replay_opts 
*opts,
        return buf.buf;
  }
+static int checkout_base_commit(struct replay_opts *opts, const char *commit,
+                               int verbose, const char *action)
+{
+       struct child_process cmd = CHILD_PROCESS_INIT;
+
+       cmd.git_cmd = 1;
+
+       argv_array_push(&cmd.args, "checkout");
+       argv_array_push(&cmd.args, commit);
+       argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+       if (verbose)
+               return run_command(&cmd);
+       else
+               return run_command_silent_on_success(&cmd);
+}
+
+int setup_reflog_action(struct replay_opts *opts, const char *commit,
+                       int verbose)
+{
+       const char *action;
+
+       if (commit && *commit) {
+               action = reflog_message(opts, "start", "checkout %s", commit);
+               if (checkout_base_commit(opts, commit, verbose, action))
+                       die(_("Could not checkout %s"), commit);
+       }
+
+       return 0;
+}
+
  static const char rescheduled_advice[] =
  N_("Could not execute the todo command\n"
  "\n"
diff --git a/sequencer.h b/sequencer.h
index 35730b13e..55e4057d8 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
  void commit_post_rewrite(const struct commit *current_head,
                         const struct object_id *new_head);
+int setup_reflog_action(struct replay_opts *opts, const char *commit,
+                       int verbose);
+
  #define SUMMARY_INITIAL_COMMIT   (1 << 0)
  #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
  void print_commit_summary(const char *prefix, const struct object_id *oid,


Reply via email to