Re: [PATCH v5] worktree: add: introduce --checkout option

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Mar 29, 2016 at 6:11 AM, Ray Zhang <zhanglei002@gmail.com> wrote:
> By adding this option which defaults to true, we can use the
> corresponding --no-checkout to make some customizations before
> the checkout, like sparse checkout, etc.
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Ray Zhang <zhanglei002@gmail.com>
> ---
> Changes since last version of this patch[v4]:
>         t/t2025-worktree-add.sh: use test -e to test file existence.
>         builtin/worktree.c: refactor the code a little bit.

Thanks, this version is still:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

A couple comments below...

> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -284,18 +285,22 @@ static int add_worktree(const char *path, const char *refname,
>         if (ret)
>                 goto done;
>
> -       cp.argv = NULL;
> -       argv_array_clear(&cp.args);
> -       argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> -       cp.env = child_env.argv;
> -       ret = run_command(&cp);
> -       if (!ret) {
> -               is_junk = 0;
> -               free(junk_work_tree);
> -               free(junk_git_dir);
> -               junk_work_tree = NULL;
> -               junk_git_dir = NULL;
> +       if (opts->checkout) {
> +               cp.argv = NULL;
> +               argv_array_clear(&cp.args);
> +               argv_array_pushl(&cp.args, "reset", "--hard", NULL);
> +               cp.env = child_env.argv;
> +               ret = run_command(&cp);
> +               if (ret)
> +                       goto done;
>         }
> +
> +       is_junk = 0;
> +       free(junk_work_tree);
> +       free(junk_git_dir);
> +       junk_work_tree = NULL;
> +       junk_git_dir = NULL;

Doing the goto-dance and outdenting the "freeing" code as suggested as
a possible improvement by [1] probably should have been done as a
separate preparatory patch since the result in this patch is fairly
noisy and more difficult to review. However, it's probably not worth
the patch churn to do so now.

>  done:
>         strbuf_reset(&sb);
>         strbuf_addf(&sb, "%s/locked", sb_repo.buf);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> +test_expect_success '"add" worktree with --no-checkout' '
> +       git worktree add --no-checkout -b swamp swamp &&
> +       ! test -e swamp/init.t &&

I realize that this was suggested by [2], however, a more modern way
to state this would be:

    test_path_is_missing swamp/init.t &&

but, as also mentioned in [2], it's probably not worth the patch churn
to change it now.

> +       git -C swamp reset --hard &&
> +       test_cmp init.t swamp/init.t
> +'
> +
> +test_expect_success '"add" worktree with --checkout' '
> +       git worktree add --checkout -b swmap2 swamp2 &&
> +       test_cmp init.t swamp2/init.t
> +'
> +
>  test_done

[1]: http://git.661346.n2.nabble.com/PATCH-add-option-n-no-checkout-to-git-worktree-add-tp7651385p7651884.html

[2]: http://article.gmane.org/gmane.comp.version-control.git/290050