| Message ID | 4503defe5912aecc4e6a50ac82a31aa7b230456b.1633082702.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: reset_head() related fixes and improvements | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > This parameter is only needed when a ref is going to be updated and > the caller does not pass an explicit reflog message. Callers that are > just discarding changes in the working tree like create_autostash() do > not update any refs so should not have to worry about passing this > parameter. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > builtin/merge.c | 6 ++---- > builtin/rebase.c | 14 ++++++-------- > reset.c | 16 ++++++++++++---- > sequencer.c | 6 +++--- > sequencer.h | 3 +-- > 5 files changed, 24 insertions(+), 21 deletions(-) > > diff --git a/builtin/merge.c b/builtin/merge.c > index d2c52b6e971..35833a963fc 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -1561,8 +1561,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > > if (autostash) > create_autostash(the_repository, > - git_path_merge_autostash(the_repository), > - "merge"); > + git_path_merge_autostash(the_repository)); > if (checkout_fast_forward(the_repository, > &head_commit->object.oid, > &commit->object.oid, The title talks about an optional change to reset_head(); perhaps a change to create_autostash() should be a separate preliminary step.
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > This parameter is only needed when a ref is going to be updated and > the caller does not pass an explicit reflog message. Callers that are > just discarding changes in the working tree like create_autostash() do > not update any refs so should not have to worry about passing this > parameter. This only talks about internal implementation details of passing a parameter that may not be used, but ... > - ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head, > - default_reflog_action, flags); > + if (oid != &head_oid || update_orig_head || switch_to_branch) > + ret = update_refs(oid, switch_to_branch, reflog_head, > + reflog_orig_head, default_reflog_action, > + flags); ... doesn't this have a more significant behaviour change? I am not sure if comparison between oid and head_oid can safely cheat like the patch does, or if it is necessary to do a proper oid comparison, but either way, this would stop calling update_refs(), which in turn means it would have an externally visible effect, like a hook no longer getting called, doesn't it? It would be a change for the good---calling post-checkout hook when you did "git checkout (no other arguments)" feels wasteful, but it deserves to be told to end users, I would think. Again, a new test to protect the change would go well in the same patch. Thanks.
Hi Junio On 01/10/2021 22:08, Junio C Hamano wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> This parameter is only needed when a ref is going to be updated and >> the caller does not pass an explicit reflog message. Callers that are >> just discarding changes in the working tree like create_autostash() do >> not update any refs so should not have to worry about passing this >> parameter. > > This only talks about internal implementation details of passing a > parameter that may not be used, but ... > >> - ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head, >> - default_reflog_action, flags); >> + if (oid != &head_oid || update_orig_head || switch_to_branch) >> + ret = update_refs(oid, switch_to_branch, reflog_head, >> + reflog_orig_head, default_reflog_action, >> + flags); > > ... doesn't this have a more significant behaviour change? > > I am not sure if comparison between oid and head_oid can safely > cheat like the patch does, or if it is necessary to do a proper oid > comparison, but either way, this would stop calling update_refs(), > which in turn means it would have an externally visible effect, like > a hook no longer getting called, doesn't it? > > It would be a change for the good---calling post-checkout hook when > you did "git checkout (no other arguments)" feels wasteful, but it > deserves to be told to end users, I would think. That would be a change for the good, but it is not what this patch does. If oid == &head_oid then it means that the caller did not give us a tree to checkout so HEAD will not change unless we are switching branches. Using a pointer comparison rather than calling oidcmp() was a deliberate choice to avoid any user visible changes as there are no callers that try to run the checkout-hook without passing an oid or branch. The aim here is to avoid having to pass default_reflog when it is not needed without making any user visible changes. > Again, a new test to protect the change would go well in the same > patch. I'm not sure what we could test Best Wishes Phillip > Thanks. > >
diff --git a/builtin/merge.c b/builtin/merge.c index d2c52b6e971..35833a963fc 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1561,8 +1561,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (autostash) create_autostash(the_repository, - git_path_merge_autostash(the_repository), - "merge"); + git_path_merge_autostash(the_repository)); if (checkout_fast_forward(the_repository, &head_commit->object.oid, &commit->object.oid, @@ -1632,8 +1631,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (autostash) create_autostash(the_repository, - git_path_merge_autostash(the_repository), - "merge"); + git_path_merge_autostash(the_repository)); /* We are going to make a new commit. */ git_committer_info(IDENT_STRICT); diff --git a/builtin/rebase.c b/builtin/rebase.c index 26f53c55cc4..1a6af508f49 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -791,8 +791,7 @@ static int move_to_original_branch(struct rebase_options *opts) opts->head_name); ret = reset_head(the_repository, NULL, opts->head_name, RESET_HEAD_REFS_ONLY, - orig_head_reflog.buf, head_reflog.buf, - DEFAULT_REFLOG_ACTION); + orig_head_reflog.buf, head_reflog.buf, NULL); strbuf_release(&orig_head_reflog); strbuf_release(&head_reflog); @@ -1109,7 +1108,7 @@ static int checkout_up_to_date(struct rebase_options *options) options->switch_to); if (reset_head(the_repository, &options->orig_head, options->head_name, RESET_HEAD_RUN_POST_CHECKOUT_HOOK, - NULL, buf.buf, DEFAULT_REFLOG_ACTION) < 0) + NULL, buf.buf, NULL) < 0) ret = error(_("could not switch to %s"), options->switch_to); strbuf_release(&buf); @@ -1558,7 +1557,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) string_list_clear(&merge_rr, 1); if (reset_head(the_repository, NULL, NULL, RESET_HEAD_HARD, - NULL, NULL, DEFAULT_REFLOG_ACTION) < 0) + NULL, NULL, NULL) < 0) die(_("could not discard worktree changes")); remove_branch_state(the_repository, 0); if (read_basic_state(&options)) @@ -1964,8 +1963,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) die(_("could not read index")); if (options.autostash) { - create_autostash(the_repository, state_dir_path("autostash", &options), - DEFAULT_REFLOG_ACTION); + create_autostash(the_repository, + state_dir_path("autostash", &options)); } if (require_clean_work_tree(the_repository, "rebase", @@ -2083,8 +2082,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) options.head_name ? options.head_name : "detached HEAD", oid_to_hex(&options.onto->object.oid)); reset_head(the_repository, NULL, options.head_name, - RESET_HEAD_REFS_ONLY, "HEAD", msg.buf, - DEFAULT_REFLOG_ACTION); + RESET_HEAD_REFS_ONLY, "HEAD", msg.buf, NULL); strbuf_release(&msg); ret = finish_rebase(&options); goto cleanup; diff --git a/reset.c b/reset.c index 668c7639127..be4e18eed98 100644 --- a/reset.c +++ b/reset.c @@ -21,8 +21,13 @@ static int update_refs(const struct object_id *oid, const char *switch_to_branch size_t prefix_len; int ret; - reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT); - strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : default_reflog_action); + if ((update_orig_head && !reflog_orig_head) || !reflog_head) { + if (!default_reflog_action) + BUG("default_reflog_action must be given when reflog messages are omitted"); + reflog_action = getenv(GIT_REFLOG_ACTION_ENVIRONMENT); + strbuf_addf(&msg, "%s: ", reflog_action ? reflog_action : + default_reflog_action); + } prefix_len = msg.len; if (update_orig_head) { @@ -71,6 +76,7 @@ int reset_head(struct repository *r, struct object_id *oid, { unsigned reset_hard = flags & RESET_HEAD_HARD; unsigned refs_only = flags & RESET_HEAD_REFS_ONLY; + unsigned update_orig_head = flags & RESET_ORIG_HEAD; struct object_id head_oid; struct tree_desc desc[2] = { { NULL }, { NULL } }; struct lock_file lock = LOCK_INIT; @@ -141,8 +147,10 @@ int reset_head(struct repository *r, struct object_id *oid, goto leave_reset_head; } - ret = update_refs(oid, switch_to_branch, reflog_head, reflog_orig_head, - default_reflog_action, flags); + if (oid != &head_oid || update_orig_head || switch_to_branch) + ret = update_refs(oid, switch_to_branch, reflog_head, + reflog_orig_head, default_reflog_action, + flags); leave_reset_head: rollback_lock_file(&lock); diff --git a/sequencer.c b/sequencer.c index 07d2a582d39..56a204f18c5 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4093,8 +4093,8 @@ static enum todo_command peek_command(struct todo_list *todo_list, int offset) return -1; } -void create_autostash(struct repository *r, const char *path, - const char *default_reflog_action) +void create_autostash(struct repository *r, const char *path) + { struct strbuf buf = STRBUF_INIT; struct lock_file lock_file = LOCK_INIT; @@ -4131,7 +4131,7 @@ void create_autostash(struct repository *r, const char *path, write_file(path, "%s", oid_to_hex(&oid)); printf(_("Created autostash: %s\n"), buf.buf); if (reset_head(r, NULL, NULL, RESET_HEAD_HARD, NULL, NULL, - default_reflog_action) < 0) + NULL) < 0) die(_("could not reset --hard")); if (discard_index(r->index) < 0 || diff --git a/sequencer.h b/sequencer.h index 2088344cb37..92357536ff4 100644 --- a/sequencer.h +++ b/sequencer.h @@ -198,8 +198,7 @@ void commit_post_rewrite(struct repository *r, const struct commit *current_head, const struct object_id *new_head); -void create_autostash(struct repository *r, const char *path, - const char *default_reflog_action); +void create_autostash(struct repository *r, const char *path); int save_autostash(const char *path); int apply_autostash(const char *path); int apply_autostash_oid(const char *stash_oid);