| Message ID | 20230323162235.995574-8-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer refactoring | expand |
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);
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?
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 >
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);
... 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(-)