Re: [PATCH 1/2] merge: setup `opts` later in `checkout_fast_forward()`

From: Jacob Keller <jacob.keller@gmail.com>
Date: Apr 24, 2018, 06:20 UTC
Message-ID: <CA+P7+xqP8b1i4-C242de2_P9dFsZ03pcRGo45scyfK2ohd-ykw@mail.gmail.com>
In-Reply-To: <f140d2795b9dce8b805cd2ebaf076742978ab8ae.1524545557.git.martin.agren@gmail.com>

On Mon, Apr 23, 2018 at 10:13 PM, Martin Ågren <martin.agren@gmail.com> wrote:

Show 41 quoted lines

> After we initialize the various fields in `opts` but before we actually
> use them, we might return early. Move the initialization further down,
> to immediately before we use `opts`.
>
> This limits the scope of `opts` and will help a subsequent commit fix a
> memory leak without having to worry about those early returns.
>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  merge.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/merge.c b/merge.c
> index f06a4773d4..f123658e58 100644
> --- a/merge.c
> +++ b/merge.c
> @@ -94,8 +94,24 @@ int checkout_fast_forward(const struct object_id *head,
>                 return -1;
>
>         memset(&trees, 0, sizeof(trees));
> -       memset(&opts, 0, sizeof(opts));
>         memset(&t, 0, sizeof(t));
> +
> +       trees[nr_trees] = parse_tree_indirect(head);
> +       if (!trees[nr_trees++]) {
> +               rollback_lock_file(&lock_file);
> +               return -1;
> +       }
> +       trees[nr_trees] = parse_tree_indirect(remote);
> +       if (!trees[nr_trees++]) {
> +               rollback_lock_file(&lock_file);
> +               return -1;
> +       }
> +       for (i = 0; i < nr_trees; i++) {
> +               parse_tree(trees[i]);
> +               init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
> +       }
> +
> +       memset(&opts, 0, sizeof(opts));
>         if (overwrite_ignore) {
>                 memset(&dir, 0, sizeof(dir));

I'm guessing the diff algorithm simply found that this was a more compact representation of the change? It's a bit confusing when your description indicates you "moved" some code down, but it looks like you moved code up.

Thanks, Jake

Show 25 quoted lines

>                 dir.flags |= DIR_SHOW_IGNORED;
> @@ -112,20 +128,6 @@ int checkout_fast_forward(const struct object_id *head,
>         opts.fn = twoway_merge;
>         setup_unpack_trees_porcelain(&opts, "merge");
>
> -       trees[nr_trees] = parse_tree_indirect(head);
> -       if (!trees[nr_trees++]) {
> -               rollback_lock_file(&lock_file);
> -               return -1;
> -       }
> -       trees[nr_trees] = parse_tree_indirect(remote);
> -       if (!trees[nr_trees++]) {
> -               rollback_lock_file(&lock_file);
> -               return -1;
> -       }
> -       for (i = 0; i < nr_trees; i++) {
> -               parse_tree(trees[i]);
> -               init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
> -       }
>         if (unpack_trees(nr_trees, t, &opts)) {
>                 rollback_lock_file(&lock_file);
>                 return -1;
> --
> 2.17.0
>