Re: [PATCH 4/5] sequencer: refactor the code to detach HEAD to checkout.c

[Stefan Beller <sbeller@google.com>]

Hi Pratik,
On Thu, Jun 28, 2018 at 12:48 AM Pratik Karki <predatoramigo@gmail.com> wrote:
>
> The motivation behind this commit is to extract the core part of
> do_reset() from sequencer.c and move it to a new detach_head_to()
> function in checkout.c.
>
[...]
>
> The new function will be used in the next commit by the builtin rebase,
> to perform the initial checkout.

This sounds like the actual motivation, which is fine.

> Here the index only gets locked after performing the first part of
> `do_reset()` rather than before which essentially derives the `oid`
> from the specified label/name passed to the `do_reset()` function.
> It also fixes two bugs: there were two `return error()` statements in
> the `[new root]` case that would have failed to unlock the index.

This sounds as if this fixes a problem? If so it would be nice to have
a test that demonstrates that these specific problems go away.
(but I think we could just argue based on the motivation above that this
is a good change on its own, with or without demonstrating these
additional issues)

> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ---
>  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 bdefc888b..da68915fd 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);

These lines are repeated as a very similar pattern after each
failing function. Maybe we can make it more readable by moving
all these to the end and then using goto to jump there.

For example see "write_pseudoref" in refs.c, that has some interesting
patterns to learn from, e.g. how the return code is constructed
(start off with setting it -1 and only if we go through the whole
function, just before the jump label, we'd set it to 0) and
how all the free/strbuf_releases are at the end (no need to
repeat them).

> +               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);

Awesome, the _indirect function can take commits/tags or trees.

> +       prime_cache_tree(&the_index, tree);

As there is a larger movement to get rid of globals, and the_index is one
of them[1]. So maybe just use the_repository->index already (the_repository
suffers a similar problem, but I think that is more futureproof for
the time being
as we'd want to kill off the_repository in library code eventually as
well and pass
through a repository struct. But for now I'd just use the_repository instead of
having a repository argument)

[1] c.f. https://public-inbox.org/git/20180616054157.32433-1-pclouds@gmail.com/


> -       struct lock_file lock = LOCK_INIT;
> -       struct tree_desc desc;
> -       struct tree *tree;
> -       struct unpack_trees_options unpack_tree_opts;
> -       int ret = 0, i;

[...]

Oh I misspoke above, this is moving code (I should have understood
the hint with 'extracting' by the commit message), so in this case we'd
rather want to move code most verbatim to make review easier, which
it is. So the idea with a goto cleanup could be an optional extra step.

Thanks,
Stefan