{"thread":{"id":"48730","subject":"[GSoC][PATCH 0/3] rebase -i: rewrite reflog operations in C","startedAt":"2018-06-18T13:19:12Z","lastAt":"2018-07-03T18:15:08Z","messageCount":51,"participants":["Alban Gruin","Phillip Wood","Christian Couder","Stefan Beller","Johannes Schindelin","Junio C Hamano","Pratik Karki"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"350378","messageId":"20180618131844.13408-1-alban.gruin@gmail.com","threadId":"48730","inReplyTo":null,"subject":"[GSoC][PATCH 0/3] rebase -i: rewrite reflog operations in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-18T13:18:41Z","receivedAt":"2018-06-18T13:19:12Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This patch series rewrites the reflog operations from shell to C. This\nis part of the effort to rewrite interactive rebase in C.\n\nThe first commit is dedicated to creating a function to silence a\ncommand, as the sequencer will do in several places with these patches.\n\nThis branch is based on ag/rebase-i-rewrite-todo, and does not conflict\nwith pu (as of 2018-06-18).\n\nAlban Gruin (3):\n sequencer: add a new function to silence a command, except if it\n fails.\n rebase -i: rewrite setup_reflog_action() in C\n rebase -i: rewrite checkout_onto() in C\n\n builtin/rebase--helper.c | 14 +++++-\n git-rebase--interactive.sh | 39 +++--------------\n sequencer.c | 103 +++++++++++++++++++++++++++++++++------------\n sequencer.h | 6 +++\n 4 files changed, 100 insertions(+), 62 deletions(-)\n\n-- \n2.16.4\n\n"},{"id":"350379","messageId":"20180618131844.13408-2-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180618131844.13408-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-18T13:18:42Z","receivedAt":"2018-06-18T13:19:14Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This adds a new function, run_command_silent_if_successful(), to\nredirect the stdout and stderr of a command to a strbuf, and then to run\nthat command. This strbuf is printed only if the command fails. It is\nfunctionnaly similar to output() from git-rebase.sh.\n\nrun_git_commit() is then refactored to use of\nrun_command_silent_if_successful().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 53 +++++++++++++++++++++++++++--------------------------\n 1 file changed, 27 insertions(+), 26 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7cc76332e..3437673d2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -766,6 +766,25 @@ N_(\"you have staged changes in your working tree\\n\"\n #define VERIFY_MSG (1<<4)\n #define CREATE_ROOT_COMMIT (1<<5)\n \n+static int run_command_silent_on_success(struct child_process *cmd)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint rc;\n+\n+\t/* hide stderr on success */\n+\tcmd->stdout_to_stderr = 1;\n+\trc = pipe_command(cmd,\n+\t\t\t NULL, 0,\n+\t\t\t /* stdout is already redirected */\n+\t\t\t NULL, 0,\n+\t\t\t &buf, 0);\n+\n+\tif (rc)\n+\t\tfputs(buf.buf, stderr);\n+\tstrbuf_release(&buf);\n+\treturn rc;\n+}\n+\n /*\n * If we are cherry-pick, and if the merge did not result in\n * hand-editing, we will hit this commit and inherit the original\n@@ -820,18 +839,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \n \tcmd.git_cmd = 1;\n \n-\tif (is_rebase_i(opts)) {\n-\t\tif (!(flags & EDIT_MSG)) {\n-\t\t\tcmd.stdout_to_stderr = 1;\n-\t\t\tcmd.err = -1;\n-\t\t}\n+\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n+\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n \n-\t\tif (read_env_script(&cmd.env_array)) {\n-\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n-\n-\t\t\treturn error(_(staged_changes_advice),\n-\t\t\t\t gpg_opt, gpg_opt);\n-\t\t}\n+\t\treturn error(_(staged_changes_advice),\n+\t\t\t gpg_opt, gpg_opt);\n \t}\n \n \targv_array_push(&cmd.args, \"commit\");\n@@ -861,21 +873,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tif (opts->allow_empty_message)\n \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n \n-\tif (cmd.err == -1) {\n-\t\t/* hide stderr on success */\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint rc = pipe_command(&cmd,\n-\t\t\t\t NULL, 0,\n-\t\t\t\t /* stdout is already redirected */\n-\t\t\t\t NULL, 0,\n-\t\t\t\t &buf, 0);\n-\t\tif (rc)\n-\t\t\tfputs(buf.buf, stderr);\n-\t\tstrbuf_release(&buf);\n-\t\treturn rc;\n-\t}\n-\n-\treturn run_command(&cmd);\n+\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n+\t\treturn run_command_silent_on_success(&cmd);\n+\telse\n+\t\treturn run_command(&cmd);\n }\n \n static int rest_is_empty(const struct strbuf *sb, int start)\n-- \n2.16.4\n\n"},{"id":"350380","messageId":"20180618131844.13408-3-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180618131844.13408-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-18T13:18:43Z","receivedAt":"2018-06-18T13:19:18Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites setup_reflog_action() from shell to C.\n\nA new command is added to rebase--helper.c, “setup-reflog”, as such as a\nnew flag, “verbose”, to silence the output of the checkout operation\ncalled by setup_reflog_action().\n\nThe shell version is then stripped in favour of a call to the helper. As\n$GIT_REFLOG_ACTION is not longer set at the first call of\ncheckout_onto(), a call to comment_for_reflog() is added at the\nbeginning of this function.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 9 +++++++--\n git-rebase--interactive.sh | 16 ++--------------\n sequencer.c | 31 +++++++++++++++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 43 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex d2990b210..d677fb663 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {\n int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n-\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0;\n+\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;\n \tint abbreviate_commands = 0, rebase_cousins = -1;\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"rebase-merges\", &rebase_merges, N_(\"rebase merge commits\")),\n \t\tOPT_BOOL(0, \"rebase-cousins\", &rebase_cousins,\n \t\t\t N_(\"keep original branch points of cousins\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"verbose\")),\n \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n \t\t\t\tCONTINUE),\n \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\n@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"edit-todo\", &command,\n \t\t\t N_(\"edit the todo list during an interactive rebase\"),\n \t\t\t EDIT_TODO),\n+\t\tOPT_CMDMODE(0, \"setup-reflog\", &command,\n+\t\t\t N_(\"setup the reflog action\"), SETUP_REFLOG),\n \t\tOPT_END()\n \t};\n \n@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!append_todo_help(0, keep_empty);\n \tif (command == EDIT_TODO && argc == 1)\n \t\treturn !!edit_todo_list(flags);\n+\tif (command == SETUP_REFLOG && argc == 2)\n+\t\treturn !!setup_reflog_action(&opts, argv[1], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2defe607f..048bbf041 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -72,6 +72,7 @@ collapse_todo_ids() {\n \n # Switch to the branch in $into and notify it in the reflog\n checkout_onto () {\n+\tcomment_for_reflog start\n \tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n \toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n \tgit update-ref ORIG_HEAD $orig_head\n@@ -119,19 +120,6 @@ initiate_action () {\n \tesac\n }\n \n-setup_reflog_action () {\n-\tcomment_for_reflog start\n-\n-\tif test ! -z \"$switch_to\"\n-\tthen\n-\t\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n-\t\toutput git checkout \"$switch_to\" -- ||\n-\t\t\tdie \"$(eval_gettext \"Could not checkout \\$switch_to\")\"\n-\n-\t\tcomment_for_reflog start\n-\tfi\n-}\n-\n init_basic_state () {\n \torig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n@@ -211,7 +199,7 @@ git_rebase__interactive () {\n \t\treturn 0\n \tfi\n \n-\tsetup_reflog_action\n+\tgit rebase--helper --setup-reflog \"$switch_to\" ${verbose:+--verbose}\n \tinit_basic_state\n \n \tinit_revisions_and_shortrevisions\ndiff --git a/sequencer.c b/sequencer.c\nindex 3437673d2..4bfe29c7b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3131,6 +3131,37 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n+static int checkout_base_commit(struct replay_opts *opts, const char *commit,\n+\t\t\t\tint verbose, const char *action)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tcmd.git_cmd = 1;\n+\n+\targv_array_push(&cmd.args, \"checkout\");\n+\targv_array_push(&cmd.args, commit);\n+\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n+\n+\tif (verbose)\n+\t\treturn run_command(&cmd);\n+\telse\n+\t\treturn run_command_silent_on_success(&cmd);\n+}\n+\n+int setup_reflog_action(struct replay_opts *opts, const char *commit,\n+\t\t\tint verbose)\n+{\n+\tconst char *action;\n+\n+\tif (commit && *commit) {\n+\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n+\t\tif (checkout_base_commit(opts, commit, verbose, action))\n+\t\t\tdie(_(\"Could not checkout %s\"), commit);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 35730b13e..55e4057d8 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,\n void commit_post_rewrite(const struct commit *current_head,\n \t\t\t const struct object_id *new_head);\n \n+int setup_reflog_action(struct replay_opts *opts, const char *commit,\n+\t\t\tint verbose);\n+\n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n void print_commit_summary(const char *prefix, const struct object_id *oid,\n-- \n2.16.4\n\n"},{"id":"350381","messageId":"20180618131844.13408-4-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180618131844.13408-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-18T13:18:44Z","receivedAt":"2018-06-18T13:19:23Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites checkout_onto() from shell to C.\n\nA new command (“checkout-onto”) is added to rebase--helper.c. The shell\nversion is then stripped.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 7 ++++++-\n git-rebase--interactive.sh | 25 ++++---------------------\n sequencer.c | 19 +++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex d677fb663..f9fffba96 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG,\n+\t\tCHECKOUT_ONTO\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\t EDIT_TODO),\n \t\tOPT_CMDMODE(0, \"setup-reflog\", &command,\n \t\t\t N_(\"setup the reflog action\"), SETUP_REFLOG),\n+\t\tOPT_CMDMODE(0, \"checkout-onto\", &command,\n+\t\t\t N_(\"checkout a commit\"), CHECKOUT_ONTO),\n \t\tOPT_END()\n \t};\n \n@@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!edit_todo_list(flags);\n \tif (command == SETUP_REFLOG && argc == 2)\n \t\treturn !!setup_reflog_action(&opts, argv[1], verbose);\n+\tif (command == CHECKOUT_ONTO && argc == 4)\n+\t\treturn !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 048bbf041..0ae053291 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -28,17 +28,6 @@ case \"$comment_char\" in\n \t;;\n esac\n \n-orig_reflog_action=\"$GIT_REFLOG_ACTION\"\n-\n-comment_for_reflog () {\n-\tcase \"$orig_reflog_action\" in\n-\t''|rebase*)\n-\t\tGIT_REFLOG_ACTION=\"rebase -i ($1)\"\n-\t\texport GIT_REFLOG_ACTION\n-\t\t;;\n-\tesac\n-}\n-\n die_abort () {\n \tapply_autostash\n \trm -rf \"$state_dir\"\n@@ -70,14 +59,6 @@ collapse_todo_ids() {\n \tgit rebase--helper --shorten-ids\n }\n \n-# Switch to the branch in $into and notify it in the reflog\n-checkout_onto () {\n-\tcomment_for_reflog start\n-\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n-\toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n-\tgit update-ref ORIG_HEAD $orig_head\n-}\n-\n get_missing_commit_check_level () {\n \tcheck_level=$(git config --get rebase.missingCommitsCheck)\n \tcheck_level=${check_level:-ignore}\n@@ -176,7 +157,8 @@ EOF\n \n \tgit rebase--helper --check-todo-list || {\n \t\tret=$?\n-\t\tcheckout_onto\n+\t\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \\\n+\t\t \"$orig_head\" ${verbose:+--verbose}\n \t\texit $ret\n \t}\n \n@@ -186,7 +168,8 @@ EOF\n \tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n \tdie \"Could not skip unnecessary pick commands\"\n \n-\tcheckout_onto\n+\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \"$orig_head\" \\\n+\t ${verbose:+--verbose}\n \trequire_clean_work_tree \"rebase\"\n \texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n \t --continue\ndiff --git a/sequencer.c b/sequencer.c\nindex 4bfe29c7b..d149cbf92 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3162,6 +3162,25 @@ int setup_reflog_action(struct replay_opts *opts, const char *commit,\n \treturn 0;\n }\n \n+int checkout_onto(struct replay_opts *opts,\n+\t\t const char *onto_name, const char *onto,\n+\t\t const char *orig_head, unsigned verbose)\n+{\n+\tstruct object_id oid;\n+\tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n+\n+\tif (get_oid(orig_head, &oid))\n+\t\tdie(_(\"%s: not a valid OID\"), orig_head);\n+\n+\tif (checkout_base_commit(opts, onto, verbose, action)) {\n+\t\tapply_autostash(opts);\n+\t\tsequencer_remove_state(opts);\n+\t\tdie(_(\"could not detach HEAD\"));\n+\t}\n+\n+\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 55e4057d8..9899d90fc 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -102,6 +102,9 @@ void commit_post_rewrite(const struct commit *current_head,\n \n int setup_reflog_action(struct replay_opts *opts, const char *commit,\n \t\t\tint verbose);\n+int checkout_onto(struct replay_opts *opts,\n+\t\t const char *onto_name, const char *onto,\n+\t\t const char *orig_head, unsigned verbose);\n \n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n-- \n2.16.4\n\n"},{"id":"350387","messageId":"b23a9e30-58bf-afde-4d77-563e0b288e70@talktalk.net","threadId":"48730","inReplyTo":"20180618131844.13408-2-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-06-18T15:26:34Z","receivedAt":"2018-06-18T15:26:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alban\n\nOn 18/06/18 14:18, Alban Gruin wrote:\n> This adds a new function, run_command_silent_if_successful(), to\n> redirect the stdout and stderr of a command to a strbuf, and then to run\n> that command. This strbuf is printed only if the command fails. It is\n> functionnaly similar to output() from git-rebase.sh.\n\ns/functionnaly/functionally/\n\nIt's not quite the same though because the shell versions handles \n--verbose where as here the caller has to put that check in every call \nsite. I wonder if it would simplify the callers if the C version did the \n--verbose handling it's self.\n\n\nBest Wishes\n\nPhillip\n> run_git_commit() is then refactored to use of\n> run_command_silent_if_successful().\n> \n> Signed-off-by: Alban Gruin \n> ---\n> sequencer.c | 53 +++++++++++++++++++++++++++--------------------------\n> 1 file changed, 27 insertions(+), 26 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 7cc76332e..3437673d2 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -766,6 +766,25 @@ N_(\"you have staged changes in your working tree\\n\"\n> #define VERIFY_MSG (1<<4)\n> #define CREATE_ROOT_COMMIT (1<<5)\n> \n> +static int run_command_silent_on_success(struct child_process *cmd)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint rc;\n> +\n> +\t/* hide stderr on success */\n> +\tcmd->stdout_to_stderr = 1;\n> +\trc = pipe_command(cmd,\n> +\t\t\t NULL, 0,\n> +\t\t\t /* stdout is already redirected */\n> +\t\t\t NULL, 0,\n> +\t\t\t &buf, 0);\n> +\n> +\tif (rc)\n> +\t\tfputs(buf.buf, stderr);\n> +\tstrbuf_release(&buf);\n> +\treturn rc;\n> +}\n> +\n> /*\n> * If we are cherry-pick, and if the merge did not result in\n> * hand-editing, we will hit this commit and inherit the original\n> @@ -820,18 +839,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> \n> \tcmd.git_cmd = 1;\n> \n> -\tif (is_rebase_i(opts)) {\n> -\t\tif (!(flags & EDIT_MSG)) {\n> -\t\t\tcmd.stdout_to_stderr = 1;\n> -\t\t\tcmd.err = -1;\n> -\t\t}\n> +\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n> +\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n> \n> -\t\tif (read_env_script(&cmd.env_array)) {\n> -\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n> -\n> -\t\t\treturn error(_(staged_changes_advice),\n> -\t\t\t\t gpg_opt, gpg_opt);\n> -\t\t}\n> +\t\treturn error(_(staged_changes_advice),\n> +\t\t\t gpg_opt, gpg_opt);\n> \t}\n> \n> \targv_array_push(&cmd.args, \"commit\");\n> @@ -861,21 +873,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> \tif (opts->allow_empty_message)\n> \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n> \n> -\tif (cmd.err == -1) {\n> -\t\t/* hide stderr on success */\n> -\t\tstruct strbuf buf = STRBUF_INIT;\n> -\t\tint rc = pipe_command(&cmd,\n> -\t\t\t\t NULL, 0,\n> -\t\t\t\t /* stdout is already redirected */\n> -\t\t\t\t NULL, 0,\n> -\t\t\t\t &buf, 0);\n> -\t\tif (rc)\n> -\t\t\tfputs(buf.buf, stderr);\n> -\t\tstrbuf_release(&buf);\n> -\t\treturn rc;\n> -\t}\n> -\n> -\treturn run_command(&cmd);\n> +\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n> +\t\treturn run_command_silent_on_success(&cmd);\n> +\telse\n> +\t\treturn run_command(&cmd);\n> }\n> \n> static int rest_is_empty(const struct strbuf *sb, int start)\n> \n\n"},{"id":"350388","messageId":"6583e68e-3aeb-3afa-8e13-9391b77e4fe7@talktalk.net","threadId":"48730","inReplyTo":"20180618131844.13408-3-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-06-18T15:34:25Z","receivedAt":"2018-06-18T15:34:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 18/06/18 14:18, Alban Gruin wrote:\n> This rewrites setup_reflog_action() from shell to C.\n> \n> A new command is added to rebase--helper.c, “setup-reflog”, as such as a\n> new flag, “verbose”, to silence the output of the checkout operation\n> called by setup_reflog_action().\n\nI'm having difficulty understanding what that means, surely the verbose \nflag is to stop the output from being silenced.\n\nI'm not that keen on the naming in this patch, if it's only a staging \npost and will be removed it probably doesn't matter too much. I can see \nwhy you've based the function and flag names on the shell version, but \nthe C version is not setting up the reflog it is checking out the branch \nwe're rebasing. --checkout-base or something like that would be clearer.\n\nAlso the name of the function that does the checkout is tied to checking \nout the base revision, but then reused in the next patch to checkout the \n'onto' commit. As such I think it would be clearer if it was called \nrun_git_checkout() or something like that.\n\nOne further thought - how easy would it be to refactor the code in \ndo_reset() to handle the checkouts in this patch series, rather than \nhaving to fork 'git checkout'\n\nBest wishes\n\nPhillip\n\n> The shell version is then stripped in favour of a call to the helper. As\n> $GIT_REFLOG_ACTION is not longer set at the first call of\n> checkout_onto(), a call to comment_for_reflog() is added at the\n> beginning of this function.\n> \n> Signed-off-by: Alban Gruin \n> ---\n> builtin/rebase--helper.c | 9 +++++++--\n> git-rebase--interactive.sh | 16 ++--------------\n> sequencer.c | 31 +++++++++++++++++++++++++++++++\n> sequencer.h | 3 +++\n> 4 files changed, 43 insertions(+), 16 deletions(-)\n> \n> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\n> index d2990b210..d677fb663 100644\n> --- a/builtin/rebase--helper.c\n> +++ b/builtin/rebase--helper.c\n> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {\n> int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n> -\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0;\n> +\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;\n> \tint abbreviate_commands = 0, rebase_cousins = -1;\n> \tenum {\n> \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n> \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n> -\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO\n> +\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG\n> \t} command = 0;\n> \tstruct option options[] = {\n> \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL(0, \"rebase-merges\", &rebase_merges, N_(\"rebase merge commits\")),\n> \t\tOPT_BOOL(0, \"rebase-cousins\", &rebase_cousins,\n> \t\t\t N_(\"keep original branch points of cousins\")),\n> +\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"verbose\")),\n> \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n> \t\t\t\tCONTINUE),\n> \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\n> @@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> \t\tOPT_CMDMODE(0, \"edit-todo\", &command,\n> \t\t\t N_(\"edit the todo list during an interactive rebase\"),\n> \t\t\t EDIT_TODO),\n> +\t\tOPT_CMDMODE(0, \"setup-reflog\", &command,\n> +\t\t\t N_(\"setup the reflog action\"), SETUP_REFLOG),\n> \t\tOPT_END()\n> \t};\n> \n> @@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> \t\treturn !!append_todo_help(0, keep_empty);\n> \tif (command == EDIT_TODO && argc == 1)\n> \t\treturn !!edit_todo_list(flags);\n> +\tif (command == SETUP_REFLOG && argc == 2)\n> +\t\treturn !!setup_reflog_action(&opts, argv[1], verbose);\n> \tusage_with_options(builtin_rebase_helper_usage, options);\n> }\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 2defe607f..048bbf041 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -72,6 +72,7 @@ collapse_todo_ids() {\n> \n> # Switch to the branch in $into and notify it in the reflog\n> checkout_onto () {\n> +\tcomment_for_reflog start\n> \tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n> \toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n> \tgit update-ref ORIG_HEAD $orig_head\n> @@ -119,19 +120,6 @@ initiate_action () {\n> \tesac\n> }\n> \n> -setup_reflog_action () {\n> -\tcomment_for_reflog start\n> -\n> -\tif test ! -z \"$switch_to\"\n> -\tthen\n> -\t\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n> -\t\toutput git checkout \"$switch_to\" -- ||\n> -\t\t\tdie \"$(eval_gettext \"Could not checkout \\$switch_to\")\"\n> -\n> -\t\tcomment_for_reflog start\n> -\tfi\n> -}\n> -\n> init_basic_state () {\n> \torig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n> \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n> @@ -211,7 +199,7 @@ git_rebase__interactive () {\n> \t\treturn 0\n> \tfi\n> \n> -\tsetup_reflog_action\n> +\tgit rebase--helper --setup-reflog \"$switch_to\" ${verbose:+--verbose}\n> \tinit_basic_state\n> \n> \tinit_revisions_and_shortrevisions\n> diff --git a/sequencer.c b/sequencer.c\n> index 3437673d2..4bfe29c7b 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3131,6 +3131,37 @@ static const char *reflog_message(struct replay_opts *opts,\n> \treturn buf.buf;\n> }\n> \n> +static int checkout_base_commit(struct replay_opts *opts, const char *commit,\n> +\t\t\t\tint verbose, const char *action)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> +\tcmd.git_cmd = 1;\n> +\n> +\targv_array_push(&cmd.args, \"checkout\");\n> +\targv_array_push(&cmd.args, commit);\n> +\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> +\n> +\tif (verbose)\n> +\t\treturn run_command(&cmd);\n> +\telse\n> +\t\treturn run_command_silent_on_success(&cmd);\n> +}\n> +\n> +int setup_reflog_action(struct replay_opts *opts, const char *commit,\n> +\t\t\tint verbose)\n> +{\n> +\tconst char *action;\n> +\n> +\tif (commit && *commit) {\n> +\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n> +\t\tif (checkout_base_commit(opts, commit, verbose, action))\n> +\t\t\tdie(_(\"Could not checkout %s\"), commit);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> static const char rescheduled_advice[] =\n> N_(\"Could not execute the todo command\\n\"\n> \"\\n\"\n> diff --git a/sequencer.h b/sequencer.h\n> index 35730b13e..55e4057d8 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,\n> void commit_post_rewrite(const struct commit *current_head,\n> \t\t\t const struct object_id *new_head);\n> \n> +int setup_reflog_action(struct replay_opts *opts, const char *commit,\n> +\t\t\tint verbose);\n> +\n> #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> void print_commit_summary(const char *prefix, const struct object_id *oid,\n> \n\n"},{"id":"350392","messageId":"290d8b7c-3d82-e914-0399-e8dd5a6e1513@talktalk.net","threadId":"48730","inReplyTo":"20180618131844.13408-4-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-06-18T16:09:03Z","receivedAt":"2018-06-18T16:09:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alban\n\nOn 18/06/18 14:18, Alban Gruin wrote:\n> This rewrites checkout_onto() from shell to C.\n> \n> A new command (“checkout-onto”) is added to rebase--helper.c. The shell\n> version is then stripped.\n> \n> Signed-off-by: Alban Gruin \n> ---\n> builtin/rebase--helper.c | 7 ++++++-\n> git-rebase--interactive.sh | 25 ++++---------------------\n> sequencer.c | 19 +++++++++++++++++++\n> sequencer.h | 3 +++\n> 4 files changed, 32 insertions(+), 22 deletions(-)\n> \n> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\n> index d677fb663..f9fffba96 100644\n> --- a/builtin/rebase--helper.c\n> +++ b/builtin/rebase--helper.c\n> @@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> \tenum {\n> \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n> \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n> -\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG\n> +\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG,\n> +\t\tCHECKOUT_ONTO\n> \t} command = 0;\n> \tstruct option options[] = {\n> \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n> @@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> \t\t\t EDIT_TODO),\n> \t\tOPT_CMDMODE(0, \"setup-reflog\", &command,\n> \t\t\t N_(\"setup the reflog action\"), SETUP_REFLOG),\n> +\t\tOPT_CMDMODE(0, \"checkout-onto\", &command,\n> +\t\t\t N_(\"checkout a commit\"), CHECKOUT_ONTO),\n> \t\tOPT_END()\n> \t};\n> \n> @@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> \t\treturn !!edit_todo_list(flags);\n> \tif (command == SETUP_REFLOG && argc == 2)\n> \t\treturn !!setup_reflog_action(&opts, argv[1], verbose);\n> +\tif (command == CHECKOUT_ONTO && argc == 4)\n> +\t\treturn !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);\n> \tusage_with_options(builtin_rebase_helper_usage, options);\n> }\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 048bbf041..0ae053291 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -28,17 +28,6 @@ case \"$comment_char\" in\n> \t;;\n> esac\n> \n> -orig_reflog_action=\"$GIT_REFLOG_ACTION\"\n> -\n> -comment_for_reflog () {\n> -\tcase \"$orig_reflog_action\" in\n> -\t''|rebase*)\n> -\t\tGIT_REFLOG_ACTION=\"rebase -i ($1)\"\n> -\t\texport GIT_REFLOG_ACTION\n> -\t\t;;\n> -\tesac\n> -}\n> -\n> die_abort () {\n> \tapply_autostash\n> \trm -rf \"$state_dir\"\n> @@ -70,14 +59,6 @@ collapse_todo_ids() {\n> \tgit rebase--helper --shorten-ids\n> }\n> \n> -# Switch to the branch in $into and notify it in the reflog\n> -checkout_onto () {\n> -\tcomment_for_reflog start\n> -\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n> -\toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n> -\tgit update-ref ORIG_HEAD $orig_head\n> -}\n> -\n> get_missing_commit_check_level () {\n> \tcheck_level=$(git config --get rebase.missingCommitsCheck)\n> \tcheck_level=${check_level:-ignore}\n> @@ -176,7 +157,8 @@ EOF\n> \n> \tgit rebase--helper --check-todo-list || {\n> \t\tret=$?\n> -\t\tcheckout_onto\n> +\t\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \\\n> +\t\t \"$orig_head\" ${verbose:+--verbose}\n> \t\texit $ret\n> \t}\n> \n> @@ -186,7 +168,8 @@ EOF\n> \tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n> \tdie \"Could not skip unnecessary pick commands\"\n> \n> -\tcheckout_onto\n> +\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \"$orig_head\" \\\n> +\t ${verbose:+--verbose}\n> \trequire_clean_work_tree \"rebase\"\n> \texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n> \t --continue\n> diff --git a/sequencer.c b/sequencer.c\n> index 4bfe29c7b..d149cbf92 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3162,6 +3162,25 @@ int setup_reflog_action(struct replay_opts *opts, const char *commit,\n> \treturn 0;\n> }\n> \n> +int checkout_onto(struct replay_opts *opts,\n> +\t\t const char *onto_name, const char *onto,\n> +\t\t const char *orig_head, unsigned verbose)\n> +{\n> +\tstruct object_id oid;\n> +\tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n> +\n> +\tif (get_oid(orig_head, &oid))\n> +\t\tdie(_(\"%s: not a valid OID\"), orig_head);\n\nIf this code is going to live long-term in sequencer.c then it would be \nbetter not to die, but return an error instead as it's part of libgit.\n\nBest Wishes\n\nPhillip\n\n> +\n> +\tif (checkout_base_commit(opts, onto, verbose, action)) {\n> +\t\tapply_autostash(opts);\n> +\t\tsequencer_remove_state(opts);\n> +\t\tdie(_(\"could not detach HEAD\"));\n> +\t}\n> +\n> +\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n> +}\n> +\n> static const char rescheduled_advice[] =\n> N_(\"Could not execute the todo command\\n\"\n> \"\\n\"\n> diff --git a/sequencer.h b/sequencer.h\n> index 55e4057d8..9899d90fc 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -102,6 +102,9 @@ void commit_post_rewrite(const struct commit *current_head,\n> \n> int setup_reflog_action(struct replay_opts *opts, const char *commit,\n> \t\t\tint verbose);\n> +int checkout_onto(struct replay_opts *opts,\n> +\t\t const char *onto_name, const char *onto,\n> +\t\t const char *orig_head, unsigned verbose);\n> \n> #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> \n\n"},{"id":"350395","messageId":"CAP8UFD1PirEpeVwR=Zr4NAccrt_zGxjZkbDrjXNbQRp49zcjtg@mail.gmail.com","threadId":"48730","inReplyTo":"20180618131844.13408-2-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-06-18T16:26:23Z","receivedAt":"2018-06-18T16:26:27Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Alban,\n\nOn Mon, Jun 18, 2018 at 3:18 PM, Alban Gruin wrote:\n> This adds a new function, run_command_silent_if_successful(),\n\nHe re the function is called run_command_silent_if_successful()...\n\n> to\n> redirect the stdout and stderr of a command to a strbuf, and then to run\n> that command. This strbuf is printed only if the command fails. It is\n> functionnaly similar to output() from git-rebase.sh.\n>\n> run_git_commit() is then refactored to use of\n> run_command_silent_if_successful().\n\n...here also...\n\n[...]\n\n> +static int run_command_silent_on_success(struct child_process *cmd)\n\n...but here it is called run_command_silent_on_success().\n\nThanks,\nChristian.\n"},{"id":"350397","messageId":"deae16f0-8852-4b98-dc14-705d8456f219@gmail.com","threadId":"48730","inReplyTo":"b23a9e30-58bf-afde-4d77-563e0b288e70@talktalk.net","subject":"Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-18T16:46:56Z","receivedAt":"2018-06-18T16:47:05Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Phillip,\n\nLe 18/06/2018 à 17:26, Phillip Wood a écrit :\n> Hi Alban\n> \n> On 18/06/18 14:18, Alban Gruin wrote:\n>> This adds a new function, run_command_silent_if_successful(), to\n>> redirect the stdout and stderr of a command to a strbuf, and then to run\n>> that command. This strbuf is printed only if the command fails. It is\n>> functionnaly similar to output() from git-rebase.sh.\n> \n> s/functionnaly/functionally/\n> \n> It's not quite the same though because the shell versions handles\n> --verbose where as here the caller has to put that check in every call\n> site. I wonder if it would simplify the callers if the C version did the\n> --verbose handling it's self.\n> \n\nThat’s what I did first, but removed it because I thought it would be\nless confusing. I’m fine with this solution, though. Do you want me to\ndo this instead?\n\nCheers,\nAlban\n\n"},{"id":"350399","messageId":"f7320b16-a543-11c1-d511-fd95ec7a7f8d@gmail.com","threadId":"48730","inReplyTo":"6583e68e-3aeb-3afa-8e13-9391b77e4fe7@talktalk.net","subject":"Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-18T17:04:27Z","receivedAt":"2018-06-18T17:04:33Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Phillip,\n\nLe 18/06/2018 à 17:34, Phillip Wood a écrit :\n> On 18/06/18 14:18, Alban Gruin wrote:\n>> This rewrites setup_reflog_action() from shell to C.\n>>\n>> A new command is added to rebase--helper.c, “setup-reflog”, as such as a\n>> new flag, “verbose”, to silence the output of the checkout operation\n>> called by setup_reflog_action().\n> \n> I'm having difficulty understanding what that means, surely the verbose\n> flag is to stop the output from being silenced.\n> \n\nI reversed the meaning of the flag, I will correct it in a reroll.\n\n> I'm not that keen on the naming in this patch, if it's only a staging\n> post and will be removed it probably doesn't matter too much. I can see\n> why you've based the function and flag names on the shell version, but\n> the C version is not setting up the reflog it is checking out the branch\n> we're rebasing. --checkout-base or something like that would be clearer.\n> \n> Also the name of the function that does the checkout is tied to checking\n> out the base revision, but then reused in the next patch to checkout the\n> 'onto' commit. As such I think it would be clearer if it was called\n> run_git_checkout() or something like that.\n> \n\nRight.\n\n> One further thought - how easy would it be to refactor the code in\n> do_reset() to handle the checkouts in this patch series, rather than\n> having to fork 'git checkout'\n> \n\nGood remark, I did not notice do_reset(). I intend to refactor and\noptimize after I have done (most of) the conversion, so this goes into\nmy todo-list, but it does not really seem difficult at first sight.\n\nCheers,\nAlban\n\n"},{"id":"350400","messageId":"3de6062e-d3cf-c85b-3663-31930b26e6e0@gmail.com","threadId":"48730","inReplyTo":"290d8b7c-3d82-e914-0399-e8dd5a6e1513@talktalk.net","subject":"Re: [GSoC][PATCH 3/3] rebase -i: rewrite checkout_onto() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-18T17:04:33Z","receivedAt":"2018-06-18T17:04:39Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Phillip,\n\nLe 18/06/2018 à 18:09, Phillip Wood a écrit :\n>> +    if (get_oid(orig_head, &oid))\n>> +        die(_(\"%s: not a valid OID\"), orig_head);\n> \n> If this code is going to live long-term in sequencer.c then it would be\n> better not to die, but return an error instead as it's part of libgit.\n> \n\nRight, I will fix this.\n\nCheers,\nAlban\n\n"},{"id":"350401","messageId":"17075b2c-93d5-1956-01e5-8e2998327101@gmail.com","threadId":"48730","inReplyTo":"CAP8UFD1PirEpeVwR=Zr4NAccrt_zGxjZkbDrjXNbQRp49zcjtg@mail.gmail.com","subject":"Re: [GSoC][PATCH 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-18T17:05:39Z","receivedAt":"2018-06-18T17:05:44Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Christian,\n\nLe 18/06/2018 à 18:26, Christian Couder a écrit :\n> Hi Alban,\n> \n> On Mon, Jun 18, 2018 at 3:18 PM, Alban Gruin wrote:\n>> This adds a new function, run_command_silent_if_successful(),\n> \n> He re the function is called run_command_silent_if_successful()...\n> \n>> to\n>> redirect the stdout and stderr of a command to a strbuf, and then to run\n>> that command. This strbuf is printed only if the command fails. It is\n>> functionnaly similar to output() from git-rebase.sh.\n>>\n>> run_git_commit() is then refactored to use of\n>> run_command_silent_if_successful().\n> \n> ...here also...\n> \n> [...]\n> \n>> +static int run_command_silent_on_success(struct child_process *cmd)\n> \n> ...but here it is called run_command_silent_on_success().\n> \n> Thanks,\n> Christian.\n> \n\nOops, my bad. I will fix this in a reroll.\n\nCheers,\nAlban\n\n"},{"id":"350432","messageId":"CAGZ79kaLSCXSDHN8=L-FCBvQ9jRJqit6VFNK3VmHm9H07ThEsg@mail.gmail.com","threadId":"48730","inReplyTo":"20180618131844.13408-3-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-18T22:01:11Z","receivedAt":"2018-06-18T22:01:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jun 18, 2018 at 6:19 AM Alban Gruin wrote:\n>\n> This rewrites setup_reflog_action() from shell to C.\n>\n> A new command is added to rebase--helper.c, “setup-reflog”, as such as a\n> new flag, “verbose”, to silence the output of the checkout operation\n> called by setup_reflog_action().\n>\n> The shell version is then stripped in favour of a call to the helper. As\n> $GIT_REFLOG_ACTION is not longer set at the first call of\n> checkout_onto(), a call to comment_for_reflog() is added at the\n> beginning of this function.\n>\n> Signed-off-by: Alban Gruin \n> ---\n> builtin/rebase--helper.c | 9 +++++++--\n> git-rebase--interactive.sh | 16 ++--------------\n> sequencer.c | 31 +++++++++++++++++++++++++++++++\n> sequencer.h | 3 +++\n> 4 files changed, 43 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\n> index d2990b210..d677fb663 100644\n> --- a/builtin/rebase--helper.c\n> +++ b/builtin/rebase--helper.c\n> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {\n> int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> {\n> struct replay_opts opts = REPLAY_OPTS_INIT;\n> - unsigned flags = 0, keep_empty = 0, rebase_merges = 0;\n> + unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;\n> int abbreviate_commands = 0, rebase_cousins = -1;\n> enum {\n> CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n> CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n> - ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO\n> + ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, SETUP_REFLOG\n> } command = 0;\n> struct option options[] = {\n> OPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> OPT_BOOL(0, \"rebase-merges\", &rebase_merges, N_(\"rebase merge commits\")),\n> OPT_BOOL(0, \"rebase-cousins\", &rebase_cousins,\n> N_(\"keep original branch points of cousins\")),\n> + OPT_BOOL(0, \"verbose\", &verbose, N_(\"verbose\")),\n\nverbose is quite a popular flag name, such that the option parsing\ndedicated it its own macro OPT__VERBOSE.\n\n\n> +int setup_reflog_action(struct replay_opts *opts, const char *commit,\n> + int verbose)\n> +{\n> + const char *action;\n> +\n> + if (commit && *commit) {\n\nWhile this is defensive programming (checking the pointer before dereferencing\nit, the first condition (commit == NULL) should never be false here,\nas the caller\nchecks for argc == 2 ? Maybe we could move the logic of the whole\ncondition there\n\n if (command == SETUP_REFLOG && argc == 2 && *argv[1])\n return !!setup_reflog_action(&opts, argv[1], verbose);\n\nas then we could loose the outer conditional here.\n"},{"id":"350452","messageId":"nycvar.QRO.7.76.6.1806190848320.77@tvgsbejvaqbjf.bet","threadId":"48730","inReplyTo":"CAGZ79kaLSCXSDHN8=L-FCBvQ9jRJqit6VFNK3VmHm9H07ThEsg@mail.gmail.com","subject":"Re: [GSoC][PATCH 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-19T06:51:27Z","receivedAt":"2018-06-19T06:52:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Mon, 18 Jun 2018, Stefan Beller wrote:\n\n> On Mon, Jun 18, 2018 at 6:19 AM Alban Gruin wrote:\n> >\n> > +int setup_reflog_action(struct replay_opts *opts, const char *commit,\n> > + int verbose)\n> > +{\n> > + const char *action;\n> > +\n> > + if (commit && *commit) {\n> \n> While this is defensive programming (checking the pointer before dereferencing\n> it, the first condition (commit == NULL) should never be false here,\n> as the caller\n> checks for argc == 2 ?\n\nBut it is not marked as `static` (or is it?). So we should not rely on the\ncaller to Do The Right Thing.\n\nCiao,\nDscho\n"},{"id":"350480","messageId":"20180619154421.14999-1-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180618131844.13408-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-19T15:44:18Z","receivedAt":"2018-06-19T15:45:03Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This patch series rewrites the reflog operations from shell to C. This\nis part of the effort to rewrite interactive rebase in C.\n\nThe first commit is dedicated to creating a function to silence a\ncommand, as the sequencer will do in several places with these patches.\n\nThis branch is based on ag/rebase-i-rewrite-todo, and does not conflict\nwith pu (as of 2018-06-19).\n\nChanges since v1:\n\n - Replacing run_command_silent_if_successful() by\n run_command_silent_on_success() in the first commit’s message (thanks\n Christian!)\n\n - Adding a “verbose” parameter to run_command_silent_on_success()\n (thanks Phillip!)\n\n - Using OPT__VERBOSE to parse the “--verbose” flag (thanks Stefan!)\n\n - Fixing some typos and errors in the commit messages\n\n - Renaming “setup-reflog” to “checkout-base”\n\n - Renaming checkout_base_commit() to run_git_checkout()\n\n - Replacing calls to die() by error()\n\nAlban Gruin (3):\n sequencer: add a new function to silence a command, except if it\n fails.\n rebase -i: rewrite setup_reflog_action() in C\n rebase -i: rewrite checkout_onto() in C\n\n builtin/rebase--helper.c | 14 ++++++-\n git-rebase--interactive.sh | 39 +++--------------\n sequencer.c | 102 +++++++++++++++++++++++++++++++++------------\n sequencer.h | 6 +++\n 4 files changed, 99 insertions(+), 62 deletions(-)\n\n-- \n2.16.4\n\n"},{"id":"350481","messageId":"20180619154421.14999-2-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180619154421.14999-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-19T15:44:19Z","receivedAt":"2018-06-19T15:45:07Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This adds a new function, run_command_silent_on_success(), to redirect\nthe stdout and stderr of a command to a strbuf, and then to run that\ncommand. This strbuf is printed only if the command fails. It also takes\na parameter, “verbose”. When true, the command is executed without\nredirecting its output. It is functionnally similar to output() from\ngit-rebase.sh.\n\nrun_git_commit() is then refactored to use of\nrun_command_silent_on_success().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 55 +++++++++++++++++++++++++++++--------------------------\n 1 file changed, 29 insertions(+), 26 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7cc76332e..9aa7ddb33 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -766,6 +766,29 @@ N_(\"you have staged changes in your working tree\\n\"\n #define VERIFY_MSG (1<<4)\n #define CREATE_ROOT_COMMIT (1<<5)\n \n+static int run_command_silent_on_success(struct child_process *cmd,\n+\t\t\t\t\t unsigned verbose)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint rc;\n+\n+\tif (verbose)\n+\t\treturn run_command(cmd);\n+\n+\t/* hide stderr on success */\n+\tcmd->stdout_to_stderr = 1;\n+\trc = pipe_command(cmd,\n+\t\t\t NULL, 0,\n+\t\t\t /* stdout is already redirected */\n+\t\t\t NULL, 0,\n+\t\t\t &buf, 0);\n+\n+\tif (rc)\n+\t\tfputs(buf.buf, stderr);\n+\tstrbuf_release(&buf);\n+\treturn rc;\n+}\n+\n /*\n * If we are cherry-pick, and if the merge did not result in\n * hand-editing, we will hit this commit and inherit the original\n@@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \n \tcmd.git_cmd = 1;\n \n-\tif (is_rebase_i(opts)) {\n-\t\tif (!(flags & EDIT_MSG)) {\n-\t\t\tcmd.stdout_to_stderr = 1;\n-\t\t\tcmd.err = -1;\n-\t\t}\n+\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n+\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n \n-\t\tif (read_env_script(&cmd.env_array)) {\n-\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n-\n-\t\t\treturn error(_(staged_changes_advice),\n-\t\t\t\t gpg_opt, gpg_opt);\n-\t\t}\n+\t\treturn error(_(staged_changes_advice),\n+\t\t\t gpg_opt, gpg_opt);\n \t}\n \n \targv_array_push(&cmd.args, \"commit\");\n@@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tif (opts->allow_empty_message)\n \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n \n-\tif (cmd.err == -1) {\n-\t\t/* hide stderr on success */\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint rc = pipe_command(&cmd,\n-\t\t\t\t NULL, 0,\n-\t\t\t\t /* stdout is already redirected */\n-\t\t\t\t NULL, 0,\n-\t\t\t\t &buf, 0);\n-\t\tif (rc)\n-\t\t\tfputs(buf.buf, stderr);\n-\t\tstrbuf_release(&buf);\n-\t\treturn rc;\n-\t}\n-\n-\treturn run_command(&cmd);\n+\treturn run_command_silent_on_success(&cmd,\n+\t\t\t\t\t !(is_rebase_i(opts) && !(flags & EDIT_MSG)));\n }\n \n static int rest_is_empty(const struct strbuf *sb, int start)\n-- \n2.16.4\n\n"},{"id":"350482","messageId":"20180619154421.14999-3-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180619154421.14999-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-19T15:44:20Z","receivedAt":"2018-06-19T15:45:11Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites setup_reflog_action() from shell to C. The new version is\ncalled checkout_base_commit().\n\nA new command is added to rebase--helper.c, “checkout-base”, as such as\na new flag, “verbose”, to avoid silencing the output of the checkout\noperation called by checkout_base_commit().\n\nThe shell version is then stripped in favour of a call to the helper. As\n$GIT_REFLOG_ACTION is not longer set at the first call of\ncheckout_onto(), a call to comment_for_reflog() is added at the\nbeginning of this function.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 9 +++++++--\n git-rebase--interactive.sh | 16 ++--------------\n sequencer.c | 28 ++++++++++++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 40 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex d2990b210..7cd74da2e 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {\n int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n-\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0;\n+\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;\n \tint abbreviate_commands = 0, rebase_cousins = -1;\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"rebase-merges\", &rebase_merges, N_(\"rebase merge commits\")),\n \t\tOPT_BOOL(0, \"rebase-cousins\", &rebase_cousins,\n \t\t\t N_(\"keep original branch points of cousins\")),\n+\t\tOPT__VERBOSE(&verbose, N_(\"print subcommands output even if they succeed\")),\n \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n \t\t\t\tCONTINUE),\n \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\n@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"edit-todo\", &command,\n \t\t\t N_(\"edit the todo list during an interactive rebase\"),\n \t\t\t EDIT_TODO),\n+\t\tOPT_CMDMODE(0, \"checkout-base\", &command,\n+\t\t\t N_(\"checkout the base commit\"), CHECKOUT_BASE),\n \t\tOPT_END()\n \t};\n \n@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!append_todo_help(0, keep_empty);\n \tif (command == EDIT_TODO && argc == 1)\n \t\treturn !!edit_todo_list(flags);\n+\tif (command == CHECKOUT_BASE && argc == 2)\n+\t\treturn !!checkout_base_commit(&opts, argv[1], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2defe607f..af46cf9c2 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -72,6 +72,7 @@ collapse_todo_ids() {\n \n # Switch to the branch in $into and notify it in the reflog\n checkout_onto () {\n+\tcomment_for_reflog start\n \tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n \toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n \tgit update-ref ORIG_HEAD $orig_head\n@@ -119,19 +120,6 @@ initiate_action () {\n \tesac\n }\n \n-setup_reflog_action () {\n-\tcomment_for_reflog start\n-\n-\tif test ! -z \"$switch_to\"\n-\tthen\n-\t\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n-\t\toutput git checkout \"$switch_to\" -- ||\n-\t\t\tdie \"$(eval_gettext \"Could not checkout \\$switch_to\")\"\n-\n-\t\tcomment_for_reflog start\n-\tfi\n-}\n-\n init_basic_state () {\n \torig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n@@ -211,7 +199,7 @@ git_rebase__interactive () {\n \t\treturn 0\n \tfi\n \n-\tsetup_reflog_action\n+\tgit rebase--helper --checkout-base \"$switch_to\" ${verbose:+--verbose}\n \tinit_basic_state\n \n \tinit_revisions_and_shortrevisions\ndiff --git a/sequencer.c b/sequencer.c\nindex 9aa7ddb33..a7a73e3ef 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3133,6 +3133,34 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n+static int run_git_checkout(struct replay_opts *opts, const char *commit,\n+\t\t\t\tint verbose, const char *action)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tcmd.git_cmd = 1;\n+\n+\targv_array_push(&cmd.args, \"checkout\");\n+\targv_array_push(&cmd.args, commit);\n+\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n+\n+\treturn run_command_silent_on_success(&cmd, verbose);\n+}\n+\n+int checkout_base_commit(struct replay_opts *opts, const char *commit,\n+\t\t\t int verbose)\n+{\n+\tconst char *action;\n+\n+\tif (commit && *commit) {\n+\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n+\t\tif (run_git_checkout(opts, commit, verbose, action))\n+\t\t\treturn error(_(\"Could not checkout %s\"), commit);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 35730b13e..42c3dda81 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,\n void commit_post_rewrite(const struct commit *current_head,\n \t\t\t const struct object_id *new_head);\n \n+int checkout_base_commit(struct replay_opts *opts, const char *commit,\n+\t\t\t int verbose);\n+\n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n void print_commit_summary(const char *prefix, const struct object_id *oid,\n-- \n2.16.4\n\n"},{"id":"350483","messageId":"20180619154421.14999-4-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180619154421.14999-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-19T15:44:21Z","receivedAt":"2018-06-19T15:45:14Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites checkout_onto() from shell to C.\n\nA new command (“checkout-onto”) is added to rebase--helper.c. The shell\nversion is then stripped.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 7 ++++++-\n git-rebase--interactive.sh | 25 ++++---------------------\n sequencer.c | 19 +++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex 7cd74da2e..19c1dba9a 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE,\n+\t\tCHECKOUT_ONTO\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\t EDIT_TODO),\n \t\tOPT_CMDMODE(0, \"checkout-base\", &command,\n \t\t\t N_(\"checkout the base commit\"), CHECKOUT_BASE),\n+\t\tOPT_CMDMODE(0, \"checkout-onto\", &command,\n+\t\t\t N_(\"checkout a commit\"), CHECKOUT_ONTO),\n \t\tOPT_END()\n \t};\n \n@@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!edit_todo_list(flags);\n \tif (command == CHECKOUT_BASE && argc == 2)\n \t\treturn !!checkout_base_commit(&opts, argv[1], verbose);\n+\tif (command == CHECKOUT_ONTO && argc == 4)\n+\t\treturn !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex af46cf9c2..7b6142a76 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -28,17 +28,6 @@ case \"$comment_char\" in\n \t;;\n esac\n \n-orig_reflog_action=\"$GIT_REFLOG_ACTION\"\n-\n-comment_for_reflog () {\n-\tcase \"$orig_reflog_action\" in\n-\t''|rebase*)\n-\t\tGIT_REFLOG_ACTION=\"rebase -i ($1)\"\n-\t\texport GIT_REFLOG_ACTION\n-\t\t;;\n-\tesac\n-}\n-\n die_abort () {\n \tapply_autostash\n \trm -rf \"$state_dir\"\n@@ -70,14 +59,6 @@ collapse_todo_ids() {\n \tgit rebase--helper --shorten-ids\n }\n \n-# Switch to the branch in $into and notify it in the reflog\n-checkout_onto () {\n-\tcomment_for_reflog start\n-\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n-\toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n-\tgit update-ref ORIG_HEAD $orig_head\n-}\n-\n get_missing_commit_check_level () {\n \tcheck_level=$(git config --get rebase.missingCommitsCheck)\n \tcheck_level=${check_level:-ignore}\n@@ -176,7 +157,8 @@ EOF\n \n \tgit rebase--helper --check-todo-list || {\n \t\tret=$?\n-\t\tcheckout_onto\n+\t\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \\\n+\t\t \"$orig_head\" ${verbose:+--verbose}\n \t\texit $ret\n \t}\n \n@@ -186,7 +168,8 @@ EOF\n \tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n \tdie \"Could not skip unnecessary pick commands\"\n \n-\tcheckout_onto\n+\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \"$orig_head\" \\\n+\t ${verbose:+--verbose}\n \trequire_clean_work_tree \"rebase\"\n \texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n \t --continue\ndiff --git a/sequencer.c b/sequencer.c\nindex a7a73e3ef..9165bf96c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3161,6 +3161,25 @@ int checkout_base_commit(struct replay_opts *opts, const char *commit,\n \treturn 0;\n }\n \n+int checkout_onto(struct replay_opts *opts,\n+\t\t const char *onto_name, const char *onto,\n+\t\t const char *orig_head, unsigned verbose)\n+{\n+\tstruct object_id oid;\n+\tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n+\n+\tif (get_oid(orig_head, &oid))\n+\t\treturn error(_(\"%s: not a valid OID\"), orig_head);\n+\n+\tif (run_git_checkout(opts, onto, verbose, action)) {\n+\t\tapply_autostash(opts);\n+\t\tsequencer_remove_state(opts);\n+\t\treturn error(_(\"could not detach HEAD\"));\n+\t}\n+\n+\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 42c3dda81..eda03ce32 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -102,6 +102,9 @@ void commit_post_rewrite(const struct commit *current_head,\n \n int checkout_base_commit(struct replay_opts *opts, const char *commit,\n \t\t\t int verbose);\n+int checkout_onto(struct replay_opts *opts,\n+\t\t const char *onto_name, const char *onto,\n+\t\t const char *orig_head, unsigned verbose);\n \n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n-- \n2.16.4\n\n"},{"id":"350521","messageId":"CAGZ79kb=T42ePDjpkXtTeiwBknR7p5eGXuOXfujoCtU9eEJm+g@mail.gmail.com","threadId":"48730","inReplyTo":"20180619154421.14999-1-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-19T18:35:33Z","receivedAt":"2018-06-19T18:35:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 19, 2018 at 8:45 AM Alban Gruin wrote:\n>\n> This patch series rewrites the reflog operations from shell to C. This\n> is part of the effort to rewrite interactive rebase in C.\n\nThis series looks good to me.\n\nThanks,\nStefan\n"},{"id":"350616","messageId":"nycvar.QRO.7.76.6.1806211031070.11870@tvgsbejvaqbjf.bet","threadId":"48730","inReplyTo":"20180619154421.14999-1-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-21T08:39:04Z","receivedAt":"2018-06-21T08:39:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Tue, 19 Jun 2018, Alban Gruin wrote:\n\n> -- \n> 2.16.4\n\nYou might want to update ;-)\n\nhttps://github.com/git-for-windows/git/wiki/FAQ#should-i-upgrade-to-a-newer-git-for-windows-version\n\nCiao,\nJohannes\n"},{"id":"350619","messageId":"nycvar.QRO.7.76.6.1806211040430.11870@tvgsbejvaqbjf.bet","threadId":"48730","inReplyTo":"20180619154421.14999-2-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-21T09:37:33Z","receivedAt":"2018-06-21T09:37:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Tue, 19 Jun 2018, Alban Gruin wrote:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 7cc76332e..9aa7ddb33 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -766,6 +766,29 @@ N_(\"you have staged changes in your working tree\\n\"\n> #define VERIFY_MSG (1<<4)\n> #define CREATE_ROOT_COMMIT (1<<5)\n> \n> +static int run_command_silent_on_success(struct child_process *cmd,\n> +\t\t\t\t\t unsigned verbose)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint rc;\n> +\n> +\tif (verbose)\n> +\t\treturn run_command(cmd);\n> +\n> +\t/* hide stderr on success */\n> +\tcmd->stdout_to_stderr = 1;\n\nThis comment is a bit misleading: that line does not hide stderr on\nsuccess, it redirects stdout to stderr instead.\n\n> +\trc = pipe_command(cmd,\n> +\t\t\t NULL, 0,\n> +\t\t\t /* stdout is already redirected */\n> +\t\t\t NULL, 0,\n> +\t\t\t &buf, 0);\n> +\n> +\tif (rc)\n> +\t\tfputs(buf.buf, stderr);\n> +\tstrbuf_release(&buf);\n> +\treturn rc;\n> +}\n> +\n> /*\n> * If we are cherry-pick, and if the merge did not result in\n> * hand-editing, we will hit this commit and inherit the original\n> @@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> \n> \tcmd.git_cmd = 1;\n> \n> -\tif (is_rebase_i(opts)) {\n> -\t\tif (!(flags & EDIT_MSG)) {\n> -\t\t\tcmd.stdout_to_stderr = 1;\n> -\t\t\tcmd.err = -1;\n> -\t\t}\n\nThis code made sure that we *only* do this redirection, and stderr\nbuffering, if `git commit` is called non-interactively. When it is called\ninteractively, redirecting stdout and stderr is absolutely not what we\nwant.\n\n> +\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n> +\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n> \n> -\t\tif (read_env_script(&cmd.env_array)) {\n> -\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n> -\n> -\t\t\treturn error(_(staged_changes_advice),\n> -\t\t\t\t gpg_opt, gpg_opt);\n> -\t\t}\n> +\t\treturn error(_(staged_changes_advice),\n> +\t\t\t gpg_opt, gpg_opt);\n> \t}\n> \n> \targv_array_push(&cmd.args, \"commit\");\n> @@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> \tif (opts->allow_empty_message)\n> \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n> \n> -\tif (cmd.err == -1) {\n> -\t\t/* hide stderr on success */\n> -\t\tstruct strbuf buf = STRBUF_INIT;\n> -\t\tint rc = pipe_command(&cmd,\n> -\t\t\t\t NULL, 0,\n> -\t\t\t\t /* stdout is already redirected */\n> -\t\t\t\t NULL, 0,\n> -\t\t\t\t &buf, 0);\n> -\t\tif (rc)\n> -\t\t\tfputs(buf.buf, stderr);\n> -\t\tstrbuf_release(&buf);\n> -\t\treturn rc;\n> -\t}\n> -\n> -\treturn run_command(&cmd);\n> +\treturn run_command_silent_on_success(&cmd,\n> +\t\t\t\t\t !(is_rebase_i(opts) && !(flags & EDIT_MSG)));\n\nIt would probably make more sense to change the signature of\n`run_command_silent_on_success()` to not even take the `int verbose`\nparameter: why call it \"silent on success\" when we can ask it *not* to be\nsilent on success?\n\nAnd then you can avoid this overly-long line (as well as the quite\nconvoluted Boolean logic that took me a couple of seconds to verify) very\nelegantly by this code:\n\n\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n\t\treturn run_command_silent_on_success(&cmd);\n\treturn run_command(&cmd);\n\nI vaguely recall that I wanted to make this an option in the `struct\nchild_process` when I originally introduced this code, but I think it was\nPeff who suggested that doing it \"by hand\" was the more appropriate way\nhere because I use it only once.\n\nMy recollection might fail me, but if it is correct, maybe that would be a\ngood way forward, to make this an `int silent_on_success:1;` field?\n\nCiao,\nDscho\n"},{"id":"350621","messageId":"nycvar.QRO.7.76.6.1806211146360.11870@tvgsbejvaqbjf.bet","threadId":"48730","inReplyTo":"20180619154421.14999-3-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-21T10:34:44Z","receivedAt":"2018-06-21T10:34:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\n\nOn Tue, 19 Jun 2018, Alban Gruin wrote:\n\n> This rewrites setup_reflog_action() from shell to C. The new version is\n> called checkout_base_commit().\n\nThis sounds like a serious mistake. How is it that a function that sets up\na reflog action all of a sudden checks out a base commit?\n\nBut you are correct, of course, that is exactly what the shell script\nfunction does!\n\nSo maybe insert \"the (misnamed)\" after \"This rewrites\"? That would help me\nin the future when I stumble across this commit again.\n\n> A new command is added to rebase--helper.c, “checkout-base”, as such as\n\ns/as such as/as well as/\n\n> a new flag, “verbose”, to avoid silencing the output of the checkout\n> operation called by checkout_base_commit().\n> \n> The shell version is then stripped in favour of a call to the helper. As\n\nPlease start a new paragraph after this sentence, before the \"As ...\",\nbecause those two sentences represent different thoughts.\n\n> $GIT_REFLOG_ACTION is not longer set at the first call of\n\ns/not longer/no longer/\n\n> checkout_onto(), a call to comment_for_reflog() is added at the\n> beginning of this function.\n> \n> Signed-off-by: Alban Gruin \n> ---\n> builtin/rebase--helper.c | 9 +++++++--\n> git-rebase--interactive.sh | 16 ++--------------\n> sequencer.c | 28 ++++++++++++++++++++++++++++\n> sequencer.h | 3 +++\n> 4 files changed, 40 insertions(+), 16 deletions(-)\n> \n> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\n> index d2990b210..7cd74da2e 100644\n> --- a/builtin/rebase--helper.c\n> +++ b/builtin/rebase--helper.c\n> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {\n> int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> {\n> \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n> -\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0;\n> +\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;\n> \tint abbreviate_commands = 0, rebase_cousins = -1;\n> \tenum {\n> \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n> \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n> -\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO\n> +\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE\n> \t} command = 0;\n> \tstruct option options[] = {\n> \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL(0, \"rebase-merges\", &rebase_merges, N_(\"rebase merge commits\")),\n> \t\tOPT_BOOL(0, \"rebase-cousins\", &rebase_cousins,\n> \t\t\t N_(\"keep original branch points of cousins\")),\n> +\t\tOPT__VERBOSE(&verbose, N_(\"print subcommands output even if they succeed\")),\n\nThat's very specific a description that applies only to this command\nmode... Maybe just say `\"be verbose\"`, and as a bonus avoid a too-long\nline?\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 9aa7ddb33..a7a73e3ef 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3133,6 +3133,34 @@ static const char *reflog_message(struct replay_opts *opts,\n> \treturn buf.buf;\n> }\n> \n> +static int run_git_checkout(struct replay_opts *opts, const char *commit,\n> +\t\t\t\tint verbose, const char *action)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> +\tcmd.git_cmd = 1;\n> +\n> +\targv_array_push(&cmd.args, \"checkout\");\n> +\targv_array_push(&cmd.args, commit);\n> +\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> +\n> +\treturn run_command_silent_on_success(&cmd, verbose);\n> +}\n\nAre you planning on reusing that code anywhere else than in\n`checkout_base_commit()`? If yes, sure, it makes sense to keep as separate\nfunction. Otherwise I would fold it into that latter function, for reading\nsimplicity.\n\n> +\n> +int checkout_base_commit(struct replay_opts *opts, const char *commit,\n> +\t\t\t int verbose)\n> +{\n> +\tconst char *action;\n> +\n> +\tif (commit && *commit) {\n> +\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n> +\t\tif (run_git_checkout(opts, commit, verbose, action))\n> +\t\t\treturn error(_(\"Could not checkout %s\"), commit);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> static const char rescheduled_advice[] =\n> N_(\"Could not execute the todo command\\n\"\n> \"\\n\"\n\nThe rest looks fine to me, thank you!\nDscho"},{"id":"350622","messageId":"nycvar.QRO.7.76.6.1806211236350.11870@tvgsbejvaqbjf.bet","threadId":"48730","inReplyTo":"20180619154421.14999-4-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-21T10:38:41Z","receivedAt":"2018-06-21T10:38:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Alban,\n\nOn Tue, 19 Jun 2018, Alban Gruin wrote:\n\n> diff --git a/sequencer.c b/sequencer.c\n> index a7a73e3ef..9165bf96c 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3161,6 +3161,25 @@ int checkout_base_commit(struct replay_opts *opts, const char *commit,\n> \treturn 0;\n> }\n> \n> +int checkout_onto(struct replay_opts *opts,\n> +\t\t const char *onto_name, const char *onto,\n> +\t\t const char *orig_head, unsigned verbose)\n> +{\n> +\tstruct object_id oid;\n> +\tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n> +\n> +\tif (get_oid(orig_head, &oid))\n> +\t\treturn error(_(\"%s: not a valid OID\"), orig_head);\n> +\n> +\tif (run_git_checkout(opts, onto, verbose, action)) {\n\nAh, so this is the reason for the split.\n\nIf you send a new iteration of this patch series, could you do me a favor\nand add a paragraph to the commit message of 2/3, saying something like\nthis: \"The function `run_git_checkout()` will be used on its own in the\nnext commit, therefore the code is not folded into\n`checkout_base_commit()`? That way, I do not have to burden my working\nmemory with this bit of information ;-)\n\nThanks,\nDscho\n"},{"id":"350627","messageId":"2dddfef3-843d-5444-c90c-45ac38919882@gmail.com","threadId":"48730","inReplyTo":"nycvar.QRO.7.76.6.1806211040430.11870@tvgsbejvaqbjf.bet","subject":"Re: [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-21T11:53:32Z","receivedAt":"2018-06-21T11:53:48Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Johannes,\n\nLe 21/06/2018 à 11:37, Johannes Schindelin a écrit :\n> Hi Alban,\n> \n> On Tue, 19 Jun 2018, Alban Gruin wrote:\n> \n>> diff --git a/sequencer.c b/sequencer.c\n>> index 7cc76332e..9aa7ddb33 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -766,6 +766,29 @@ N_(\"you have staged changes in your working tree\\n\"\n>> #define VERIFY_MSG (1<<4)\n>> #define CREATE_ROOT_COMMIT (1<<5)\n>> \n>> +static int run_command_silent_on_success(struct child_process *cmd,\n>> +\t\t\t\t\t unsigned verbose)\n>> +{\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> +\tint rc;\n>> +\n>> +\tif (verbose)\n>> +\t\treturn run_command(cmd);\n>> +\n>> +\t/* hide stderr on success */\n>> +\tcmd->stdout_to_stderr = 1;\n> \n> This comment is a bit misleading: that line does not hide stderr on\n> success, it redirects stdout to stderr instead.\n> \n>> +\trc = pipe_command(cmd,\n>> +\t\t\t NULL, 0,\n>> +\t\t\t /* stdout is already redirected */\n>> +\t\t\t NULL, 0,\n>> +\t\t\t &buf, 0);\n>> +\n>> +\tif (rc)\n>> +\t\tfputs(buf.buf, stderr);\n>> +\tstrbuf_release(&buf);\n>> +\treturn rc;\n>> +}\n>> +\n>> /*\n>> * If we are cherry-pick, and if the merge did not result in\n>> * hand-editing, we will hit this commit and inherit the original\n>> @@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n>> \n>> \tcmd.git_cmd = 1;\n>> \n>> -\tif (is_rebase_i(opts)) {\n>> -\t\tif (!(flags & EDIT_MSG)) {\n>> -\t\t\tcmd.stdout_to_stderr = 1;\n>> -\t\t\tcmd.err = -1;\n>> -\t\t}\n> \n> This code made sure that we *only* do this redirection, and stderr\n> buffering, if `git commit` is called non-interactively. When it is called\n> interactively, redirecting stdout and stderr is absolutely not what we\n> want.\n> \n>> +\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n>> +\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n>> \n>> -\t\tif (read_env_script(&cmd.env_array)) {\n>> -\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n>> -\n>> -\t\t\treturn error(_(staged_changes_advice),\n>> -\t\t\t\t gpg_opt, gpg_opt);\n>> -\t\t}\n>> +\t\treturn error(_(staged_changes_advice),\n>> +\t\t\t gpg_opt, gpg_opt);\n>> \t}\n>> \n>> \targv_array_push(&cmd.args, \"commit\");\n>> @@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n>> \tif (opts->allow_empty_message)\n>> \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n>> \n>> -\tif (cmd.err == -1) {\n>> -\t\t/* hide stderr on success */\n>> -\t\tstruct strbuf buf = STRBUF_INIT;\n>> -\t\tint rc = pipe_command(&cmd,\n>> -\t\t\t\t NULL, 0,\n>> -\t\t\t\t /* stdout is already redirected */\n>> -\t\t\t\t NULL, 0,\n>> -\t\t\t\t &buf, 0);\n>> -\t\tif (rc)\n>> -\t\t\tfputs(buf.buf, stderr);\n>> -\t\tstrbuf_release(&buf);\n>> -\t\treturn rc;\n>> -\t}\n>> -\n>> -\treturn run_command(&cmd);\n>> +\treturn run_command_silent_on_success(&cmd,\n>> +\t\t\t\t\t !(is_rebase_i(opts) && !(flags & EDIT_MSG)));\n> \n> It would probably make more sense to change the signature of\n> `run_command_silent_on_success()` to not even take the `int verbose`\n> parameter: why call it \"silent on success\" when we can ask it *not* to be\n> silent on success?\n> \n> And then you can avoid this overly-long line (as well as the quite\n> convoluted Boolean logic that took me a couple of seconds to verify) very\n> elegantly by this code:\n> \n> \tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n> \t\treturn run_command_silent_on_success(&cmd);\n> \treturn run_command(&cmd);\n> \n> I vaguely recall that I wanted to make this an option in the `struct\n> child_process` when I originally introduced this code, but I think it was\n> Peff who suggested that doing it \"by hand\" was the more appropriate way\n> here because I use it only once.\n> \n> My recollection might fail me, but if it is correct, maybe that would be a\n> good way forward, to make this an `int silent_on_success:1;` field?\n> \n\nI think I found it:\n\nhttps://public-inbox.org/git/1e82aeabb906a35175362418b2b4957fae50c3b0.1481642927.git.johannes.schindelin@gmx.de/\n\nApparently, you wanted to introduce a new RUN_ flag for\nrun_command_v_opt_cd_env(), but the change was qualified as a “bolted-on\nfeature” by Johannes Sixt.\n\nSo, I will remove the “verbose” argument in the reroll.\n\nCheers,\nAlban\n\n"},{"id":"350631","messageId":"20180621141732.19952-1-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180619154421.14999-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v3 0/3] rebase -i: rewrite reflog operations in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-21T14:17:29Z","receivedAt":"2018-06-21T14:30:55Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Thes patch series rewrites the reflog operations from shell to C. This\nis part of the effort to rewrite interactive rebase in C.\n\nThe first commit is dedicated to creating a function to silence a\ncommand, as the sequencer will do in several places with these patches.\n\nThis branch is based on ag/rebase-i-rewrite-todo, and does not conflict\nwith pu (as of 2018-06-21).\n\nChanges since v2:\n\n - Removing the “verbose” parameter to run_command_silent_on_success()\n\n - Rewording some parts of the second commit\n\n - Changing the help for the “--verbose” flag\n\nAlban Gruin (3):\n sequencer: add a new function to silence a command, except if it\n fails.\n rebase -i: rewrite setup_reflog_action() in C\n rebase -i: rewrite checkout_onto() in C\n\n builtin/rebase--helper.c | 14 +++++-\n git-rebase--interactive.sh | 39 ++-------------\n sequencer.c | 98 ++++++++++++++++++++++++++++----------\n sequencer.h | 6 +++\n 4 files changed, 96 insertions(+), 61 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"350632","messageId":"20180621141732.19952-2-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180621141732.19952-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-21T14:17:30Z","receivedAt":"2018-06-21T14:30:58Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This adds a new function, run_command_silent_on_success(), to\nredirect the stdout and stderr of a command to a strbuf, and then to run\nthat command. This strbuf is printed only if the command fails. It is\nfunctionnaly similar to output() from git-rebase.sh.\n\nrun_git_commit() is then refactored to use of\nrun_command_silent_on_success().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 49 ++++++++++++++++++++++++-------------------------\n 1 file changed, 24 insertions(+), 25 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7cc76332e..51c8ab7ac 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -766,6 +766,24 @@ N_(\"you have staged changes in your working tree\\n\"\n #define VERIFY_MSG (1<<4)\n #define CREATE_ROOT_COMMIT (1<<5)\n \n+static int run_command_silent_on_success(struct child_process *cmd)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint rc;\n+\n+\tcmd->stdout_to_stderr = 1;\n+\trc = pipe_command(cmd,\n+\t\t\t NULL, 0,\n+\t\t\t /* stdout is already redirected */\n+\t\t\t NULL, 0,\n+\t\t\t &buf, 0);\n+\n+\tif (rc)\n+\t\tfputs(buf.buf, stderr);\n+\tstrbuf_release(&buf);\n+\treturn rc;\n+}\n+\n /*\n * If we are cherry-pick, and if the merge did not result in\n * hand-editing, we will hit this commit and inherit the original\n@@ -820,18 +838,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \n \tcmd.git_cmd = 1;\n \n-\tif (is_rebase_i(opts)) {\n-\t\tif (!(flags & EDIT_MSG)) {\n-\t\t\tcmd.stdout_to_stderr = 1;\n-\t\t\tcmd.err = -1;\n-\t\t}\n+\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n+\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n \n-\t\tif (read_env_script(&cmd.env_array)) {\n-\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n-\n-\t\t\treturn error(_(staged_changes_advice),\n-\t\t\t\t gpg_opt, gpg_opt);\n-\t\t}\n+\t\treturn error(_(staged_changes_advice),\n+\t\t\t gpg_opt, gpg_opt);\n \t}\n \n \targv_array_push(&cmd.args, \"commit\");\n@@ -861,20 +872,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tif (opts->allow_empty_message)\n \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n \n-\tif (cmd.err == -1) {\n-\t\t/* hide stderr on success */\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint rc = pipe_command(&cmd,\n-\t\t\t\t NULL, 0,\n-\t\t\t\t /* stdout is already redirected */\n-\t\t\t\t NULL, 0,\n-\t\t\t\t &buf, 0);\n-\t\tif (rc)\n-\t\t\tfputs(buf.buf, stderr);\n-\t\tstrbuf_release(&buf);\n-\t\treturn rc;\n-\t}\n-\n+\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n+\t\treturn run_command_silent_on_success(&cmd);\n \treturn run_command(&cmd);\n }\n \n-- \n2.17.1\n\n"},{"id":"350633","messageId":"20180621141732.19952-3-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180621141732.19952-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-21T14:17:31Z","receivedAt":"2018-06-21T14:31:00Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites (the misnamed) setup_reflog_action() from shell to C. The\nnew version is called checkout_base_commit().\n\nA new command is added to rebase--helper.c, “checkout-base”, as well as\na new flag, “verbose”, to avoid silencing the output of the checkout\noperation called by checkout_base_commit().\n\nThe function `run_git_checkout()` will also be used in the next commit,\ntherefore its code is not part of `checkout_base_commit()`.\n\nThe shell version is then stripped in favour of a call to the helper.\n\nAs $GIT_REFLOG_ACTION is no longer set at the first call of\ncheckout_onto(), a call to comment_for_reflog() is added at the\nbeginning of this function.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 9 +++++++--\n git-rebase--interactive.sh | 16 ++--------------\n sequencer.c | 30 ++++++++++++++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 42 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex d2990b210..fb5996a2c 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {\n int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n-\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0;\n+\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;\n \tint abbreviate_commands = 0, rebase_cousins = -1;\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"rebase-merges\", &rebase_merges, N_(\"rebase merge commits\")),\n \t\tOPT_BOOL(0, \"rebase-cousins\", &rebase_cousins,\n \t\t\t N_(\"keep original branch points of cousins\")),\n+\t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n \t\t\t\tCONTINUE),\n \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\n@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"edit-todo\", &command,\n \t\t\t N_(\"edit the todo list during an interactive rebase\"),\n \t\t\t EDIT_TODO),\n+\t\tOPT_CMDMODE(0, \"checkout-base\", &command,\n+\t\t\t N_(\"checkout the base commit\"), CHECKOUT_BASE),\n \t\tOPT_END()\n \t};\n \n@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!append_todo_help(0, keep_empty);\n \tif (command == EDIT_TODO && argc == 1)\n \t\treturn !!edit_todo_list(flags);\n+\tif (command == CHECKOUT_BASE && argc == 2)\n+\t\treturn !!checkout_base_commit(&opts, argv[1], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2defe607f..af46cf9c2 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -72,6 +72,7 @@ collapse_todo_ids() {\n \n # Switch to the branch in $into and notify it in the reflog\n checkout_onto () {\n+\tcomment_for_reflog start\n \tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n \toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n \tgit update-ref ORIG_HEAD $orig_head\n@@ -119,19 +120,6 @@ initiate_action () {\n \tesac\n }\n \n-setup_reflog_action () {\n-\tcomment_for_reflog start\n-\n-\tif test ! -z \"$switch_to\"\n-\tthen\n-\t\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n-\t\toutput git checkout \"$switch_to\" -- ||\n-\t\t\tdie \"$(eval_gettext \"Could not checkout \\$switch_to\")\"\n-\n-\t\tcomment_for_reflog start\n-\tfi\n-}\n-\n init_basic_state () {\n \torig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n@@ -211,7 +199,7 @@ git_rebase__interactive () {\n \t\treturn 0\n \tfi\n \n-\tsetup_reflog_action\n+\tgit rebase--helper --checkout-base \"$switch_to\" ${verbose:+--verbose}\n \tinit_basic_state\n \n \tinit_revisions_and_shortrevisions\ndiff --git a/sequencer.c b/sequencer.c\nindex 51c8ab7ac..27f8453fe 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3129,6 +3129,36 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n+static int run_git_checkout(struct replay_opts *opts, const char *commit,\n+\t\t\t\tint verbose, const char *action)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tcmd.git_cmd = 1;\n+\n+\targv_array_push(&cmd.args, \"checkout\");\n+\targv_array_push(&cmd.args, commit);\n+\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n+\n+\tif (verbose)\n+\t\treturn run_command(&cmd);\n+\treturn run_command_silent_on_success(&cmd);\n+}\n+\n+int checkout_base_commit(struct replay_opts *opts, const char *commit,\n+\t\t\t int verbose)\n+{\n+\tconst char *action;\n+\n+\tif (commit && *commit) {\n+\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n+\t\tif (run_git_checkout(opts, commit, verbose, action))\n+\t\t\treturn error(_(\"Could not checkout %s\"), commit);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 35730b13e..42c3dda81 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,\n void commit_post_rewrite(const struct commit *current_head,\n \t\t\t const struct object_id *new_head);\n \n+int checkout_base_commit(struct replay_opts *opts, const char *commit,\n+\t\t\t int verbose);\n+\n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n void print_commit_summary(const char *prefix, const struct object_id *oid,\n-- \n2.17.1\n\n"},{"id":"350634","messageId":"20180621141732.19952-4-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180621141732.19952-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v3 3/3] rebase -i: rewrite checkout_onto() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-21T14:17:32Z","receivedAt":"2018-06-21T14:31:02Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites checkout_onto() from shell to C.\n\nA new command (“checkout-onto”) is added to rebase--helper.c. The shell\nversion is then stripped.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 7 ++++++-\n git-rebase--interactive.sh | 25 ++++---------------------\n sequencer.c | 19 +++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex fb5996a2c..20dea4b3a 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE,\n+\t\tCHECKOUT_ONTO\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\t EDIT_TODO),\n \t\tOPT_CMDMODE(0, \"checkout-base\", &command,\n \t\t\t N_(\"checkout the base commit\"), CHECKOUT_BASE),\n+\t\tOPT_CMDMODE(0, \"checkout-onto\", &command,\n+\t\t\t N_(\"checkout a commit\"), CHECKOUT_ONTO),\n \t\tOPT_END()\n \t};\n \n@@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!edit_todo_list(flags);\n \tif (command == CHECKOUT_BASE && argc == 2)\n \t\treturn !!checkout_base_commit(&opts, argv[1], verbose);\n+\tif (command == CHECKOUT_ONTO && argc == 4)\n+\t\treturn !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex af46cf9c2..7b6142a76 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -28,17 +28,6 @@ case \"$comment_char\" in\n \t;;\n esac\n \n-orig_reflog_action=\"$GIT_REFLOG_ACTION\"\n-\n-comment_for_reflog () {\n-\tcase \"$orig_reflog_action\" in\n-\t''|rebase*)\n-\t\tGIT_REFLOG_ACTION=\"rebase -i ($1)\"\n-\t\texport GIT_REFLOG_ACTION\n-\t\t;;\n-\tesac\n-}\n-\n die_abort () {\n \tapply_autostash\n \trm -rf \"$state_dir\"\n@@ -70,14 +59,6 @@ collapse_todo_ids() {\n \tgit rebase--helper --shorten-ids\n }\n \n-# Switch to the branch in $into and notify it in the reflog\n-checkout_onto () {\n-\tcomment_for_reflog start\n-\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n-\toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n-\tgit update-ref ORIG_HEAD $orig_head\n-}\n-\n get_missing_commit_check_level () {\n \tcheck_level=$(git config --get rebase.missingCommitsCheck)\n \tcheck_level=${check_level:-ignore}\n@@ -176,7 +157,8 @@ EOF\n \n \tgit rebase--helper --check-todo-list || {\n \t\tret=$?\n-\t\tcheckout_onto\n+\t\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \\\n+\t\t \"$orig_head\" ${verbose:+--verbose}\n \t\texit $ret\n \t}\n \n@@ -186,7 +168,8 @@ EOF\n \tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n \tdie \"Could not skip unnecessary pick commands\"\n \n-\tcheckout_onto\n+\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \"$orig_head\" \\\n+\t ${verbose:+--verbose}\n \trequire_clean_work_tree \"rebase\"\n \texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n \t --continue\ndiff --git a/sequencer.c b/sequencer.c\nindex 27f8453fe..b3b1a2e18 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3159,6 +3159,25 @@ int checkout_base_commit(struct replay_opts *opts, const char *commit,\n \treturn 0;\n }\n \n+int checkout_onto(struct replay_opts *opts,\n+\t\t const char *onto_name, const char *onto,\n+\t\t const char *orig_head, unsigned verbose)\n+{\n+\tstruct object_id oid;\n+\tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n+\n+\tif (get_oid(orig_head, &oid))\n+\t\treturn error(_(\"%s: not a valid OID\"), orig_head);\n+\n+\tif (run_git_checkout(opts, onto, verbose, action)) {\n+\t\tapply_autostash(opts);\n+\t\tsequencer_remove_state(opts);\n+\t\treturn error(_(\"could not detach HEAD\"));\n+\t}\n+\n+\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 42c3dda81..eda03ce32 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -102,6 +102,9 @@ void commit_post_rewrite(const struct commit *current_head,\n \n int checkout_base_commit(struct replay_opts *opts, const char *commit,\n \t\t\t int verbose);\n+int checkout_onto(struct replay_opts *opts,\n+\t\t const char *onto_name, const char *onto,\n+\t\t const char *orig_head, unsigned verbose);\n \n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n-- \n2.17.1\n\n"},{"id":"350692","messageId":"xmqqk1qrre4w.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"20180621141732.19952-2-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-21T22:03:27Z","receivedAt":"2018-06-21T22:03:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> This adds a new function, run_command_silent_on_success(), to\n> redirect the stdout and stderr of a command to a strbuf, and then to run\n> that command. This strbuf is printed only if the command fails. It is\n> functionnaly similar to output() from git-rebase.sh.\n>\n> run_git_commit() is then refactored to use of\n> run_command_silent_on_success().\n\nIt might be just a difference in viewpoint, but I think it is more\ncustomary in this project (hence it will be easier to understand and\naccept by readers of the patch) if a change like this is explained\nNOT as \"introducing a new function and then rewrite an existing code\nto use it\", and instead as \"extract a helper function from an\nexisting code so that future callers can reuse it.\"\n\n> +static int run_command_silent_on_success(struct child_process *cmd)\n> +{\n> +\tstruct strbuf buf = STRBUF_INIT;\n> +\tint rc;\n> +\n> +\tcmd->stdout_to_stderr = 1;\n> +\trc = pipe_command(cmd,\n> +\t\t\t NULL, 0,\n> +\t\t\t /* stdout is already redirected */\n\nI can see that this comment was inherited from the original place\nthis code was lifted from (and that is why I say this is not \"adding\na new helper and rewriting an existing piece of code to use it\" but\nis \"extracting this function out of an existing code for future\nreuse\"), but does it still make sense in the new context to keep the\nsame comment?\n\nThe original in run_git_commit() made the .stdout_to_stderr=1\nassignment many lines before it called pipe_command(), and it made\nsense to remind readers why we are passing NULL to the out buffer\nand only passing the err buffer here. But in the context of this\nnew helper function, the redirection that may make such a reminder\nnecessary sits immediately before the pipe_command() call for\neverybody to see.\n\n> +\t\t\t NULL, 0,\n> +\t\t\t &buf, 0);\n> +\n> +\tif (rc)\n> +\t\tfputs(buf.buf, stderr);\n> +\tstrbuf_release(&buf);\n> +\treturn rc;\n> +}\n\n> @@ -820,18 +838,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> \n> \tcmd.git_cmd = 1;\n> \n> -\tif (is_rebase_i(opts)) {\n> -\t\tif (!(flags & EDIT_MSG)) {\n> -\t\t\tcmd.stdout_to_stderr = 1;\n> -\t\t\tcmd.err = -1;\n> -\t\t}\n> +\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n> +\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n> \n> -\t\tif (read_env_script(&cmd.env_array)) {\n> -\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n> -\n> -\t\t\treturn error(_(staged_changes_advice),\n> -\t\t\t\t gpg_opt, gpg_opt);\n> -\t\t}\n> +\t\treturn error(_(staged_changes_advice),\n> +\t\t\t gpg_opt, gpg_opt);\n> \t}\n\nThe original in this hunk is criminally incomprehensible ;-) The\nassignment of .stdout_to_stderr=1 does not have to happen this early\nin the code, and also using .err being -1 to communicate that the\n\"capture and show error only upon error\" magic is going to be used\nto the later code is unnecessarily opaque. The new code gets rid\nof that ugliness from this earlier hunk and instead makes this part\nonly about reading the various state files (and reporting failure to\ndo so).\n\n> @@ -861,20 +872,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n> \tif (opts->allow_empty_message)\n> \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n> \n> -\tif (cmd.err == -1) {\n> -\t\t/* hide stderr on success */\n> -\t\tstruct strbuf buf = STRBUF_INIT;\n> -\t\tint rc = pipe_command(&cmd,\n> -\t\t\t\t NULL, 0,\n> -\t\t\t\t /* stdout is already redirected */\n> -\t\t\t\t NULL, 0,\n> -\t\t\t\t &buf, 0);\n> -\t\tif (rc)\n> -\t\t\tfputs(buf.buf, stderr);\n> -\t\tstrbuf_release(&buf);\n> -\t\treturn rc;\n> -\t}\n> -\n> +\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n> +\t\treturn run_command_silent_on_success(&cmd);\n> \treturn run_command(&cmd);\n> }\n\nAnd then, the updated code in this hunk checks the EDIT_MSG bit\ninstead. It probably is easier to understand the code if you added\nan \"else\" after, to highlight the fact that this is choosing one out\nof two possible ways to run \"cmd\", i.e.\n\n\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n\t\treturn run_command_silent_on_success(&cmd);\n\telse\n\t\treturn run_command(&cmd);\n\n"},{"id":"350734","messageId":"xmqq4lhurdl7.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"20180621141732.19952-3-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-22T16:27:32Z","receivedAt":"2018-06-22T16:27:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> This rewrites (the misnamed) setup_reflog_action() from shell to C. The\n> new version is called checkout_base_commit().\n\n;-) on the \"misnamed\" part. Indeed, setting up the comment for the\nreflog entry is secondary to what this function wants to do, which\nis to check out the branch to be rebased.\n\nI do not think \"base_commit\" is a good name, either, though. When I\nhear 'base' in the context of 'rebase', I would imagine that the\nspeaker is talking about the bottom of the range of the commits to\nbe rebased (i.e. \"rebase --onto ONTO BASE BRANCH\", which replays\ncommits BASE..BRANCH on top of ONTO and then points BRANCH at the\nresult), not the top of the range or the branch these commits are\ntaken from.\n\n> index 51c8ab7ac..27f8453fe 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3129,6 +3129,36 @@ static const char *reflog_message(struct replay_opts *opts,\n> \treturn buf.buf;\n> }\n> \n> +static int run_git_checkout(struct replay_opts *opts, const char *commit,\n> +\t\t\t\tint verbose, const char *action)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> +\tcmd.git_cmd = 1;\n> +\n> +\targv_array_push(&cmd.args, \"checkout\");\n> +\targv_array_push(&cmd.args, commit);\n> +\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> +\n> +\tif (verbose)\n> +\t\treturn run_command(&cmd);\n> +\treturn run_command_silent_on_success(&cmd);\n\nFor the same reason as 1/3, I think it makes more sense to have\n\"else\" here.\n\n> +int checkout_base_commit(struct replay_opts *opts, const char *commit,\n> +\t\t\t int verbose)\n> +{\n> +\tconst char *action;\n> +\n> +\tif (commit && *commit) {\n\nHmm, isn't it a programming error to feed !commit or !*commit here?\nI offhand do not think of a reason why making such an input a silent\nno-op success, instead of making it error out, would be a good idea.\n\n> +\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n> +\t\tif (run_git_checkout(opts, commit, verbose, action))\n> +\t\t\treturn error(_(\"Could not checkout %s\"), commit);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> static const char rescheduled_advice[] =\n> N_(\"Could not execute the todo command\\n\"\n> \"\\n\"\n> diff --git a/sequencer.h b/sequencer.h\n> index 35730b13e..42c3dda81 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,\n> void commit_post_rewrite(const struct commit *current_head,\n> \t\t\t const struct object_id *new_head);\n> \n> +int checkout_base_commit(struct replay_opts *opts, const char *commit,\n> +\t\t\t int verbose);\n> +\n> #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> void print_commit_summary(const char *prefix, const struct object_id *oid,\n"},{"id":"350761","messageId":"6634943.MY9lBgqpaT@andromeda","threadId":"48730","inReplyTo":"xmqqk1qrre4w.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-22T20:47:32Z","receivedAt":"2018-06-22T20:47:44Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Junio,\n\nLe 22/06/2018 à 00:03, Junio C Hamano a écrit :\n> Alban Gruin writes:\n> > This adds a new function, run_command_silent_on_success(), to\n> > redirect the stdout and stderr of a command to a strbuf, and then to run\n> > that command. This strbuf is printed only if the command fails. It is\n> > functionnaly similar to output() from git-rebase.sh.\n> > \n> > run_git_commit() is then refactored to use of\n> > run_command_silent_on_success().\n> \n> It might be just a difference in viewpoint, but I think it is more\n> customary in this project (hence it will be easier to understand and\n> accept by readers of the patch) if a change like this is explained\n> NOT as \"introducing a new function and then rewrite an existing code\n> to use it\", and instead as \"extract a helper function from an\n> existing code so that future callers can reuse it.\"\n> \n\nI like your wording. It’s not a difference of point of view, I’m just bad at \nwriting commit messages ;)\n\n> > +static int run_command_silent_on_success(struct child_process *cmd)\n> > +{\n> > +\tstruct strbuf buf = STRBUF_INIT;\n> > +\tint rc;\n> > +\n> > +\tcmd->stdout_to_stderr = 1;\n> > +\trc = pipe_command(cmd,\n> > +\t\t\t NULL, 0,\n> > +\t\t\t /* stdout is already redirected */\n> \n> I can see that this comment was inherited from the original place\n> this code was lifted from (and that is why I say this is not \"adding\n> a new helper and rewriting an existing piece of code to use it\" but\n> is \"extracting this function out of an existing code for future\n> reuse\"), but does it still make sense in the new context to keep the\n> same comment?\n> \n> The original in run_git_commit() made the .stdout_to_stderr=1\n> assignment many lines before it called pipe_command(), and it made\n> sense to remind readers why we are passing NULL to the out buffer\n> and only passing the err buffer here. But in the context of this\n> new helper function, the redirection that may make such a reminder\n> necessary sits immediately before the pipe_command() call for\n> everybody to see.\n> \n\nYeah, you’re right.\n\n> > @@ -861,20 +872,8 @@ static int run_git_commit(const char *defmsg, struct\n> > replay_opts *opts,> \n> > \tif (opts->allow_empty_message)\n> > \t\n> > \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n> > \n> > -\tif (cmd.err == -1) {\n> > -\t\t/* hide stderr on success */\n> > -\t\tstruct strbuf buf = STRBUF_INIT;\n> > -\t\tint rc = pipe_command(&cmd,\n> > -\t\t\t\t NULL, 0,\n> > -\t\t\t\t /* stdout is already redirected */\n> > -\t\t\t\t NULL, 0,\n> > -\t\t\t\t &buf, 0);\n> > -\t\tif (rc)\n> > -\t\t\tfputs(buf.buf, stderr);\n> > -\t\tstrbuf_release(&buf);\n> > -\t\treturn rc;\n> > -\t}\n> > -\n> > +\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n> > +\t\treturn run_command_silent_on_success(&cmd);\n> > \n> > \treturn run_command(&cmd);\n> > \n> > }\n> \n> It probably is easier to understand the code if you added\n> an \"else\" after, to highlight the fact that this is choosing one out\n> of two possible ways to run \"cmd\", i.e.\n> \n> \tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n> \t\treturn run_command_silent_on_success(&cmd);\n> \telse\n> \t\treturn run_command(&cmd);\n\nOkay.\n\nCheers,\nAlban\n\n\n\n\n"},{"id":"350762","messageId":"7094810.i0lmrLi0k7@andromeda","threadId":"48730","inReplyTo":"xmqq4lhurdl7.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-22T20:48:03Z","receivedAt":"2018-06-22T20:48:13Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Junio,\n\nLe 22/06/2018 à 18:27, Junio C Hamano a écrit :\n> Alban Gruin writes:\n> > This rewrites (the misnamed) setup_reflog_action() from shell to C. The\n> > new version is called checkout_base_commit().\n> \n> ;-) on the \"misnamed\" part. Indeed, setting up the comment for the\n> reflog entry is secondary to what this function wants to do, which\n> is to check out the branch to be rebased.\n> \n> I do not think \"base_commit\" is a good name, either, though. When I\n> hear 'base' in the context of 'rebase', I would imagine that the\n> speaker is talking about the bottom of the range of the commits to\n> be rebased (i.e. \"rebase --onto ONTO BASE BRANCH\", which replays\n> commits BASE..BRANCH on top of ONTO and then points BRANCH at the\n> result), not the top of the range or the branch these commits are\n> taken from.\n> \n\nPerhaps should I name this function checkout_onto(), and rename \ncheckout_onto() to \"detach_onto()\"? And I would reorder those two commits in \nthe series, as this would be very confusing.\n\n> > index 51c8ab7ac..27f8453fe 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -3129,6 +3129,36 @@ static const char *reflog_message(struct\n> > replay_opts *opts,> \n> > \treturn buf.buf;\n> > \n> > }\n> > \n> > +static int run_git_checkout(struct replay_opts *opts, const char *commit,\n> > +\t\t\t\tint verbose, const char *action)\n> > +{\n> > +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> > +\n> > +\tcmd.git_cmd = 1;\n> > +\n> > +\targv_array_push(&cmd.args, \"checkout\");\n> > +\targv_array_push(&cmd.args, commit);\n> > +\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> > +\n> > +\tif (verbose)\n> > +\t\treturn run_command(&cmd);\n> > +\treturn run_command_silent_on_success(&cmd);\n> \n> For the same reason as 1/3, I think it makes more sense to have\n> \"else\" here.\n> \n\nRight.\n\n> > +int checkout_base_commit(struct replay_opts *opts, const char *commit,\n> > +\t\t\t int verbose)\n> > +{\n> > +\tconst char *action;\n> > +\n> > +\tif (commit && *commit) {\n> \n> Hmm, isn't it a programming error to feed !commit or !*commit here?\n> I offhand do not think of a reason why making such an input a silent\n> no-op success, instead of making it error out, would be a good idea.\n> \n\nI think it’s correct. \n\n> > +\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n> > +\t\tif (run_git_checkout(opts, commit, verbose, action))\n> > +\t\t\treturn error(_(\"Could not checkout %s\"), commit);\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > \n> > static const char rescheduled_advice[] =\n> > N_(\"Could not execute the todo command\\n\"\n> > \"\\n\"\n> > \n> > diff --git a/sequencer.h b/sequencer.h\n> > index 35730b13e..42c3dda81 100644\n> > --- a/sequencer.h\n> > +++ b/sequencer.h\n> > @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit\n> > *old_head,> \n> > void commit_post_rewrite(const struct commit *current_head,\n> > \n> > \t\t\t const struct object_id *new_head);\n> > \n> > +int checkout_base_commit(struct replay_opts *opts, const char *commit,\n> > +\t\t\t int verbose);\n> > +\n> > \n> > #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> > #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> > void print_commit_summary(const char *prefix, const struct object_id\n> > *oid,\n\nCheers,\nAlban\n\n\n\n\n"},{"id":"350803","messageId":"20180625134419.18435-1-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180621141732.19952-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-25T13:44:16Z","receivedAt":"2018-06-25T13:44:48Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This patch series rewrites the reflog operations from shell to C. This\nis part of the effort to rewrite interactive rebase in C.\n\nThe first commit is dedicated to creating a function to silence a\ncommand, as the sequencer will do in several places with these patches.\n\nThis branch is based on ag/rebase-i-rewrite-todo, and does not conflict\nwith pu (as of 2018-06-25).\n\nChanges since v3:\n\n - Removing a comment from run_command_silent_on_success()\n\n - Changing the order of setup_reflog_action() and checkout_onto()\n rewrites in the series\n\n - Renaming checkout_onto() to detach_onto()\n\n - Renaming checkout_base_commit() (rewrite of setup_reflog_action()) to\n checkout_onto()\n\n - Using the `else` keyword to call run_command_silent_on_success() or\n run_command() in run_git_commit() and run_git_checkout().\n\nAlban Gruin (3):\n sequencer: extract a function to silence a command, except if it fails\n rebase -i: rewrite checkout_onto() in C\n rebase -i: rewrite setup_reflog_action() in C\n\n builtin/rebase--helper.c | 13 ++++-\n git-rebase--interactive.sh | 28 ++--------\n sequencer.c | 101 +++++++++++++++++++++++++++----------\n sequencer.h | 6 +++\n 4 files changed, 97 insertions(+), 51 deletions(-)\n\n-- \n2.18.0\n\n"},{"id":"350804","messageId":"20180625134419.18435-2-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180625134419.18435-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v4 1/3] sequencer: extract a function to silence a command, except if it fails","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-25T13:44:17Z","receivedAt":"2018-06-25T13:44:50Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This extracts the part of run_git_commit() that redirects the stdout and\nstderr of a command to a strbuf, and prints it if the command fails, to\na helper function: run_command_silent_on_success(). It is intended to\nreplace output() from git-rebase.sh in the rewrite of\nsetup_reflog_action() and checkout_onto().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 51 +++++++++++++++++++++++++--------------------------\n 1 file changed, 25 insertions(+), 26 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 7cc76332e..9a9725e23 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -766,6 +766,23 @@ N_(\"you have staged changes in your working tree\\n\"\n #define VERIFY_MSG (1<<4)\n #define CREATE_ROOT_COMMIT (1<<5)\n \n+static int run_command_silent_on_success(struct child_process *cmd)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint rc;\n+\n+\tcmd->stdout_to_stderr = 1;\n+\trc = pipe_command(cmd,\n+\t\t\t NULL, 0,\n+\t\t\t NULL, 0,\n+\t\t\t &buf, 0);\n+\n+\tif (rc)\n+\t\tfputs(buf.buf, stderr);\n+\tstrbuf_release(&buf);\n+\treturn rc;\n+}\n+\n /*\n * If we are cherry-pick, and if the merge did not result in\n * hand-editing, we will hit this commit and inherit the original\n@@ -820,18 +837,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \n \tcmd.git_cmd = 1;\n \n-\tif (is_rebase_i(opts)) {\n-\t\tif (!(flags & EDIT_MSG)) {\n-\t\t\tcmd.stdout_to_stderr = 1;\n-\t\t\tcmd.err = -1;\n-\t\t}\n-\n-\t\tif (read_env_script(&cmd.env_array)) {\n-\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n+\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n+\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n \n-\t\t\treturn error(_(staged_changes_advice),\n-\t\t\t\t gpg_opt, gpg_opt);\n-\t\t}\n+\t\treturn error(_(staged_changes_advice),\n+\t\t\t gpg_opt, gpg_opt);\n \t}\n \n \targv_array_push(&cmd.args, \"commit\");\n@@ -861,21 +871,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tif (opts->allow_empty_message)\n \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n \n-\tif (cmd.err == -1) {\n-\t\t/* hide stderr on success */\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint rc = pipe_command(&cmd,\n-\t\t\t\t NULL, 0,\n-\t\t\t\t /* stdout is already redirected */\n-\t\t\t\t NULL, 0,\n-\t\t\t\t &buf, 0);\n-\t\tif (rc)\n-\t\t\tfputs(buf.buf, stderr);\n-\t\tstrbuf_release(&buf);\n-\t\treturn rc;\n-\t}\n-\n-\treturn run_command(&cmd);\n+\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n+\t\treturn run_command_silent_on_success(&cmd);\n+\telse\n+\t\treturn run_command(&cmd);\n }\n \n static int rest_is_empty(const struct strbuf *sb, int start)\n-- \n2.18.0\n\n"},{"id":"350805","messageId":"20180625134419.18435-3-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180625134419.18435-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-25T13:44:18Z","receivedAt":"2018-06-25T13:44:54Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites checkout_onto() from shell to C. The new version is called\ndetach_onto(), given its role.\n\nA new command (“detach-onto”) is added to rebase--helper.c, as well as\na new flag, “verbose”, to avoid silencing the output of the checkout\noperation called by detach_onto().\n\nThe function `run_git_checkout()` will also be used in the next commit,\ntherefore its code is not part of `detach_onto()`.\n\nThe shell version is then stripped in favour of a call to the helper.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 9 +++++++--\n git-rebase--interactive.sh | 13 ++++---------\n sequencer.c | 36 ++++++++++++++++++++++++++++++++++++\n sequencer.h | 4 ++++\n 4 files changed, 51 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex d2990b210..2dfd55c76 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {\n int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n-\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0;\n+\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;\n \tint abbreviate_commands = 0, rebase_cousins = -1;\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"rebase-merges\", &rebase_merges, N_(\"rebase merge commits\")),\n \t\tOPT_BOOL(0, \"rebase-cousins\", &rebase_cousins,\n \t\t\t N_(\"keep original branch points of cousins\")),\n+\t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n \t\t\t\tCONTINUE),\n \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\n@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"edit-todo\", &command,\n \t\t\t N_(\"edit the todo list during an interactive rebase\"),\n \t\t\t EDIT_TODO),\n+\t\tOPT_CMDMODE(0, \"detach-onto\", &command,\n+\t\t\t N_(\"checkout a commit\"), DETACH_ONTO),\n \t\tOPT_END()\n \t};\n \n@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!append_todo_help(0, keep_empty);\n \tif (command == EDIT_TODO && argc == 1)\n \t\treturn !!edit_todo_list(flags);\n+\tif (command == DETACH_ONTO && argc == 4)\n+\t\treturn !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2defe607f..2f5761d49 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -70,13 +70,6 @@ collapse_todo_ids() {\n \tgit rebase--helper --shorten-ids\n }\n \n-# Switch to the branch in $into and notify it in the reflog\n-checkout_onto () {\n-\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n-\toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n-\tgit update-ref ORIG_HEAD $orig_head\n-}\n-\n get_missing_commit_check_level () {\n \tcheck_level=$(git config --get rebase.missingCommitsCheck)\n \tcheck_level=${check_level:-ignore}\n@@ -188,7 +181,8 @@ EOF\n \n \tgit rebase--helper --check-todo-list || {\n \t\tret=$?\n-\t\tcheckout_onto\n+\t\tgit rebase--helper --detach-onto \"$onto_name\" \"$onto\" \\\n+\t\t \"$orig_head\" ${verbose:+--verbose}\n \t\texit $ret\n \t}\n \n@@ -198,7 +192,8 @@ EOF\n \tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n \tdie \"Could not skip unnecessary pick commands\"\n \n-\tcheckout_onto\n+\tgit rebase--helper --detach-onto \"$onto_name\" \"$onto\" \"$orig_head\" \\\n+\t ${verbose:+--verbose}\n \trequire_clean_work_tree \"rebase\"\n \texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n \t --continue\ndiff --git a/sequencer.c b/sequencer.c\nindex 9a9725e23..ee6374921 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3129,6 +3129,42 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n+static int run_git_checkout(struct replay_opts *opts, const char *commit,\n+\t\t\t\tint verbose, const char *action)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tcmd.git_cmd = 1;\n+\n+\targv_array_push(&cmd.args, \"checkout\");\n+\targv_array_push(&cmd.args, commit);\n+\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n+\n+\tif (verbose)\n+\t\treturn run_command(&cmd);\n+\telse\n+\t\treturn run_command_silent_on_success(&cmd);\n+}\n+\n+int detach_onto(struct replay_opts *opts,\n+\t\tconst char *onto_name, const char *onto,\n+\t\tconst char *orig_head, unsigned verbose)\n+{\n+\tstruct object_id oid;\n+\tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n+\n+\tif (get_oid(orig_head, &oid))\n+\t\treturn error(_(\"%s: not a valid OID\"), orig_head);\n+\n+\tif (run_git_checkout(opts, onto, verbose, action)) {\n+\t\tapply_autostash(opts);\n+\t\tsequencer_remove_state(opts);\n+\t\treturn error(_(\"could not detach HEAD\"));\n+\t}\n+\n+\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 35730b13e..9f0ac5e75 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -100,6 +100,10 @@ int update_head_with_reflog(const struct commit *old_head,\n void commit_post_rewrite(const struct commit *current_head,\n \t\t\t const struct object_id *new_head);\n \n+int detach_onto(struct replay_opts *opts,\n+\t\tconst char *onto_name, const char *onto,\n+\t\tconst char *orig_head, unsigned verbose);\n+\n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n void print_commit_summary(const char *prefix, const struct object_id *oid,\n-- \n2.18.0\n\n"},{"id":"350806","messageId":"20180625134419.18435-4-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180625134419.18435-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v4 3/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-25T13:44:19Z","receivedAt":"2018-06-25T13:44:59Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites (the misnamed) setup_reflog_action() from shell to C. The\nnew version is called checkout_onto().\n\nA new command is added to rebase--helper.c, “checkout-base”. The shell\nversion is then stripped.\n\nAs $GIT_REFLOG_ACTION is no longer set at the first call of\ncheckout_onto(), a call to comment_for_reflog() is added at the\nbeginning of this function.\n\nIf the commit OID provided to checkout_onto() is empty, nothing happens\nand no errors are returned, otherwise it would break some\ntests (t3404.92 and t3404.93).\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 6 +++++-\n git-rebase--interactive.sh | 15 +--------------\n sequencer.c | 14 ++++++++++++++\n sequencer.h | 2 ++\n 4 files changed, 22 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex 2dfd55c76..a00b1091d 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -17,7 +17,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO, CHECKOUT_ONTO\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -53,6 +53,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\t EDIT_TODO),\n \t\tOPT_CMDMODE(0, \"detach-onto\", &command,\n \t\t\t N_(\"checkout a commit\"), DETACH_ONTO),\n+\t\tOPT_CMDMODE(0, \"checkout-onto\", &command,\n+\t\t\t N_(\"checkout the base commit\"), CHECKOUT_ONTO),\n \t\tOPT_END()\n \t};\n \n@@ -98,5 +100,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!edit_todo_list(flags);\n \tif (command == DETACH_ONTO && argc == 4)\n \t\treturn !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);\n+\tif (command == CHECKOUT_ONTO && argc == 2)\n+\t\treturn !!checkout_onto(&opts, argv[1], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2f5761d49..375c36fb5 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -112,19 +112,6 @@ initiate_action () {\n \tesac\n }\n \n-setup_reflog_action () {\n-\tcomment_for_reflog start\n-\n-\tif test ! -z \"$switch_to\"\n-\tthen\n-\t\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n-\t\toutput git checkout \"$switch_to\" -- ||\n-\t\t\tdie \"$(eval_gettext \"Could not checkout \\$switch_to\")\"\n-\n-\t\tcomment_for_reflog start\n-\tfi\n-}\n-\n init_basic_state () {\n \torig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n@@ -206,7 +193,7 @@ git_rebase__interactive () {\n \t\treturn 0\n \tfi\n \n-\tsetup_reflog_action\n+\tgit rebase--helper --checkout-onto \"$switch_to\" ${verbose:+--verbose}\n \tinit_basic_state\n \n \tinit_revisions_and_shortrevisions\ndiff --git a/sequencer.c b/sequencer.c\nindex ee6374921..620b41e96 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3165,6 +3165,20 @@ int detach_onto(struct replay_opts *opts,\n \treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n }\n \n+int checkout_onto(struct replay_opts *opts, const char *commit,\n+\t\t int verbose)\n+{\n+\tconst char *action;\n+\n+\tif (commit && *commit) {\n+\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n+\t\tif (run_git_checkout(opts, commit, verbose, action))\n+\t\t\treturn error(_(\"Could not checkout %s\"), commit);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 9f0ac5e75..afbb2edf1 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -103,6 +103,8 @@ void commit_post_rewrite(const struct commit *current_head,\n int detach_onto(struct replay_opts *opts,\n \t\tconst char *onto_name, const char *onto,\n \t\tconst char *orig_head, unsigned verbose);\n+int checkout_onto(struct replay_opts *opts, const char *commit,\n+\t\t int verbose);\n \n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n-- \n2.18.0\n\n"},{"id":"350834","messageId":"xmqqr2kuq3qg.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"7094810.i0lmrLi0k7@andromeda","subject":"Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-25T15:34:47Z","receivedAt":"2018-06-25T15:34:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> Hi Junio,\n>\n> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :\n>> Alban Gruin writes:\n>> > This rewrites (the misnamed) setup_reflog_action() from shell to C. The\n>> > new version is called checkout_base_commit().\n>> \n>> ;-) on the \"misnamed\" part. Indeed, setting up the comment for the\n>> reflog entry is secondary to what this function wants to do, which\n>> is to check out the branch to be rebased.\n>> \n>> I do not think \"base_commit\" is a good name, either, though. When I\n>> hear 'base' in the context of 'rebase', I would imagine that the\n>> speaker is talking about the bottom of the range of the commits to\n>> be rebased (i.e. \"rebase --onto ONTO BASE BRANCH\", which replays\n>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the\n>> result), not the top of the range or the branch these commits are\n>> taken from.\n>> \n>\n> Perhaps should I name this function checkout_onto(), and rename \n> checkout_onto() to \"detach_onto()\"? And I would reorder those two commits in \n> the series, as this would be very confusing.\n\nI may be misunderstanding what is happening in the function, but I\nthink it is checking out neither the onto or the base commit. The\nfunction instead is about checking out the branch to be rebased\nbefore anything else happens when the optional argument is\ngiven (and when the optional argument is not given, then we rebase\nthe current branch so there is no need to check it out upfront), no?\n\n\n"},{"id":"350876","messageId":"4dfaa8d4-09d0-1156-43a4-687c3389e039@gmail.com","threadId":"48730","inReplyTo":"xmqqr2kuq3qg.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-25T18:21:31Z","receivedAt":"2018-06-25T18:21:48Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Junio,\n\nLe 25/06/2018 à 17:34, Junio C Hamano a écrit :\n> Alban Gruin writes:\n> \n>> Hi Junio,\n>>\n>> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :\n>>> Alban Gruin writes:\n>>>> This rewrites (the misnamed) setup_reflog_action() from shell to C. The\n>>>> new version is called checkout_base_commit().\n>>>\n>>> ;-) on the \"misnamed\" part. Indeed, setting up the comment for the\n>>> reflog entry is secondary to what this function wants to do, which\n>>> is to check out the branch to be rebased.\n>>>\n>>> I do not think \"base_commit\" is a good name, either, though. When I\n>>> hear 'base' in the context of 'rebase', I would imagine that the\n>>> speaker is talking about the bottom of the range of the commits to\n>>> be rebased (i.e. \"rebase --onto ONTO BASE BRANCH\", which replays\n>>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the\n>>> result), not the top of the range or the branch these commits are\n>>> taken from.\n>>>\n>>\n>> Perhaps should I name this function checkout_onto(), and rename \n>> checkout_onto() to \"detach_onto()\"? And I would reorder those two commits in \n>> the series, as this would be very confusing.\n> \n> I may be misunderstanding what is happening in the function, but I\n> think it is checking out neither the onto or the base commit. The\n> function instead is about checking out the branch to be rebased\n> before anything else happens when the optional argument is\n> given (and when the optional argument is not given, then we rebase\n> the current branch so there is no need to check it out upfront), no?\n> \n> \n\nYes, you’re right.\n\nNow I really don’t know how to call this function.\ncheckout_top_of_range(), perhaps?\n\nCheers,\nAlban\n\n"},{"id":"350901","messageId":"nycvar.QRO.7.76.6.1806252313140.11870@tvgsbejvaqbjf.bet","threadId":"48730","inReplyTo":"4dfaa8d4-09d0-1156-43a4-687c3389e039@gmail.com","subject":"Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-06-25T21:14:57Z","receivedAt":"2018-06-25T21:15:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 25 Jun 2018, Alban Gruin wrote:\n\n> Le 25/06/2018 à 17:34, Junio C Hamano a écrit :\n> > Alban Gruin writes:\n> > \n> >> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :\n> >>> Alban Gruin writes:\n> >>>> This rewrites (the misnamed) setup_reflog_action() from shell to C. The\n> >>>> new version is called checkout_base_commit().\n> >>>\n> >>> ;-) on the \"misnamed\" part. Indeed, setting up the comment for the\n> >>> reflog entry is secondary to what this function wants to do, which\n> >>> is to check out the branch to be rebased.\n> >>>\n> >>> I do not think \"base_commit\" is a good name, either, though. When I\n> >>> hear 'base' in the context of 'rebase', I would imagine that the\n> >>> speaker is talking about the bottom of the range of the commits to\n> >>> be rebased (i.e. \"rebase --onto ONTO BASE BRANCH\", which replays\n> >>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the\n> >>> result), not the top of the range or the branch these commits are\n> >>> taken from.\n> >>>\n> >>\n> >> Perhaps should I name this function checkout_onto(), and rename \n> >> checkout_onto() to \"detach_onto()\"? And I would reorder those two commits in \n> >> the series, as this would be very confusing.\n> > \n> > I may be misunderstanding what is happening in the function, but I\n> > think it is checking out neither the onto or the base commit. The\n> > function instead is about checking out the branch to be rebased\n> > before anything else happens when the optional argument is\n> > given (and when the optional argument is not given, then we rebase\n> > the current branch so there is no need to check it out upfront), no?\n> > \n> > \n> \n> Yes, you’re right.\n> \n> Now I really don’t know how to call this function.\n> checkout_top_of_range(), perhaps?\n\nPratik refactored some code from sequencer.c into checkout.c/checkout.h\ntoday to do exactly that. It is not polished yet, but probably will be\ntomorrow. It provides a function `int detach_head_to(struct object_oid\n*oid, const char *action, const char *reflog_message)`. Maybe use that\ndirectly, once the commit is available?\n\nCiao,\nDscho"},{"id":"350948","messageId":"CAOZc8M-oYND29eiZOfmKk7O-b34dxWphdgix0PgeryfYpu26Ng@mail.gmail.com","threadId":"48730","inReplyTo":"nycvar.QRO.7.76.6.1806252313140.11870@tvgsbejvaqbjf.bet","subject":"Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Pratik Karki","fromEmail":"predatoramigo@gmail.com","sentAt":"2018-06-26T09:13:59Z","receivedAt":"2018-06-26T09:14:15Z","isPatch":true,"sender":{"key":"predatoramigo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12194950?v=4"},"body":"Hi,\n\nOn Tue, Jun 26, 2018 at 3:00 AM Johannes Schindelin\n wrote:\n\n> Pratik refactored some code from sequencer.c into checkout.c/checkout.h\n> today to do exactly that. It is not polished yet, but probably will be\n> tomorrow. It provides a function `int detach_head_to(struct object_oid\n> *oid, const char *action, const char *reflog_message)`. Maybe use that\n> directly, once the commit is available?\n\nThe commit is here[1].\n\n[1]: https://github.com/git/git/pull/505/commits/47031d4706bd1eccd2816ecdaaf6e9cd700909aa\n\nCheers,\nPratik\n"},{"id":"350984","messageId":"xmqqsh59lad7.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"20180625134419.18435-3-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T17:35:00Z","receivedAt":"2018-06-26T17:35:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> This rewrites checkout_onto() from shell to C. The new version is called\n> detach_onto(), given its role.\n\nThe name, given its role, may be good, but is the implementtaion\nrobust enough to fulfill the promise its name gives?\n\n> \tgit rebase--helper --check-todo-list || {\n> \t\tret=$?\n> -\t\tcheckout_onto\n> +\t\tgit rebase--helper --detach-onto \"$onto_name\" \"$onto\" \\\n> +\t\t \"$orig_head\" ${verbose:+--verbose}\n\nHere, $onto_name is what the end-user gave us (e.g. it is\n\"master...\" in \"git rebase --onto=master... base\"), while $onto is a\n40-hex object name of the commit. $orig_head is also a 40-hex\nobject name.\n\nAnd this call shows how the above shell scriptlet calls into the\ndetach_onto() thing ...\n\n> +\tif (command == DETACH_ONTO && argc == 4)\n> +\t\treturn !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);\n\n... which is defined like so:\n\n> +int detach_onto(struct replay_opts *opts,\n> +\t\tconst char *onto_name, const char *onto,\n> +\t\tconst char *orig_head, unsigned verbose)\n> +{\n> +\tstruct object_id oid;\n> +\tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n> +\n> +\tif (get_oid(orig_head, &oid))\n> +\t\treturn error(_(\"%s: not a valid OID\"), orig_head);\n\nWhich means that this can be more strict to use get_oid_hex() to\ncatch possible mistakes in the caller.\n\n> +\tif (run_git_checkout(opts, onto, verbose, action)) {\n\nAnd this could be a bit problematic, as we can see below how the\n\"checkout\" thing does not guarantee \"detaching\" at all ...\n\n> +\t\tapply_autostash(opts);\n> +\t\tsequencer_remove_state(opts);\n> +\t\treturn error(_(\"could not detach HEAD\"));\n> +\t}\n> +\n> +\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n> +}\n> +\n\n... which can be seen here ...\n\n> +static int run_git_checkout(struct replay_opts *opts, const char *commit,\n> +\t\t\t\tint verbose, const char *action)\n> +{\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> +\tcmd.git_cmd = 1;\n> +\n> +\targv_array_push(&cmd.args, \"checkout\");\n> +\targv_array_push(&cmd.args, commit);\n> +\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n> +\n> +\tif (verbose)\n> +\t\treturn run_command(&cmd);\n> +\telse\n> +\t\treturn run_command_silent_on_success(&cmd);\n> +}\n\nThis drives the external command \"git checkout\" with _any_ string\nthe caller passes in \"commit\". If the variable happens to have\n'master', for example, it would be \"git checkout master\" and if you\nhave a branch with that name, it will not detach but check out the\nbranch to build on it. It is a caller's responsibility to give a\nsuitable \"commit\" if it wants to use this helper to detach.\n\nSo perhaps the caller of this function in detach_onto() should pass\n\"%s^0\" or even do something like\n\n\tstruct object_id onto_oid;\n\tchar onto_hex[GIT_MAX_HEXSZ + 1];\n\n\tif (get_oid(onto, &onto_oid) || oid_to_hex_r(onto_hex, &onto_oid))\n\t\treturn error(...);\n\tif (run_git_checkout(opts, onto_hex, verbose, action)) {\n\t\t...\n\nto ensure that it keeps the promise its name gives.\n\nI can hear \"Oh, but it is a bug in the caller to give anything that\nwon't result in detaching in 'onto'\" but that is not a valid excuse,\ngiven that this _public_ function is called \"detach_onto\". Making\nsure detachment happens is its responsibility, not its callers'.\n\nOr we could do a cop-out alternative of commenting the function in *.h\nfile to say \"onto must be given as 40-hex\", with a code to make sure\nthe caller really gave us a 40-hex and not a branch name. That is a\nless ideal but probably acceptable alternative.\n\n> static const char rescheduled_advice[] =\n> N_(\"Could not execute the todo command\\n\"\n> \"\\n\"\n> diff --git a/sequencer.h b/sequencer.h\n> index 35730b13e..9f0ac5e75 100644\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -100,6 +100,10 @@ int update_head_with_reflog(const struct commit *old_head,\n> void commit_post_rewrite(const struct commit *current_head,\n> \t\t\t const struct object_id *new_head);\n> \n> +int detach_onto(struct replay_opts *opts,\n> +\t\tconst char *onto_name, const char *onto,\n> +\t\tconst char *orig_head, unsigned verbose);\n> +\n> #define SUMMARY_INITIAL_COMMIT (1 << 0)\n> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n> void print_commit_summary(const char *prefix, const struct object_id *oid,\n"},{"id":"350985","messageId":"xmqqo9fxl9wn.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"4dfaa8d4-09d0-1156-43a4-687c3389e039@gmail.com","subject":"Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-26T17:44:56Z","receivedAt":"2018-06-26T17:45:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n>>>> I do not think \"base_commit\" is a good name, either, though. When I\n>>>> hear 'base' in the context of 'rebase', I would imagine that the\n>>>> speaker is talking about the bottom of the range of the commits to\n>>>> be rebased (i.e. \"rebase --onto ONTO BASE BRANCH\", which replays\n>>>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the\n>>>> result), not the top of the range or the branch these commits are\n>>>> taken from.\n>>> ...\n> Now I really don’t know how to call this function.\n> checkout_top_of_range(), perhaps?\n\nIf this is a straight rewrite of setup_reflog_action, i.e. the\ndivision of labor between its caller and this function is such that\nthe caller blindly calls it without even checking if it got the\noptional \"branch to be rebased\" argument and this function is\nresponsible to decide if the preparatory checkout of the named\nbranch is necessary, then any name that does not even hint that\ncheckout is done conditionally would not work well.\n\nHow about callilng it \"prepare_branch_to_be_rebased()\"? This\nfunction, at least the original setup_reflog_action, responds to\n\n\tgit rebase [--onto ONTO] UPSTREAM\n\nby doing nothing (i.e. the result of preparation is to do nothing\nbecause we are rebasing the commits between UPSTREAM and currently\nchecked out state on top of ONTO, or UPSTREAM if ONTO is not\ngiven) and it responds to\n\n\tgit rebase [--onto ONTO] UPSTREAM BRANCH\n\nby checking out BRANCH (most importantly, when given a concrete\nbranch name, it checks the branch out, and does not detach at the\ncommit at the tip of the branch), because that is the first\npreparatory step to rebase the BRANCH.\n\n\n"},{"id":"351339","messageId":"20180629151435.31868-1-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180625134419.18435-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-29T15:14:32Z","receivedAt":"2018-06-29T15:15:09Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This patch series rewrites the reflog operations from shell to C. This\nis part of the effort to rewrite interactive rebase in C.\n\nThe first commit is dedicated to creating a function to silence a\ncommand, as the sequencer will do in several places with these patches.\n\nThis branch is based on ag/rebase-i-rewrite-todo, and does not conflict\nwith pu (as of 2018-06-29).\n\nChanges since v4:\n\n - Changing the order of setup_reflog_action() and checkout_onto()\n rewrites in the series\n\n - checkout_onto() is no longer renamed in C\n\n - setup_reflog_action() is renamed to prepare_branch_to_be_rebased(),\n and not to checkout_onto().\n\nAlban Gruin (3):\n sequencer: add a new function to silence a command, except if it\n fails.\n rebase -i: rewrite setup_reflog_action() in C\n rebase -i: rewrite checkout_onto() in C\n\n builtin/rebase--helper.c | 14 ++++-\n git-rebase--interactive.sh | 39 ++------------\n sequencer.c | 101 +++++++++++++++++++++++++++----------\n sequencer.h | 6 +++\n 4 files changed, 98 insertions(+), 62 deletions(-)\n\n-- \n2.18.0\n\n"},{"id":"351340","messageId":"20180629151435.31868-2-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180629151435.31868-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v5 1/3] sequencer: add a new function to silence a command, except if it fails.","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-29T15:14:33Z","receivedAt":"2018-06-29T15:15:12Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This adds a new function, run_command_silent_on_success(), to\nredirect the stdout and stderr of a command to a strbuf, and then to run\nthat command. This strbuf is printed only if the command fails. It is\nfunctionnaly similar to output() from git-rebase.sh.\n\nrun_git_commit() is then refactored to use of\nrun_command_silent_on_success().\n\nSigned-off-by: Alban Gruin \n---\n sequencer.c | 51 +++++++++++++++++++++++++--------------------------\n 1 file changed, 25 insertions(+), 26 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cb7ec9807..d9545b366 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -769,6 +769,23 @@ N_(\"you have staged changes in your working tree\\n\"\n #define VERIFY_MSG (1<<4)\n #define CREATE_ROOT_COMMIT (1<<5)\n \n+static int run_command_silent_on_success(struct child_process *cmd)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint rc;\n+\n+\tcmd->stdout_to_stderr = 1;\n+\trc = pipe_command(cmd,\n+\t\t\t NULL, 0,\n+\t\t\t NULL, 0,\n+\t\t\t &buf, 0);\n+\n+\tif (rc)\n+\t\tfputs(buf.buf, stderr);\n+\tstrbuf_release(&buf);\n+\treturn rc;\n+}\n+\n /*\n * If we are cherry-pick, and if the merge did not result in\n * hand-editing, we will hit this commit and inherit the original\n@@ -823,18 +840,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \n \tcmd.git_cmd = 1;\n \n-\tif (is_rebase_i(opts)) {\n-\t\tif (!(flags & EDIT_MSG)) {\n-\t\t\tcmd.stdout_to_stderr = 1;\n-\t\t\tcmd.err = -1;\n-\t\t}\n-\n-\t\tif (read_env_script(&cmd.env_array)) {\n-\t\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n+\tif (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {\n+\t\tconst char *gpg_opt = gpg_sign_opt_quoted(opts);\n \n-\t\t\treturn error(_(staged_changes_advice),\n-\t\t\t\t gpg_opt, gpg_opt);\n-\t\t}\n+\t\treturn error(_(staged_changes_advice),\n+\t\t\t gpg_opt, gpg_opt);\n \t}\n \n \targv_array_push(&cmd.args, \"commit\");\n@@ -864,21 +874,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,\n \tif (opts->allow_empty_message)\n \t\targv_array_push(&cmd.args, \"--allow-empty-message\");\n \n-\tif (cmd.err == -1) {\n-\t\t/* hide stderr on success */\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tint rc = pipe_command(&cmd,\n-\t\t\t\t NULL, 0,\n-\t\t\t\t /* stdout is already redirected */\n-\t\t\t\t NULL, 0,\n-\t\t\t\t &buf, 0);\n-\t\tif (rc)\n-\t\t\tfputs(buf.buf, stderr);\n-\t\tstrbuf_release(&buf);\n-\t\treturn rc;\n-\t}\n-\n-\treturn run_command(&cmd);\n+\tif (is_rebase_i(opts) && !(flags & EDIT_MSG))\n+\t\treturn run_command_silent_on_success(&cmd);\n+\telse\n+\t\treturn run_command(&cmd);\n }\n \n static int rest_is_empty(const struct strbuf *sb, int start)\n-- \n2.18.0\n\n"},{"id":"351341","messageId":"20180629151435.31868-3-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180629151435.31868-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-29T15:14:34Z","receivedAt":"2018-06-29T15:15:14Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites (the misnamed) setup_reflog_action() from shell to C. The\nnew version is called prepare_branch_to_be_rebased().\n\nA new command is added to rebase--helper.c, “checkout-base”, as well as\na new flag, “verbose”, to avoid silencing the output of the checkout\noperation called by checkout_base_commit().\n\nThe function `run_git_checkout()` will also be used in the next commit,\ntherefore its code is not part of `checkout_base_commit()`.\n\nThe shell version is then stripped in favour of a call to the helper.\n\nAs $GIT_REFLOG_ACTION is no longer set at the first call of\ncheckout_onto(), a call to comment_for_reflog() is added at the\nbeginning of this function.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 9 +++++++--\n git-rebase--interactive.sh | 16 ++--------------\n sequencer.c | 31 +++++++++++++++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 43 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex 731a64971..6c789e78b 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -13,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {\n int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts = REPLAY_OPTS_INIT;\n-\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0;\n+\tunsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;\n \tint abbreviate_commands = 0, rebase_cousins = -1;\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -28,6 +28,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"rebase-merges\", &rebase_merges, N_(\"rebase merge commits\")),\n \t\tOPT_BOOL(0, \"rebase-cousins\", &rebase_cousins,\n \t\t\t N_(\"keep original branch points of cousins\")),\n+\t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n \t\tOPT_CMDMODE(0, \"continue\", &command, N_(\"continue rebase\"),\n \t\t\t\tCONTINUE),\n \t\tOPT_CMDMODE(0, \"abort\", &command, N_(\"abort rebase\"),\n@@ -51,6 +52,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"edit-todo\", &command,\n \t\t\t N_(\"edit the todo list during an interactive rebase\"),\n \t\t\t EDIT_TODO),\n+\t\tOPT_CMDMODE(0, \"prepare-branch\", &command,\n+\t\t\t N_(\"prepare the branch to be rebased\"), PREPARE_BRANCH),\n \t\tOPT_END()\n \t};\n \n@@ -94,5 +97,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!append_todo_help(0, keep_empty);\n \tif (command == EDIT_TODO && argc == 1)\n \t\treturn !!edit_todo_list(flags);\n+\tif (command == PREPARE_BRANCH && argc == 2)\n+\t\treturn !!prepare_branch_to_be_rebased(&opts, argv[1], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 2defe607f..77e972bb6 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -72,6 +72,7 @@ collapse_todo_ids() {\n \n # Switch to the branch in $into and notify it in the reflog\n checkout_onto () {\n+\tcomment_for_reflog start\n \tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n \toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n \tgit update-ref ORIG_HEAD $orig_head\n@@ -119,19 +120,6 @@ initiate_action () {\n \tesac\n }\n \n-setup_reflog_action () {\n-\tcomment_for_reflog start\n-\n-\tif test ! -z \"$switch_to\"\n-\tthen\n-\t\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n-\t\toutput git checkout \"$switch_to\" -- ||\n-\t\t\tdie \"$(eval_gettext \"Could not checkout \\$switch_to\")\"\n-\n-\t\tcomment_for_reflog start\n-\tfi\n-}\n-\n init_basic_state () {\n \torig_head=$(git rev-parse --verify HEAD) || die \"$(gettext \"No HEAD?\")\"\n \tmkdir -p \"$state_dir\" || die \"$(eval_gettext \"Could not create temporary \\$state_dir\")\"\n@@ -211,7 +199,7 @@ git_rebase__interactive () {\n \t\treturn 0\n \tfi\n \n-\tsetup_reflog_action\n+\tgit rebase--helper --prepare-branch \"$switch_to\" ${verbose:+--verbose}\n \tinit_basic_state\n \n \tinit_revisions_and_shortrevisions\ndiff --git a/sequencer.c b/sequencer.c\nindex d9545b366..dd0cf3cb5 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3134,6 +3134,37 @@ static const char *reflog_message(struct replay_opts *opts,\n \treturn buf.buf;\n }\n \n+static int run_git_checkout(struct replay_opts *opts, const char *commit,\n+\t\t\t\tint verbose, const char *action)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tcmd.git_cmd = 1;\n+\n+\targv_array_push(&cmd.args, \"checkout\");\n+\targv_array_push(&cmd.args, commit);\n+\targv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION \"=%s\", action);\n+\n+\tif (verbose)\n+\t\treturn run_command(&cmd);\n+\telse\n+\t\treturn run_command_silent_on_success(&cmd);\n+}\n+\n+int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,\n+\t\t\t\t int verbose)\n+{\n+\tconst char *action;\n+\n+\tif (commit && *commit) {\n+\t\taction = reflog_message(opts, \"start\", \"checkout %s\", commit);\n+\t\tif (run_git_checkout(opts, commit, verbose, action))\n+\t\t\treturn error(_(\"Could not checkout %s\"), commit);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex ffab798f1..3821420b0 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -106,6 +106,9 @@ int update_head_with_reflog(const struct commit *old_head,\n void commit_post_rewrite(const struct commit *current_head,\n \t\t\t const struct object_id *new_head);\n \n+int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,\n+\t\t\t\t int verbose);\n+\n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n void print_commit_summary(const char *prefix, const struct object_id *oid,\n-- \n2.18.0\n\n"},{"id":"351342","messageId":"20180629151435.31868-4-alban.gruin@gmail.com","threadId":"48730","inReplyTo":"20180629151435.31868-1-alban.gruin@gmail.com","subject":"[GSoC][PATCH v5 3/3] rebase -i: rewrite checkout_onto() in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-06-29T15:14:35Z","receivedAt":"2018-06-29T15:15:21Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"This rewrites checkout_onto() from shell to C.\n\nA new command (“checkout-onto”) is added to rebase--helper.c. The shell\nversion is then stripped.\n\nSigned-off-by: Alban Gruin \n---\n builtin/rebase--helper.c | 7 ++++++-\n git-rebase--interactive.sh | 25 ++++---------------------\n sequencer.c | 19 +++++++++++++++++++\n sequencer.h | 3 +++\n 4 files changed, 32 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c\nindex 6c789e78b..0091e094b 100644\n--- a/builtin/rebase--helper.c\n+++ b/builtin/rebase--helper.c\n@@ -18,7 +18,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \tenum {\n \t\tCONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,\n \t\tCHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,\n-\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH\n+\t\tADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,\n+\t\tCHECKOUT_ONTO\n \t} command = 0;\n \tstruct option options[] = {\n \t\tOPT_BOOL(0, \"ff\", &opts.allow_ff, N_(\"allow fast-forward\")),\n@@ -54,6 +55,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\t\t EDIT_TODO),\n \t\tOPT_CMDMODE(0, \"prepare-branch\", &command,\n \t\t\t N_(\"prepare the branch to be rebased\"), PREPARE_BRANCH),\n+\t\tOPT_CMDMODE(0, \"checkout-onto\", &command,\n+\t\t\t N_(\"checkout a commit\"), CHECKOUT_ONTO),\n \t\tOPT_END()\n \t};\n \n@@ -99,5 +102,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)\n \t\treturn !!edit_todo_list(flags);\n \tif (command == PREPARE_BRANCH && argc == 2)\n \t\treturn !!prepare_branch_to_be_rebased(&opts, argv[1], verbose);\n+\tif (command == CHECKOUT_ONTO && argc == 4)\n+\t\treturn !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);\n \tusage_with_options(builtin_rebase_helper_usage, options);\n }\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 77e972bb6..b68f108f2 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -28,17 +28,6 @@ case \"$comment_char\" in\n \t;;\n esac\n \n-orig_reflog_action=\"$GIT_REFLOG_ACTION\"\n-\n-comment_for_reflog () {\n-\tcase \"$orig_reflog_action\" in\n-\t''|rebase*)\n-\t\tGIT_REFLOG_ACTION=\"rebase -i ($1)\"\n-\t\texport GIT_REFLOG_ACTION\n-\t\t;;\n-\tesac\n-}\n-\n die_abort () {\n \tapply_autostash\n \trm -rf \"$state_dir\"\n@@ -70,14 +59,6 @@ collapse_todo_ids() {\n \tgit rebase--helper --shorten-ids\n }\n \n-# Switch to the branch in $into and notify it in the reflog\n-checkout_onto () {\n-\tcomment_for_reflog start\n-\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n-\toutput git checkout $onto || die_abort \"$(gettext \"could not detach HEAD\")\"\n-\tgit update-ref ORIG_HEAD $orig_head\n-}\n-\n get_missing_commit_check_level () {\n \tcheck_level=$(git config --get rebase.missingCommitsCheck)\n \tcheck_level=${check_level:-ignore}\n@@ -176,7 +157,8 @@ EOF\n \n \tgit rebase--helper --check-todo-list || {\n \t\tret=$?\n-\t\tcheckout_onto\n+\t\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \\\n+\t\t \"$orig_head\" ${verbose:+--verbose}\n \t\texit $ret\n \t}\n \n@@ -186,7 +168,8 @@ EOF\n \tonto=\"$(git rebase--helper --skip-unnecessary-picks)\" ||\n \tdie \"Could not skip unnecessary pick commands\"\n \n-\tcheckout_onto\n+\tgit rebase--helper --checkout-onto \"$onto_name\" \"$onto\" \"$orig_head\" \\\n+\t ${verbose:+--verbose}\n \trequire_clean_work_tree \"rebase\"\n \texec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \\\n \t --continue\ndiff --git a/sequencer.c b/sequencer.c\nindex dd0cf3cb5..ec6de4eda 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3165,6 +3165,25 @@ int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,\n \treturn 0;\n }\n \n+int checkout_onto(struct replay_opts *opts,\n+\t\t const char *onto_name, const char *onto,\n+\t\t const char *orig_head, unsigned verbose)\n+{\n+\tstruct object_id oid;\n+\tconst char *action = reflog_message(opts, \"start\", \"checkout %s\", onto_name);\n+\n+\tif (get_oid(orig_head, &oid))\n+\t\treturn error(_(\"%s: not a valid OID\"), orig_head);\n+\n+\tif (run_git_checkout(opts, onto, verbose, action)) {\n+\t\tapply_autostash(opts);\n+\t\tsequencer_remove_state(opts);\n+\t\treturn error(_(\"could not detach HEAD\"));\n+\t}\n+\n+\treturn update_ref(NULL, \"ORIG_HEAD\", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);\n+}\n+\n static const char rescheduled_advice[] =\n N_(\"Could not execute the todo command\\n\"\n \"\\n\"\ndiff --git a/sequencer.h b/sequencer.h\nindex 3821420b0..ba5e59f02 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -108,6 +108,9 @@ void commit_post_rewrite(const struct commit *current_head,\n \n int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,\n \t\t\t\t int verbose);\n+int checkout_onto(struct replay_opts *opts,\n+\t\t const char *onto_name, const char *onto,\n+\t\t const char *orig_head, unsigned verbose);\n \n #define SUMMARY_INITIAL_COMMIT (1 << 0)\n #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)\n-- \n2.18.0\n\n"},{"id":"351360","messageId":"xmqqvaa1v8op.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"20180629151435.31868-3-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-29T16:50:14Z","receivedAt":"2018-06-29T16:50:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> +\tgit rebase--helper --prepare-branch \"$switch_to\" ${verbose:+--verbose}\n> \tinit_basic_state\n> \n> \tinit_revisions_and_shortrevisions\n> diff --git a/sequencer.c b/sequencer.c\n> index d9545b366..dd0cf3cb5 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3134,6 +3134,37 @@ static const char *reflog_message(struct replay_opts *opts,\n> \treturn buf.buf;\n> }\n> ...\n> +int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,\n> +\t\t\t\t int verbose)\n> +{\n> +\tconst char *action;\n> +\n> +\tif (commit && *commit) {\n\nThe sloppyness of this callee is forced by the sloppy caller, which\ndoes not check if \"$switch_to\" has any value before calling \"prepare\nbranch to be rebased\" function. A less sloppy caller would check to\nsee if we have the optional \"before doing anything, check out this\nbranch, as it is the branch we are trying to rebase\" argument, and\nrefrain from calling this function, so there is no need for this \"if\ncommit is given and is not an empty string\" check.\n\nAnd hopefully, when GSoC is over, the caller, that is still in shell\nat this step, would also be rewritten in C and by that time the\ncaller will become less sloppy, removing the need for this check.\n\nHence, I think the reason why the check is still here, and our\ndesire to eventually remove the check, should be documented with an\nin-code comment around here (the usual \"NEEDSWORK:\" comment is\nfine).\n\n"},{"id":"351361","messageId":"xmqqr2kpv8ft.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"20180629151435.31868-1-alban.gruin@gmail.com","subject":"Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-29T16:55:34Z","receivedAt":"2018-06-29T16:55:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> This patch series rewrites the reflog operations from shell to C. This\n> is part of the effort to rewrite interactive rebase in C.\n>\n> The first commit is dedicated to creating a function to silence a\n> command, as the sequencer will do in several places with these patches.\n>\n> This branch is based on ag/rebase-i-rewrite-todo, and does not conflict\n> with pu (as of 2018-06-29).\n\nLet's aggregate these topics into a single topic, and perhaps call\nit ag/rebase-i-in-c or something like that. Pretending as if they\nare separately replaceable does not make much sense, as you are not\nrerolling the earlier one and keep going forward with producing more\nparts that depends on the parts that have been submitted earlier.\n\nThese three patches looked more-or-less good; I may have spotted a\nfew nits and suggested improvements, though.\n\nThanks.\n"},{"id":"351367","messageId":"xmqqzhzdtpt4.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"xmqqr2kpv8ft.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-29T18:23:19Z","receivedAt":"2018-06-29T18:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Let's aggregate these topics into a single topic, and perhaps call\n> it ag/rebase-i-in-c or something like that. Pretending as if they\n> are separately replaceable does not make much sense, as you are not\n> rerolling the earlier one and keep going forward with producing more\n> parts that depends on the parts that have been submitted earlier.\n\nSo here is what I tentatively did.\n\n $ git log --oneline --reverse master..ag/rebase-i-in-c\n 4d303fb608 rebase--interactive: rewrite append_todo_help() in C\n b4ffe143a9 editor: add a function to launch the sequence editor\n 4ebe39cef9 rebase--interactive: rewrite the edit-todo functionality in C\n 0ff6bf7646 sequencer: add a new function to silence a command, except if it fails.\n 36784b351f rebase -i: rewrite setup_reflog_action() in C\n 415cac57ee rebase -i: rewrite checkout_onto() in C\n\nIn several hours please fetch from me and look for \"Merge branch\n'ag/rebase-i-in-c' to pu\" to see how they exactly look like; some of\nthe patches might not be the latest ones, in which case you may need\nto prod me to get them replaced (resending them as a whole with\nincremented v$n header is probably the easiest if we need to do so).\n\nThanks.\n"},{"id":"351515","messageId":"e703f571-4966-f3aa-da81-fae1962c8906@gmail.com","threadId":"48730","inReplyTo":"xmqqzhzdtpt4.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2018-07-02T10:36:21Z","receivedAt":"2018-07-02T10:36:33Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Junio,\n\nLe 29/06/2018 à 20:23, Junio C Hamano a écrit :\n> Junio C Hamano writes:\n> \n>> Let's aggregate these topics into a single topic, and perhaps call\n>> it ag/rebase-i-in-c or something like that. Pretending as if they\n>> are separately replaceable does not make much sense, as you are not\n>> rerolling the earlier one and keep going forward with producing more\n>> parts that depends on the parts that have been submitted earlier.\n> \n> So here is what I tentatively did.\n> \n> $ git log --oneline --reverse master..ag/rebase-i-in-c\n> 4d303fb608 rebase--interactive: rewrite append_todo_help() in C\n> b4ffe143a9 editor: add a function to launch the sequence editor\n> 4ebe39cef9 rebase--interactive: rewrite the edit-todo functionality in C\n> 0ff6bf7646 sequencer: add a new function to silence a command, except if it fails.\n> 36784b351f rebase -i: rewrite setup_reflog_action() in C\n> 415cac57ee rebase -i: rewrite checkout_onto() in C\n> \n> In several hours please fetch from me and look for \"Merge branch\n> 'ag/rebase-i-in-c' to pu\" to see how they exactly look like; some of\n> the patches might not be the latest ones, in which case you may need\n> to prod me to get them replaced (resending them as a whole with\n> incremented v$n header is probably the easiest if we need to do so).\n> \n> Thanks.\n> \n\nThe patches about append_todo_help() and edit-todo are not up to date,\nso I’ll resend them in a few minutes. Otherwise, this looks good to me.\n\nCheers,\nAlban\n\n"},{"id":"351634","messageId":"xmqqwoucqj89.fsf@gitster-ct.c.googlers.com","threadId":"48730","inReplyTo":"e703f571-4966-f3aa-da81-fae1962c8906@gmail.com","subject":"Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-03T18:15:02Z","receivedAt":"2018-07-03T18:15:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n> The patches about append_todo_help() and edit-todo are not up to date,\n> so I’ll resend them in a few minutes\n\nGood.\n"}]}