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

From: Stefan Beller <sbeller@google.com>
Date: Jun 28, 2018, 21:19 UTC
Message-ID: <CAGZ79kZbrk6rSNYx9XP+PWEgghMrgR6JKtdyS+3ku8eOk0X+-w@mail.gmail.com>
In-Reply-To: <20180628074655.5756-5-predatoramigo@gmail.com>

Hi Pratik, On Thu, Jun 28, 2018 at 12:48 AM Pratik Karki <predatoramigo@gmail.com> wrote:

Show 5 quoted lines

>
> 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.

Show 5 quoted lines

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

Show 61 quoted lines

> 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).

Show 11 quoted lines

> +               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/

Show 5 quoted lines

> -       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