Pratik Karki <predatoramigo@xxxxxxxxx> writes:
> In the upcoming builtin rebase, we will have to start by detaching
> the HEAD, just like shell script version does. Essentially, we have
> to do the same thing as `git checkout -q <revision>^0 --`, in pure C.
>
> The aforementioned functionality was already present in `sequencer.c`
> in `do_reset()` function. But `do_reset()` performs more than detaching
> the HEAD, and performs action specific to `sequencer.c`.
>
> So this commit refactors out that part from `do_reset()`, and moves it
> to a new function called `detach_head_to()`. As this function has
> nothing to do with the sequencer, and everything to do with what `git
> checkout -q <revision>^0 --` does, we move that function to checkout.c.
>
> This refactoring actually introduces a slight change in behavior:
> previously, the index was locked before parsing the argument to the
> todo command `reset`, while it now gets locked *after* that, in the
> `detach_head_to()` function.
>
> It does not make a huge difference, and the upside is that this closes
> a few (unlikely) code paths where the index would not be unlocked upon
> error.
>
> Signed-off-by: Pratik Karki <predatoramigo@xxxxxxxxx>
> ---
Here is a place to say "unchanged since v3" for a change like this
one that changes neither the proposed log message above nor the
patch below to help reviewers who have seen the previous round (they
can stop reading here). Hopefully there are more reviewers than you
who write new code, so spending a bit more time to help them spend
less would be an overall win for the project.
Thanks.
> checkout.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> checkout.h | 3 +++
> sequencer.c | 58 +++++-------------------------------------------
> 3 files changed, 72 insertions(+), 53 deletions(-)
>
> diff --git a/checkout.c b/checkout.c
> index bdefc888ba..da68915fd7 100644
> --- a/checkout.c
> +++ b/checkout.c
> @@ -2,6 +2,11 @@
> #include "remote.h"
> #include "refspec.h"
> #include "checkout.h"
> +#include "unpack-trees.h"
> +#include "lockfile.h"
> +#include "refs.h"
> +#include "tree.h"
> +#include "cache-tree.h"
>
> struct tracking_name_data {
> /* const */ char *src_ref;
> @@ -42,3 +47,62 @@ const char *unique_tracking_name(const char *name, struct object_id *oid)
> free(cb_data.dst_ref);
> return NULL;
> }
> +
> +int detach_head_to(struct object_id *oid, const char *action,
> + const char *reflog_message)
> +{
> + struct strbuf ref_name = STRBUF_INIT;
> + struct tree_desc desc;
> + struct lock_file lock = LOCK_INIT;
> + struct unpack_trees_options unpack_tree_opts;
> + struct tree *tree;
> + int ret = 0;
> +
> + if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> + return -1;
> +
> + memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> + setup_unpack_trees_porcelain(&unpack_tree_opts, action);
> + unpack_tree_opts.head_idx = 1;
> + unpack_tree_opts.src_index = &the_index;
> + unpack_tree_opts.dst_index = &the_index;
> + unpack_tree_opts.fn = oneway_merge;
> + unpack_tree_opts.merge = 1;
> + unpack_tree_opts.update = 1;
> +
> + if (read_cache_unmerged()) {
> + rollback_lock_file(&lock);
> + strbuf_release(&ref_name);
> + return error_resolve_conflict(_(action));
> + }
> +
> + if (!fill_tree_descriptor(&desc, oid)) {
> + error(_("failed to find tree of %s"), oid_to_hex(oid));
> + rollback_lock_file(&lock);
> + free((void *)desc.buffer);
> + strbuf_release(&ref_name);
> + return -1;
> + }
> +
> + if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> + rollback_lock_file(&lock);
> + free((void *)desc.buffer);
> + strbuf_release(&ref_name);
> + return -1;
> + }
> +
> + tree = parse_tree_indirect(oid);
> + prime_cache_tree(&the_index, tree);
> +
> + if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> + ret = error(_("could not write index"));
> + free((void *)desc.buffer);
> +
> + if (!ret)
> + ret = update_ref(reflog_message, "HEAD", oid,
> + NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +
> + strbuf_release(&ref_name);
> + return ret;
> +
> +}
> diff --git a/checkout.h b/checkout.h
> index 9980711179..6ce46cf4e8 100644
> --- a/checkout.h
> +++ b/checkout.h
> @@ -10,4 +10,7 @@
> */
> extern const char *unique_tracking_name(const char *name, struct object_id *oid);
>
> +int detach_head_to(struct object_id *oid, const char *action,
> + const char *reflog_message);
> +
> #endif /* CHECKOUT_H */
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..106d518f4d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -29,6 +29,7 @@
> #include "oidset.h"
> #include "commit-slab.h"
> #include "alias.h"
> +#include "checkout.h"
>
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> @@ -2756,14 +2757,7 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
> {
> struct strbuf ref_name = STRBUF_INIT;
> struct object_id oid;
> - struct lock_file lock = LOCK_INIT;
> - struct tree_desc desc;
> - struct tree *tree;
> - struct unpack_trees_options unpack_tree_opts;
> - int ret = 0, i;
> -
> - if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
> - return -1;
> + int i;
>
> if (len == 10 && !strncmp("[new root]", name, len)) {
> if (!opts->have_squash_onto) {
> @@ -2789,56 +2783,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
> if (get_oid(ref_name.buf, &oid) &&
> get_oid(ref_name.buf + strlen("refs/rewritten/"), &oid)) {
> error(_("could not read '%s'"), ref_name.buf);
> - rollback_lock_file(&lock);
> strbuf_release(&ref_name);
> return -1;
> }
> }
>
> - memset(&unpack_tree_opts, 0, sizeof(unpack_tree_opts));
> - setup_unpack_trees_porcelain(&unpack_tree_opts, "reset");
> - unpack_tree_opts.head_idx = 1;
> - unpack_tree_opts.src_index = &the_index;
> - unpack_tree_opts.dst_index = &the_index;
> - unpack_tree_opts.fn = oneway_merge;
> - unpack_tree_opts.merge = 1;
> - unpack_tree_opts.update = 1;
> -
> - if (read_cache_unmerged()) {
> - rollback_lock_file(&lock);
> - strbuf_release(&ref_name);
> - return error_resolve_conflict(_(action_name(opts)));
> - }
> -
> - if (!fill_tree_descriptor(&desc, &oid)) {
> - error(_("failed to find tree of %s"), oid_to_hex(&oid));
> - rollback_lock_file(&lock);
> - free((void *)desc.buffer);
> - strbuf_release(&ref_name);
> - return -1;
> - }
> -
> - if (unpack_trees(1, &desc, &unpack_tree_opts)) {
> - rollback_lock_file(&lock);
> - free((void *)desc.buffer);
> - strbuf_release(&ref_name);
> - return -1;
> - }
> -
> - tree = parse_tree_indirect(&oid);
> - prime_cache_tree(&the_index, tree);
> -
> - if (write_locked_index(&the_index, &lock, COMMIT_LOCK) < 0)
> - ret = error(_("could not write index"));
> - free((void *)desc.buffer);
> -
> - if (!ret)
> - ret = update_ref(reflog_message(opts, "reset", "'%.*s'",
> - len, name), "HEAD", &oid,
> - NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> -
> - strbuf_release(&ref_name);
> - return ret;
> + return detach_head_to(&oid, action_name(opts),
> + reflog_message(opts, "reset", "'%.*s'",
> + len, name));
> }
>
> static int do_merge(struct commit *commit, const char *arg, int arg_len,
