* [PATCH 0/8] sequencer refactoring
@ 2023-03-23 16:22 Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
` (10 more replies)
0 siblings, 11 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
This is a preparatory series for the separately posted 'rebase --rewind' patch,
but I think it has value in itself.
Oswald Buddenhagen (8):
rebase: simplify code related to imply_merge()
rebase: move parse_opt_keep_empty() down
sequencer: pass around rebase action explicitly
sequencer: create enum for edit_todo_list() return value
rebase: preserve interactive todo file on checkout failure
sequencer: simplify allocation of result array in
todo_list_rearrange_squash()
sequencer: pass `onto` to complete_action() as object-id
rebase: improve resumption from incorrect initial todo list
builtin/rebase.c | 63 +++++++--------
builtin/revert.c | 3 +-
rebase-interactive.c | 36 ++++-----
rebase-interactive.h | 27 ++++++-
sequencer.c | 139 +++++++++++++++++++---------------
sequencer.h | 15 ++--
t/t3404-rebase-interactive.sh | 34 ++++++++-
7 files changed, 196 insertions(+), 121 deletions(-)
--
2.40.0.152.g15d061e6df
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH 1/8] rebase: simplify code related to imply_merge()
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
@ 2023-03-23 16:22 ` Oswald Buddenhagen
2023-03-23 19:40 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 2/8] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
` (9 subsequent siblings)
10 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
The code's evolution left in some bits surrounding enum rebase_type that
don't really make sense any more. In particular, it makes no sense to
invoke imply_merge() if the type is already known not to be
REBASE_APPLY, and it makes no sense to assign the type after calling
imply_merge().
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
builtin/rebase.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b7b908b66..8ffea0f0d8 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -372,7 +372,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
opts->keep_empty = !unset;
- opts->type = REBASE_MERGE;
return 0;
}
@@ -1494,9 +1493,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
}
- if (options.type == REBASE_MERGE)
- imply_merge(&options, "--merge");
-
if (options.root && !options.onto_name)
imply_merge(&options, "--root without --onto");
@@ -1534,7 +1530,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
- imply_merge(&options, "--merge");
+ options.type = REBASE_MERGE;
else if (!strcmp(options.default_backend, "apply"))
options.type = REBASE_APPLY;
else
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 2/8] rebase: move parse_opt_keep_empty() down
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
@ 2023-03-23 16:22 ` Oswald Buddenhagen
2023-03-23 19:39 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 3/8] sequencer: pass around rebase action explicitly Oswald Buddenhagen
` (8 subsequent siblings)
10 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
This moves it right next to parse_opt_empty(), which is a much more
logical place. As a side effect, this removes the need for a forward
declaration of imply_merge().
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
builtin/rebase.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 8ffea0f0d8..491759db19 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -362,19 +362,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)
return ret;
}
-static void imply_merge(struct rebase_options *opts, const char *option);
-static int parse_opt_keep_empty(const struct option *opt, const char *arg,
- int unset)
-{
- struct rebase_options *opts = opt->value;
-
- BUG_ON_OPT_ARG(arg);
-
- imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
- opts->keep_empty = !unset;
- return 0;
-}
-
static int is_merge(struct rebase_options *opts)
{
return opts->type == REBASE_MERGE;
@@ -969,6 +956,18 @@ static enum empty_type parse_empty_value(const char *value)
die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
}
+static int parse_opt_keep_empty(const struct option *opt, const char *arg,
+ int unset)
+{
+ struct rebase_options *opts = opt->value;
+
+ BUG_ON_OPT_ARG(arg);
+
+ imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
+ opts->keep_empty = !unset;
+ return 0;
+}
+
static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
{
struct rebase_options *options = opt->value;
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 3/8] sequencer: pass around rebase action explicitly
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 2/8] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
@ 2023-03-23 16:22 ` Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 4/8] sequencer: create enum for edit_todo_list() return value Oswald Buddenhagen
` (7 subsequent siblings)
10 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
... instead of deriving it from other arguments. This is a lot cleaner
and more extensible.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
builtin/rebase.c | 25 ++++++++++---------------
rebase-interactive.c | 21 +++++++++++----------
rebase-interactive.h | 16 ++++++++++++++--
sequencer.c | 16 +++++++++-------
sequencer.h | 8 +++++---
5 files changed, 49 insertions(+), 37 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 491759db19..a309addd50 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -58,16 +58,6 @@ enum empty_type {
EMPTY_ASK
};
-enum action {
- ACTION_NONE = 0,
- ACTION_CONTINUE,
- ACTION_SKIP,
- ACTION_ABORT,
- ACTION_QUIT,
- ACTION_EDIT_TODO,
- ACTION_SHOW_CURRENT_PATCH
-};
-
static const char *action_names[] = {
"undefined",
"continue",
@@ -104,7 +94,7 @@ struct rebase_options {
REBASE_INTERACTIVE_EXPLICIT = 1<<4,
} flags;
struct strvec git_am_opts;
- enum action action;
+ enum rebase_action action;
char *reflog_action;
int signoff;
int allow_rerere_autoupdate;
@@ -198,9 +188,11 @@ static int edit_todo_file(unsigned flags)
return error_errno(_("could not read '%s'."), todo_file);
strbuf_stripspace(&todo_list.buf, 1);
- res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);
+ res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags,
+ ACTION_EDIT_TODO);
if (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,
- NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))
+ NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS),
+ ACTION_EDIT_TODO))
res = error_errno(_("could not write '%s'"), todo_file);
todo_list_release(&todo_list);
@@ -294,7 +286,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
ret = complete_action(the_repository, &replay, flags,
shortrevisions, opts->onto_name, opts->onto,
&opts->orig_head->object.oid, &opts->exec,
- opts->autosquash, opts->update_refs, &todo_list);
+ opts->autosquash, opts->update_refs, &todo_list,
+ opts->action);
}
cleanup:
@@ -1246,7 +1239,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
else if (options.exec.nr)
trace2_cmd_mode("interactive-exec");
else
- trace2_cmd_mode(action_names[options.action]);
+ trace2_cmd_mode(
+ (BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),
+ action_names[options.action]));
}
options.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 7407c59319..111a2071ae 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -35,7 +35,7 @@ static enum missing_commit_check_level get_missing_commit_check_level(void)
return MISSING_COMMIT_CHECK_IGNORE;
}
-void append_todo_help(int command_count,
+void append_todo_help(int command_count, enum rebase_action action,
const char *shortrevisions, const char *shortonto,
struct strbuf *buf)
{
@@ -62,9 +62,8 @@ void append_todo_help(int command_count,
" updated at the end of the rebase\n"
"\n"
"These lines can be re-ordered; they are executed from top to bottom.\n");
- unsigned edit_todo = !(shortrevisions && shortonto);
- if (!edit_todo) {
+ if (action == ACTION_NONE) {
strbuf_addch(buf, '\n');
strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
"Rebase %s onto %s (%d commands)",
@@ -83,7 +82,7 @@ void append_todo_help(int command_count,
strbuf_add_commented_lines(buf, msg, strlen(msg));
- if (edit_todo)
+ if (action == ACTION_EDIT_TODO)
msg = _("\nYou are editing the todo file "
"of an ongoing interactive rebase.\n"
"To continue rebase after editing, run:\n"
@@ -97,35 +96,37 @@ void append_todo_help(int command_count,
int edit_todo_list(struct repository *r, struct todo_list *todo_list,
struct todo_list *new_todo, const char *shortrevisions,
- const char *shortonto, unsigned flags)
+ const char *shortonto, unsigned flags,
+ enum rebase_action action)
{
const char *todo_file = rebase_path_todo(),
*todo_backup = rebase_path_todo_backup();
- unsigned initial = shortrevisions && shortonto;
int incorrect = 0;
/* If the user is editing the todo list, we first try to parse
* it. If there is an error, we do not return, because the user
* might want to fix it in the first place. */
- if (!initial)
+ if (action != ACTION_NONE)
incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
file_exists(rebase_path_dropped());
if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
- -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
+ -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP,
+ action))
return error_errno(_("could not write '%s'"), todo_file);
if (!incorrect &&
todo_list_write_to_file(r, todo_list, todo_backup,
shortrevisions, shortonto, -1,
- (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
+ (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS,
+ action) < 0)
return error(_("could not write '%s'."), rebase_path_todo_backup());
if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
return -2;
strbuf_stripspace(&new_todo->buf, 1);
- if (initial && new_todo->buf.len == 0)
+ if (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)
return -3;
if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 7239c60f79..d9873d3497 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -5,12 +5,24 @@ struct strbuf;
struct repository;
struct todo_list;
-void append_todo_help(int command_count,
+enum rebase_action {
+ ACTION_NONE = 0,
+ ACTION_CONTINUE,
+ ACTION_SKIP,
+ ACTION_ABORT,
+ ACTION_QUIT,
+ ACTION_EDIT_TODO,
+ ACTION_SHOW_CURRENT_PATCH,
+ ACTION_LAST
+};
+
+void append_todo_help(int command_count, enum rebase_action action,
const char *shortrevisions, const char *shortonto,
struct strbuf *buf);
int edit_todo_list(struct repository *r, struct todo_list *todo_list,
struct todo_list *new_todo, const char *shortrevisions,
- const char *shortonto, unsigned flags);
+ const char *shortonto, unsigned flags,
+ enum rebase_action action);
int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
int todo_list_check_against_backup(struct repository *r,
diff --git a/sequencer.c b/sequencer.c
index 7c275c9a65..f05174d151 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5894,14 +5894,15 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
const char *file, const char *shortrevisions,
- const char *shortonto, int num, unsigned flags)
+ const char *shortonto, int num, unsigned flags,
+ enum rebase_action action)
{
int res;
struct strbuf buf = STRBUF_INIT;
todo_list_to_strbuf(r, todo_list, &buf, num, flags);
if (flags & TODO_LIST_APPEND_TODO_HELP)
- append_todo_help(count_commands(todo_list),
+ append_todo_help(count_commands(todo_list), action,
shortrevisions, shortonto, &buf);
res = write_message(buf.buf, buf.len, file, 0);
@@ -5941,7 +5942,8 @@ static int skip_unnecessary_picks(struct repository *r,
if (i > 0) {
const char *done_path = rebase_path_done();
- if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) {
+ if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0,
+ ACTION_NONE)) {
error_errno(_("could not write to '%s'"), done_path);
return -1;
}
@@ -6086,8 +6088,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
const char *shortrevisions, const char *onto_name,
struct commit *onto, const struct object_id *orig_head,
struct string_list *commands, unsigned autosquash,
- unsigned update_refs,
- struct todo_list *todo_list)
+ unsigned update_refs, struct todo_list *todo_list,
+ enum rebase_action action)
{
char shortonto[GIT_MAX_HEXSZ + 1];
const char *todo_file = rebase_path_todo();
@@ -6122,7 +6124,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
}
res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
- shortonto, flags);
+ shortonto, flags, action);
if (res == -1)
return -1;
else if (res == -2) {
@@ -6158,7 +6160,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
}
if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
- flags & ~(TODO_LIST_SHORTEN_IDS))) {
+ flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
todo_list_release(&new_todo);
return error_errno(_("could not write '%s'"), todo_file);
}
diff --git a/sequencer.h b/sequencer.h
index 33dbaf5b66..1a3e616af2 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -4,6 +4,7 @@
#include "strbuf.h"
#include "wt-status.h"
+enum rebase_action;
struct commit;
struct index_state;
struct repository;
@@ -134,7 +135,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
struct todo_list *todo_list);
int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
const char *file, const char *shortrevisions,
- const char *shortonto, int num, unsigned flags);
+ const char *shortonto, int num, unsigned flags,
+ enum rebase_action action);
void todo_list_release(struct todo_list *todo_list);
const char *todo_item_get_arg(struct todo_list *todo_list,
struct todo_item *item);
@@ -187,8 +189,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
const char *shortrevisions, const char *onto_name,
struct commit *onto, const struct object_id *orig_head,
struct string_list *commands, unsigned autosquash,
- unsigned update_refs,
- struct todo_list *todo_list);
+ unsigned update_refs, struct todo_list *todo_list,
+ enum rebase_action action);
int todo_list_rearrange_squash(struct todo_list *todo_list);
/*
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 4/8] sequencer: create enum for edit_todo_list() return value
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
` (2 preceding siblings ...)
2023-03-23 16:22 ` [PATCH 3/8] sequencer: pass around rebase action explicitly Oswald Buddenhagen
@ 2023-03-23 16:22 ` Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
` (6 subsequent siblings)
10 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
This is a lot cleaner than open-coding magic numbers.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
rebase-interactive.c | 15 ++++++++-------
rebase-interactive.h | 11 ++++++++++-
sequencer.c | 8 ++++----
3 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 111a2071ae..a3d8925b06 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -94,7 +94,8 @@ void append_todo_help(int command_count, enum rebase_action action,
strbuf_add_commented_lines(buf, msg, strlen(msg));
}
-int edit_todo_list(struct repository *r, struct todo_list *todo_list,
+enum edit_todo_result edit_todo_list(
+ struct repository *r, struct todo_list *todo_list,
struct todo_list *new_todo, const char *shortrevisions,
const char *shortonto, unsigned flags,
enum rebase_action action)
@@ -123,37 +124,37 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
return error(_("could not write '%s'."), rebase_path_todo_backup());
if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
- return -2;
+ return EDIT_TODO_FAILED;
strbuf_stripspace(&new_todo->buf, 1);
if (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)
- return -3;
+ return EDIT_TODO_ABORT;
if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
fprintf(stderr, _(edit_todo_list_advice));
- return -4;
+ return EDIT_TODO_INCORRECT;
}
if (incorrect) {
if (todo_list_check_against_backup(r, new_todo)) {
write_file(rebase_path_dropped(), "%s", "");
- return -4;
+ return EDIT_TODO_INCORRECT;
}
if (incorrect > 0)
unlink(rebase_path_dropped());
} else if (todo_list_check(todo_list, new_todo)) {
write_file(rebase_path_dropped(), "%s", "");
- return -4;
+ return EDIT_TODO_INCORRECT;
}
/*
* See if branches need to be added or removed from the update-refs
* file based on the new todo list.
*/
todo_list_filter_update_refs(r, new_todo);
- return 0;
+ return EDIT_TODO_OK;
}
define_commit_slab(commit_seen, unsigned char);
diff --git a/rebase-interactive.h b/rebase-interactive.h
index d9873d3497..5aa4111b4f 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -16,10 +16,19 @@ enum rebase_action {
ACTION_LAST
};
+enum edit_todo_result {
+ EDIT_TODO_OK = 0, // must be 0
+ EDIT_TODO_IOERROR = -1, // generic i/o error; must be -1
+ EDIT_TODO_FAILED = -2, // editing failed
+ EDIT_TODO_ABORT = -3, // user requested abort
+ EDIT_TODO_INCORRECT = -4 // file violates syntax or constraints
+};
+
void append_todo_help(int command_count, enum rebase_action action,
const char *shortrevisions, const char *shortonto,
struct strbuf *buf);
-int edit_todo_list(struct repository *r, struct todo_list *todo_list,
+enum edit_todo_result edit_todo_list(
+ struct repository *r, struct todo_list *todo_list,
struct todo_list *new_todo, const char *shortrevisions,
const char *shortonto, unsigned flags,
enum rebase_action action);
diff --git a/sequencer.c b/sequencer.c
index f05174d151..b1c29c8802 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6125,20 +6125,20 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
shortonto, flags, action);
- if (res == -1)
+ if (res == EDIT_TODO_IOERROR)
return -1;
- else if (res == -2) {
+ else if (res == EDIT_TODO_FAILED) {
apply_autostash(rebase_path_autostash());
sequencer_remove_state(opts);
return -1;
- } else if (res == -3) {
+ } else if (res == EDIT_TODO_ABORT) {
apply_autostash(rebase_path_autostash());
sequencer_remove_state(opts);
todo_list_release(&new_todo);
return error(_("nothing to do"));
- } else if (res == -4) {
+ } else if (res == EDIT_TODO_INCORRECT) {
checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
todo_list_release(&new_todo);
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 5/8] rebase: preserve interactive todo file on checkout failure
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
` (3 preceding siblings ...)
2023-03-23 16:22 ` [PATCH 4/8] sequencer: create enum for edit_todo_list() return value Oswald Buddenhagen
@ 2023-03-23 16:22 ` Oswald Buddenhagen
2023-03-23 19:31 ` Phillip Wood
2023-03-23 20:16 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Oswald Buddenhagen
` (5 subsequent siblings)
10 siblings, 2 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
Creating a suitable todo file is a potentially labor-intensive process,
so be less cavalier about discarding it when something goes wrong (e.g.,
the user messed with the repo while editing the todo).
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
builtin/rebase.c | 1 +
sequencer.c | 4 ++++
sequencer.h | 1 +
t/t3404-rebase-interactive.sh | 3 ++-
4 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index a309addd50..728c869db4 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
replay.quiet = !(opts->flags & REBASE_NO_QUIET);
replay.verbose = opts->flags & REBASE_VERBOSE;
+ replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;
replay.reschedule_failed_exec = opts->reschedule_failed_exec;
replay.committer_date_is_author_date =
opts->committer_date_is_author_date;
diff --git a/sequencer.c b/sequencer.c
index b1c29c8802..f8a7f4e721 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
.default_reflog_action = sequencer_reflog_action(opts)
};
if (reset_head(r, &ropts)) {
+ // Editing the todo may have been costly; don't just discard it.
+ if (opts->precious_todo)
+ exit(1); // Error was already printed
+
apply_autostash(rebase_path_autostash());
sequencer_remove_state(opts);
return error(_("could not detach HEAD"));
diff --git a/sequencer.h b/sequencer.h
index 1a3e616af2..a1b8ca6eb1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -47,6 +47,7 @@ struct replay_opts {
int keep_redundant_commits;
int verbose;
int quiet;
+ int precious_todo;
int reschedule_failed_exec;
int committer_date_is_author_date;
int ignore_date;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ff0afad63e..c625aad10a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -288,13 +288,14 @@ test_expect_success 'abort' '
'
test_expect_success 'abort with error when new base cannot be checked out' '
+ test_when_finished "git rebase --abort ||:" &&
git rm --cached file1 &&
git commit -m "remove file in base" &&
test_must_fail git rebase -i primary > output 2>&1 &&
test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \
output &&
test_i18ngrep "file1" output &&
- test_path_is_missing .git/rebase-merge &&
+ test_path_is_dir .git/rebase-merge &&
rm file1 &&
git reset --hard HEAD^
'
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash()
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
` (4 preceding siblings ...)
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
@ 2023-03-23 16:22 ` Oswald Buddenhagen
2023-03-23 19:46 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id Oswald Buddenhagen
` (4 subsequent siblings)
10 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
The operation doesn't change the number of elements in the array, so we do
not need to allocate the result piecewise.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
sequencer.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index f8a7f4e721..fb224445fa 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6225,7 +6225,7 @@ static int skip_fixupish(const char *subject, const char **p) {
int todo_list_rearrange_squash(struct todo_list *todo_list)
{
struct hashmap subject2item;
- int rearranged = 0, *next, *tail, i, nr = 0, alloc = 0;
+ int rearranged = 0, *next, *tail, i, nr = 0;
char **subjects;
struct commit_todo_item commit_todo;
struct todo_item *items = NULL;
@@ -6334,6 +6334,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
}
if (rearranged) {
+ items = ALLOC_ARRAY(items, todo_list->nr);
+
for (i = 0; i < todo_list->nr; i++) {
enum todo_command command = todo_list->items[i].command;
int cur = i;
@@ -6346,16 +6348,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
continue;
while (cur >= 0) {
- ALLOC_GROW(items, nr + 1, alloc);
items[nr++] = todo_list->items[cur];
cur = next[cur];
}
}
+ assert(nr == todo_list->nr);
+ todo_list->alloc = nr;
FREE_AND_NULL(todo_list->items);
todo_list->items = items;
- todo_list->nr = nr;
- todo_list->alloc = alloc;
}
free(next);
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
` (5 preceding siblings ...)
2023-03-23 16:22 ` [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Oswald Buddenhagen
@ 2023-03-23 16:22 ` Oswald Buddenhagen
2023-03-23 19:34 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 8/8] rebase: improve resumption from incorrect initial todo list Oswald Buddenhagen
` (3 subsequent siblings)
10 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
... instead of as a commit, which makes the purpose clearer and will
simplify things later.
As a side effect, this change revealed that skip_unnecessary_picks() was
butchering the commit object due to missing const-correctness. Slightly
adjust its API to rectify this.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
builtin/rebase.c | 2 +-
sequencer.c | 21 ++++++++++-----------
sequencer.h | 2 +-
3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 728c869db4..e703b29835 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -285,7 +285,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
BUG("unusable todo list");
ret = complete_action(the_repository, &replay, flags,
- shortrevisions, opts->onto_name, opts->onto,
+ shortrevisions, opts->onto_name, &opts->onto->object.oid,
&opts->orig_head->object.oid, &opts->exec,
opts->autosquash, opts->update_refs, &todo_list,
opts->action);
diff --git a/sequencer.c b/sequencer.c
index fb224445fa..aef42122f1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2083,7 +2083,7 @@ static void flush_rewritten_pending(void)
strbuf_release(&buf);
}
-static void record_in_rewritten(struct object_id *oid,
+static void record_in_rewritten(const struct object_id *oid,
enum todo_command next_command)
{
FILE *out = fopen_or_warn(rebase_path_rewritten_pending(), "a");
@@ -5918,7 +5918,7 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
/* skip picking commits whose parents are unchanged */
static int skip_unnecessary_picks(struct repository *r,
struct todo_list *todo_list,
- struct object_id *base_oid)
+ const struct object_id **base_oid)
{
struct object_id *parent_oid;
int i;
@@ -5939,9 +5939,9 @@ static int skip_unnecessary_picks(struct repository *r,
if (item->commit->parents->next)
break; /* merge commit */
parent_oid = &item->commit->parents->item->object.oid;
- if (!oideq(parent_oid, base_oid))
+ if (!oideq(parent_oid, *base_oid))
break;
- oidcpy(base_oid, &item->commit->object.oid);
+ *base_oid = &item->commit->object.oid;
}
if (i > 0) {
const char *done_path = rebase_path_done();
@@ -5958,7 +5958,7 @@ static int skip_unnecessary_picks(struct repository *r,
todo_list->done_nr += i;
if (is_fixup(peek_command(todo_list, 0)))
- record_in_rewritten(base_oid, peek_command(todo_list, 0));
+ record_in_rewritten(*base_oid, peek_command(todo_list, 0));
}
return 0;
@@ -6090,19 +6090,18 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
const char *shortrevisions, const char *onto_name,
- struct commit *onto, const struct object_id *orig_head,
+ const struct object_id *onto, const struct object_id *orig_head,
struct string_list *commands, unsigned autosquash,
unsigned update_refs, struct todo_list *todo_list,
enum rebase_action action)
{
char shortonto[GIT_MAX_HEXSZ + 1];
const char *todo_file = rebase_path_todo();
struct todo_list new_todo = TODO_LIST_INIT;
struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
- struct object_id oid = onto->object.oid;
int res;
- find_unique_abbrev_r(shortonto, &oid, DEFAULT_ABBREV);
+ find_unique_abbrev_r(shortonto, onto, DEFAULT_ABBREV);
if (buf->len == 0) {
struct todo_item *item = append_new_todo(todo_list);
@@ -6143,7 +6142,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
return error(_("nothing to do"));
} else if (res == EDIT_TODO_INCORRECT) {
- checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
+ checkout_onto(r, opts, onto_name, onto, orig_head);
todo_list_release(&new_todo);
return -1;
@@ -6158,7 +6157,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
BUG("invalid todo list after expanding IDs:\n%s",
new_todo.buf.buf);
- if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {
+ if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
todo_list_release(&new_todo);
return error(_("could not skip unnecessary pick commands"));
}
@@ -6171,7 +6170,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
res = -1;
- if (checkout_onto(r, opts, onto_name, &oid, orig_head))
+ if (checkout_onto(r, opts, onto_name, onto, orig_head))
goto cleanup;
if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
diff --git a/sequencer.h b/sequencer.h
index a1b8ca6eb1..24bf71d5db 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -188,7 +188,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
const char *shortrevisions, const char *onto_name,
- struct commit *onto, const struct object_id *orig_head,
+ const struct object_id *onto, const struct object_id *orig_head,
struct string_list *commands, unsigned autosquash,
unsigned update_refs, struct todo_list *todo_list,
enum rebase_action action);
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
` (6 preceding siblings ...)
2023-03-23 16:22 ` [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id Oswald Buddenhagen
@ 2023-03-23 16:22 ` Oswald Buddenhagen
2023-03-26 14:28 ` Phillip Wood
2023-03-23 19:38 ` [PATCH 0/8] sequencer refactoring Phillip Wood
` (2 subsequent siblings)
10 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 16:22 UTC (permalink / raw)
To: git
When the user butchers the todo file during rebase -i setup, the
--continue which would follow --edit-todo would have skipped the last
steps of the setup. Notably, this would bypass the fast-forward over
untouched picks (though the actual picking loop would still fast-forward
the commits, one by one).
Fix this by splitting off the tail of complete_action() to a new
start_rebase() function and call that from sequencer_continue() when no
commands have been executed yet.
More or less as a side effect, we no longer checkout `onto` before exiting
when the todo file is bad. This makes aborting cheaper and will simplify
things in a later change.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
builtin/rebase.c | 4 +-
builtin/revert.c | 3 +-
sequencer.c | 89 ++++++++++++++++++++---------------
sequencer.h | 4 +-
t/t3404-rebase-interactive.sh | 31 ++++++++++++
5 files changed, 91 insertions(+), 40 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e703b29835..61e5363ac7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -333,7 +333,9 @@ static int run_sequencer_rebase(struct rebase_options *opts)
case ACTION_CONTINUE: {
struct replay_opts replay_opts = get_replay_opts(opts);
- ret = sequencer_continue(the_repository, &replay_opts);
+ ret = sequencer_continue(the_repository, &replay_opts, flags,
+ opts->onto_name, &opts->onto->object.oid,
+ &opts->orig_head->object.oid);
replay_opts_release(&replay_opts);
break;
}
diff --git a/builtin/revert.c b/builtin/revert.c
index 62986a7b1b..00d3e19c62 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
return ret;
}
if (cmd == 'c')
- return sequencer_continue(the_repository, opts);
+ return sequencer_continue(the_repository, opts,
+ 0, NULL, NULL, NULL);
if (cmd == 'a')
return sequencer_rollback(the_repository, opts);
if (cmd == 's')
diff --git a/sequencer.c b/sequencer.c
index aef42122f1..0b4d16b8e8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3369,7 +3369,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts)
if (!is_directory(git_path_seq_dir()))
return 0;
- return sequencer_continue(r, opts);
+ return sequencer_continue(r, opts, 0, NULL, NULL, NULL);
give_advice:
error(_("there is nothing to skip"));
@@ -5096,7 +5096,13 @@ static int commit_staged_changes(struct repository *r,
return 0;
}
-int sequencer_continue(struct repository *r, struct replay_opts *opts)
+static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,
+ const char *onto_name, const struct object_id *onto,
+ const struct object_id *orig_head, struct todo_list *todo_list);
+
+int sequencer_continue(struct repository *r, struct replay_opts *opts, unsigned flags,
+ const char *onto_name, const struct object_id *onto,
+ const struct object_id *orig_head)
{
struct todo_list todo_list = TODO_LIST_INIT;
int res;
@@ -5117,6 +5123,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts)
unlink(rebase_path_dropped());
}
+ if (!todo_list.done_nr) {
+ res = start_rebase(r, opts, flags,
+ onto_name, onto,
+ orig_head, &todo_list);
+ goto release_todo_list;
+ }
+
opts->reflog_message = reflog_message(opts, "continue", NULL);
if (commit_staged_changes(r, opts, &todo_list)) {
res = -1;
@@ -6096,9 +6109,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
enum rebase_action action)
{
char shortonto[GIT_MAX_HEXSZ + 1];
- const char *todo_file = rebase_path_todo();
struct todo_list new_todo = TODO_LIST_INIT;
- struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
+ struct strbuf *buf = &todo_list->buf;
int res;
find_unique_abbrev_r(shortonto, onto, DEFAULT_ABBREV);
@@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
return error(_("nothing to do"));
} else if (res == EDIT_TODO_INCORRECT) {
- checkout_onto(r, opts, onto_name, onto, orig_head);
todo_list_release(&new_todo);
return -1;
}
- /* Expand the commit IDs */
- todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
- strbuf_swap(&new_todo.buf, &buf2);
- strbuf_release(&buf2);
- new_todo.total_nr -= new_todo.nr;
- if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
- BUG("invalid todo list after expanding IDs:\n%s",
- new_todo.buf.buf);
-
- if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
- todo_list_release(&new_todo);
- return error(_("could not skip unnecessary pick commands"));
- }
-
- if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
- flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
- todo_list_release(&new_todo);
- return error_errno(_("could not write '%s'"), todo_file);
- }
-
- res = -1;
-
- if (checkout_onto(r, opts, onto_name, onto, orig_head))
- goto cleanup;
-
- if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
- goto cleanup;
-
- todo_list_write_total_nr(&new_todo);
- res = pick_commits(r, &new_todo, opts);
-
-cleanup:
+ res = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo);
todo_list_release(&new_todo);
return res;
}
+static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,
+ const char *onto_name, const struct object_id *onto,
+ const struct object_id *orig_head, struct todo_list *todo_list)
+{
+ const char *todo_file = rebase_path_todo();
+ struct strbuf buf2 = STRBUF_INIT;
+
+ /* Expand the commit IDs */
+ todo_list_to_strbuf(r, todo_list, &buf2, -1, 0);
+ strbuf_swap(&todo_list->buf, &buf2);
+ strbuf_release(&buf2);
+ todo_list->total_nr -= todo_list->nr;
+ if (todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) < 0)
+ BUG("invalid todo list after expanding IDs:\n%s",
+ todo_list->buf.buf);
+
+ if (opts->allow_ff && skip_unnecessary_picks(r, todo_list, &onto))
+ return error(_("could not skip unnecessary pick commands"));
+
+ if (todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1,
+ flags & ~(TODO_LIST_SHORTEN_IDS |
+ TODO_LIST_APPEND_TODO_HELP),
+ ACTION_CONTINUE))
+ return error_errno(_("could not write '%s'"), todo_file);
+
+ if (checkout_onto(r, opts, onto_name, onto, orig_head))
+ return -1;
+
+ if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
+ return -1;
+
+ todo_list_write_total_nr(todo_list);
+ return pick_commits(r, todo_list, opts);
+}
+
struct subject2item_entry {
struct hashmap_entry entry;
int i;
diff --git a/sequencer.h b/sequencer.h
index 24bf71d5db..33bcff89e0 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,7 +158,9 @@ void todo_list_filter_update_refs(struct repository *r,
void sequencer_init_config(struct replay_opts *opts);
int sequencer_pick_revisions(struct repository *repo,
struct replay_opts *opts);
-int sequencer_continue(struct repository *repo, struct replay_opts *opts);
+int sequencer_continue(struct repository *repo, struct replay_opts *opts, unsigned flags,
+ const char *onto_name, const struct object_id *onto,
+ const struct object_id *orig_head);
int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
int sequencer_skip(struct repository *repo, struct replay_opts *opts);
void replay_opts_release(struct replay_opts *opts);
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c625aad10a..dd47f0bbce 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1597,6 +1597,37 @@ test_expect_success 'static check of bad command' '
test C = $(git cat-file commit HEAD^ | sed -ne \$p)
'
+
+test_expect_success 'continue after bad first command' '
+ test_when_finished "git rebase --abort ||:" &&
+ git checkout primary^0 &&
+ git reflog expire --expire=all HEAD &&
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
+ git rebase -i HEAD~3 &&
+ test_cmp_rev HEAD primary &&
+ FAKE_LINES="pick 2 pick 3 reword 4" git rebase --edit-todo &&
+ FAKE_COMMIT_MESSAGE="E_reworded" git rebase --continue
+ ) &&
+ git reflog > reflog &&
+ test $(grep -c fast-forward reflog) = 1 &&
+ test_cmp_rev HEAD~1 primary~1 &&
+ test "$(git log -1 --format=%B)" = "E_reworded"
+'
+
+test_expect_success 'abort after bad first command' '
+ test_when_finished "git rebase --abort ||:" &&
+ git checkout primary^0 &&
+ (
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
+ git rebase -i HEAD~3
+ ) &&
+ git rebase --abort &&
+ test_cmp_rev HEAD primary
+'
+
test_expect_success 'tabs and spaces are accepted in the todolist' '
rebase_setup_and_clean indented-comment &&
write_script add-indent.sh <<-\EOF &&
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH 3/8] sequencer: pass around rebase action explicitly
2023-03-23 16:22 ` [PATCH 3/8] sequencer: pass around rebase action explicitly Oswald Buddenhagen
@ 2023-03-23 19:27 ` Phillip Wood
2023-03-23 21:27 ` Oswald Buddenhagen
0 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-03-23 19:27 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
I appreciate the sentiment behind this patch, but it looks like an
awful lot of churn just to clean up a couple of lines in
append_todo_help() and edit_todo_list(). I suspect you may want this
change for your --rewind patch, if so I think it would be better to post
it in that series where we can better judge the benefit.
Best Wishes
Phillip
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> ... instead of deriving it from other arguments. This is a lot cleaner
> and more extensible.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> builtin/rebase.c | 25 ++++++++++---------------
> rebase-interactive.c | 21 +++++++++++----------
> rebase-interactive.h | 16 ++++++++++++++--
> sequencer.c | 16 +++++++++-------
> sequencer.h | 8 +++++---
> 5 files changed, 49 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 491759db19..a309addd50 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -58,16 +58,6 @@ enum empty_type {
> EMPTY_ASK
> };
>
> -enum action {
> - ACTION_NONE = 0,
> - ACTION_CONTINUE,
> - ACTION_SKIP,
> - ACTION_ABORT,
> - ACTION_QUIT,
> - ACTION_EDIT_TODO,
> - ACTION_SHOW_CURRENT_PATCH
> -};
> -
> static const char *action_names[] = {
> "undefined",
> "continue",
> @@ -104,7 +94,7 @@ struct rebase_options {
> REBASE_INTERACTIVE_EXPLICIT = 1<<4,
> } flags;
> struct strvec git_am_opts;
> - enum action action;
> + enum rebase_action action;
> char *reflog_action;
> int signoff;
> int allow_rerere_autoupdate;
> @@ -198,9 +188,11 @@ static int edit_todo_file(unsigned flags)
> return error_errno(_("could not read '%s'."), todo_file);
>
> strbuf_stripspace(&todo_list.buf, 1);
> - res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags);
> + res = edit_todo_list(the_repository, &todo_list, &new_todo, NULL, NULL, flags,
> + ACTION_EDIT_TODO);
> if (!res && todo_list_write_to_file(the_repository, &new_todo, todo_file,
> - NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS)))
> + NULL, NULL, -1, flags & ~(TODO_LIST_SHORTEN_IDS),
> + ACTION_EDIT_TODO))
> res = error_errno(_("could not write '%s'"), todo_file);
>
> todo_list_release(&todo_list);
> @@ -294,7 +286,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
> ret = complete_action(the_repository, &replay, flags,
> shortrevisions, opts->onto_name, opts->onto,
> &opts->orig_head->object.oid, &opts->exec,
> - opts->autosquash, opts->update_refs, &todo_list);
> + opts->autosquash, opts->update_refs, &todo_list,
> + opts->action);
> }
>
> cleanup:
> @@ -1246,7 +1239,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> else if (options.exec.nr)
> trace2_cmd_mode("interactive-exec");
> else
> - trace2_cmd_mode(action_names[options.action]);
> + trace2_cmd_mode(
> + (BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),
> + action_names[options.action]));
> }
>
> options.reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT);
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 7407c59319..111a2071ae 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -35,7 +35,7 @@ static enum missing_commit_check_level get_missing_commit_check_level(void)
> return MISSING_COMMIT_CHECK_IGNORE;
> }
>
> -void append_todo_help(int command_count,
> +void append_todo_help(int command_count, enum rebase_action action,
> const char *shortrevisions, const char *shortonto,
> struct strbuf *buf)
> {
> @@ -62,9 +62,8 @@ void append_todo_help(int command_count,
> " updated at the end of the rebase\n"
> "\n"
> "These lines can be re-ordered; they are executed from top to bottom.\n");
> - unsigned edit_todo = !(shortrevisions && shortonto);
>
> - if (!edit_todo) {
> + if (action == ACTION_NONE) {
> strbuf_addch(buf, '\n');
> strbuf_commented_addf(buf, Q_("Rebase %s onto %s (%d command)",
> "Rebase %s onto %s (%d commands)",
> @@ -83,7 +82,7 @@ void append_todo_help(int command_count,
>
> strbuf_add_commented_lines(buf, msg, strlen(msg));
>
> - if (edit_todo)
> + if (action == ACTION_EDIT_TODO)
> msg = _("\nYou are editing the todo file "
> "of an ongoing interactive rebase.\n"
> "To continue rebase after editing, run:\n"
> @@ -97,35 +96,37 @@ void append_todo_help(int command_count,
>
> int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> struct todo_list *new_todo, const char *shortrevisions,
> - const char *shortonto, unsigned flags)
> + const char *shortonto, unsigned flags,
> + enum rebase_action action)
> {
> const char *todo_file = rebase_path_todo(),
> *todo_backup = rebase_path_todo_backup();
> - unsigned initial = shortrevisions && shortonto;
> int incorrect = 0;
>
> /* If the user is editing the todo list, we first try to parse
> * it. If there is an error, we do not return, because the user
> * might want to fix it in the first place. */
> - if (!initial)
> + if (action != ACTION_NONE)
> incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
> file_exists(rebase_path_dropped());
>
> if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
> - -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP))
> + -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP,
> + action))
> return error_errno(_("could not write '%s'"), todo_file);
>
> if (!incorrect &&
> todo_list_write_to_file(r, todo_list, todo_backup,
> shortrevisions, shortonto, -1,
> - (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS) < 0)
> + (flags | TODO_LIST_APPEND_TODO_HELP) & ~TODO_LIST_SHORTEN_IDS,
> + action) < 0)
> return error(_("could not write '%s'."), rebase_path_todo_backup());
>
> if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
> return -2;
>
> strbuf_stripspace(&new_todo->buf, 1);
> - if (initial && new_todo->buf.len == 0)
> + if (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)
> return -3;
>
> if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index 7239c60f79..d9873d3497 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -5,12 +5,24 @@ struct strbuf;
> struct repository;
> struct todo_list;
>
> -void append_todo_help(int command_count,
> +enum rebase_action {
> + ACTION_NONE = 0,
> + ACTION_CONTINUE,
> + ACTION_SKIP,
> + ACTION_ABORT,
> + ACTION_QUIT,
> + ACTION_EDIT_TODO,
> + ACTION_SHOW_CURRENT_PATCH,
> + ACTION_LAST
> +};
> +
> +void append_todo_help(int command_count, enum rebase_action action,
> const char *shortrevisions, const char *shortonto,
> struct strbuf *buf);
> int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> struct todo_list *new_todo, const char *shortrevisions,
> - const char *shortonto, unsigned flags);
> + const char *shortonto, unsigned flags,
> + enum rebase_action action);
>
> int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo);
> int todo_list_check_against_backup(struct repository *r,
> diff --git a/sequencer.c b/sequencer.c
> index 7c275c9a65..f05174d151 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5894,14 +5894,15 @@ static void todo_list_to_strbuf(struct repository *r, struct todo_list *todo_lis
>
> int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
> const char *file, const char *shortrevisions,
> - const char *shortonto, int num, unsigned flags)
> + const char *shortonto, int num, unsigned flags,
> + enum rebase_action action)
> {
> int res;
> struct strbuf buf = STRBUF_INIT;
>
> todo_list_to_strbuf(r, todo_list, &buf, num, flags);
> if (flags & TODO_LIST_APPEND_TODO_HELP)
> - append_todo_help(count_commands(todo_list),
> + append_todo_help(count_commands(todo_list), action,
> shortrevisions, shortonto, &buf);
>
> res = write_message(buf.buf, buf.len, file, 0);
> @@ -5941,7 +5942,8 @@ static int skip_unnecessary_picks(struct repository *r,
> if (i > 0) {
> const char *done_path = rebase_path_done();
>
> - if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0)) {
> + if (todo_list_write_to_file(r, todo_list, done_path, NULL, NULL, i, 0,
> + ACTION_NONE)) {
> error_errno(_("could not write to '%s'"), done_path);
> return -1;
> }
> @@ -6086,8 +6088,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> const char *shortrevisions, const char *onto_name,
> struct commit *onto, const struct object_id *orig_head,
> struct string_list *commands, unsigned autosquash,
> - unsigned update_refs,
> - struct todo_list *todo_list)
> + unsigned update_refs, struct todo_list *todo_list,
> + enum rebase_action action)
> {
> char shortonto[GIT_MAX_HEXSZ + 1];
> const char *todo_file = rebase_path_todo();
> @@ -6122,7 +6124,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> }
>
> res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> - shortonto, flags);
> + shortonto, flags, action);
> if (res == -1)
> return -1;
> else if (res == -2) {
> @@ -6158,7 +6160,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> }
>
> if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
> - flags & ~(TODO_LIST_SHORTEN_IDS))) {
> + flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
> todo_list_release(&new_todo);
> return error_errno(_("could not write '%s'"), todo_file);
> }
> diff --git a/sequencer.h b/sequencer.h
> index 33dbaf5b66..1a3e616af2 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -4,6 +4,7 @@
> #include "strbuf.h"
> #include "wt-status.h"
>
> +enum rebase_action;
> struct commit;
> struct index_state;
> struct repository;
> @@ -134,7 +135,8 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
> struct todo_list *todo_list);
> int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
> const char *file, const char *shortrevisions,
> - const char *shortonto, int num, unsigned flags);
> + const char *shortonto, int num, unsigned flags,
> + enum rebase_action action);
> void todo_list_release(struct todo_list *todo_list);
> const char *todo_item_get_arg(struct todo_list *todo_list,
> struct todo_item *item);
> @@ -187,8 +189,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> const char *shortrevisions, const char *onto_name,
> struct commit *onto, const struct object_id *orig_head,
> struct string_list *commands, unsigned autosquash,
> - unsigned update_refs,
> - struct todo_list *todo_list);
> + unsigned update_refs, struct todo_list *todo_list,
> + enum rebase_action action);
> int todo_list_rearrange_squash(struct todo_list *todo_list);
>
> /*
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 4/8] sequencer: create enum for edit_todo_list() return value
2023-03-23 16:22 ` [PATCH 4/8] sequencer: create enum for edit_todo_list() return value Oswald Buddenhagen
@ 2023-03-23 19:27 ` Phillip Wood
0 siblings, 0 replies; 49+ messages in thread
From: Phillip Wood @ 2023-03-23 19:27 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
This is a really useful cleanup
Thanks
Phillip
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> This is a lot cleaner than open-coding magic numbers.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> rebase-interactive.c | 15 ++++++++-------
> rebase-interactive.h | 11 ++++++++++-
> sequencer.c | 8 ++++----
> 3 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/rebase-interactive.c b/rebase-interactive.c
> index 111a2071ae..a3d8925b06 100644
> --- a/rebase-interactive.c
> +++ b/rebase-interactive.c
> @@ -94,7 +94,8 @@ void append_todo_help(int command_count, enum rebase_action action,
> strbuf_add_commented_lines(buf, msg, strlen(msg));
> }
>
> -int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> +enum edit_todo_result edit_todo_list(
> + struct repository *r, struct todo_list *todo_list,
> struct todo_list *new_todo, const char *shortrevisions,
> const char *shortonto, unsigned flags,
> enum rebase_action action)
> @@ -123,37 +124,37 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> return error(_("could not write '%s'."), rebase_path_todo_backup());
>
> if (launch_sequence_editor(todo_file, &new_todo->buf, NULL))
> - return -2;
> + return EDIT_TODO_FAILED;
>
> strbuf_stripspace(&new_todo->buf, 1);
> if (action != ACTION_EDIT_TODO && new_todo->buf.len == 0)
> - return -3;
> + return EDIT_TODO_ABORT;
>
> if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
> fprintf(stderr, _(edit_todo_list_advice));
> - return -4;
> + return EDIT_TODO_INCORRECT;
> }
>
> if (incorrect) {
> if (todo_list_check_against_backup(r, new_todo)) {
> write_file(rebase_path_dropped(), "%s", "");
> - return -4;
> + return EDIT_TODO_INCORRECT;
> }
>
> if (incorrect > 0)
> unlink(rebase_path_dropped());
> } else if (todo_list_check(todo_list, new_todo)) {
> write_file(rebase_path_dropped(), "%s", "");
> - return -4;
> + return EDIT_TODO_INCORRECT;
> }
>
> /*
> * See if branches need to be added or removed from the update-refs
> * file based on the new todo list.
> */
> todo_list_filter_update_refs(r, new_todo);
>
> - return 0;
> + return EDIT_TODO_OK;
> }
>
> define_commit_slab(commit_seen, unsigned char);
> diff --git a/rebase-interactive.h b/rebase-interactive.h
> index d9873d3497..5aa4111b4f 100644
> --- a/rebase-interactive.h
> +++ b/rebase-interactive.h
> @@ -16,10 +16,19 @@ enum rebase_action {
> ACTION_LAST
> };
>
> +enum edit_todo_result {
> + EDIT_TODO_OK = 0, // must be 0
> + EDIT_TODO_IOERROR = -1, // generic i/o error; must be -1
> + EDIT_TODO_FAILED = -2, // editing failed
> + EDIT_TODO_ABORT = -3, // user requested abort
> + EDIT_TODO_INCORRECT = -4 // file violates syntax or constraints
> +};
> +
> void append_todo_help(int command_count, enum rebase_action action,
> const char *shortrevisions, const char *shortonto,
> struct strbuf *buf);
> -int edit_todo_list(struct repository *r, struct todo_list *todo_list,
> +enum edit_todo_result edit_todo_list(
> + struct repository *r, struct todo_list *todo_list,
> struct todo_list *new_todo, const char *shortrevisions,
> const char *shortonto, unsigned flags,
> enum rebase_action action);
> diff --git a/sequencer.c b/sequencer.c
> index f05174d151..b1c29c8802 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6125,20 +6125,20 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>
> res = edit_todo_list(r, todo_list, &new_todo, shortrevisions,
> shortonto, flags, action);
> - if (res == -1)
> + if (res == EDIT_TODO_IOERROR)
> return -1;
> - else if (res == -2) {
> + else if (res == EDIT_TODO_FAILED) {
> apply_autostash(rebase_path_autostash());
> sequencer_remove_state(opts);
>
> return -1;
> - } else if (res == -3) {
> + } else if (res == EDIT_TODO_ABORT) {
> apply_autostash(rebase_path_autostash());
> sequencer_remove_state(opts);
> todo_list_release(&new_todo);
>
> return error(_("nothing to do"));
> - } else if (res == -4) {
> + } else if (res == EDIT_TODO_INCORRECT) {
> checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head);
> todo_list_release(&new_todo);
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
@ 2023-03-23 19:31 ` Phillip Wood
2023-03-23 22:38 ` Oswald Buddenhagen
2023-03-23 20:16 ` Junio C Hamano
1 sibling, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-03-23 19:31 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> Creating a suitable todo file is a potentially labor-intensive process,
> so be less cavalier about discarding it when something goes wrong (e.g.,
> the user messed with the repo while editing the todo).
I was thinking about this problem the other day in the context of
rescheduling commands when they cannot be executed because they would
overwrite an untracked file. My thought was that we should prepend a
"reset" command to the todo list so that the checkout happened when the
user continued the rebase. How does this patch ensure the checkout
happens when the user continues the rebase?
Best Wishes
Phillip
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> builtin/rebase.c | 1 +
> sequencer.c | 4 ++++
> sequencer.h | 1 +
> t/t3404-rebase-interactive.sh | 3 ++-
> 4 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index a309addd50..728c869db4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
> replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
> replay.quiet = !(opts->flags & REBASE_NO_QUIET);
> replay.verbose = opts->flags & REBASE_VERBOSE;
> + replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;
> replay.reschedule_failed_exec = opts->reschedule_failed_exec;
> replay.committer_date_is_author_date =
> opts->committer_date_is_author_date;
> diff --git a/sequencer.c b/sequencer.c
> index b1c29c8802..f8a7f4e721 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
> .default_reflog_action = sequencer_reflog_action(opts)
> };
> if (reset_head(r, &ropts)) {
> + // Editing the todo may have been costly; don't just discard it.
> + if (opts->precious_todo)
> + exit(1); // Error was already printed
> +
> apply_autostash(rebase_path_autostash());
> sequencer_remove_state(opts);
> return error(_("could not detach HEAD"));
> diff --git a/sequencer.h b/sequencer.h
> index 1a3e616af2..a1b8ca6eb1 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -47,6 +47,7 @@ struct replay_opts {
> int keep_redundant_commits;
> int verbose;
> int quiet;
> + int precious_todo;
> int reschedule_failed_exec;
> int committer_date_is_author_date;
> int ignore_date;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ff0afad63e..c625aad10a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -288,13 +288,14 @@ test_expect_success 'abort' '
> '
>
> test_expect_success 'abort with error when new base cannot be checked out' '
> + test_when_finished "git rebase --abort ||:" &&
> git rm --cached file1 &&
> git commit -m "remove file in base" &&
> test_must_fail git rebase -i primary > output 2>&1 &&
> test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \
> output &&
> test_i18ngrep "file1" output &&
> - test_path_is_missing .git/rebase-merge &&
> + test_path_is_dir .git/rebase-merge &&
> rm file1 &&
> git reset --hard HEAD^
> '
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id
2023-03-23 16:22 ` [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id Oswald Buddenhagen
@ 2023-03-23 19:34 ` Phillip Wood
2023-03-23 21:36 ` Oswald Buddenhagen
0 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-03-23 19:34 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> ... instead of as a commit, which makes the purpose clearer and will
> simplify things later.
given that we want onto to be a commit I'm not sure how this makes
anything clearer.
> As a side effect, this change revealed that skip_unnecessary_picks() was
> butchering the commit object due to missing const-correctness. Slightly
> adjust its API to rectify this.
I don't think this is correct. If you look at the original code it makes
a copy of the oid and uses the copy when calling skip_unnecessary_picks()
> int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
> const char *shortrevisions, const char *onto_name,
> - struct commit *onto, const struct object_id *orig_head,
> + const struct object_id *onto, const struct object_id *orig_head,
> struct string_list *commands, unsigned autosquash,
> unsigned update_refs, struct todo_list *todo_list,
> enum rebase_action action)
> {
> char shortonto[GIT_MAX_HEXSZ + 1];
> const char *todo_file = rebase_path_todo();
> struct todo_list new_todo = TODO_LIST_INIT;
> struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT;
> - struct object_id oid = onto->object.oid;
Here we copy the onto's oid
> @@ -6158,7 +6157,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
> BUG("invalid todo list after expanding IDs:\n%s",
> new_todo.buf.buf);
>
> - if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &oid)) {
Here we pass the copy to skip_unnecessary_picks()
Best Wishes
Phillip
> + if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
> todo_list_release(&new_todo);
> return error(_("could not skip unnecessary pick commands"));
> }
> @@ -6171,7 +6170,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>
> res = -1;
>
> - if (checkout_onto(r, opts, onto_name, &oid, orig_head))
> + if (checkout_onto(r, opts, onto_name, onto, orig_head))
> goto cleanup;
>
> if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
> diff --git a/sequencer.h b/sequencer.h
> index a1b8ca6eb1..24bf71d5db 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -188,7 +188,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>
> int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,
> const char *shortrevisions, const char *onto_name,
> - struct commit *onto, const struct object_id *orig_head,
> + const struct object_id *onto, const struct object_id *orig_head,
> struct string_list *commands, unsigned autosquash,
> unsigned update_refs, struct todo_list *todo_list,
> enum rebase_action action);
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 0/8] sequencer refactoring
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
` (7 preceding siblings ...)
2023-03-23 16:22 ` [PATCH 8/8] rebase: improve resumption from incorrect initial todo list Oswald Buddenhagen
@ 2023-03-23 19:38 ` Phillip Wood
2023-03-25 11:08 ` Phillip Wood
2023-05-17 13:10 ` Phillip Wood
10 siblings, 0 replies; 49+ messages in thread
From: Phillip Wood @ 2023-03-23 19:38 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
Thanks for working on this. I've only had time for a quick read of the
first 7 patches but there are some worthwhile clean ups here and the
series is well structured. I'll try and have a thorough look at the last
patch but I'm going to be off line next week so it may take a while.
Best Wishes
Phillip
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> This is a preparatory series for the separately posted 'rebase --rewind' patch,
> but I think it has value in itself.
>
>
> Oswald Buddenhagen (8):
> rebase: simplify code related to imply_merge()
> rebase: move parse_opt_keep_empty() down
> sequencer: pass around rebase action explicitly
> sequencer: create enum for edit_todo_list() return value
> rebase: preserve interactive todo file on checkout failure
> sequencer: simplify allocation of result array in
> todo_list_rearrange_squash()
> sequencer: pass `onto` to complete_action() as object-id
> rebase: improve resumption from incorrect initial todo list
>
> builtin/rebase.c | 63 +++++++--------
> builtin/revert.c | 3 +-
> rebase-interactive.c | 36 ++++-----
> rebase-interactive.h | 27 ++++++-
> sequencer.c | 139 +++++++++++++++++++---------------
> sequencer.h | 15 ++--
> t/t3404-rebase-interactive.sh | 34 ++++++++-
> 7 files changed, 196 insertions(+), 121 deletions(-)
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 2/8] rebase: move parse_opt_keep_empty() down
2023-03-23 16:22 ` [PATCH 2/8] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
@ 2023-03-23 19:39 ` Phillip Wood
0 siblings, 0 replies; 49+ messages in thread
From: Phillip Wood @ 2023-03-23 19:39 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> This moves it right next to parse_opt_empty(), which is a much more
> logical place. As a side effect, this removes the need for a forward
> declaration of imply_merge().
This looks good, it is nice to get rid of that forward declaration
Best Wishes
Phillip
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> builtin/rebase.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 8ffea0f0d8..491759db19 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -362,19 +362,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)
> return ret;
> }
>
> -static void imply_merge(struct rebase_options *opts, const char *option);
> -static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> - int unset)
> -{
> - struct rebase_options *opts = opt->value;
> -
> - BUG_ON_OPT_ARG(arg);
> -
> - imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> - opts->keep_empty = !unset;
> - return 0;
> -}
> -
> static int is_merge(struct rebase_options *opts)
> {
> return opts->type == REBASE_MERGE;
> @@ -969,6 +956,18 @@ static enum empty_type parse_empty_value(const char *value)
> die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
> }
>
> +static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> + int unset)
> +{
> + struct rebase_options *opts = opt->value;
> +
> + BUG_ON_OPT_ARG(arg);
> +
> + imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> + opts->keep_empty = !unset;
> + return 0;
> +}
> +
> static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
> {
> struct rebase_options *options = opt->value;
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/8] rebase: simplify code related to imply_merge()
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
@ 2023-03-23 19:40 ` Phillip Wood
2023-03-23 20:00 ` Junio C Hamano
0 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-03-23 19:40 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> The code's evolution left in some bits surrounding enum rebase_type that
> don't really make sense any more. In particular, it makes no sense to
> invoke imply_merge() if the type is already known not to be
> REBASE_APPLY, and it makes no sense to assign the type after calling
> imply_merge().
These look sensible, did imply_merges() use to do something more which
made these calls useful?
Best Wishes
Phillip
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> builtin/rebase.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 5b7b908b66..8ffea0f0d8 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -372,7 +372,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
>
> imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> opts->keep_empty = !unset;
> - opts->type = REBASE_MERGE;
> return 0;
> }
>
> @@ -1494,9 +1493,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> }
> }
>
> - if (options.type == REBASE_MERGE)
> - imply_merge(&options, "--merge");
> -
> if (options.root && !options.onto_name)
> imply_merge(&options, "--root without --onto");
>
> @@ -1534,7 +1530,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>
> if (options.type == REBASE_UNSPECIFIED) {
> if (!strcmp(options.default_backend, "merge"))
> - imply_merge(&options, "--merge");
> + options.type = REBASE_MERGE;
> else if (!strcmp(options.default_backend, "apply"))
> options.type = REBASE_APPLY;
> else
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash()
2023-03-23 16:22 ` [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Oswald Buddenhagen
@ 2023-03-23 19:46 ` Phillip Wood
2023-03-23 22:13 ` Oswald Buddenhagen
0 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-03-23 19:46 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> The operation doesn't change the number of elements in the array, so we do
> not need to allocate the result piecewise.
I think the reasoning behind this patch is sound.
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> sequencer.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index f8a7f4e721..fb224445fa 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6225,7 +6225,7 @@ static int skip_fixupish(const char *subject, const char **p) {
> int todo_list_rearrange_squash(struct todo_list *todo_list)
> {
> struct hashmap subject2item;
> - int rearranged = 0, *next, *tail, i, nr = 0, alloc = 0;
> + int rearranged = 0, *next, *tail, i, nr = 0;
> char **subjects;
> struct commit_todo_item commit_todo;
> struct todo_item *items = NULL;
> @@ -6334,6 +6334,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
> }
>
> if (rearranged) {
> + items = ALLOC_ARRAY(items, todo_list->nr);
> +
> for (i = 0; i < todo_list->nr; i++) {
> enum todo_command command = todo_list->items[i].command;
> int cur = i;
> @@ -6346,16 +6348,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
> continue;
>
> while (cur >= 0) {
> - ALLOC_GROW(items, nr + 1, alloc);
> items[nr++] = todo_list->items[cur];
> cur = next[cur];
> }
> }
>
> + assert(nr == todo_list->nr);
If this assert fails we may have already had some out of bounds memory
accesses.
> + todo_list->alloc = nr;
> FREE_AND_NULL(todo_list->items);
I think it would be cleaner to keep the original ordering and free the
old list before assigning todo_list->alloc
> todo_list->items = items;
> - todo_list->nr = nr;
> - todo_list->alloc = alloc;
> }
Best Wishes
Phillip
> free(next);
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/8] rebase: simplify code related to imply_merge()
2023-03-23 19:40 ` Phillip Wood
@ 2023-03-23 20:00 ` Junio C Hamano
2023-03-23 21:08 ` Felipe Contreras
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
0 siblings, 2 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-03-23 20:00 UTC (permalink / raw)
To: Phillip Wood; +Cc: Oswald Buddenhagen, git
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> The code's evolution left in some bits surrounding enum rebase_type that
>> don't really make sense any more. In particular, it makes no sense to
>> invoke imply_merge() if the type is already known not to be
>> REBASE_APPLY, and it makes no sense to assign the type after calling
>> imply_merge().
>
> These look sensible, did imply_merges() use to do something more which
> made these calls useful?
Good question.
>> @@ -1494,9 +1493,6 @@ int cmd_rebase(int argc, const char **argv,
>> const char *prefix)
>> }
>> }
>> - if (options.type == REBASE_MERGE)
>> - imply_merge(&options, "--merge");
This piece is reasonable, of course. We already know we are in
merge mode so there is nothing implied.
Before this hunk, there is a bit of code to react to
options.strategy given. The code complains if we are using the
apply backend, and sets the options.type to REBASE_MERGE, which is
suspiciously similar to what imply_merge() is doing. I wonder if
the code should be simplified to make a call to imply_merge() while
we are doing similar simplification like this patch does?
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
2023-03-23 19:31 ` Phillip Wood
@ 2023-03-23 20:16 ` Junio C Hamano
2023-03-23 23:23 ` Oswald Buddenhagen
1 sibling, 1 reply; 49+ messages in thread
From: Junio C Hamano @ 2023-03-23 20:16 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> Creating a suitable todo file is a potentially labor-intensive process,
> so be less cavalier about discarding it when something goes wrong (e.g.,
> the user messed with the repo while editing the todo).
Is there a reason why we do not always keep it? Why is the file
sometimes precious but not precious at all in other times?
Tying the previous bit to "-i was explicitly given" feels a bit
unintuitive---when the sequencer machinery was implicitly chosen,
and gives the control back to the user, should a user be forbidden
to muck with the todo list?
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index a309addd50..728c869db4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -153,6 +153,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
> replay.keep_redundant_commits = (opts->empty == EMPTY_KEEP);
> replay.quiet = !(opts->flags & REBASE_NO_QUIET);
> replay.verbose = opts->flags & REBASE_VERBOSE;
> + replay.precious_todo = opts->flags & REBASE_INTERACTIVE_EXPLICIT;
> replay.reschedule_failed_exec = opts->reschedule_failed_exec;
> replay.committer_date_is_author_date =
> opts->committer_date_is_author_date;
> diff --git a/sequencer.c b/sequencer.c
> index b1c29c8802..f8a7f4e721 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4570,6 +4570,10 @@ static int checkout_onto(struct repository *r, struct replay_opts *opts,
> .default_reflog_action = sequencer_reflog_action(opts)
> };
> if (reset_head(r, &ropts)) {
> + // Editing the todo may have been costly; don't just discard it.
> + if (opts->precious_todo)
> + exit(1); // Error was already printed
No // comments, please.
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ff0afad63e..c625aad10a 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -288,13 +288,14 @@ test_expect_success 'abort' '
> '
>
> test_expect_success 'abort with error when new base cannot be checked out' '
> + test_when_finished "git rebase --abort ||:" &&
> git rm --cached file1 &&
> git commit -m "remove file in base" &&
> test_must_fail git rebase -i primary > output 2>&1 &&
> test_i18ngrep "The following untracked working tree files would be overwritten by checkout:" \
> output &&
> test_i18ngrep "file1" output &&
> - test_path_is_missing .git/rebase-merge &&
> + test_path_is_dir .git/rebase-merge &&
> rm file1 &&
> git reset --hard HEAD^
> '
Are we happy to just see that the directory still exists? I thought
the original motivation explained in the proposed log message was to
keep the todo list file, so shouldn't you be checking if the file is
there (and if you can reliably ensure that the file has contents
that are expected, that would be even better)?
Also, as the keeping of the todo list is now conditional, we should
have another test that checks that the file is gone when that
condition ("INTERACTIVE_EXPLICIT"?) does not trigger, I think.
Other than that, nicely written. Thanks.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 1/8] rebase: simplify code related to imply_merge()
2023-03-23 20:00 ` Junio C Hamano
@ 2023-03-23 21:08 ` Felipe Contreras
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
1 sibling, 0 replies; 49+ messages in thread
From: Felipe Contreras @ 2023-03-23 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, Oswald Buddenhagen, git
On Thu, Mar 23, 2023 at 2:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> >> The code's evolution left in some bits surrounding enum rebase_type that
> >> don't really make sense any more. In particular, it makes no sense to
> >> invoke imply_merge() if the type is already known not to be
> >> REBASE_APPLY, and it makes no sense to assign the type after calling
> >> imply_merge().
> >
> > These look sensible, did imply_merges() use to do something more which
> > made these calls useful?
>
> Good question.
It used to be called imply_interactive(), so --merge did require an
interactive rebase.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 3/8] sequencer: pass around rebase action explicitly
2023-03-23 19:27 ` Phillip Wood
@ 2023-03-23 21:27 ` Oswald Buddenhagen
0 siblings, 0 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 21:27 UTC (permalink / raw)
To: git
On Thu, Mar 23, 2023 at 07:27:25PM +0000, Phillip Wood wrote:
>I appreciate the sentiment behind this patch, but it looks like an
>awful lot of churn just to clean up a couple of lines in
>append_todo_help() and edit_todo_list().
>
as much as i dislike churn, i dislike fragile "magic" code even more.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id
2023-03-23 19:34 ` Phillip Wood
@ 2023-03-23 21:36 ` Oswald Buddenhagen
2023-03-24 14:18 ` Phillip Wood
0 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 21:36 UTC (permalink / raw)
To: git
On Thu, Mar 23, 2023 at 07:34:57PM +0000, Phillip Wood wrote:
>On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> ... instead of as a commit, which makes the purpose clearer and will
>> simplify things later.
>
>given that we want onto to be a commit I'm not sure how this makes
>anything clearer.
>
it makes it clearer that we need only the oid, not any other part of the
commit object. and pulling ahead the "extraction" reduces the visual
noise further down.
>> As a side effect, this change revealed that skip_unnecessary_picks() was
>> butchering the commit object due to missing const-correctness. Slightly
>> adjust its API to rectify this.
>
>I don't think this is correct. If you look at the original code it makes
>a copy of the oid and uses the copy when calling skip_unnecessary_picks()
>
oops, you're quite right. (facepalm)
imo the change still makes sense, though, as it replaces the relatively
expensive deep copies with simple pointer updates. so just fix the
commit message?
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash()
2023-03-23 19:46 ` Phillip Wood
@ 2023-03-23 22:13 ` Oswald Buddenhagen
0 siblings, 0 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 22:13 UTC (permalink / raw)
To: git
On Thu, Mar 23, 2023 at 07:46:28PM +0000, Phillip Wood wrote:
>> + assert(nr == todo_list->nr);
>
>If this assert fails we may have already had some out of bounds memory
>accesses.
>
the loop could have run short, too.
but anyway, this isn't a runtime check, it's an assertion of a loop
invariant.
>> + todo_list->alloc = nr;
>> FREE_AND_NULL(todo_list->items);
>
>I think it would be cleaner to keep the original ordering and free the
>old list before assigning todo_list->alloc
>
my reasoning is that it's closer to the assert which also refers to it,
and it really makes sense to have _that_ first. also, the value is more
likely to be still in a register at that point.
>> todo_list->items = items;
>> - todo_list->nr = nr;
>> - todo_list->alloc = alloc;
>> }
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure
2023-03-23 19:31 ` Phillip Wood
@ 2023-03-23 22:38 ` Oswald Buddenhagen
2023-03-24 14:15 ` Phillip Wood
0 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 22:38 UTC (permalink / raw)
To: git
On Thu, Mar 23, 2023 at 07:31:04PM +0000, Phillip Wood wrote:
>On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> Creating a suitable todo file is a potentially labor-intensive process,
>> so be less cavalier about discarding it when something goes wrong (e.g.,
>> the user messed with the repo while editing the todo).
>
>I was thinking about this problem the other day in the context of
>rescheduling commands when they cannot be executed because they would
>overwrite an untracked file. My thought was that we should prepend a
>"reset" command to the todo list so that the checkout happened when the
>user continued the rebase.
>
so you basically want to convert the magic `onto` into an explicit todo
command? i'm not sure what the advantage would be, and i certainly can
think of disadvantages re. usability and backwards compat.
>How does this patch ensure the checkout happens when the user continues
>the rebase?
>
the idea was never that the user --continue's. we're talking about a
fatal error, and the patch's purpose is only to allow the user to
salvage their work manually.
it's an interesting question, though, esp. in light of patch 8/8 of this
series.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure
2023-03-23 20:16 ` Junio C Hamano
@ 2023-03-23 23:23 ` Oswald Buddenhagen
2023-03-24 4:31 ` Junio C Hamano
0 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-23 23:23 UTC (permalink / raw)
To: git
On Thu, Mar 23, 2023 at 01:16:47PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> Creating a suitable todo file is a potentially labor-intensive process,
>> so be less cavalier about discarding it when something goes wrong (e.g.,
>> the user messed with the repo while editing the todo).
>
>Is there a reason why we do not always keep it? Why is the file
>sometimes precious but not precious at all in other times?
>
the unedited initial todo just isn't precious. that implies that in a
non-interactive rebase, it is always worthless at the time of the
initial reset.
>Tying the previous bit to "-i was explicitly given" feels a bit
>unintuitive---when the sequencer machinery was implicitly chosen,
>and gives the control back to the user, should a user be forbidden
>to muck with the todo list?
>
that would be an --edit-todo and --continue during a mid-rebase stop.
rather different case.
>No // comments, please.
>
(apparently with a special exception for examples in the apidocs,
presumably because escaping nested comments would be just too ugly.)
>> - test_path_is_missing .git/rebase-merge &&
>> + test_path_is_dir .git/rebase-merge &&
>
>Are we happy to just see that the directory still exists? I thought
>the original motivation explained in the proposed log message was to
>keep the todo list file, so shouldn't you be checking if the file is
>there
>
fair point.
>(and if you can reliably ensure that the file has contents
>that are expected, that would be even better)?
>
i could grep for a shortened sha1 i would obtain from the branch. but
given that the error scenario of a present but somehow corrupted todo
seems implausible given the circumstances, that seems like overkill.
>Also, as the keeping of the todo list is now conditional, we should
>have another test that checks that the file is gone when that
>condition ("INTERACTIVE_EXPLICIT"?) does not trigger, I think.
>
that would be for t3400-rebase.sh.
i suppose we could extend 'Show verbose error when HEAD could not be
detached'.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure
2023-03-23 23:23 ` Oswald Buddenhagen
@ 2023-03-24 4:31 ` Junio C Hamano
0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-03-24 4:31 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Thu, Mar 23, 2023 at 01:16:47PM -0700, Junio C Hamano wrote:
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> Creating a suitable todo file is a potentially labor-intensive process,
>>> so be less cavalier about discarding it when something goes wrong (e.g.,
>>> the user messed with the repo while editing the todo).
>>
>>Is there a reason why we do not always keep it? Why is the file
>>sometimes precious but not precious at all in other times?
>>
> the unedited initial todo just isn't precious. that implies that in a
> non-interactive rebase, it is always worthless at the time of the
> initial reset.
I see. Thanks for clarifying.
Just FYI, the primary purpose reviewers ask questions on the
proposed change is to help submitters polish their patch (both the
proposed log message text and the code) to clarify points they found
hard to understand and/or they suspect would be hard to understand
for other readers. So please do not be happy by just receiving "I
see, thanks" and stop there. Instead, please update the patch so
that future readers would not have to ask similar question again.
>>(and if you can reliably ensure that the file has contents
>>that are expected, that would be even better)?
>>
> i could grep for a shortened sha1 i would obtain from the branch. but
> given that the error scenario of a present but somehow corrupted todo
> seems implausible given the circumstances, that seems like overkill.
It is OK. If it were easy to prepare the "todo should look like
this" golden copy, then doing test_cmp the actual file with it would
have been a simple way to ensure both existence of and sane contents
in the file at the same time, but if it isn't cheap to prepare such
an expected output, I agree with you that it is not worth the extra
effort.
Thanks.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure
2023-03-23 22:38 ` Oswald Buddenhagen
@ 2023-03-24 14:15 ` Phillip Wood
2023-03-24 14:42 ` Oswald Buddenhagen
0 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-03-24 14:15 UTC (permalink / raw)
To: git, Oswald Buddenhagen
Hi Oswald
On 23/03/2023 22:38, Oswald Buddenhagen wrote:
> On Thu, Mar 23, 2023 at 07:31:04PM +0000, Phillip Wood wrote:
>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>>> Creating a suitable todo file is a potentially labor-intensive process,
>>> so be less cavalier about discarding it when something goes wrong (e.g.,
>>> the user messed with the repo while editing the todo).
>>
>> I was thinking about this problem the other day in the context of
>> rescheduling commands when they cannot be executed because they would
>> overwrite an untracked file. My thought was that we should prepend a
>> "reset" command to the todo list so that the checkout happened when
>> the user continued the rebase.
>>
> so you basically want to convert the magic `onto` into an explicit todo
> command? i'm not sure what the advantage would be, and i certainly can
> think of disadvantages re. usability and backwards compat.
If the initial checkout of "onto" fails I want the rebase to stop so the
user can try and fix the problem (usually remove an untracked file) and
then run "git rebase --continue" to continue the rebase including the
initial checkout. Adding a "reset" command to the beginning of the todo
list when the initial checkout fails is one way of achieving that.
>> How does this patch ensure the checkout happens when the user
>> continues the rebase?
>>
> the idea was never that the user --continue's. we're talking about a
> fatal error,
If it is a fatal error what is stopping the user from running "rebase
--continue" and wreaking havoc? You seem to be expecting the user to
know that they need to
(1) run "git rebase --edit-todo" to save the todo list somewhere safe
(2) run "git rebase --abort" to abort the rebase and restore any
autostash. (Have you checked that --abort is safe to run when
HEAD is not detached?)
(3) fix whatever prevented the checkout from working
(4) re-run "git rebase" and restore the saved todo list when prompted
to edit it
It would be much more user friendly to simply allow them to fix the
problem with the checkout and run "git rebase --continue"
Best Wishes
Phillip
> and the patch's purpose is only to allow the user to
> salvage their work manually.
> it's an interesting question, though, esp. in light of patch 8/8 of this
> series.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id
2023-03-23 21:36 ` Oswald Buddenhagen
@ 2023-03-24 14:18 ` Phillip Wood
0 siblings, 0 replies; 49+ messages in thread
From: Phillip Wood @ 2023-03-24 14:18 UTC (permalink / raw)
To: git, Oswald Buddenhagen
Hi Oswald
On 23/03/2023 21:36, Oswald Buddenhagen wrote:
> On Thu, Mar 23, 2023 at 07:34:57PM +0000, Phillip Wood wrote:
>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>>> ... instead of as a commit, which makes the purpose clearer and will
>>> simplify things later.
>>
>> given that we want onto to be a commit I'm not sure how this makes
>> anything clearer.
>>
> it makes it clearer that we need only the oid, not any other part of the
> commit object. and pulling ahead the "extraction" reduces the visual
> noise further down.
>
>>> As a side effect, this change revealed that skip_unnecessary_picks() was
>>> butchering the commit object due to missing const-correctness. Slightly
>>> adjust its API to rectify this.
>>
>> I don't think this is correct. If you look at the original code it
>> makes a copy of the oid and uses the copy when calling
>> skip_unnecessary_picks()
>>
> oops, you're quite right. (facepalm)
> imo the change still makes sense, though, as it replaces the relatively
> expensive deep copies with simple pointer updates. so just fix the
> commit message?
I think you should just drop this patch. Copying struct object_id is
cheap and idiomatic in the code base. If you grep for
"struct object_id \*\*[a-zA-Z0-9_]*[,)]"
you'll see that there are very few matches and all but one of those are
passing an array.
Best Wishes
Phillip
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 5/8] rebase: preserve interactive todo file on checkout failure
2023-03-24 14:15 ` Phillip Wood
@ 2023-03-24 14:42 ` Oswald Buddenhagen
0 siblings, 0 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-03-24 14:42 UTC (permalink / raw)
To: git
On Fri, Mar 24, 2023 at 02:15:47PM +0000, Phillip Wood wrote:
>On 23/03/2023 22:38, Oswald Buddenhagen wrote:
>> so you basically want to convert the magic `onto` into an explicit
>> todo command? i'm not sure what the advantage would be, and i
>> certainly can think of disadvantages re. usability and backwards
>> compat.
>
>If the initial checkout of "onto" fails I want the rebase to stop so the
>user can try and fix the problem (usually remove an untracked file) and
>then run "git rebase --continue" to continue the rebase including the
>initial checkout. Adding a "reset" command to the beginning of the todo
>list when the initial checkout fails is one way of achieving that.
>
i suppose, but patch 8/8 does pretty much the same, only with fewer side
effects.
>>> How does this patch ensure the checkout happens when the user
>>> continues the rebase?
>>>
>> the idea was never that the user --continue's. we're talking about a
>> fatal error,
>
>If it is a fatal error what is stopping the user from running "rebase
>--continue" and wreaking havoc? [...]
>
>It would be much more user friendly to simply allow them to fix the
>problem with the checkout and run "git rebase --continue"
>
i'll reorder the patch to the end of the series, as after the currently
last patch we'll be in a much better position to deal with the fallout
in a sane way.
thanks!
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 0/8] sequencer refactoring
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
` (8 preceding siblings ...)
2023-03-23 19:38 ` [PATCH 0/8] sequencer refactoring Phillip Wood
@ 2023-03-25 11:08 ` Phillip Wood
2023-04-06 12:09 ` Phillip Wood
2023-05-17 13:10 ` Phillip Wood
10 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-03-25 11:08 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> This is a preparatory series for the separately posted 'rebase --rewind' patch,
> but I think it has value in itself.
I had a hard time applying these patches. In the end I had success with
checking out next, applying
https://lore.kernel.org/git/20230323162234.995514-1-oswald.buddenhagen@gmx.de
and then applying this series. It is very helpful to detail the base
commit in the cover letter. A series like this should normally be based
on master see Documentation/SubmittingPatches.
Having applied the patches I'm unable to compile them with DEVELOPER=1
(see Documentation/CodingGuidelines)
In file included from log-tree.c:20:
sequencer.h:7:6: error: ISO C forbids forward references to ‘enum’ types
[-Werror=pedantic]
7 | enum rebase_action;
| ^~~~~~~~~~~~~
sequencer.h:140:34: error: ISO C forbids forward references to ‘enum’
types [-Werror=pedantic]
140 | enum rebase_action action);
| ^~~~~~~~~~~~~
sequencer.h:196:26: error: ISO C forbids forward references to ‘enum’
types [-Werror=pedantic]
196 | enum rebase_action action);
| ^~~~~~~~~~~~~
In file included from ./cache.h:12,
from ./builtin.h:6,
from builtin/rebase.c:8:
builtin/rebase.c: In function ‘cmd_rebase’:
builtin/rebase.c:1246:95: error: left-hand operand of comma expression
has no effect [-Werror=unused-value]
1246 |
(BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),
|
^
./trace2.h:158:69: note: in definition of macro ‘trace2_cmd_mode’
158 | #define trace2_cmd_mode(sv) trace2_cmd_mode_fl(__FILE__,
__LINE__, (sv))
|
^~
sequencer.c: In function ‘todo_list_rearrange_squash’:
sequencer.c:6346:23: error: operation on ‘items’ may be undefined
[-Werror=sequence-point]
6346 | items = ALLOC_ARRAY(items, todo_list->nr);
Best Wishes
Phillip
>
> Oswald Buddenhagen (8):
> rebase: simplify code related to imply_merge()
> rebase: move parse_opt_keep_empty() down
> sequencer: pass around rebase action explicitly
> sequencer: create enum for edit_todo_list() return value
> rebase: preserve interactive todo file on checkout failure
> sequencer: simplify allocation of result array in
> todo_list_rearrange_squash()
> sequencer: pass `onto` to complete_action() as object-id
> rebase: improve resumption from incorrect initial todo list
>
> builtin/rebase.c | 63 +++++++--------
> builtin/revert.c | 3 +-
> rebase-interactive.c | 36 ++++-----
> rebase-interactive.h | 27 ++++++-
> sequencer.c | 139 +++++++++++++++++++---------------
> sequencer.h | 15 ++--
> t/t3404-rebase-interactive.sh | 34 ++++++++-
> 7 files changed, 196 insertions(+), 121 deletions(-)
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
2023-03-23 16:22 ` [PATCH 8/8] rebase: improve resumption from incorrect initial todo list Oswald Buddenhagen
@ 2023-03-26 14:28 ` Phillip Wood
2023-04-26 15:34 ` Oswald Buddenhagen
0 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-03-26 14:28 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> When the user butchers the todo file during rebase -i setup, the
> --continue which would follow --edit-todo would have skipped the last
> steps of the setup. Notably, this would bypass the fast-forward over
> untouched picks (though the actual picking loop would still fast-forward
> the commits, one by one).
>
> Fix this by splitting off the tail of complete_action() to a new
> start_rebase() function and call that from sequencer_continue() when no
> commands have been executed yet.
>
> More or less as a side effect, we no longer checkout `onto` before exiting
> when the todo file is bad.
I think the implications of this change deserve to be discussed in the
commit message. Three things spring to mind but there may be others I
haven't thought of
- Previously when rebase stopped and handed control back to the user
HEAD would have already been detached. This patch changes that
meaning we can have an active rebase of a branch while that branch is
checked out. What does "git status" show in this case? What does the
shell prompt show? Will it confuse users?
- Previously if the user created a commit before running "rebase
--continue" we'd rebase on to that commit. Now that commit will be
silently dropped.
- Previously if the user checkout out another commit before running
"rebase --continue" we'd rebase on to that commit. Now we we rebase
on to the original "onto" commit.
> This makes aborting cheaper and will simplify
> things in a later change.
Given that we're stopping so the user can fix the problem and continue
the rebase I don't think optimizing for aborting is a convincing reason
for this change on its own.
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 62986a7b1b..00d3e19c62 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
> return ret;
> }
> if (cmd == 'c')
> - return sequencer_continue(the_repository, opts);
> + return sequencer_continue(the_repository, opts,
> + 0, NULL, NULL, NULL);
It's a bit unfortunate that we have to start passing all these extra
parameters, could the sequencer read them itself in read_populate_opts()?
> -int sequencer_continue(struct repository *r, struct replay_opts *opts)
> +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,
> + const char *onto_name, const struct object_id *onto,
> + const struct object_id *orig_head, struct todo_list *todo_list);
It would be nice to avoid this forward declaration. I think you could do
that by adding a preparatory patch that moves either checkout_onto() or
sequencer_continue()
> @@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>
> return error(_("nothing to do"));
> } else if (res == EDIT_TODO_INCORRECT) {
> - checkout_onto(r, opts, onto_name, onto, orig_head);
> todo_list_release(&new_todo);
>
> return -1;
> }
>
> - /* Expand the commit IDs */
> - todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
> - strbuf_swap(&new_todo.buf, &buf2);
> - strbuf_release(&buf2);
> - new_todo.total_nr -= new_todo.nr;
> - if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
> - BUG("invalid todo list after expanding IDs:\n%s",
> - new_todo.buf.buf);
I don't think we need to move this code. If start_rebase() is called
from sequencer_continue() the initial edit of the todo list failed and
has been fixed by running "git rebase --edit-todo". In that case the
oids have already been expanded on disc.
> - if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
> - todo_list_release(&new_todo);
> - return error(_("could not skip unnecessary pick commands"));
> - }
> -
> - if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
> - flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
> - todo_list_release(&new_todo);
> - return error_errno(_("could not write '%s'"), todo_file);
> - }
> -
> - res = -1;
> -
> - if (checkout_onto(r, opts, onto_name, onto, orig_head))
> - goto cleanup;
> -
> - if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
> - goto cleanup;
> -
> - todo_list_write_total_nr(&new_todo);
> - res = pick_commits(r, &new_todo, opts);
> -
> -cleanup:
> + res = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo);
> todo_list_release(&new_todo);
>
> return res;
> }
>
> +test_expect_success 'continue after bad first command' '
> + test_when_finished "git rebase --abort ||:" &&
> + git checkout primary^0 &&
If you want a specific commit it's better to use a tag name as those are
fixed whereas the branches get rebased all over the place in this test file.
> + git reflog expire --expire=all HEAD &&
Is this really necessary, can you pass -n to "git reflog" below?
> + (
> + set_fake_editor &&
> + test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
> + git rebase -i HEAD~3 &&
> + test_cmp_rev HEAD primary &&
> + FAKE_LINES="pick 2 pick 3 reword 4" git rebase --edit-todo &&
> + FAKE_COMMIT_MESSAGE="E_reworded" git rebase --continue
> + ) &&
> + git reflog > reflog &&
> + test $(grep -c fast-forward reflog) = 1 &&
Using test_line_count would make test failures easier to debug.
> + test_cmp_rev HEAD~1 primary~1 &&
> + test "$(git log -1 --format=%B)" = "E_reworded"
It is slightly more work, but please use test_cmp for things like this
as it makes it so much easier to debug test failures.
Best Wishes
Phillip
> +'
> +
> +test_expect_success 'abort after bad first command' '
> + test_when_finished "git rebase --abort ||:" &&
> + git checkout primary^0 &&
> + (
> + set_fake_editor &&
> + test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
> + git rebase -i HEAD~3
> + ) &&
> + git rebase --abort &&
> + test_cmp_rev HEAD primary
> +'
> +
> test_expect_success 'tabs and spaces are accepted in the todolist' '
> rebase_setup_and_clean indented-comment &&
> write_script add-indent.sh <<-\EOF &&
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 0/8] sequencer refactoring
2023-03-25 11:08 ` Phillip Wood
@ 2023-04-06 12:09 ` Phillip Wood
0 siblings, 0 replies; 49+ messages in thread
From: Phillip Wood @ 2023-04-06 12:09 UTC (permalink / raw)
To: Oswald Buddenhagen, git
On 25/03/2023 11:08, Phillip Wood wrote:
> Hi Oswald
>
> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> This is a preparatory series for the separately posted 'rebase
>> --rewind' patch,
>> but I think it has value in itself.
>
> I had a hard time applying these patches. In the end I had success with
> checking out next, applying
> https://lore.kernel.org/git/20230323162234.995514-1-oswald.buddenhagen@gmx.de and then applying this series. It is very helpful to detail the base commit in the cover letter. A series like this should normally be based on master see Documentation/SubmittingPatches.
>
> Having applied the patches I'm unable to compile them with DEVELOPER=1
> (see Documentation/CodingGuidelines)
>
> In file included from log-tree.c:20:
> sequencer.h:7:6: error: ISO C forbids forward references to ‘enum’ types
> [-Werror=pedantic]
> 7 | enum rebase_action;
> | ^~~~~~~~~~~~~
> sequencer.h:140:34: error: ISO C forbids forward references to ‘enum’
> types [-Werror=pedantic]
> 140 | enum rebase_action action);
> | ^~~~~~~~~~~~~
> sequencer.h:196:26: error: ISO C forbids forward references to ‘enum’
> types [-Werror=pedantic]
> 196 | enum rebase_action action);
> | ^~~~~~~~~~~~~
>
> In file included from ./cache.h:12,
> from ./builtin.h:6,
> from builtin/rebase.c:8:
> builtin/rebase.c: In function ‘cmd_rebase’:
> builtin/rebase.c:1246:95: error: left-hand operand of comma expression
> has no effect [-Werror=unused-value]
> 1246 | (BUILD_ASSERT_OR_ZERO(ARRAY_SIZE(action_names) == ACTION_LAST),
> | ^
> ./trace2.h:158:69: note: in definition of macro ‘trace2_cmd_mode’
> 158 | #define trace2_cmd_mode(sv) trace2_cmd_mode_fl(__FILE__,
> __LINE__, (sv))
> | ^~
I think the errors above are best addressed by dropping patch 3 as I
don't think the benefit is worth the churn. You say that the existing
code is fragile but it is not that hard to follow and is battle tested
and known to work. If you need to change things to support --rewind then
it would be better to do so in a series that adds that option.
> sequencer.c: In function ‘todo_list_rearrange_squash’:
> sequencer.c:6346:23: error: operation on ‘items’ may be undefined
> [-Werror=sequence-point]
> 6346 | items = ALLOC_ARRAY(items, todo_list->nr);
This is easily fixed by deleting "items =" as ALLOC_ARRAY() does the
assignment for us.
After dropping patches 3 and 7 and fixing the ARROC_ARRAY() above all
the rebase tests pass for each commit and the CI passes -
https://github.com/phillipwood/git/actions/runs/4627831184
Best Wishes
Phillip
>
> Best Wishes
>
> Phillip
>
>>
>> Oswald Buddenhagen (8):
>> rebase: simplify code related to imply_merge()
>> rebase: move parse_opt_keep_empty() down
>> sequencer: pass around rebase action explicitly
>> sequencer: create enum for edit_todo_list() return value
>> rebase: preserve interactive todo file on checkout failure
>> sequencer: simplify allocation of result array in
>> todo_list_rearrange_squash()
>> sequencer: pass `onto` to complete_action() as object-id
>> rebase: improve resumption from incorrect initial todo list
>>
>> builtin/rebase.c | 63 +++++++--------
>> builtin/revert.c | 3 +-
>> rebase-interactive.c | 36 ++++-----
>> rebase-interactive.h | 27 ++++++-
>> sequencer.c | 139 +++++++++++++++++++---------------
>> sequencer.h | 15 ++--
>> t/t3404-rebase-interactive.sh | 34 ++++++++-
>> 7 files changed, 196 insertions(+), 121 deletions(-)
>>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
2023-03-26 14:28 ` Phillip Wood
@ 2023-04-26 15:34 ` Oswald Buddenhagen
2023-05-17 12:13 ` Phillip Wood
0 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-04-26 15:34 UTC (permalink / raw)
To: phillip.wood; +Cc: git
On Sun, Mar 26, 2023 at 03:28:01PM +0100, Phillip Wood wrote:
>On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>> When the user butchers the todo file during rebase -i setup, the
>> --continue which would follow --edit-todo would have skipped the last
>> steps of the setup. Notably, this would bypass the fast-forward over
>> untouched picks (though the actual picking loop would still fast-forward
>> the commits, one by one).
>>
>> Fix this by splitting off the tail of complete_action() to a new
>> start_rebase() function and call that from sequencer_continue() when no
>> commands have been executed yet.
>>
>> More or less as a side effect, we no longer checkout `onto` before exiting
>> when the todo file is bad.
>
>I think the implications of this change deserve to be discussed in the
>commit message. Three things spring to mind but there may be others I
>haven't thought of
>
> - Previously when rebase stopped and handed control back to the user
> HEAD would have already been detached. This patch changes that
> meaning we can have an active rebase of a branch while that branch is
> checked out. What does "git status" show in this case? What does the
> shell prompt show? Will it confuse users?
>
the failed state is identical to the "still editing the initial todo"
state as far as "git status" and the shell prompt are concerned. this
seems reasonable. i'll add it to the commit message.
> - Previously if the user created a commit before running "rebase
> --continue" we'd rebase on to that commit. Now that commit will be
> silently dropped.
>
this is arguably a problem, but not much different from the pre-existing
behavior of changes to HEAD done during the initial todo edit being
lost.
to avoid that, we'd need to lock HEAD while editing the todo. is that
realistic at all?
on top of that, i should verify HEAD against orig-head in
start_rebase(). though the only way for the user to get out of that
situation is saving the todo contents and --abort'ing (and we must take
care not the touch HEAD).
this is somewhat similar to the abysmal situation of the final
update-ref failing if the target ref has been modified while being
rebased. we'd need to lock that ref for the entire duration of the
rebase to avoid that.
> - Previously if the user checkout out another commit before running
> "rebase --continue" we'd rebase on to that commit. Now we we rebase
> on to the original "onto" commit.
>
this can be subsumed into the above case.
> > This makes aborting cheaper and will simplify
> > things in a later change.
>
>Given that we're stopping so the user can fix the problem and continue
>the rebase I don't think optimizing for aborting is a convincing reason
>for this change on its own.
>
this is all part of the "More or less as a side effect" paragraph, so
this isn't a relevant objection.
>> diff --git a/builtin/revert.c b/builtin/revert.c
>> index 62986a7b1b..00d3e19c62 100644
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
>> return ret;
>> }
>> if (cmd == 'c')
>> - return sequencer_continue(the_repository, opts);
>> + return sequencer_continue(the_repository, opts,
>> + 0, NULL, NULL, NULL);
>
>It's a bit unfortunate that we have to start passing all these extra
>parameters, could the sequencer read them itself in read_populate_opts()?
>
that wouldn't help in this case, as these are dummy values which aren't
going to be used.
but more broadly, the whole state management is a total mess. i have
this notes-to-self patch on top of my local branch:
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -476,6 +476,7 @@ static const char *state_dir_path(const char *filename, struct rebase_options *o
}
/* Initialize the rebase options from the state directory. */
+// FIXME: this is partly redundant with the sequencer's read_populate_opts().
static int read_basic_state(struct rebase_options *opts)
{
struct strbuf head_name = STRBUF_INIT;
@@ -552,6 +553,7 @@ static int read_basic_state(struct rebase_options *opts)
return 0;
}
+// This is written only by the apply backend
static int rebase_write_basic_state(struct rebase_options *opts)
{
write_file(state_dir_path("head-name", opts), "%s",
>> -int sequencer_continue(struct repository *r, struct replay_opts *opts)
>> +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,
>> + const char *onto_name, const struct object_id *onto,
>> + const struct object_id *orig_head, struct todo_list *todo_list);
>
>It would be nice to avoid this forward declaration. I think you could do
>that by adding a preparatory patch that moves either checkout_onto() or
>sequencer_continue()
>
i went for the "minimal churn" approach.
but more broadly, the code distribution between rebase.c and sequencer.c
needs a *major* re-think. moving these functions into place could be
part of that effort.
>> + git reflog expire --expire=all HEAD &&
>
>Is this really necessary, can you pass -n to "git reflog" below?
>
starting from a clean slate makes it more straight-forward to make it
reliable. i don't see any real downsides to the approach.
>> + git reflog > reflog &&
>> + test $(grep -c fast-forward reflog) = 1 &&
>
>Using test_line_count would make test failures easier to debug.
>
that's calling for a new test_filtered_line_count function which would
have quite some users.
for the time being, both grep + test_line_count and grep -c are rather
prevalent, in this file the latter in particular.
>> + test_cmp_rev HEAD~1 primary~1 &&
>> + test "$(git log -1 --format=%B)" = "E_reworded"
>
>It is slightly more work, but please use test_cmp for things like this
>as it makes it so much easier to debug test failures.
>
fair enough, but the precedents again speak a different language.
regards
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
2023-04-26 15:34 ` Oswald Buddenhagen
@ 2023-05-17 12:13 ` Phillip Wood
2023-08-24 16:46 ` Oswald Buddenhagen
0 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-05-17 12:13 UTC (permalink / raw)
To: phillip.wood, git
Hi Oswald
On 26/04/2023 16:34, Oswald Buddenhagen wrote:
> On Sun, Mar 26, 2023 at 03:28:01PM +0100, Phillip Wood wrote:
>> On 23/03/2023 16:22, Oswald Buddenhagen wrote:
>>> When the user butchers the todo file during rebase -i setup, the
>>> --continue which would follow --edit-todo would have skipped the last
>>> steps of the setup. Notably, this would bypass the fast-forward over
>>> untouched picks (though the actual picking loop would still fast-forward
>>> the commits, one by one).
>>>
>>> Fix this by splitting off the tail of complete_action() to a new
>>> start_rebase() function and call that from sequencer_continue() when no
>>> commands have been executed yet.
>>>
>>> More or less as a side effect, we no longer checkout `onto` before
>>> exiting
>>> when the todo file is bad.
>>
>> I think the implications of this change deserve to be discussed in the
>> commit message. Three things spring to mind but there may be others I
>> haven't thought of
>>
>> - Previously when rebase stopped and handed control back to the user
>> HEAD would have already been detached. This patch changes that
>> meaning we can have an active rebase of a branch while that branch is
>> checked out. What does "git status" show in this case? What does the
>> shell prompt show? Will it confuse users?
>>
> the failed state is identical to the "still editing the initial todo"
> state as far as "git status" and the shell prompt are concerned. this
> seems reasonable. i'll add it to the commit message.
When you do that please mention what "git status" and the shell prompt
actually print in this case. Ideally "git status" should mention that
the todo list needs to be edited if there are still errors in it, though
it would not surprise me if it is not that helpful at the moment.
>> - Previously if the user created a commit before running "rebase
>> --continue" we'd rebase on to that commit. Now that commit will be
>> silently dropped.
>>
> this is arguably a problem, but not much different from the pre-existing
> behavior of changes to HEAD done during the initial todo edit being lost.
I think there is a significant difference in that we're moving from a
situation where we lose commits that are created while rebase is running
to one where we're losing commits created while rebase is stopped. If a
user tries to create a commit while rebase is running then they're
asking for trouble. I don't think creating commits when rebase is
stopped is unreasonable in the same way.
> to avoid that, we'd need to lock HEAD while editing the todo. is that
> realistic at all?
I don't think it is practical to lock HEAD while git is not running. We
could just check HEAD has not changed when the rebase continues after
the user has fixed the todo list as you suggest below.
> on top of that, i should verify HEAD against orig-head in
> start_rebase(). though the only way for the user to get out of that
> situation is saving the todo contents and --abort'ing (and we must take
> care not the touch HEAD).
I think in that case it wouldn't be terrible to lose the edited todo
list as it is a bit of a corner case. The simplest thing to do would be
to print an error and remove .git/rebase-merge.
> this is somewhat similar to the abysmal situation of the final
> update-ref failing if the target ref has been modified while being
> rebased. we'd need to lock that ref for the entire duration of the
> rebase to avoid that.
"abysmal" is rather harsh - it would also be bad to overwrite the ref in
that case. I think it in relatively hard to get into that situation
though as "git checkout" wont checkout a branch that is being updated by
a rebase.
>> - Previously if the user checkout out another commit before running
>> "rebase --continue" we'd rebase on to that commit. Now we we rebase
>> on to the original "onto" commit.
>>
> this can be subsumed into the above case.
Meaning check and error out if HEAD has changed?
>> > This makes aborting cheaper and will simplify
>> > things in a later change.
>>
>> Given that we're stopping so the user can fix the problem and continue
>> the rebase I don't think optimizing for aborting is a convincing
>> reason for this change on its own.
>>
> this is all part of the "More or less as a side effect" paragraph, so
> this isn't a relevant objection.
I'm simply saying that we should not be optimizing for "rebase --abort"
in this case. Do you think we should?
>>> diff --git a/builtin/revert.c b/builtin/revert.c
>>> index 62986a7b1b..00d3e19c62 100644
>>> --- a/builtin/revert.c
>>> +++ b/builtin/revert.c
>>> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char
>>> **argv, struct replay_opts *opts)
>>> return ret;
>>> }
>>> if (cmd == 'c')
>>> - return sequencer_continue(the_repository, opts);
>>> + return sequencer_continue(the_repository, opts,
>>> + 0, NULL, NULL, NULL);
>>
>> It's a bit unfortunate that we have to start passing all these extra
>> parameters, could the sequencer read them itself in read_populate_opts()?
>>
> that wouldn't help in this case, as these are dummy values which aren't
> going to be used.
If we only need to pass these when rebasing maybe we should have
separate wrappers for continuing a rebase and a cherry-pick/revert. If
we don't always need these parameters when continuing a rebase we could
have a separate function when these parameters are required and leave
the signature of sequencer_continue() unchanged.
> but more broadly, the whole state management is a total mess.
For historic reasons there are separate functions to write the state for
the "merge" backed and the "apply" backend. That is not ideal but it is
hardly a "total mess". The code for reading the state files is more
contrived than the code that writes them. I do have some patches to try
and reduce the duplication when reading the state files.
>>> -int sequencer_continue(struct repository *r, struct replay_opts *opts)
>>> +static int start_rebase(struct repository *r, struct replay_opts
>>> *opts, unsigned flags,
>>> + const char *onto_name, const struct object_id *onto,
>>> + const struct object_id *orig_head, struct todo_list
>>> *todo_list);
>>
>> It would be nice to avoid this forward declaration. I think you could
>> do that by adding a preparatory patch that moves either
>> checkout_onto() or sequencer_continue()
>>
> i went for the "minimal churn" approach.
There is a balance to be had, but we don't want to build up a lot of
forward declarations over time just because it is easier than moving the
function in a preparatory patch. A simple patch to move a function is
easy to review with --color-moved.
>>> + git reflog > reflog &&
>>> + test $(grep -c fast-forward reflog) = 1 &&
>>
>> Using test_line_count would make test failures easier to debug.
>>
> that's calling for a new test_filtered_line_count function which would
> have quite some users.
> for the time being, both grep + test_line_count and grep -c are rather
> prevalent, in this file the latter in particular.
The style of our tests has evolved over time. When adding new tests it
is better to focus on making them easy to debug. I don't think you need
to add a new function here, just
grep fast-forward reflog >filtered-reflog
test_line_count = 1 filtered-reflog
>>> + test_cmp_rev HEAD~1 primary~1 &&
>>> + test "$(git log -1 --format=%B)" = "E_reworded"
>>
>> It is slightly more work, but please use test_cmp for things like this
>> as it makes it so much easier to debug test failures.
>>
> fair enough, but the precedents again speak a different language.
Yes older tests tend to be written in a style that is harder to debug.
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 0/8] sequencer refactoring
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
` (9 preceding siblings ...)
2023-03-25 11:08 ` Phillip Wood
@ 2023-05-17 13:10 ` Phillip Wood
10 siblings, 0 replies; 49+ messages in thread
From: Phillip Wood @ 2023-05-17 13:10 UTC (permalink / raw)
To: Oswald Buddenhagen, git
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> This is a preparatory series for the separately posted 'rebase --rewind' patch,
> but I think it has value in itself.
When you re-roll this I think it would be worth splitting it into two
separate series.
Patches 1, 2, 4 & 6 are simple clean ups which don't need much work
beyond making sure that (a) the commit messages have a good explanation
of the reason for the change (try "git log --author "Jeff King" for
examples of good commit messages) and (b) the code follows our coding
guidelines (mostly no '//' comments if I remember correctly).
Patches 5 & 8 address real problems but are more involved and it will
take more time to consider the UI changes and get them right.
I'd rather we dropped patches 3 & 7.
Best Wishes
Phillip
>
> Oswald Buddenhagen (8):
> rebase: simplify code related to imply_merge()
> rebase: move parse_opt_keep_empty() down
> sequencer: pass around rebase action explicitly
> sequencer: create enum for edit_todo_list() return value
> rebase: preserve interactive todo file on checkout failure
> sequencer: simplify allocation of result array in
> todo_list_rearrange_squash()
> sequencer: pass `onto` to complete_action() as object-id
> rebase: improve resumption from incorrect initial todo list
>
> builtin/rebase.c | 63 +++++++--------
> builtin/revert.c | 3 +-
> rebase-interactive.c | 36 ++++-----
> rebase-interactive.h | 27 ++++++-
> sequencer.c | 139 +++++++++++++++++++---------------
> sequencer.h | 15 ++--
> t/t3404-rebase-interactive.sh | 34 ++++++++-
> 7 files changed, 196 insertions(+), 121 deletions(-)
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 0/3] rebase refactoring
2023-03-23 20:00 ` Junio C Hamano
2023-03-23 21:08 ` Felipe Contreras
@ 2023-08-09 17:15 ` Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
` (3 more replies)
1 sibling, 4 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-08-09 17:15 UTC (permalink / raw)
To: git
broken out of the bigger series, as the aggregation just unnecessarily holds it
up.
Oswald Buddenhagen (3):
rebase: simplify code related to imply_merge()
rebase: handle --strategy via imply_merge() as well
rebase: move parse_opt_keep_empty() down
builtin/rebase.c | 44 ++++++++++++++------------------------------
1 file changed, 14 insertions(+), 30 deletions(-)
--
2.40.0.152.g15d061e6df
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v2 1/3] rebase: simplify code related to imply_merge()
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
@ 2023-08-09 17:15 ` Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
` (2 subsequent siblings)
3 siblings, 0 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-08-09 17:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood
The code's evolution left in some bits surrounding enum rebase_type that
don't really make sense any more. In particular, it makes no sense to
invoke imply_merge() if the type is already known not to be
REBASE_APPLY, and it makes no sense to assign the type after calling
imply_merge().
enum rebase_type had more values until commit a74b35081c ("rebase: drop
support for `--preserve-merges`") and commit 10cdb9f38a ("rebase: rename
the two primary rebase backends"). The latter commit also renamed
imply_interactive() to imply_merge().
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2:
- more verbose commit message
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
builtin/rebase.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..44cc1eed12 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -386,7 +386,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
opts->keep_empty = !unset;
- opts->type = REBASE_MERGE;
return 0;
}
@@ -1505,9 +1504,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
}
- if (options.type == REBASE_MERGE)
- imply_merge(&options, "--merge");
-
if (options.root && !options.onto_name)
imply_merge(&options, "--root without --onto");
@@ -1552,7 +1548,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
- imply_merge(&options, "--merge");
+ options.type = REBASE_MERGE;
else if (!strcmp(options.default_backend, "apply"))
options.type = REBASE_APPLY;
else
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
@ 2023-08-09 17:15 ` Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
3 siblings, 0 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-08-09 17:15 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood
At least after the successive trimming of enum rebase_type mentioned in
the previous commit, this code did exactly what imply_merge() does, so
just call it instead.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
builtin/rebase.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 44cc1eed12..4a093bb125 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.strategy) {
options.strategy = xstrdup(options.strategy);
- switch (options.type) {
- case REBASE_APPLY:
- die(_("--strategy requires --merge or --interactive"));
- case REBASE_MERGE:
- /* compatible */
- break;
- case REBASE_UNSPECIFIED:
- options.type = REBASE_MERGE;
- break;
- default:
- BUG("unhandled rebase type (%d)", options.type);
- }
+ imply_merge(&options, "--strategy");
}
if (options.root && !options.onto_name)
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
@ 2023-08-09 17:15 ` Oswald Buddenhagen
2023-08-15 14:01 ` Phillip Wood
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
3 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-08-09 17:15 UTC (permalink / raw)
To: git; +Cc: Phillip Wood, Junio C Hamano
This moves it right next to parse_opt_empty(), which is a much more
logical place. As a side effect, this removes the need for a forward
declaration of imply_merge().
Acked-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
i'm not sure how to "translate" phillip's informal approval; the
acked-by doesn't seem quite right. please adjust as necessary.
Cc: Junio C Hamano <gitster@pobox.com>
---
builtin/rebase.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a093bb125..13ca5a644b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -376,19 +376,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)
return ret;
}
-static void imply_merge(struct rebase_options *opts, const char *option);
-static int parse_opt_keep_empty(const struct option *opt, const char *arg,
- int unset)
-{
- struct rebase_options *opts = opt->value;
-
- BUG_ON_OPT_ARG(arg);
-
- imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
- opts->keep_empty = !unset;
- return 0;
-}
-
static int is_merge(struct rebase_options *opts)
{
return opts->type == REBASE_MERGE;
@@ -982,6 +969,18 @@ static enum empty_type parse_empty_value(const char *value)
die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
}
+static int parse_opt_keep_empty(const struct option *opt, const char *arg,
+ int unset)
+{
+ struct rebase_options *opts = opt->value;
+
+ BUG_ON_OPT_ARG(arg);
+
+ imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
+ opts->keep_empty = !unset;
+ return 0;
+}
+
static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
{
struct rebase_options *options = opt->value;
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down
2023-08-09 17:15 ` [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
@ 2023-08-15 14:01 ` Phillip Wood
0 siblings, 0 replies; 49+ messages in thread
From: Phillip Wood @ 2023-08-15 14:01 UTC (permalink / raw)
To: Oswald Buddenhagen, git; +Cc: Junio C Hamano
Hi Oswald
On 09/08/2023 18:15, Oswald Buddenhagen wrote:
> This moves it right next to parse_opt_empty(), which is a much more
> logical place. As a side effect, this removes the need for a forward
> declaration of imply_merge().
>
> Acked-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
>
> ---
> i'm not sure how to "translate" phillip's informal approval; the
> acked-by doesn't seem quite right. please adjust as necessary.
I think we should just delete that trailer, I don't approve of this
change any more strongly than I do the rest of the series - they all
look like useful improvements to me, thanks for working on them.
Best Wishes
Phillip
> Cc: Junio C Hamano <gitster@pobox.com>
> ---
> builtin/rebase.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 4a093bb125..13ca5a644b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -376,19 +376,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)
> return ret;
> }
>
> -static void imply_merge(struct rebase_options *opts, const char *option);
> -static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> - int unset)
> -{
> - struct rebase_options *opts = opt->value;
> -
> - BUG_ON_OPT_ARG(arg);
> -
> - imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> - opts->keep_empty = !unset;
> - return 0;
> -}
> -
> static int is_merge(struct rebase_options *opts)
> {
> return opts->type == REBASE_MERGE;
> @@ -982,6 +969,18 @@ static enum empty_type parse_empty_value(const char *value)
> die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
> }
>
> +static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> + int unset)
> +{
> + struct rebase_options *opts = opt->value;
> +
> + BUG_ON_OPT_ARG(arg);
> +
> + imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
> + opts->keep_empty = !unset;
> + return 0;
> +}
> +
> static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
> {
> struct rebase_options *options = opt->value;
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
2023-05-17 12:13 ` Phillip Wood
@ 2023-08-24 16:46 ` Oswald Buddenhagen
0 siblings, 0 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-08-24 16:46 UTC (permalink / raw)
To: phillip.wood; +Cc: git
On Wed, May 17, 2023 at 01:13:28PM +0100, Phillip Wood wrote:
>On 26/04/2023 16:34, Oswald Buddenhagen wrote:
>> the failed state is identical to the "still editing the initial todo"
>> state as far as "git status" and the shell prompt are concerned. this
>> seems reasonable. i'll add it to the commit message.
>
>When you do that please mention what "git status" and the shell prompt
>actually print in this case.
>
i'll go with "This seems reasonable, irrespective of the actual
presentation (which could be improved)".
>Ideally "git status" should mention that the todo list needs to be
>edited if there are still errors in it, though it would not surprise me
>if it is not that helpful at the moment.
>
that would require actually validating the todo instead of just printing
it. or maybe the presence of the backup file could be used to make
reliable inferences. have fun! ;)
>>> - Previously if the user created a commit before running "rebase
>>> --continue" we'd rebase on to that commit. Now that commit will be
>>> silently dropped.
>>>
>> this is arguably a problem, but not much different from the pre-existing
>> behavior of changes to HEAD done during the initial todo edit being lost.
>
>I think there is a significant difference in that we're moving from a
>situation where we lose commits that are created while rebase is running
>to one where we're losing commits created while rebase is stopped. If a
>user tries to create a commit while rebase is running then they're
>asking for trouble. I don't think creating commits when rebase is
>stopped is unreasonable in the same way.
>
i think that this is a completely meaningless distinction. a rebase is
"running" while the state directory exists. having multiple terminals
open is the norm, and when havoc ensues it doesn't matter to the user
whether one of the terminals had an editor launched by git open at the
time.
>> to avoid that, we'd need to lock HEAD while editing the todo. is that
>> realistic at all?
>
>I don't think it is practical to lock HEAD while git is not running.
>
what measure of "practical" are you applying?
i'm assuming that no persistent locking infra exists currently. but i
don't see a reason why it _couldn't_ - having some functions to populate
and query .git/locked-refs/** in the right places doesn't seem like a
fundamentally hard problem.
>We could just check HEAD has not changed when the rebase continues
>after the user has fixed the todo list as you suggest below.
>
that's a good safeguard which i intend to implement, but when it
triggers, the user will have to deal with the conflict. it would be much
nicer to avoid it in the first place.
>> on top of that, i should verify HEAD against orig-head in
>> start_rebase(). though the only way for the user to get out of that
>> situation is saving the todo contents and --abort'ing (and we must
>> take care not the touch HEAD).
>
>I think in that case it wouldn't be terrible to lose the edited todo
>list as it is a bit of a corner case.
>
actually, yes, it would be. that's why i posted a patch that avoids it.
>> this is somewhat similar to the abysmal situation of the final
>> update-ref failing if the target ref has been modified while being
>> rebased. we'd need to lock that ref for the entire duration of the
>> rebase to avoid that.
>
>"abysmal" is rather harsh - it would also be bad to overwrite the ref in
>that case. I think it in relatively hard to get into that situation
>though as "git checkout" wont checkout a branch that is being updated by
>a rebase.
>
i have no clue how it happened (certainly something to do with many open
terminals), but i actually got into that situation shortly before
writing that mail, and i assure you that "abysmal" is absolutely not an
overstatement. i mean, what do you expect a user to think when presented
with two diverging heads when trying to finish a rebase?
>>> - Previously if the user checkout out another commit before running
>>> "rebase --continue" we'd rebase on to that commit. Now we we rebase
>>> on to the original "onto" commit.
>>>
>> this can be subsumed into the above case.
>
>Meaning check and error out if HEAD has changed?
>
yes
>>> > This makes aborting cheaper and will simplify
>>> > things in a later change.
>>>
>>> Given that we're stopping so the user can fix the problem and continue
>>> the rebase I don't think optimizing for aborting is a convincing
>>> reason for this change on its own.
>>>
>> this is all part of the "More or less as a side effect" paragraph, so
>> this isn't a relevant objection.
>
>I'm simply saying that we should not be optimizing for "rebase --abort"
>in this case. Do you think we should?
>
you're missing the point. the optimization isn't something anyone aimed
for.
regards
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3 0/3] rebase refactoring
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
` (2 preceding siblings ...)
2023-08-09 17:15 ` [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
@ 2023-10-20 9:36 ` Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
` (3 more replies)
3 siblings, 4 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-10-20 9:36 UTC (permalink / raw)
To: git
broken out of the bigger series, as the aggregation just unnecessarily holds it
up.
v3: removed "stray" footer. so more of a RESEND than an actual new version.
Oswald Buddenhagen (3):
rebase: simplify code related to imply_merge()
rebase: handle --strategy via imply_merge() as well
rebase: move parse_opt_keep_empty() down
builtin/rebase.c | 44 ++++++++++++++------------------------------
1 file changed, 14 insertions(+), 30 deletions(-)
--
2.42.0.419.g70bf8a5751
^ permalink raw reply [flat|nested] 49+ messages in thread
* [PATCH v3 1/3] rebase: simplify code related to imply_merge()
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
@ 2023-10-20 9:36 ` Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
` (2 subsequent siblings)
3 siblings, 0 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-10-20 9:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood
The code's evolution left in some bits surrounding enum rebase_type that
don't really make sense any more. In particular, it makes no sense to
invoke imply_merge() if the type is already known not to be
REBASE_APPLY, and it makes no sense to assign the type after calling
imply_merge().
enum rebase_type had more values until commit a74b35081c ("rebase: drop
support for `--preserve-merges`") and commit 10cdb9f38a ("rebase: rename
the two primary rebase backends"). The latter commit also renamed
imply_interactive() to imply_merge().
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2:
- more verbose commit message
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
builtin/rebase.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 50cb85751f..44cc1eed12 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -386,7 +386,6 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
opts->keep_empty = !unset;
- opts->type = REBASE_MERGE;
return 0;
}
@@ -1505,9 +1504,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
}
- if (options.type == REBASE_MERGE)
- imply_merge(&options, "--merge");
-
if (options.root && !options.onto_name)
imply_merge(&options, "--root without --onto");
@@ -1552,7 +1548,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.type == REBASE_UNSPECIFIED) {
if (!strcmp(options.default_backend, "merge"))
- imply_merge(&options, "--merge");
+ options.type = REBASE_MERGE;
else if (!strcmp(options.default_backend, "apply"))
options.type = REBASE_APPLY;
else
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
@ 2023-10-20 9:36 ` Oswald Buddenhagen
2023-10-20 21:51 ` Junio C Hamano
2023-10-20 9:36 ` [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-10-20 22:07 ` [PATCH v3 0/3] rebase refactoring Junio C Hamano
3 siblings, 1 reply; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-10-20 9:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood
At least after the successive trimming of enum rebase_type mentioned in
the previous commit, this code did exactly what imply_merge() does, so
just call it instead.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
builtin/rebase.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 44cc1eed12..4a093bb125 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.strategy) {
options.strategy = xstrdup(options.strategy);
- switch (options.type) {
- case REBASE_APPLY:
- die(_("--strategy requires --merge or --interactive"));
- case REBASE_MERGE:
- /* compatible */
- break;
- case REBASE_UNSPECIFIED:
- options.type = REBASE_MERGE;
- break;
- default:
- BUG("unhandled rebase type (%d)", options.type);
- }
+ imply_merge(&options, "--strategy");
}
if (options.root && !options.onto_name)
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related [flat|nested] 49+ messages in thread
* [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
@ 2023-10-20 9:36 ` Oswald Buddenhagen
2023-10-20 22:07 ` [PATCH v3 0/3] rebase refactoring Junio C Hamano
3 siblings, 0 replies; 49+ messages in thread
From: Oswald Buddenhagen @ 2023-10-20 9:36 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phillip Wood
This moves it right next to parse_opt_empty(), which is a much more
logical place. As a side effect, this removes the need for a forward
declaration of imply_merge().
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
Cc: Junio C Hamano <gitster@pobox.com>
Cc: Phillip Wood <phillip.wood123@gmail.com>
---
builtin/rebase.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4a093bb125..13ca5a644b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -376,19 +376,6 @@ static int run_sequencer_rebase(struct rebase_options *opts)
return ret;
}
-static void imply_merge(struct rebase_options *opts, const char *option);
-static int parse_opt_keep_empty(const struct option *opt, const char *arg,
- int unset)
-{
- struct rebase_options *opts = opt->value;
-
- BUG_ON_OPT_ARG(arg);
-
- imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
- opts->keep_empty = !unset;
- return 0;
-}
-
static int is_merge(struct rebase_options *opts)
{
return opts->type == REBASE_MERGE;
@@ -982,6 +969,18 @@ static enum empty_type parse_empty_value(const char *value)
die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
}
+static int parse_opt_keep_empty(const struct option *opt, const char *arg,
+ int unset)
+{
+ struct rebase_options *opts = opt->value;
+
+ BUG_ON_OPT_ARG(arg);
+
+ imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
+ opts->keep_empty = !unset;
+ return 0;
+}
+
static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
{
struct rebase_options *options = opt->value;
--
2.42.0.419.g70bf8a5751
^ permalink raw reply related [flat|nested] 49+ messages in thread
* Re: [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well
2023-10-20 9:36 ` [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
@ 2023-10-20 21:51 ` Junio C Hamano
0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-10-20 21:51 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git, Phillip Wood
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> At least after the successive trimming of enum rebase_type mentioned in
> the previous commit, this code did exactly what imply_merge() does, so
> just call it instead.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Hmph, I do not recall suggesting it, but the resulting code does
make sense. ;-)
>
> ---
> Cc: Phillip Wood <phillip.wood123@gmail.com>
> ---
> builtin/rebase.c | 13 +------------
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 44cc1eed12..4a093bb125 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1490,18 +1490,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>
> if (options.strategy) {
> options.strategy = xstrdup(options.strategy);
> - switch (options.type) {
> - case REBASE_APPLY:
> - die(_("--strategy requires --merge or --interactive"));
> - case REBASE_MERGE:
> - /* compatible */
> - break;
> - case REBASE_UNSPECIFIED:
> - options.type = REBASE_MERGE;
> - break;
> - default:
> - BUG("unhandled rebase type (%d)", options.type);
> - }
> + imply_merge(&options, "--strategy");
> }
>
> if (options.root && !options.onto_name)
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3 0/3] rebase refactoring
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
` (2 preceding siblings ...)
2023-10-20 9:36 ` [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
@ 2023-10-20 22:07 ` Junio C Hamano
2023-10-23 15:43 ` Phillip Wood
3 siblings, 1 reply; 49+ messages in thread
From: Junio C Hamano @ 2023-10-20 22:07 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> broken out of the bigger series, as the aggregation just unnecessarily holds it
> up.
>
> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>
> Oswald Buddenhagen (3):
> rebase: simplify code related to imply_merge()
> rebase: handle --strategy via imply_merge() as well
> rebase: move parse_opt_keep_empty() down
>
> builtin/rebase.c | 44 ++++++++++++++------------------------------
> 1 file changed, 14 insertions(+), 30 deletions(-)
Looking quite straight-forward and I didn't see anythihng
potentially controversial.
Will queue. Thanks.
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3 0/3] rebase refactoring
2023-10-20 22:07 ` [PATCH v3 0/3] rebase refactoring Junio C Hamano
@ 2023-10-23 15:43 ` Phillip Wood
2023-10-23 19:02 ` Junio C Hamano
0 siblings, 1 reply; 49+ messages in thread
From: Phillip Wood @ 2023-10-23 15:43 UTC (permalink / raw)
To: Junio C Hamano, Oswald Buddenhagen; +Cc: git
On 20/10/2023 23:07, Junio C Hamano wrote:
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> broken out of the bigger series, as the aggregation just unnecessarily holds it
>> up.
>>
>> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>>
>> Oswald Buddenhagen (3):
>> rebase: simplify code related to imply_merge()
>> rebase: handle --strategy via imply_merge() as well
>> rebase: move parse_opt_keep_empty() down
>>
>> builtin/rebase.c | 44 ++++++++++++++------------------------------
>> 1 file changed, 14 insertions(+), 30 deletions(-)
>
> Looking quite straight-forward and I didn't see anythihng
> potentially controversial.
Yes they look good, thanks Oswald
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [PATCH v3 0/3] rebase refactoring
2023-10-23 15:43 ` Phillip Wood
@ 2023-10-23 19:02 ` Junio C Hamano
0 siblings, 0 replies; 49+ messages in thread
From: Junio C Hamano @ 2023-10-23 19:02 UTC (permalink / raw)
To: Phillip Wood; +Cc: Oswald Buddenhagen, git
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 20/10/2023 23:07, Junio C Hamano wrote:
>> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> broken out of the bigger series, as the aggregation just unnecessarily holds it
>>> up.
>>>
>>> v3: removed "stray" footer. so more of a RESEND than an actual new version.
>>>
>>> Oswald Buddenhagen (3):
>>> rebase: simplify code related to imply_merge()
>>> rebase: handle --strategy via imply_merge() as well
>>> rebase: move parse_opt_keep_empty() down
>>>
>>> builtin/rebase.c | 44 ++++++++++++++------------------------------
>>> 1 file changed, 14 insertions(+), 30 deletions(-)
>> Looking quite straight-forward and I didn't see anythihng
>> potentially controversial.
>
> Yes they look good, thanks Oswald
Thanks, both. The topic has already been merged to 'next'.
^ permalink raw reply [flat|nested] 49+ messages in thread
end of thread, other threads:[~2023-10-23 19:02 UTC | newest]
Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-03-23 19:40 ` Phillip Wood
2023-03-23 20:00 ` Junio C Hamano
2023-03-23 21:08 ` Felipe Contreras
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-08-15 14:01 ` Phillip Wood
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-10-20 21:51 ` Junio C Hamano
2023-10-20 9:36 ` [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-10-20 22:07 ` [PATCH v3 0/3] rebase refactoring Junio C Hamano
2023-10-23 15:43 ` Phillip Wood
2023-10-23 19:02 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 2/8] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-03-23 19:39 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 3/8] sequencer: pass around rebase action explicitly Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 21:27 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 4/8] sequencer: create enum for edit_todo_list() return value Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
2023-03-23 19:31 ` Phillip Wood
2023-03-23 22:38 ` Oswald Buddenhagen
2023-03-24 14:15 ` Phillip Wood
2023-03-24 14:42 ` Oswald Buddenhagen
2023-03-23 20:16 ` Junio C Hamano
2023-03-23 23:23 ` Oswald Buddenhagen
2023-03-24 4:31 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Oswald Buddenhagen
2023-03-23 19:46 ` Phillip Wood
2023-03-23 22:13 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id Oswald Buddenhagen
2023-03-23 19:34 ` Phillip Wood
2023-03-23 21:36 ` Oswald Buddenhagen
2023-03-24 14:18 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 8/8] rebase: improve resumption from incorrect initial todo list Oswald Buddenhagen
2023-03-26 14:28 ` Phillip Wood
2023-04-26 15:34 ` Oswald Buddenhagen
2023-05-17 12:13 ` Phillip Wood
2023-08-24 16:46 ` Oswald Buddenhagen
2023-03-23 19:38 ` [PATCH 0/8] sequencer refactoring Phillip Wood
2023-03-25 11:08 ` Phillip Wood
2023-04-06 12:09 ` Phillip Wood
2023-05-17 13:10 ` Phillip Wood
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).