* [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.
2018-06-19 15:44 ` [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations " Alban Gruin
@ 2018-06-19 15:44 ` Alban Gruin
2018-06-21 9:37 ` Johannes Schindelin
2018-06-19 15:44 ` [GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
` (4 subsequent siblings)
5 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-06-19 15:44 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This adds a new function, run_command_silent_on_success(), to redirect
the stdout and stderr of a command to a strbuf, and then to run that
command. This strbuf is printed only if the command fails. It also takes
a parameter, “verbose”. When true, the command is executed without
redirecting its output. It is functionnally similar to output() from
git-rebase.sh.
run_git_commit() is then refactored to use of
run_command_silent_on_success().
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
sequencer.c | 55 +++++++++++++++++++++++++++++--------------------------
1 file changed, 29 insertions(+), 26 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7cc76332e..9aa7ddb33 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -766,6 +766,29 @@ N_("you have staged changes in your working tree\n"
#define VERIFY_MSG (1<<4)
#define CREATE_ROOT_COMMIT (1<<5)
+static int run_command_silent_on_success(struct child_process *cmd,
+ unsigned verbose)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ if (verbose)
+ return run_command(cmd);
+
+ /* hide stderr on success */
+ cmd->stdout_to_stderr = 1;
+ rc = pipe_command(cmd,
+ NULL, 0,
+ /* stdout is already redirected */
+ NULL, 0,
+ &buf, 0);
+
+ if (rc)
+ fputs(buf.buf, stderr);
+ strbuf_release(&buf);
+ return rc;
+}
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
cmd.git_cmd = 1;
- if (is_rebase_i(opts)) {
- if (!(flags & EDIT_MSG)) {
- cmd.stdout_to_stderr = 1;
- cmd.err = -1;
- }
+ if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
+ const char *gpg_opt = gpg_sign_opt_quoted(opts);
- if (read_env_script(&cmd.env_array)) {
- const char *gpg_opt = gpg_sign_opt_quoted(opts);
-
- return error(_(staged_changes_advice),
- gpg_opt, gpg_opt);
- }
+ return error(_(staged_changes_advice),
+ gpg_opt, gpg_opt);
}
argv_array_push(&cmd.args, "commit");
@@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(&cmd.args, "--allow-empty-message");
- if (cmd.err == -1) {
- /* hide stderr on success */
- struct strbuf buf = STRBUF_INIT;
- int rc = pipe_command(&cmd,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- &buf, 0);
- if (rc)
- fputs(buf.buf, stderr);
- strbuf_release(&buf);
- return rc;
- }
-
- return run_command(&cmd);
+ return run_command_silent_on_success(&cmd,
+ !(is_rebase_i(opts) && !(flags & EDIT_MSG)));
}
static int rest_is_empty(const struct strbuf *sb, int start)
--
2.16.4
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.
2018-06-19 15:44 ` [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-06-21 9:37 ` Johannes Schindelin
2018-06-21 11:53 ` Alban Gruin
0 siblings, 1 reply; 51+ messages in thread
From: Johannes Schindelin @ 2018-06-21 9:37 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki, phillip.wood,
Elijah Newren
Hi Alban,
On Tue, 19 Jun 2018, Alban Gruin wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 7cc76332e..9aa7ddb33 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -766,6 +766,29 @@ N_("you have staged changes in your working tree\n"
> #define VERIFY_MSG (1<<4)
> #define CREATE_ROOT_COMMIT (1<<5)
>
> +static int run_command_silent_on_success(struct child_process *cmd,
> + unsigned verbose)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + int rc;
> +
> + if (verbose)
> + return run_command(cmd);
> +
> + /* hide stderr on success */
> + cmd->stdout_to_stderr = 1;
This comment is a bit misleading: that line does not hide stderr on
success, it redirects stdout to stderr instead.
> + rc = pipe_command(cmd,
> + NULL, 0,
> + /* stdout is already redirected */
> + NULL, 0,
> + &buf, 0);
> +
> + if (rc)
> + fputs(buf.buf, stderr);
> + strbuf_release(&buf);
> + return rc;
> +}
> +
> /*
> * If we are cherry-pick, and if the merge did not result in
> * hand-editing, we will hit this commit and inherit the original
> @@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>
> cmd.git_cmd = 1;
>
> - if (is_rebase_i(opts)) {
> - if (!(flags & EDIT_MSG)) {
> - cmd.stdout_to_stderr = 1;
> - cmd.err = -1;
> - }
This code made sure that we *only* do this redirection, and stderr
buffering, if `git commit` is called non-interactively. When it is called
interactively, redirecting stdout and stderr is absolutely not what we
want.
> + if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
> + const char *gpg_opt = gpg_sign_opt_quoted(opts);
>
> - if (read_env_script(&cmd.env_array)) {
> - const char *gpg_opt = gpg_sign_opt_quoted(opts);
> -
> - return error(_(staged_changes_advice),
> - gpg_opt, gpg_opt);
> - }
> + return error(_(staged_changes_advice),
> + gpg_opt, gpg_opt);
> }
>
> argv_array_push(&cmd.args, "commit");
> @@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> if (opts->allow_empty_message)
> argv_array_push(&cmd.args, "--allow-empty-message");
>
> - if (cmd.err == -1) {
> - /* hide stderr on success */
> - struct strbuf buf = STRBUF_INIT;
> - int rc = pipe_command(&cmd,
> - NULL, 0,
> - /* stdout is already redirected */
> - NULL, 0,
> - &buf, 0);
> - if (rc)
> - fputs(buf.buf, stderr);
> - strbuf_release(&buf);
> - return rc;
> - }
> -
> - return run_command(&cmd);
> + return run_command_silent_on_success(&cmd,
> + !(is_rebase_i(opts) && !(flags & EDIT_MSG)));
It would probably make more sense to change the signature of
`run_command_silent_on_success()` to not even take the `int verbose`
parameter: why call it "silent on success" when we can ask it *not* to be
silent on success?
And then you can avoid this overly-long line (as well as the quite
convoluted Boolean logic that took me a couple of seconds to verify) very
elegantly by this code:
if (is_rebase_i(opts) && !(flags & EDIT_MSG))
return run_command_silent_on_success(&cmd);
return run_command(&cmd);
I vaguely recall that I wanted to make this an option in the `struct
child_process` when I originally introduced this code, but I think it was
Peff who suggested that doing it "by hand" was the more appropriate way
here because I use it only once.
My recollection might fail me, but if it is correct, maybe that would be a
good way forward, to make this an `int silent_on_success:1;` field?
Ciao,
Dscho
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails.
2018-06-21 9:37 ` Johannes Schindelin
@ 2018-06-21 11:53 ` Alban Gruin
0 siblings, 0 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-21 11:53 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki, phillip.wood,
Elijah Newren
Hi Johannes,
Le 21/06/2018 à 11:37, Johannes Schindelin a écrit :
> Hi Alban,
>
> On Tue, 19 Jun 2018, Alban Gruin wrote:
>
>> diff --git a/sequencer.c b/sequencer.c
>> index 7cc76332e..9aa7ddb33 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -766,6 +766,29 @@ N_("you have staged changes in your working tree\n"
>> #define VERIFY_MSG (1<<4)
>> #define CREATE_ROOT_COMMIT (1<<5)
>>
>> +static int run_command_silent_on_success(struct child_process *cmd,
>> + unsigned verbose)
>> +{
>> + struct strbuf buf = STRBUF_INIT;
>> + int rc;
>> +
>> + if (verbose)
>> + return run_command(cmd);
>> +
>> + /* hide stderr on success */
>> + cmd->stdout_to_stderr = 1;
>
> This comment is a bit misleading: that line does not hide stderr on
> success, it redirects stdout to stderr instead.
>
>> + rc = pipe_command(cmd,
>> + NULL, 0,
>> + /* stdout is already redirected */
>> + NULL, 0,
>> + &buf, 0);
>> +
>> + if (rc)
>> + fputs(buf.buf, stderr);
>> + strbuf_release(&buf);
>> + return rc;
>> +}
>> +
>> /*
>> * If we are cherry-pick, and if the merge did not result in
>> * hand-editing, we will hit this commit and inherit the original
>> @@ -820,18 +843,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>>
>> cmd.git_cmd = 1;
>>
>> - if (is_rebase_i(opts)) {
>> - if (!(flags & EDIT_MSG)) {
>> - cmd.stdout_to_stderr = 1;
>> - cmd.err = -1;
>> - }
>
> This code made sure that we *only* do this redirection, and stderr
> buffering, if `git commit` is called non-interactively. When it is called
> interactively, redirecting stdout and stderr is absolutely not what we
> want.
>
>> + if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
>> + const char *gpg_opt = gpg_sign_opt_quoted(opts);
>>
>> - if (read_env_script(&cmd.env_array)) {
>> - const char *gpg_opt = gpg_sign_opt_quoted(opts);
>> -
>> - return error(_(staged_changes_advice),
>> - gpg_opt, gpg_opt);
>> - }
>> + return error(_(staged_changes_advice),
>> + gpg_opt, gpg_opt);
>> }
>>
>> argv_array_push(&cmd.args, "commit");
>> @@ -861,21 +877,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>> if (opts->allow_empty_message)
>> argv_array_push(&cmd.args, "--allow-empty-message");
>>
>> - if (cmd.err == -1) {
>> - /* hide stderr on success */
>> - struct strbuf buf = STRBUF_INIT;
>> - int rc = pipe_command(&cmd,
>> - NULL, 0,
>> - /* stdout is already redirected */
>> - NULL, 0,
>> - &buf, 0);
>> - if (rc)
>> - fputs(buf.buf, stderr);
>> - strbuf_release(&buf);
>> - return rc;
>> - }
>> -
>> - return run_command(&cmd);
>> + return run_command_silent_on_success(&cmd,
>> + !(is_rebase_i(opts) && !(flags & EDIT_MSG)));
>
> It would probably make more sense to change the signature of
> `run_command_silent_on_success()` to not even take the `int verbose`
> parameter: why call it "silent on success" when we can ask it *not* to be
> silent on success?
>
> And then you can avoid this overly-long line (as well as the quite
> convoluted Boolean logic that took me a couple of seconds to verify) very
> elegantly by this code:
>
> if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> return run_command_silent_on_success(&cmd);
> return run_command(&cmd);
>
> I vaguely recall that I wanted to make this an option in the `struct
> child_process` when I originally introduced this code, but I think it was
> Peff who suggested that doing it "by hand" was the more appropriate way
> here because I use it only once.
>
> My recollection might fail me, but if it is correct, maybe that would be a
> good way forward, to make this an `int silent_on_success:1;` field?
>
I think I found it:
https://public-inbox.org/git/1e82aeabb906a35175362418b2b4957fae50c3b0.1481642927.git.johannes.schindelin@gmx.de/
Apparently, you wanted to introduce a new RUN_ flag for
run_command_v_opt_cd_env(), but the change was qualified as a “bolted-on
feature” by Johannes Sixt.
So, I will remove the “verbose” argument in the reroll.
Cheers,
Alban
^ permalink raw reply [flat|nested] 51+ messages in thread
* [GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-19 15:44 ` [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations " Alban Gruin
2018-06-19 15:44 ` [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-06-19 15:44 ` Alban Gruin
2018-06-21 10:34 ` Johannes Schindelin
2018-06-19 15:44 ` [GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() " Alban Gruin
` (3 subsequent siblings)
5 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-06-19 15:44 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This rewrites setup_reflog_action() from shell to C. The new version is
called checkout_base_commit().
A new command is added to rebase--helper.c, “checkout-base”, as such as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by checkout_base_commit().
The shell version is then stripped in favour of a call to the helper. As
$GIT_REFLOG_ACTION is not longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
builtin/rebase--helper.c | 9 +++++++--
git-rebase--interactive.sh | 16 ++--------------
sequencer.c | 28 ++++++++++++++++++++++++++++
sequencer.h | 3 +++
4 files changed, 40 insertions(+), 16 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..7cd74da2e 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
N_("keep original branch points of cousins")),
+ OPT__VERBOSE(&verbose, N_("print subcommands output even if they succeed")),
OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
OPT_CMDMODE(0, "edit-todo", &command,
N_("edit the todo list during an interactive rebase"),
EDIT_TODO),
+ OPT_CMDMODE(0, "checkout-base", &command,
+ N_("checkout the base commit"), CHECKOUT_BASE),
OPT_END()
};
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+ if (command == CHECKOUT_BASE && argc == 2)
+ return !!checkout_base_commit(&opts, argv[1], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..af46cf9c2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
# Switch to the branch in $into and notify it in the reflog
checkout_onto () {
+ comment_for_reflog start
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
esac
}
-setup_reflog_action () {
- comment_for_reflog start
-
- if test ! -z "$switch_to"
- then
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
- output git checkout "$switch_to" -- ||
- die "$(eval_gettext "Could not checkout \$switch_to")"
-
- comment_for_reflog start
- fi
-}
-
init_basic_state () {
orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
return 0
fi
- setup_reflog_action
+ git rebase--helper --checkout-base "$switch_to" ${verbose:+--verbose}
init_basic_state
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 9aa7ddb33..a7a73e3ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3133,6 +3133,34 @@ static const char *reflog_message(struct replay_opts *opts,
return buf.buf;
}
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+ int verbose, const char *action)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
+ cmd.git_cmd = 1;
+
+ argv_array_push(&cmd.args, "checkout");
+ argv_array_push(&cmd.args, commit);
+ argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+ return run_command_silent_on_success(&cmd, verbose);
+}
+
+int checkout_base_commit(struct replay_opts *opts, const char *commit,
+ int verbose)
+{
+ const char *action;
+
+ if (commit && *commit) {
+ action = reflog_message(opts, "start", "checkout %s", commit);
+ if (run_git_checkout(opts, commit, verbose, action))
+ return error(_("Could not checkout %s"), commit);
+ }
+
+ return 0;
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 35730b13e..42c3dda81 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
void commit_post_rewrite(const struct commit *current_head,
const struct object_id *new_head);
+int checkout_base_commit(struct replay_opts *opts, const char *commit,
+ int verbose);
+
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
void print_commit_summary(const char *prefix, const struct object_id *oid,
--
2.16.4
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-19 15:44 ` [GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-06-21 10:34 ` Johannes Schindelin
0 siblings, 0 replies; 51+ messages in thread
From: Johannes Schindelin @ 2018-06-21 10:34 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki, phillip.wood,
Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 4392 bytes --]
Hi Alban,
On Tue, 19 Jun 2018, Alban Gruin wrote:
> This rewrites setup_reflog_action() from shell to C. The new version is
> called checkout_base_commit().
This sounds like a serious mistake. How is it that a function that sets up
a reflog action all of a sudden checks out a base commit?
But you are correct, of course, that is exactly what the shell script
function does!
So maybe insert "the (misnamed)" after "This rewrites"? That would help me
in the future when I stumble across this commit again.
> A new command is added to rebase--helper.c, “checkout-base”, as such as
s/as such as/as well as/
> a new flag, “verbose”, to avoid silencing the output of the checkout
> operation called by checkout_base_commit().
>
> The shell version is then stripped in favour of a call to the helper. As
Please start a new paragraph after this sentence, before the "As ...",
because those two sentences represent different thoughts.
> $GIT_REFLOG_ACTION is not longer set at the first call of
s/not longer/no longer/
> checkout_onto(), a call to comment_for_reflog() is added at the
> beginning of this function.
>
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
> builtin/rebase--helper.c | 9 +++++++--
> git-rebase--interactive.sh | 16 ++--------------
> sequencer.c | 28 ++++++++++++++++++++++++++++
> sequencer.h | 3 +++
> 4 files changed, 40 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
> index d2990b210..7cd74da2e 100644
> --- a/builtin/rebase--helper.c
> +++ b/builtin/rebase--helper.c
> @@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
> int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> {
> struct replay_opts opts = REPLAY_OPTS_INIT;
> - unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
> + unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
> int abbreviate_commands = 0, rebase_cousins = -1;
> enum {
> CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
> CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
> - ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
> + ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
> } command = 0;
> struct option options[] = {
> OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
> @@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
> OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
> N_("keep original branch points of cousins")),
> + OPT__VERBOSE(&verbose, N_("print subcommands output even if they succeed")),
That's very specific a description that applies only to this command
mode... Maybe just say `"be verbose"`, and as a bonus avoid a too-long
line?
> diff --git a/sequencer.c b/sequencer.c
> index 9aa7ddb33..a7a73e3ef 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3133,6 +3133,34 @@ static const char *reflog_message(struct replay_opts *opts,
> return buf.buf;
> }
>
> +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> + int verbose, const char *action)
> +{
> + struct child_process cmd = CHILD_PROCESS_INIT;
> +
> + cmd.git_cmd = 1;
> +
> + argv_array_push(&cmd.args, "checkout");
> + argv_array_push(&cmd.args, commit);
> + argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> + return run_command_silent_on_success(&cmd, verbose);
> +}
Are you planning on reusing that code anywhere else than in
`checkout_base_commit()`? If yes, sure, it makes sense to keep as separate
function. Otherwise I would fold it into that latter function, for reading
simplicity.
> +
> +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> + int verbose)
> +{
> + const char *action;
> +
> + if (commit && *commit) {
> + action = reflog_message(opts, "start", "checkout %s", commit);
> + if (run_git_checkout(opts, commit, verbose, action))
> + return error(_("Could not checkout %s"), commit);
> + }
> +
> + return 0;
> +}
> +
> static const char rescheduled_advice[] =
> N_("Could not execute the todo command\n"
> "\n"
The rest looks fine to me, thank you!
Dscho
^ permalink raw reply [flat|nested] 51+ messages in thread
* [GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() in C
2018-06-19 15:44 ` [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations " Alban Gruin
2018-06-19 15:44 ` [GSoC][PATCH v2 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
2018-06-19 15:44 ` [GSoC][PATCH v2 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-06-19 15:44 ` Alban Gruin
2018-06-21 10:38 ` Johannes Schindelin
2018-06-19 18:35 ` [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations " Stefan Beller
` (2 subsequent siblings)
5 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-06-19 15:44 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This rewrites checkout_onto() from shell to C.
A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
builtin/rebase--helper.c | 7 ++++++-
git-rebase--interactive.sh | 25 ++++---------------------
sequencer.c | 19 +++++++++++++++++++
sequencer.h | 3 +++
4 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7cd74da2e..19c1dba9a 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE,
+ CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "checkout-base", &command,
N_("checkout the base commit"), CHECKOUT_BASE),
+ OPT_CMDMODE(0, "checkout-onto", &command,
+ N_("checkout a commit"), CHECKOUT_ONTO),
OPT_END()
};
@@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!edit_todo_list(flags);
if (command == CHECKOUT_BASE && argc == 2)
return !!checkout_base_commit(&opts, argv[1], verbose);
+ if (command == CHECKOUT_ONTO && argc == 4)
+ return !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index af46cf9c2..7b6142a76 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
;;
esac
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
- case "$orig_reflog_action" in
- ''|rebase*)
- GIT_REFLOG_ACTION="rebase -i ($1)"
- export GIT_REFLOG_ACTION
- ;;
- esac
-}
-
die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
}
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
- comment_for_reflog start
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
- output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
- git update-ref ORIG_HEAD $orig_head
-}
-
get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
git rebase--helper --check-todo-list || {
ret=$?
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" \
+ "$orig_head" ${verbose:+--verbose}
exit $ret
}
@@ -186,7 +168,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+ ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
--continue
diff --git a/sequencer.c b/sequencer.c
index a7a73e3ef..9165bf96c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3161,6 +3161,25 @@ int checkout_base_commit(struct replay_opts *opts, const char *commit,
return 0;
}
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose)
+{
+ struct object_id oid;
+ const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
+
+ if (get_oid(orig_head, &oid))
+ return error(_("%s: not a valid OID"), orig_head);
+
+ if (run_git_checkout(opts, onto, verbose, action)) {
+ apply_autostash(opts);
+ sequencer_remove_state(opts);
+ return error(_("could not detach HEAD"));
+ }
+
+ return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 42c3dda81..eda03ce32 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -102,6 +102,9 @@ void commit_post_rewrite(const struct commit *current_head,
int checkout_base_commit(struct replay_opts *opts, const char *commit,
int verbose);
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose);
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
--
2.16.4
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() in C
2018-06-19 15:44 ` [GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() " Alban Gruin
@ 2018-06-21 10:38 ` Johannes Schindelin
0 siblings, 0 replies; 51+ messages in thread
From: Johannes Schindelin @ 2018-06-21 10:38 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki, phillip.wood,
Elijah Newren
Hi Alban,
On Tue, 19 Jun 2018, Alban Gruin wrote:
> diff --git a/sequencer.c b/sequencer.c
> index a7a73e3ef..9165bf96c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3161,6 +3161,25 @@ int checkout_base_commit(struct replay_opts *opts, const char *commit,
> return 0;
> }
>
> +int checkout_onto(struct replay_opts *opts,
> + const char *onto_name, const char *onto,
> + const char *orig_head, unsigned verbose)
> +{
> + struct object_id oid;
> + const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
> +
> + if (get_oid(orig_head, &oid))
> + return error(_("%s: not a valid OID"), orig_head);
> +
> + if (run_git_checkout(opts, onto, verbose, action)) {
Ah, so this is the reason for the split.
If you send a new iteration of this patch series, could you do me a favor
and add a paragraph to the commit message of 2/3, saying something like
this: "The function `run_git_checkout()` will be used on its own in the
next commit, therefore the code is not folded into
`checkout_base_commit()`? That way, I do not have to burden my working
memory with this bit of information ;-)
Thanks,
Dscho
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C
2018-06-19 15:44 ` [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations " Alban Gruin
` (2 preceding siblings ...)
2018-06-19 15:44 ` [GSoC][PATCH v2 3/3] rebase -i: rewrite checkout_onto() " Alban Gruin
@ 2018-06-19 18:35 ` Stefan Beller
2018-06-21 8:39 ` Johannes Schindelin
2018-06-21 14:17 ` [GSoC][PATCH v3 " Alban Gruin
5 siblings, 0 replies; 51+ messages in thread
From: Stefan Beller @ 2018-06-19 18:35 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Christian Couder, Pratik Karki, Johannes Schindelin,
Phillip Wood, Elijah Newren
On Tue, Jun 19, 2018 at 8:45 AM Alban Gruin <alban.gruin@gmail.com> wrote:
>
> This patch series rewrites the reflog operations from shell to C. This
> is part of the effort to rewrite interactive rebase in C.
This series looks good to me.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations in C
2018-06-19 15:44 ` [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations " Alban Gruin
` (3 preceding siblings ...)
2018-06-19 18:35 ` [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations " Stefan Beller
@ 2018-06-21 8:39 ` Johannes Schindelin
2018-06-21 14:17 ` [GSoC][PATCH v3 " Alban Gruin
5 siblings, 0 replies; 51+ messages in thread
From: Johannes Schindelin @ 2018-06-21 8:39 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki, phillip.wood,
Elijah Newren
Hi Alban,
On Tue, 19 Jun 2018, Alban Gruin wrote:
> --
> 2.16.4
You might want to update ;-)
https://github.com/git-for-windows/git/wiki/FAQ#should-i-upgrade-to-a-newer-git-for-windows-version
Ciao,
Johannes
^ permalink raw reply [flat|nested] 51+ messages in thread* [GSoC][PATCH v3 0/3] rebase -i: rewrite reflog operations in C
2018-06-19 15:44 ` [GSoC][PATCH v2 0/3] rebase -i: rewrite reflog operations " Alban Gruin
` (4 preceding siblings ...)
2018-06-21 8:39 ` Johannes Schindelin
@ 2018-06-21 14:17 ` Alban Gruin
2018-06-21 14:17 ` [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
` (3 more replies)
5 siblings, 4 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-21 14:17 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
Thes patch series rewrites the reflog operations from shell to C. This
is part of the effort to rewrite interactive rebase in C.
The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.
This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-21).
Changes since v2:
- Removing the “verbose” parameter to run_command_silent_on_success()
- Rewording some parts of the second commit
- Changing the help for the “--verbose” flag
Alban Gruin (3):
sequencer: add a new function to silence a command, except if it
fails.
rebase -i: rewrite setup_reflog_action() in C
rebase -i: rewrite checkout_onto() in C
builtin/rebase--helper.c | 14 +++++-
git-rebase--interactive.sh | 39 ++-------------
sequencer.c | 98 ++++++++++++++++++++++++++++----------
sequencer.h | 6 +++
4 files changed, 96 insertions(+), 61 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 51+ messages in thread* [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.
2018-06-21 14:17 ` [GSoC][PATCH v3 " Alban Gruin
@ 2018-06-21 14:17 ` Alban Gruin
2018-06-21 22:03 ` Junio C Hamano
2018-06-21 14:17 ` [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
` (2 subsequent siblings)
3 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-06-21 14:17 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This adds a new function, run_command_silent_on_success(), to
redirect the stdout and stderr of a command to a strbuf, and then to run
that command. This strbuf is printed only if the command fails. It is
functionnaly similar to output() from git-rebase.sh.
run_git_commit() is then refactored to use of
run_command_silent_on_success().
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
sequencer.c | 49 ++++++++++++++++++++++++-------------------------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7cc76332e..51c8ab7ac 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -766,6 +766,24 @@ N_("you have staged changes in your working tree\n"
#define VERIFY_MSG (1<<4)
#define CREATE_ROOT_COMMIT (1<<5)
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ cmd->stdout_to_stderr = 1;
+ rc = pipe_command(cmd,
+ NULL, 0,
+ /* stdout is already redirected */
+ NULL, 0,
+ &buf, 0);
+
+ if (rc)
+ fputs(buf.buf, stderr);
+ strbuf_release(&buf);
+ return rc;
+}
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -820,18 +838,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
cmd.git_cmd = 1;
- if (is_rebase_i(opts)) {
- if (!(flags & EDIT_MSG)) {
- cmd.stdout_to_stderr = 1;
- cmd.err = -1;
- }
+ if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
+ const char *gpg_opt = gpg_sign_opt_quoted(opts);
- if (read_env_script(&cmd.env_array)) {
- const char *gpg_opt = gpg_sign_opt_quoted(opts);
-
- return error(_(staged_changes_advice),
- gpg_opt, gpg_opt);
- }
+ return error(_(staged_changes_advice),
+ gpg_opt, gpg_opt);
}
argv_array_push(&cmd.args, "commit");
@@ -861,20 +872,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(&cmd.args, "--allow-empty-message");
- if (cmd.err == -1) {
- /* hide stderr on success */
- struct strbuf buf = STRBUF_INIT;
- int rc = pipe_command(&cmd,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- &buf, 0);
- if (rc)
- fputs(buf.buf, stderr);
- strbuf_release(&buf);
- return rc;
- }
-
+ if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+ return run_command_silent_on_success(&cmd);
return run_command(&cmd);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.
2018-06-21 14:17 ` [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-06-21 22:03 ` Junio C Hamano
2018-06-22 20:47 ` Alban Gruin
0 siblings, 1 reply; 51+ messages in thread
From: Junio C Hamano @ 2018-06-21 22:03 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren
Alban Gruin <alban.gruin@gmail.com> writes:
> This adds a new function, run_command_silent_on_success(), to
> redirect the stdout and stderr of a command to a strbuf, and then to run
> that command. This strbuf is printed only if the command fails. It is
> functionnaly similar to output() from git-rebase.sh.
>
> run_git_commit() is then refactored to use of
> run_command_silent_on_success().
It might be just a difference in viewpoint, but I think it is more
customary in this project (hence it will be easier to understand and
accept by readers of the patch) if a change like this is explained
NOT as "introducing a new function and then rewrite an existing code
to use it", and instead as "extract a helper function from an
existing code so that future callers can reuse it."
> +static int run_command_silent_on_success(struct child_process *cmd)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + int rc;
> +
> + cmd->stdout_to_stderr = 1;
> + rc = pipe_command(cmd,
> + NULL, 0,
> + /* stdout is already redirected */
I can see that this comment was inherited from the original place
this code was lifted from (and that is why I say this is not "adding
a new helper and rewriting an existing piece of code to use it" but
is "extracting this function out of an existing code for future
reuse"), but does it still make sense in the new context to keep the
same comment?
The original in run_git_commit() made the .stdout_to_stderr=1
assignment many lines before it called pipe_command(), and it made
sense to remind readers why we are passing NULL to the out buffer
and only passing the err buffer here. But in the context of this
new helper function, the redirection that may make such a reminder
necessary sits immediately before the pipe_command() call for
everybody to see.
> + NULL, 0,
> + &buf, 0);
> +
> + if (rc)
> + fputs(buf.buf, stderr);
> + strbuf_release(&buf);
> + return rc;
> +}
> @@ -820,18 +838,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
>
> cmd.git_cmd = 1;
>
> - if (is_rebase_i(opts)) {
> - if (!(flags & EDIT_MSG)) {
> - cmd.stdout_to_stderr = 1;
> - cmd.err = -1;
> - }
> + if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
> + const char *gpg_opt = gpg_sign_opt_quoted(opts);
>
> - if (read_env_script(&cmd.env_array)) {
> - const char *gpg_opt = gpg_sign_opt_quoted(opts);
> -
> - return error(_(staged_changes_advice),
> - gpg_opt, gpg_opt);
> - }
> + return error(_(staged_changes_advice),
> + gpg_opt, gpg_opt);
> }
The original in this hunk is criminally incomprehensible ;-) The
assignment of .stdout_to_stderr=1 does not have to happen this early
in the code, and also using .err being -1 to communicate that the
"capture and show error only upon error" magic is going to be used
to the later code is unnecessarily opaque. The new code gets rid
of that ugliness from this earlier hunk and instead makes this part
only about reading the various state files (and reporting failure to
do so).
> @@ -861,20 +872,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> if (opts->allow_empty_message)
> argv_array_push(&cmd.args, "--allow-empty-message");
>
> - if (cmd.err == -1) {
> - /* hide stderr on success */
> - struct strbuf buf = STRBUF_INIT;
> - int rc = pipe_command(&cmd,
> - NULL, 0,
> - /* stdout is already redirected */
> - NULL, 0,
> - &buf, 0);
> - if (rc)
> - fputs(buf.buf, stderr);
> - strbuf_release(&buf);
> - return rc;
> - }
> -
> + if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> + return run_command_silent_on_success(&cmd);
> return run_command(&cmd);
> }
And then, the updated code in this hunk checks the EDIT_MSG bit
instead. It probably is easier to understand the code if you added
an "else" after, to highlight the fact that this is choosing one out
of two possible ways to run "cmd", i.e.
if (is_rebase_i(opts) && !(flags & EDIT_MSG))
return run_command_silent_on_success(&cmd);
else
return run_command(&cmd);
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails.
2018-06-21 22:03 ` Junio C Hamano
@ 2018-06-22 20:47 ` Alban Gruin
0 siblings, 0 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-22 20:47 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren
Hi Junio,
Le 22/06/2018 à 00:03, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > This adds a new function, run_command_silent_on_success(), to
> > redirect the stdout and stderr of a command to a strbuf, and then to run
> > that command. This strbuf is printed only if the command fails. It is
> > functionnaly similar to output() from git-rebase.sh.
> >
> > run_git_commit() is then refactored to use of
> > run_command_silent_on_success().
>
> It might be just a difference in viewpoint, but I think it is more
> customary in this project (hence it will be easier to understand and
> accept by readers of the patch) if a change like this is explained
> NOT as "introducing a new function and then rewrite an existing code
> to use it", and instead as "extract a helper function from an
> existing code so that future callers can reuse it."
>
I like your wording. It’s not a difference of point of view, I’m just bad at
writing commit messages ;)
> > +static int run_command_silent_on_success(struct child_process *cmd)
> > +{
> > + struct strbuf buf = STRBUF_INIT;
> > + int rc;
> > +
> > + cmd->stdout_to_stderr = 1;
> > + rc = pipe_command(cmd,
> > + NULL, 0,
> > + /* stdout is already redirected */
>
> I can see that this comment was inherited from the original place
> this code was lifted from (and that is why I say this is not "adding
> a new helper and rewriting an existing piece of code to use it" but
> is "extracting this function out of an existing code for future
> reuse"), but does it still make sense in the new context to keep the
> same comment?
>
> The original in run_git_commit() made the .stdout_to_stderr=1
> assignment many lines before it called pipe_command(), and it made
> sense to remind readers why we are passing NULL to the out buffer
> and only passing the err buffer here. But in the context of this
> new helper function, the redirection that may make such a reminder
> necessary sits immediately before the pipe_command() call for
> everybody to see.
>
Yeah, you’re right.
> > @@ -861,20 +872,8 @@ static int run_git_commit(const char *defmsg, struct
> > replay_opts *opts,>
> > if (opts->allow_empty_message)
> >
> > argv_array_push(&cmd.args, "--allow-empty-message");
> >
> > - if (cmd.err == -1) {
> > - /* hide stderr on success */
> > - struct strbuf buf = STRBUF_INIT;
> > - int rc = pipe_command(&cmd,
> > - NULL, 0,
> > - /* stdout is already redirected */
> > - NULL, 0,
> > - &buf, 0);
> > - if (rc)
> > - fputs(buf.buf, stderr);
> > - strbuf_release(&buf);
> > - return rc;
> > - }
> > -
> > + if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> > + return run_command_silent_on_success(&cmd);
> >
> > return run_command(&cmd);
> >
> > }
>
> It probably is easier to understand the code if you added
> an "else" after, to highlight the fact that this is choosing one out
> of two possible ways to run "cmd", i.e.
>
> if (is_rebase_i(opts) && !(flags & EDIT_MSG))
> return run_command_silent_on_success(&cmd);
> else
> return run_command(&cmd);
Okay.
Cheers,
Alban
^ permalink raw reply [flat|nested] 51+ messages in thread
* [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-21 14:17 ` [GSoC][PATCH v3 " Alban Gruin
2018-06-21 14:17 ` [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-06-21 14:17 ` Alban Gruin
2018-06-22 16:27 ` Junio C Hamano
2018-06-21 14:17 ` [GSoC][PATCH v3 3/3] rebase -i: rewrite checkout_onto() " Alban Gruin
2018-06-25 13:44 ` [GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations " Alban Gruin
3 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-06-21 14:17 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This rewrites (the misnamed) setup_reflog_action() from shell to C. The
new version is called checkout_base_commit().
A new command is added to rebase--helper.c, “checkout-base”, as well as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by checkout_base_commit().
The function `run_git_checkout()` will also be used in the next commit,
therefore its code is not part of `checkout_base_commit()`.
The shell version is then stripped in favour of a call to the helper.
As $GIT_REFLOG_ACTION is no longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
builtin/rebase--helper.c | 9 +++++++--
git-rebase--interactive.sh | 16 ++--------------
sequencer.c | 30 ++++++++++++++++++++++++++++++
sequencer.h | 3 +++
4 files changed, 42 insertions(+), 16 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..fb5996a2c 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
N_("keep original branch points of cousins")),
+ OPT__VERBOSE(&verbose, N_("be verbose")),
OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
OPT_CMDMODE(0, "edit-todo", &command,
N_("edit the todo list during an interactive rebase"),
EDIT_TODO),
+ OPT_CMDMODE(0, "checkout-base", &command,
+ N_("checkout the base commit"), CHECKOUT_BASE),
OPT_END()
};
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+ if (command == CHECKOUT_BASE && argc == 2)
+ return !!checkout_base_commit(&opts, argv[1], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..af46cf9c2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
# Switch to the branch in $into and notify it in the reflog
checkout_onto () {
+ comment_for_reflog start
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
esac
}
-setup_reflog_action () {
- comment_for_reflog start
-
- if test ! -z "$switch_to"
- then
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
- output git checkout "$switch_to" -- ||
- die "$(eval_gettext "Could not checkout \$switch_to")"
-
- comment_for_reflog start
- fi
-}
-
init_basic_state () {
orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
return 0
fi
- setup_reflog_action
+ git rebase--helper --checkout-base "$switch_to" ${verbose:+--verbose}
init_basic_state
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 51c8ab7ac..27f8453fe 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3129,6 +3129,36 @@ static const char *reflog_message(struct replay_opts *opts,
return buf.buf;
}
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+ int verbose, const char *action)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
+ cmd.git_cmd = 1;
+
+ argv_array_push(&cmd.args, "checkout");
+ argv_array_push(&cmd.args, commit);
+ argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+ if (verbose)
+ return run_command(&cmd);
+ return run_command_silent_on_success(&cmd);
+}
+
+int checkout_base_commit(struct replay_opts *opts, const char *commit,
+ int verbose)
+{
+ const char *action;
+
+ if (commit && *commit) {
+ action = reflog_message(opts, "start", "checkout %s", commit);
+ if (run_git_checkout(opts, commit, verbose, action))
+ return error(_("Could not checkout %s"), commit);
+ }
+
+ return 0;
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 35730b13e..42c3dda81 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
void commit_post_rewrite(const struct commit *current_head,
const struct object_id *new_head);
+int checkout_base_commit(struct replay_opts *opts, const char *commit,
+ int verbose);
+
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
void print_commit_summary(const char *prefix, const struct object_id *oid,
--
2.17.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-21 14:17 ` [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-06-22 16:27 ` Junio C Hamano
2018-06-22 20:48 ` Alban Gruin
0 siblings, 1 reply; 51+ messages in thread
From: Junio C Hamano @ 2018-06-22 16:27 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren
Alban Gruin <alban.gruin@gmail.com> writes:
> This rewrites (the misnamed) setup_reflog_action() from shell to C. The
> new version is called checkout_base_commit().
;-) on the "misnamed" part. Indeed, setting up the comment for the
reflog entry is secondary to what this function wants to do, which
is to check out the branch to be rebased.
I do not think "base_commit" is a good name, either, though. When I
hear 'base' in the context of 'rebase', I would imagine that the
speaker is talking about the bottom of the range of the commits to
be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
commits BASE..BRANCH on top of ONTO and then points BRANCH at the
result), not the top of the range or the branch these commits are
taken from.
> index 51c8ab7ac..27f8453fe 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3129,6 +3129,36 @@ static const char *reflog_message(struct replay_opts *opts,
> return buf.buf;
> }
>
> +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> + int verbose, const char *action)
> +{
> + struct child_process cmd = CHILD_PROCESS_INIT;
> +
> + cmd.git_cmd = 1;
> +
> + argv_array_push(&cmd.args, "checkout");
> + argv_array_push(&cmd.args, commit);
> + argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> + if (verbose)
> + return run_command(&cmd);
> + return run_command_silent_on_success(&cmd);
For the same reason as 1/3, I think it makes more sense to have
"else" here.
> +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> + int verbose)
> +{
> + const char *action;
> +
> + if (commit && *commit) {
Hmm, isn't it a programming error to feed !commit or !*commit here?
I offhand do not think of a reason why making such an input a silent
no-op success, instead of making it error out, would be a good idea.
> + action = reflog_message(opts, "start", "checkout %s", commit);
> + if (run_git_checkout(opts, commit, verbose, action))
> + return error(_("Could not checkout %s"), commit);
> + }
> +
> + return 0;
> +}
> +
> static const char rescheduled_advice[] =
> N_("Could not execute the todo command\n"
> "\n"
> diff --git a/sequencer.h b/sequencer.h
> index 35730b13e..42c3dda81 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit *old_head,
> void commit_post_rewrite(const struct commit *current_head,
> const struct object_id *new_head);
>
> +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> + int verbose);
> +
> #define SUMMARY_INITIAL_COMMIT (1 << 0)
> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
> void print_commit_summary(const char *prefix, const struct object_id *oid,
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-22 16:27 ` Junio C Hamano
@ 2018-06-22 20:48 ` Alban Gruin
2018-06-25 15:34 ` Junio C Hamano
0 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-06-22 20:48 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren
Hi Junio,
Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
> > This rewrites (the misnamed) setup_reflog_action() from shell to C. The
> > new version is called checkout_base_commit().
>
> ;-) on the "misnamed" part. Indeed, setting up the comment for the
> reflog entry is secondary to what this function wants to do, which
> is to check out the branch to be rebased.
>
> I do not think "base_commit" is a good name, either, though. When I
> hear 'base' in the context of 'rebase', I would imagine that the
> speaker is talking about the bottom of the range of the commits to
> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
> result), not the top of the range or the branch these commits are
> taken from.
>
Perhaps should I name this function checkout_onto(), and rename
checkout_onto() to "detach_onto()"? And I would reorder those two commits in
the series, as this would be very confusing.
> > index 51c8ab7ac..27f8453fe 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -3129,6 +3129,36 @@ static const char *reflog_message(struct
> > replay_opts *opts,>
> > return buf.buf;
> >
> > }
> >
> > +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> > + int verbose, const char *action)
> > +{
> > + struct child_process cmd = CHILD_PROCESS_INIT;
> > +
> > + cmd.git_cmd = 1;
> > +
> > + argv_array_push(&cmd.args, "checkout");
> > + argv_array_push(&cmd.args, commit);
> > + argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> > +
> > + if (verbose)
> > + return run_command(&cmd);
> > + return run_command_silent_on_success(&cmd);
>
> For the same reason as 1/3, I think it makes more sense to have
> "else" here.
>
Right.
> > +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> > + int verbose)
> > +{
> > + const char *action;
> > +
> > + if (commit && *commit) {
>
> Hmm, isn't it a programming error to feed !commit or !*commit here?
> I offhand do not think of a reason why making such an input a silent
> no-op success, instead of making it error out, would be a good idea.
>
I think it’s correct.
> > + action = reflog_message(opts, "start", "checkout %s", commit);
> > + if (run_git_checkout(opts, commit, verbose, action))
> > + return error(_("Could not checkout %s"), commit);
> > + }
> > +
> > + return 0;
> > +}
> > +
> >
> > static const char rescheduled_advice[] =
> > N_("Could not execute the todo command\n"
> > "\n"
> >
> > diff --git a/sequencer.h b/sequencer.h
> > index 35730b13e..42c3dda81 100644
> > --- a/sequencer.h
> > +++ b/sequencer.h
> > @@ -100,6 +100,9 @@ int update_head_with_reflog(const struct commit
> > *old_head,>
> > void commit_post_rewrite(const struct commit *current_head,
> >
> > const struct object_id *new_head);
> >
> > +int checkout_base_commit(struct replay_opts *opts, const char *commit,
> > + int verbose);
> > +
> >
> > #define SUMMARY_INITIAL_COMMIT (1 << 0)
> > #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
> > void print_commit_summary(const char *prefix, const struct object_id
> > *oid,
Cheers,
Alban
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-22 20:48 ` Alban Gruin
@ 2018-06-25 15:34 ` Junio C Hamano
2018-06-25 18:21 ` Alban Gruin
0 siblings, 1 reply; 51+ messages in thread
From: Junio C Hamano @ 2018-06-25 15:34 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren
Alban Gruin <alban.gruin@gmail.com> writes:
> Hi Junio,
>
> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
>> Alban Gruin <alban.gruin@gmail.com> writes:
>> > This rewrites (the misnamed) setup_reflog_action() from shell to C. The
>> > new version is called checkout_base_commit().
>>
>> ;-) on the "misnamed" part. Indeed, setting up the comment for the
>> reflog entry is secondary to what this function wants to do, which
>> is to check out the branch to be rebased.
>>
>> I do not think "base_commit" is a good name, either, though. When I
>> hear 'base' in the context of 'rebase', I would imagine that the
>> speaker is talking about the bottom of the range of the commits to
>> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
>> result), not the top of the range or the branch these commits are
>> taken from.
>>
>
> Perhaps should I name this function checkout_onto(), and rename
> checkout_onto() to "detach_onto()"? And I would reorder those two commits in
> the series, as this would be very confusing.
I may be misunderstanding what is happening in the function, but I
think it is checking out neither the onto or the base commit. The
function instead is about checking out the branch to be rebased
before anything else happens when the optional <branch> argument is
given (and when the optional argument is not given, then we rebase
the current branch so there is no need to check it out upfront), no?
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-25 15:34 ` Junio C Hamano
@ 2018-06-25 18:21 ` Alban Gruin
2018-06-25 21:14 ` Johannes Schindelin
2018-06-26 17:44 ` Junio C Hamano
0 siblings, 2 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-25 18:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren
Hi Junio,
Le 25/06/2018 à 17:34, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@gmail.com> writes:
>
>> Hi Junio,
>>
>> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
>>> Alban Gruin <alban.gruin@gmail.com> writes:
>>>> This rewrites (the misnamed) setup_reflog_action() from shell to C. The
>>>> new version is called checkout_base_commit().
>>>
>>> ;-) on the "misnamed" part. Indeed, setting up the comment for the
>>> reflog entry is secondary to what this function wants to do, which
>>> is to check out the branch to be rebased.
>>>
>>> I do not think "base_commit" is a good name, either, though. When I
>>> hear 'base' in the context of 'rebase', I would imagine that the
>>> speaker is talking about the bottom of the range of the commits to
>>> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
>>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
>>> result), not the top of the range or the branch these commits are
>>> taken from.
>>>
>>
>> Perhaps should I name this function checkout_onto(), and rename
>> checkout_onto() to "detach_onto()"? And I would reorder those two commits in
>> the series, as this would be very confusing.
>
> I may be misunderstanding what is happening in the function, but I
> think it is checking out neither the onto or the base commit. The
> function instead is about checking out the branch to be rebased
> before anything else happens when the optional <branch> argument is
> given (and when the optional argument is not given, then we rebase
> the current branch so there is no need to check it out upfront), no?
>
>
Yes, you’re right.
Now I really don’t know how to call this function.
checkout_top_of_range(), perhaps?
Cheers,
Alban
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-25 18:21 ` Alban Gruin
@ 2018-06-25 21:14 ` Johannes Schindelin
2018-06-26 9:13 ` Pratik Karki
2018-06-26 17:44 ` Junio C Hamano
1 sibling, 1 reply; 51+ messages in thread
From: Johannes Schindelin @ 2018-06-25 21:14 UTC (permalink / raw)
To: Alban Gruin
Cc: Junio C Hamano, git, Stefan Beller, Christian Couder,
Pratik Karki, phillip.wood, Elijah Newren
[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]
Hi,
On Mon, 25 Jun 2018, Alban Gruin wrote:
> Le 25/06/2018 à 17:34, Junio C Hamano a écrit :
> > Alban Gruin <alban.gruin@gmail.com> writes:
> >
> >> Le 22/06/2018 à 18:27, Junio C Hamano a écrit :
> >>> Alban Gruin <alban.gruin@gmail.com> writes:
> >>>> This rewrites (the misnamed) setup_reflog_action() from shell to C. The
> >>>> new version is called checkout_base_commit().
> >>>
> >>> ;-) on the "misnamed" part. Indeed, setting up the comment for the
> >>> reflog entry is secondary to what this function wants to do, which
> >>> is to check out the branch to be rebased.
> >>>
> >>> I do not think "base_commit" is a good name, either, though. When I
> >>> hear 'base' in the context of 'rebase', I would imagine that the
> >>> speaker is talking about the bottom of the range of the commits to
> >>> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
> >>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
> >>> result), not the top of the range or the branch these commits are
> >>> taken from.
> >>>
> >>
> >> Perhaps should I name this function checkout_onto(), and rename
> >> checkout_onto() to "detach_onto()"? And I would reorder those two commits in
> >> the series, as this would be very confusing.
> >
> > I may be misunderstanding what is happening in the function, but I
> > think it is checking out neither the onto or the base commit. The
> > function instead is about checking out the branch to be rebased
> > before anything else happens when the optional <branch> argument is
> > given (and when the optional argument is not given, then we rebase
> > the current branch so there is no need to check it out upfront), no?
> >
> >
>
> Yes, you’re right.
>
> Now I really don’t know how to call this function.
> checkout_top_of_range(), perhaps?
Pratik refactored some code from sequencer.c into checkout.c/checkout.h
today to do exactly that. It is not polished yet, but probably will be
tomorrow. It provides a function `int detach_head_to(struct object_oid
*oid, const char *action, const char *reflog_message)`. Maybe use that
directly, once the commit is available?
Ciao,
Dscho
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-25 21:14 ` Johannes Schindelin
@ 2018-06-26 9:13 ` Pratik Karki
0 siblings, 0 replies; 51+ messages in thread
From: Pratik Karki @ 2018-06-26 9:13 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Alban Gruin, Junio C Hamano, git, Stefan Beller, Christian Couder,
phillip.wood, newren
Hi,
On Tue, Jun 26, 2018 at 3:00 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Pratik refactored some code from sequencer.c into checkout.c/checkout.h
> today to do exactly that. It is not polished yet, but probably will be
> tomorrow. It provides a function `int detach_head_to(struct object_oid
> *oid, const char *action, const char *reflog_message)`. Maybe use that
> directly, once the commit is available?
The commit is here[1].
[1]: https://github.com/git/git/pull/505/commits/47031d4706bd1eccd2816ecdaaf6e9cd700909aa
Cheers,
Pratik
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-25 18:21 ` Alban Gruin
2018-06-25 21:14 ` Johannes Schindelin
@ 2018-06-26 17:44 ` Junio C Hamano
1 sibling, 0 replies; 51+ messages in thread
From: Junio C Hamano @ 2018-06-26 17:44 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren
Alban Gruin <alban.gruin@gmail.com> writes:
>>>> I do not think "base_commit" is a good name, either, though. When I
>>>> hear 'base' in the context of 'rebase', I would imagine that the
>>>> speaker is talking about the bottom of the range of the commits to
>>>> be rebased (i.e. "rebase --onto ONTO BASE BRANCH", which replays
>>>> commits BASE..BRANCH on top of ONTO and then points BRANCH at the
>>>> result), not the top of the range or the branch these commits are
>>>> taken from.
>>> ...
> Now I really don’t know how to call this function.
> checkout_top_of_range(), perhaps?
If this is a straight rewrite of setup_reflog_action, i.e. the
division of labor between its caller and this function is such that
the caller blindly calls it without even checking if it got the
optional "branch to be rebased" argument and this function is
responsible to decide if the preparatory checkout of the named
branch is necessary, then any name that does not even hint that
checkout is done conditionally would not work well.
How about callilng it "prepare_branch_to_be_rebased()"? This
function, at least the original setup_reflog_action, responds to
git rebase [--onto ONTO] UPSTREAM
by doing nothing (i.e. the result of preparation is to do nothing
because we are rebasing the commits between UPSTREAM and currently
checked out state on top of ONTO, or UPSTREAM if ONTO is not
given) and it responds to
git rebase [--onto ONTO] UPSTREAM BRANCH
by checking out BRANCH (most importantly, when given a concrete
branch name, it checks the branch out, and does not detach at the
commit at the tip of the branch), because that is the first
preparatory step to rebase the BRANCH.
^ permalink raw reply [flat|nested] 51+ messages in thread
* [GSoC][PATCH v3 3/3] rebase -i: rewrite checkout_onto() in C
2018-06-21 14:17 ` [GSoC][PATCH v3 " Alban Gruin
2018-06-21 14:17 ` [GSoC][PATCH v3 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
2018-06-21 14:17 ` [GSoC][PATCH v3 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-06-21 14:17 ` Alban Gruin
2018-06-25 13:44 ` [GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations " Alban Gruin
3 siblings, 0 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-21 14:17 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This rewrites checkout_onto() from shell to C.
A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
builtin/rebase--helper.c | 7 ++++++-
git-rebase--interactive.sh | 25 ++++---------------------
sequencer.c | 19 +++++++++++++++++++
sequencer.h | 3 +++
4 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index fb5996a2c..20dea4b3a 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,7 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, CHECKOUT_BASE,
+ CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -53,6 +54,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "checkout-base", &command,
N_("checkout the base commit"), CHECKOUT_BASE),
+ OPT_CMDMODE(0, "checkout-onto", &command,
+ N_("checkout a commit"), CHECKOUT_ONTO),
OPT_END()
};
@@ -98,5 +101,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!edit_todo_list(flags);
if (command == CHECKOUT_BASE && argc == 2)
return !!checkout_base_commit(&opts, argv[1], verbose);
+ if (command == CHECKOUT_ONTO && argc == 4)
+ return !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index af46cf9c2..7b6142a76 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
;;
esac
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
- case "$orig_reflog_action" in
- ''|rebase*)
- GIT_REFLOG_ACTION="rebase -i ($1)"
- export GIT_REFLOG_ACTION
- ;;
- esac
-}
-
die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
}
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
- comment_for_reflog start
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
- output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
- git update-ref ORIG_HEAD $orig_head
-}
-
get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
git rebase--helper --check-todo-list || {
ret=$?
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" \
+ "$orig_head" ${verbose:+--verbose}
exit $ret
}
@@ -186,7 +168,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+ ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
--continue
diff --git a/sequencer.c b/sequencer.c
index 27f8453fe..b3b1a2e18 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3159,6 +3159,25 @@ int checkout_base_commit(struct replay_opts *opts, const char *commit,
return 0;
}
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose)
+{
+ struct object_id oid;
+ const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
+
+ if (get_oid(orig_head, &oid))
+ return error(_("%s: not a valid OID"), orig_head);
+
+ if (run_git_checkout(opts, onto, verbose, action)) {
+ apply_autostash(opts);
+ sequencer_remove_state(opts);
+ return error(_("could not detach HEAD"));
+ }
+
+ return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 42c3dda81..eda03ce32 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -102,6 +102,9 @@ void commit_post_rewrite(const struct commit *current_head,
int checkout_base_commit(struct replay_opts *opts, const char *commit,
int verbose);
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose);
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
--
2.17.1
^ permalink raw reply related [flat|nested] 51+ messages in thread* [GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations in C
2018-06-21 14:17 ` [GSoC][PATCH v3 " Alban Gruin
` (2 preceding siblings ...)
2018-06-21 14:17 ` [GSoC][PATCH v3 3/3] rebase -i: rewrite checkout_onto() " Alban Gruin
@ 2018-06-25 13:44 ` Alban Gruin
2018-06-25 13:44 ` [GSoC][PATCH v4 1/3] sequencer: extract a function to silence a command, except if it fails Alban Gruin
` (3 more replies)
3 siblings, 4 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-25 13:44 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This patch series rewrites the reflog operations from shell to C. This
is part of the effort to rewrite interactive rebase in C.
The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.
This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-25).
Changes since v3:
- Removing a comment from run_command_silent_on_success()
- Changing the order of setup_reflog_action() and checkout_onto()
rewrites in the series
- Renaming checkout_onto() to detach_onto()
- Renaming checkout_base_commit() (rewrite of setup_reflog_action()) to
checkout_onto()
- Using the `else` keyword to call run_command_silent_on_success() or
run_command() in run_git_commit() and run_git_checkout().
Alban Gruin (3):
sequencer: extract a function to silence a command, except if it fails
rebase -i: rewrite checkout_onto() in C
rebase -i: rewrite setup_reflog_action() in C
builtin/rebase--helper.c | 13 ++++-
git-rebase--interactive.sh | 28 ++--------
sequencer.c | 101 +++++++++++++++++++++++++++----------
sequencer.h | 6 +++
4 files changed, 97 insertions(+), 51 deletions(-)
--
2.18.0
^ permalink raw reply [flat|nested] 51+ messages in thread* [GSoC][PATCH v4 1/3] sequencer: extract a function to silence a command, except if it fails
2018-06-25 13:44 ` [GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations " Alban Gruin
@ 2018-06-25 13:44 ` Alban Gruin
2018-06-25 13:44 ` [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C Alban Gruin
` (2 subsequent siblings)
3 siblings, 0 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-25 13:44 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This extracts the part of run_git_commit() that redirects the stdout and
stderr of a command to a strbuf, and prints it if the command fails, to
a helper function: run_command_silent_on_success(). It is intended to
replace output() from git-rebase.sh in the rewrite of
setup_reflog_action() and checkout_onto().
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
sequencer.c | 51 +++++++++++++++++++++++++--------------------------
1 file changed, 25 insertions(+), 26 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 7cc76332e..9a9725e23 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -766,6 +766,23 @@ N_("you have staged changes in your working tree\n"
#define VERIFY_MSG (1<<4)
#define CREATE_ROOT_COMMIT (1<<5)
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ cmd->stdout_to_stderr = 1;
+ rc = pipe_command(cmd,
+ NULL, 0,
+ NULL, 0,
+ &buf, 0);
+
+ if (rc)
+ fputs(buf.buf, stderr);
+ strbuf_release(&buf);
+ return rc;
+}
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -820,18 +837,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
cmd.git_cmd = 1;
- if (is_rebase_i(opts)) {
- if (!(flags & EDIT_MSG)) {
- cmd.stdout_to_stderr = 1;
- cmd.err = -1;
- }
-
- if (read_env_script(&cmd.env_array)) {
- const char *gpg_opt = gpg_sign_opt_quoted(opts);
+ if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
+ const char *gpg_opt = gpg_sign_opt_quoted(opts);
- return error(_(staged_changes_advice),
- gpg_opt, gpg_opt);
- }
+ return error(_(staged_changes_advice),
+ gpg_opt, gpg_opt);
}
argv_array_push(&cmd.args, "commit");
@@ -861,21 +871,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(&cmd.args, "--allow-empty-message");
- if (cmd.err == -1) {
- /* hide stderr on success */
- struct strbuf buf = STRBUF_INIT;
- int rc = pipe_command(&cmd,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- &buf, 0);
- if (rc)
- fputs(buf.buf, stderr);
- strbuf_release(&buf);
- return rc;
- }
-
- return run_command(&cmd);
+ if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+ return run_command_silent_on_success(&cmd);
+ else
+ return run_command(&cmd);
}
static int rest_is_empty(const struct strbuf *sb, int start)
--
2.18.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C
2018-06-25 13:44 ` [GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations " Alban Gruin
2018-06-25 13:44 ` [GSoC][PATCH v4 1/3] sequencer: extract a function to silence a command, except if it fails Alban Gruin
@ 2018-06-25 13:44 ` Alban Gruin
2018-06-26 17:35 ` Junio C Hamano
2018-06-25 13:44 ` [GSoC][PATCH v4 3/3] rebase -i: rewrite setup_reflog_action() " Alban Gruin
2018-06-29 15:14 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Alban Gruin
3 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-06-25 13:44 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This rewrites checkout_onto() from shell to C. The new version is called
detach_onto(), given its role.
A new command (“detach-onto”) is added to rebase--helper.c, as well as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by detach_onto().
The function `run_git_checkout()` will also be used in the next commit,
therefore its code is not part of `detach_onto()`.
The shell version is then stripped in favour of a call to the helper.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
builtin/rebase--helper.c | 9 +++++++--
git-rebase--interactive.sh | 13 ++++---------
sequencer.c | 36 ++++++++++++++++++++++++++++++++++++
sequencer.h | 4 ++++
4 files changed, 51 insertions(+), 11 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index d2990b210..2dfd55c76 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -12,12 +12,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -27,6 +27,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
N_("keep original branch points of cousins")),
+ OPT__VERBOSE(&verbose, N_("be verbose")),
OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -50,6 +51,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
OPT_CMDMODE(0, "edit-todo", &command,
N_("edit the todo list during an interactive rebase"),
EDIT_TODO),
+ OPT_CMDMODE(0, "detach-onto", &command,
+ N_("checkout a commit"), DETACH_ONTO),
OPT_END()
};
@@ -93,5 +96,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+ if (command == DETACH_ONTO && argc == 4)
+ return !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..2f5761d49 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -70,13 +70,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
}
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
- output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
- git update-ref ORIG_HEAD $orig_head
-}
-
get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -188,7 +181,8 @@ EOF
git rebase--helper --check-todo-list || {
ret=$?
- checkout_onto
+ git rebase--helper --detach-onto "$onto_name" "$onto" \
+ "$orig_head" ${verbose:+--verbose}
exit $ret
}
@@ -198,7 +192,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
- checkout_onto
+ git rebase--helper --detach-onto "$onto_name" "$onto" "$orig_head" \
+ ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
--continue
diff --git a/sequencer.c b/sequencer.c
index 9a9725e23..ee6374921 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3129,6 +3129,42 @@ static const char *reflog_message(struct replay_opts *opts,
return buf.buf;
}
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+ int verbose, const char *action)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
+ cmd.git_cmd = 1;
+
+ argv_array_push(&cmd.args, "checkout");
+ argv_array_push(&cmd.args, commit);
+ argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+ if (verbose)
+ return run_command(&cmd);
+ else
+ return run_command_silent_on_success(&cmd);
+}
+
+int detach_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose)
+{
+ struct object_id oid;
+ const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
+
+ if (get_oid(orig_head, &oid))
+ return error(_("%s: not a valid OID"), orig_head);
+
+ if (run_git_checkout(opts, onto, verbose, action)) {
+ apply_autostash(opts);
+ sequencer_remove_state(opts);
+ return error(_("could not detach HEAD"));
+ }
+
+ return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 35730b13e..9f0ac5e75 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -100,6 +100,10 @@ int update_head_with_reflog(const struct commit *old_head,
void commit_post_rewrite(const struct commit *current_head,
const struct object_id *new_head);
+int detach_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose);
+
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
void print_commit_summary(const char *prefix, const struct object_id *oid,
--
2.18.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C
2018-06-25 13:44 ` [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C Alban Gruin
@ 2018-06-26 17:35 ` Junio C Hamano
0 siblings, 0 replies; 51+ messages in thread
From: Junio C Hamano @ 2018-06-26 17:35 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren
Alban Gruin <alban.gruin@gmail.com> writes:
> This rewrites checkout_onto() from shell to C. The new version is called
> detach_onto(), given its role.
The name, given its role, may be good, but is the implementtaion
robust enough to fulfill the promise its name gives?
> git rebase--helper --check-todo-list || {
> ret=$?
> - checkout_onto
> + git rebase--helper --detach-onto "$onto_name" "$onto" \
> + "$orig_head" ${verbose:+--verbose}
Here, $onto_name is what the end-user gave us (e.g. it is
"master..." in "git rebase --onto=master... base"), while $onto is a
40-hex object name of the commit. $orig_head is also a 40-hex
object name.
And this call shows how the above shell scriptlet calls into the
detach_onto() thing ...
> + if (command == DETACH_ONTO && argc == 4)
> + return !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);
... which is defined like so:
> +int detach_onto(struct replay_opts *opts,
> + const char *onto_name, const char *onto,
> + const char *orig_head, unsigned verbose)
> +{
> + struct object_id oid;
> + const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
> +
> + if (get_oid(orig_head, &oid))
> + return error(_("%s: not a valid OID"), orig_head);
Which means that this can be more strict to use get_oid_hex() to
catch possible mistakes in the caller.
> + if (run_git_checkout(opts, onto, verbose, action)) {
And this could be a bit problematic, as we can see below how the
"checkout" thing does not guarantee "detaching" at all ...
> + apply_autostash(opts);
> + sequencer_remove_state(opts);
> + return error(_("could not detach HEAD"));
> + }
> +
> + return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +}
> +
... which can be seen here ...
> +static int run_git_checkout(struct replay_opts *opts, const char *commit,
> + int verbose, const char *action)
> +{
> + struct child_process cmd = CHILD_PROCESS_INIT;
> +
> + cmd.git_cmd = 1;
> +
> + argv_array_push(&cmd.args, "checkout");
> + argv_array_push(&cmd.args, commit);
> + argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> +
> + if (verbose)
> + return run_command(&cmd);
> + else
> + return run_command_silent_on_success(&cmd);
> +}
This drives the external command "git checkout" with _any_ string
the caller passes in "commit". If the variable happens to have
'master', for example, it would be "git checkout master" and if you
have a branch with that name, it will not detach but check out the
branch to build on it. It is a caller's responsibility to give a
suitable "commit" if it wants to use this helper to detach.
So perhaps the caller of this function in detach_onto() should pass
"%s^0" or even do something like
struct object_id onto_oid;
char onto_hex[GIT_MAX_HEXSZ + 1];
if (get_oid(onto, &onto_oid) || oid_to_hex_r(onto_hex, &onto_oid))
return error(...);
if (run_git_checkout(opts, onto_hex, verbose, action)) {
...
to ensure that it keeps the promise its name gives.
I can hear "Oh, but it is a bug in the caller to give anything that
won't result in detaching in 'onto'" but that is not a valid excuse,
given that this _public_ function is called "detach_onto". Making
sure detachment happens is its responsibility, not its callers'.
Or we could do a cop-out alternative of commenting the function in *.h
file to say "onto must be given as 40-hex", with a code to make sure
the caller really gave us a 40-hex and not a branch name. That is a
less ideal but probably acceptable alternative.
> static const char rescheduled_advice[] =
> N_("Could not execute the todo command\n"
> "\n"
> diff --git a/sequencer.h b/sequencer.h
> index 35730b13e..9f0ac5e75 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -100,6 +100,10 @@ int update_head_with_reflog(const struct commit *old_head,
> void commit_post_rewrite(const struct commit *current_head,
> const struct object_id *new_head);
>
> +int detach_onto(struct replay_opts *opts,
> + const char *onto_name, const char *onto,
> + const char *orig_head, unsigned verbose);
> +
> #define SUMMARY_INITIAL_COMMIT (1 << 0)
> #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
> void print_commit_summary(const char *prefix, const struct object_id *oid,
^ permalink raw reply [flat|nested] 51+ messages in thread
* [GSoC][PATCH v4 3/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-25 13:44 ` [GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations " Alban Gruin
2018-06-25 13:44 ` [GSoC][PATCH v4 1/3] sequencer: extract a function to silence a command, except if it fails Alban Gruin
2018-06-25 13:44 ` [GSoC][PATCH v4 2/3] rebase -i: rewrite checkout_onto() in C Alban Gruin
@ 2018-06-25 13:44 ` Alban Gruin
2018-06-29 15:14 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Alban Gruin
3 siblings, 0 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-25 13:44 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Elijah Newren, Alban Gruin
This rewrites (the misnamed) setup_reflog_action() from shell to C. The
new version is called checkout_onto().
A new command is added to rebase--helper.c, “checkout-base”. The shell
version is then stripped.
As $GIT_REFLOG_ACTION is no longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.
If the commit OID provided to checkout_onto() is empty, nothing happens
and no errors are returned, otherwise it would break some
tests (t3404.92 and t3404.93).
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
builtin/rebase--helper.c | 6 +++++-
git-rebase--interactive.sh | 15 +--------------
sequencer.c | 14 ++++++++++++++
sequencer.h | 2 ++
4 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 2dfd55c76..a00b1091d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,7 +17,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, DETACH_ONTO, CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -53,6 +53,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "detach-onto", &command,
N_("checkout a commit"), DETACH_ONTO),
+ OPT_CMDMODE(0, "checkout-onto", &command,
+ N_("checkout the base commit"), CHECKOUT_ONTO),
OPT_END()
};
@@ -98,5 +100,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!edit_todo_list(flags);
if (command == DETACH_ONTO && argc == 4)
return !!detach_onto(&opts, argv[1], argv[2], argv[3], verbose);
+ if (command == CHECKOUT_ONTO && argc == 2)
+ return !!checkout_onto(&opts, argv[1], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2f5761d49..375c36fb5 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -112,19 +112,6 @@ initiate_action () {
esac
}
-setup_reflog_action () {
- comment_for_reflog start
-
- if test ! -z "$switch_to"
- then
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
- output git checkout "$switch_to" -- ||
- die "$(eval_gettext "Could not checkout \$switch_to")"
-
- comment_for_reflog start
- fi
-}
-
init_basic_state () {
orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -206,7 +193,7 @@ git_rebase__interactive () {
return 0
fi
- setup_reflog_action
+ git rebase--helper --checkout-onto "$switch_to" ${verbose:+--verbose}
init_basic_state
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index ee6374921..620b41e96 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3165,6 +3165,20 @@ int detach_onto(struct replay_opts *opts,
return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
}
+int checkout_onto(struct replay_opts *opts, const char *commit,
+ int verbose)
+{
+ const char *action;
+
+ if (commit && *commit) {
+ action = reflog_message(opts, "start", "checkout %s", commit);
+ if (run_git_checkout(opts, commit, verbose, action))
+ return error(_("Could not checkout %s"), commit);
+ }
+
+ return 0;
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 9f0ac5e75..afbb2edf1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -103,6 +103,8 @@ void commit_post_rewrite(const struct commit *current_head,
int detach_onto(struct replay_opts *opts,
const char *onto_name, const char *onto,
const char *orig_head, unsigned verbose);
+int checkout_onto(struct replay_opts *opts, const char *commit,
+ int verbose);
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
--
2.18.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C
2018-06-25 13:44 ` [GSoC][PATCH v4 0/3] rebase -i: rewrite reflog operations " Alban Gruin
` (2 preceding siblings ...)
2018-06-25 13:44 ` [GSoC][PATCH v4 3/3] rebase -i: rewrite setup_reflog_action() " Alban Gruin
@ 2018-06-29 15:14 ` Alban Gruin
2018-06-29 15:14 ` [GSoC][PATCH v5 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
` (3 more replies)
3 siblings, 4 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-29 15:14 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Alban Gruin
This patch series rewrites the reflog operations from shell to C. This
is part of the effort to rewrite interactive rebase in C.
The first commit is dedicated to creating a function to silence a
command, as the sequencer will do in several places with these patches.
This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
with pu (as of 2018-06-29).
Changes since v4:
- Changing the order of setup_reflog_action() and checkout_onto()
rewrites in the series
- checkout_onto() is no longer renamed in C
- setup_reflog_action() is renamed to prepare_branch_to_be_rebased(),
and not to checkout_onto().
Alban Gruin (3):
sequencer: add a new function to silence a command, except if it
fails.
rebase -i: rewrite setup_reflog_action() in C
rebase -i: rewrite checkout_onto() in C
builtin/rebase--helper.c | 14 ++++-
git-rebase--interactive.sh | 39 ++------------
sequencer.c | 101 +++++++++++++++++++++++++++----------
sequencer.h | 6 +++
4 files changed, 98 insertions(+), 62 deletions(-)
--
2.18.0
^ permalink raw reply [flat|nested] 51+ messages in thread* [GSoC][PATCH v5 1/3] sequencer: add a new function to silence a command, except if it fails.
2018-06-29 15:14 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Alban Gruin
@ 2018-06-29 15:14 ` Alban Gruin
2018-06-29 15:14 ` [GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
` (2 subsequent siblings)
3 siblings, 0 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-29 15:14 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Alban Gruin
This adds a new function, run_command_silent_on_success(), to
redirect the stdout and stderr of a command to a strbuf, and then to run
that command. This strbuf is printed only if the command fails. It is
functionnaly similar to output() from git-rebase.sh.
run_git_commit() is then refactored to use of
run_command_silent_on_success().
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
sequencer.c | 51 +++++++++++++++++++++++++--------------------------
1 file changed, 25 insertions(+), 26 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index cb7ec9807..d9545b366 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -769,6 +769,23 @@ N_("you have staged changes in your working tree\n"
#define VERIFY_MSG (1<<4)
#define CREATE_ROOT_COMMIT (1<<5)
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+ struct strbuf buf = STRBUF_INIT;
+ int rc;
+
+ cmd->stdout_to_stderr = 1;
+ rc = pipe_command(cmd,
+ NULL, 0,
+ NULL, 0,
+ &buf, 0);
+
+ if (rc)
+ fputs(buf.buf, stderr);
+ strbuf_release(&buf);
+ return rc;
+}
+
/*
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
@@ -823,18 +840,11 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
cmd.git_cmd = 1;
- if (is_rebase_i(opts)) {
- if (!(flags & EDIT_MSG)) {
- cmd.stdout_to_stderr = 1;
- cmd.err = -1;
- }
-
- if (read_env_script(&cmd.env_array)) {
- const char *gpg_opt = gpg_sign_opt_quoted(opts);
+ if (is_rebase_i(opts) && read_env_script(&cmd.env_array)) {
+ const char *gpg_opt = gpg_sign_opt_quoted(opts);
- return error(_(staged_changes_advice),
- gpg_opt, gpg_opt);
- }
+ return error(_(staged_changes_advice),
+ gpg_opt, gpg_opt);
}
argv_array_push(&cmd.args, "commit");
@@ -864,21 +874,10 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
if (opts->allow_empty_message)
argv_array_push(&cmd.args, "--allow-empty-message");
- if (cmd.err == -1) {
- /* hide stderr on success */
- struct strbuf buf = STRBUF_INIT;
- int rc = pipe_command(&cmd,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- &buf, 0);
- if (rc)
- fputs(buf.buf, stderr);
- strbuf_release(&buf);
- return rc;
- }
-
- return run_command(&cmd);
+ if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+ return run_command_silent_on_success(&cmd);
+ else
+ return run_command(&cmd);
}
static int rest_is_empty(const struct strbuf *sb, int start)
--
2.18.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* [GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-29 15:14 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Alban Gruin
2018-06-29 15:14 ` [GSoC][PATCH v5 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
@ 2018-06-29 15:14 ` Alban Gruin
2018-06-29 16:50 ` Junio C Hamano
2018-06-29 15:14 ` [GSoC][PATCH v5 3/3] rebase -i: rewrite checkout_onto() " Alban Gruin
2018-06-29 16:55 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Junio C Hamano
3 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-06-29 15:14 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Alban Gruin
This rewrites (the misnamed) setup_reflog_action() from shell to C. The
new version is called prepare_branch_to_be_rebased().
A new command is added to rebase--helper.c, “checkout-base”, as well as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by checkout_base_commit().
The function `run_git_checkout()` will also be used in the next commit,
therefore its code is not part of `checkout_base_commit()`.
The shell version is then stripped in favour of a call to the helper.
As $GIT_REFLOG_ACTION is no longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
builtin/rebase--helper.c | 9 +++++++--
git-rebase--interactive.sh | 16 ++--------------
sequencer.c | 31 +++++++++++++++++++++++++++++++
sequencer.h | 3 +++
4 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 731a64971..6c789e78b 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts = REPLAY_OPTS_INIT;
- unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+ unsigned flags = 0, keep_empty = 0, rebase_merges = 0, verbose = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -28,6 +28,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "rebase-merges", &rebase_merges, N_("rebase merge commits")),
OPT_BOOL(0, "rebase-cousins", &rebase_cousins,
N_("keep original branch points of cousins")),
+ OPT__VERBOSE(&verbose, N_("be verbose")),
OPT_CMDMODE(0, "continue", &command, N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", &command, N_("abort rebase"),
@@ -51,6 +52,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
OPT_CMDMODE(0, "edit-todo", &command,
N_("edit the todo list during an interactive rebase"),
EDIT_TODO),
+ OPT_CMDMODE(0, "prepare-branch", &command,
+ N_("prepare the branch to be rebased"), PREPARE_BRANCH),
OPT_END()
};
@@ -94,5 +97,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+ if (command == PREPARE_BRANCH && argc == 2)
+ return !!prepare_branch_to_be_rebased(&opts, argv[1], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f..77e972bb6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
# Switch to the branch in $into and notify it in the reflog
checkout_onto () {
+ comment_for_reflog start
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
esac
}
-setup_reflog_action () {
- comment_for_reflog start
-
- if test ! -z "$switch_to"
- then
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
- output git checkout "$switch_to" -- ||
- die "$(eval_gettext "Could not checkout \$switch_to")"
-
- comment_for_reflog start
- fi
-}
-
init_basic_state () {
orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
return 0
fi
- setup_reflog_action
+ git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
init_basic_state
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index d9545b366..dd0cf3cb5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3134,6 +3134,37 @@ static const char *reflog_message(struct replay_opts *opts,
return buf.buf;
}
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+ int verbose, const char *action)
+{
+ struct child_process cmd = CHILD_PROCESS_INIT;
+
+ cmd.git_cmd = 1;
+
+ argv_array_push(&cmd.args, "checkout");
+ argv_array_push(&cmd.args, commit);
+ argv_array_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
+
+ if (verbose)
+ return run_command(&cmd);
+ else
+ return run_command_silent_on_success(&cmd);
+}
+
+int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
+ int verbose)
+{
+ const char *action;
+
+ if (commit && *commit) {
+ action = reflog_message(opts, "start", "checkout %s", commit);
+ if (run_git_checkout(opts, commit, verbose, action))
+ return error(_("Could not checkout %s"), commit);
+ }
+
+ return 0;
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index ffab798f1..3821420b0 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -106,6 +106,9 @@ int update_head_with_reflog(const struct commit *old_head,
void commit_post_rewrite(const struct commit *current_head,
const struct object_id *new_head);
+int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
+ int verbose);
+
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
void print_commit_summary(const char *prefix, const struct object_id *oid,
--
2.18.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C
2018-06-29 15:14 ` [GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-06-29 16:50 ` Junio C Hamano
0 siblings, 0 replies; 51+ messages in thread
From: Junio C Hamano @ 2018-06-29 16:50 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood
Alban Gruin <alban.gruin@gmail.com> writes:
> + git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
> init_basic_state
>
> init_revisions_and_shortrevisions
> diff --git a/sequencer.c b/sequencer.c
> index d9545b366..dd0cf3cb5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3134,6 +3134,37 @@ static const char *reflog_message(struct replay_opts *opts,
> return buf.buf;
> }
> ...
> +int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
> + int verbose)
> +{
> + const char *action;
> +
> + if (commit && *commit) {
The sloppyness of this callee is forced by the sloppy caller, which
does not check if "$switch_to" has any value before calling "prepare
branch to be rebased" function. A less sloppy caller would check to
see if we have the optional "before doing anything, check out this
branch, as it is the branch we are trying to rebase" argument, and
refrain from calling this function, so there is no need for this "if
commit is given and is not an empty string" check.
And hopefully, when GSoC is over, the caller, that is still in shell
at this step, would also be rewritten in C and by that time the
caller will become less sloppy, removing the need for this check.
Hence, I think the reason why the check is still here, and our
desire to eventually remove the check, should be documented with an
in-code comment around here (the usual "NEEDSWORK:" comment is
fine).
^ permalink raw reply [flat|nested] 51+ messages in thread
* [GSoC][PATCH v5 3/3] rebase -i: rewrite checkout_onto() in C
2018-06-29 15:14 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Alban Gruin
2018-06-29 15:14 ` [GSoC][PATCH v5 1/3] sequencer: add a new function to silence a command, except if it fails Alban Gruin
2018-06-29 15:14 ` [GSoC][PATCH v5 2/3] rebase -i: rewrite setup_reflog_action() in C Alban Gruin
@ 2018-06-29 15:14 ` Alban Gruin
2018-06-29 16:55 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Junio C Hamano
3 siblings, 0 replies; 51+ messages in thread
From: Alban Gruin @ 2018-06-29 15:14 UTC (permalink / raw)
To: git
Cc: Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood, Alban Gruin
This rewrites checkout_onto() from shell to C.
A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
builtin/rebase--helper.c | 7 ++++++-
git-rebase--interactive.sh | 25 ++++---------------------
sequencer.c | 19 +++++++++++++++++++
sequencer.h | 3 +++
4 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 6c789e78b..0091e094b 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -18,7 +18,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
- ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
+ ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
+ CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", &opts.allow_ff, N_("allow fast-forward")),
@@ -54,6 +55,8 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "prepare-branch", &command,
N_("prepare the branch to be rebased"), PREPARE_BRANCH),
+ OPT_CMDMODE(0, "checkout-onto", &command,
+ N_("checkout a commit"), CHECKOUT_ONTO),
OPT_END()
};
@@ -99,5 +102,7 @@ int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
return !!prepare_branch_to_be_rebased(&opts, argv[1], verbose);
+ if (command == CHECKOUT_ONTO && argc == 4)
+ return !!checkout_onto(&opts, argv[1], argv[2], argv[3], verbose);
usage_with_options(builtin_rebase_helper_usage, options);
}
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 77e972bb6..b68f108f2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
;;
esac
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
- case "$orig_reflog_action" in
- ''|rebase*)
- GIT_REFLOG_ACTION="rebase -i ($1)"
- export GIT_REFLOG_ACTION
- ;;
- esac
-}
-
die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
}
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
- comment_for_reflog start
- GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
- output git checkout $onto || die_abort "$(gettext "could not detach HEAD")"
- git update-ref ORIG_HEAD $orig_head
-}
-
get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
git rebase--helper --check-todo-list || {
ret=$?
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" \
+ "$orig_head" ${verbose:+--verbose}
exit $ret
}
@@ -186,7 +168,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
- checkout_onto
+ git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+ ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
--continue
diff --git a/sequencer.c b/sequencer.c
index dd0cf3cb5..ec6de4eda 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3165,6 +3165,25 @@ int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
return 0;
}
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose)
+{
+ struct object_id oid;
+ const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
+
+ if (get_oid(orig_head, &oid))
+ return error(_("%s: not a valid OID"), orig_head);
+
+ if (run_git_checkout(opts, onto, verbose, action)) {
+ apply_autostash(opts);
+ sequencer_remove_state(opts);
+ return error(_("could not detach HEAD"));
+ }
+
+ return update_ref(NULL, "ORIG_HEAD", &oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
+}
+
static const char rescheduled_advice[] =
N_("Could not execute the todo command\n"
"\n"
diff --git a/sequencer.h b/sequencer.h
index 3821420b0..ba5e59f02 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -108,6 +108,9 @@ void commit_post_rewrite(const struct commit *current_head,
int prepare_branch_to_be_rebased(struct replay_opts *opts, const char *commit,
int verbose);
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head, unsigned verbose);
#define SUMMARY_INITIAL_COMMIT (1 << 0)
#define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
--
2.18.0
^ permalink raw reply related [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C
2018-06-29 15:14 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Alban Gruin
` (2 preceding siblings ...)
2018-06-29 15:14 ` [GSoC][PATCH v5 3/3] rebase -i: rewrite checkout_onto() " Alban Gruin
@ 2018-06-29 16:55 ` Junio C Hamano
2018-06-29 18:23 ` Junio C Hamano
3 siblings, 1 reply; 51+ messages in thread
From: Junio C Hamano @ 2018-06-29 16:55 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood
Alban Gruin <alban.gruin@gmail.com> writes:
> This patch series rewrites the reflog operations from shell to C. This
> is part of the effort to rewrite interactive rebase in C.
>
> The first commit is dedicated to creating a function to silence a
> command, as the sequencer will do in several places with these patches.
>
> This branch is based on ag/rebase-i-rewrite-todo, and does not conflict
> with pu (as of 2018-06-29).
Let's aggregate these topics into a single topic, and perhaps call
it ag/rebase-i-in-c or something like that. Pretending as if they
are separately replaceable does not make much sense, as you are not
rerolling the earlier one and keep going forward with producing more
parts that depends on the parts that have been submitted earlier.
These three patches looked more-or-less good; I may have spotted a
few nits and suggested improvements, though.
Thanks.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C
2018-06-29 16:55 ` [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations " Junio C Hamano
@ 2018-06-29 18:23 ` Junio C Hamano
2018-07-02 10:36 ` Alban Gruin
0 siblings, 1 reply; 51+ messages in thread
From: Junio C Hamano @ 2018-06-29 18:23 UTC (permalink / raw)
To: Alban Gruin
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood
Junio C Hamano <gitster@pobox.com> writes:
> Let's aggregate these topics into a single topic, and perhaps call
> it ag/rebase-i-in-c or something like that. Pretending as if they
> are separately replaceable does not make much sense, as you are not
> rerolling the earlier one and keep going forward with producing more
> parts that depends on the parts that have been submitted earlier.
So here is what I tentatively did.
$ git log --oneline --reverse master..ag/rebase-i-in-c
4d303fb608 rebase--interactive: rewrite append_todo_help() in C
b4ffe143a9 editor: add a function to launch the sequence editor
4ebe39cef9 rebase--interactive: rewrite the edit-todo functionality in C
0ff6bf7646 sequencer: add a new function to silence a command, except if it fails.
36784b351f rebase -i: rewrite setup_reflog_action() in C
415cac57ee rebase -i: rewrite checkout_onto() in C
In several hours please fetch from me and look for "Merge branch
'ag/rebase-i-in-c' to pu" to see how they exactly look like; some of
the patches might not be the latest ones, in which case you may need
to prod me to get them replaced (resending them as a whole with
incremented v$n header is probably the easiest if we need to do so).
Thanks.
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [GSoC][PATCH v5 0/3] rebase -i: rewrite reflog operations in C
2018-06-29 18:23 ` Junio C Hamano
@ 2018-07-02 10:36 ` Alban Gruin
2018-07-03 18:15 ` Junio C Hamano
0 siblings, 1 reply; 51+ messages in thread
From: Alban Gruin @ 2018-07-02 10:36 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Stefan Beller, Christian Couder, Pratik Karki,
Johannes Schindelin, phillip.wood
Hi Junio,
Le 29/06/2018 à 20:23, Junio C Hamano a écrit :
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Let's aggregate these topics into a single topic, and perhaps call
>> it ag/rebase-i-in-c or something like that. Pretending as if they
>> are separately replaceable does not make much sense, as you are not
>> rerolling the earlier one and keep going forward with producing more
>> parts that depends on the parts that have been submitted earlier.
>
> So here is what I tentatively did.
>
> $ git log --oneline --reverse master..ag/rebase-i-in-c
> 4d303fb608 rebase--interactive: rewrite append_todo_help() in C
> b4ffe143a9 editor: add a function to launch the sequence editor
> 4ebe39cef9 rebase--interactive: rewrite the edit-todo functionality in C
> 0ff6bf7646 sequencer: add a new function to silence a command, except if it fails.
> 36784b351f rebase -i: rewrite setup_reflog_action() in C
> 415cac57ee rebase -i: rewrite checkout_onto() in C
>
> In several hours please fetch from me and look for "Merge branch
> 'ag/rebase-i-in-c' to pu" to see how they exactly look like; some of
> the patches might not be the latest ones, in which case you may need
> to prod me to get them replaced (resending them as a whole with
> incremented v$n header is probably the easiest if we need to do so).
>
> Thanks.
>
The patches about append_todo_help() and edit-todo are not up to date,
so I’ll resend them in a few minutes. Otherwise, this looks good to me.
Cheers,
Alban
^ permalink raw reply [flat|nested] 51+ messages in thread