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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 29, 2016, 19:20 UTC
Message-ID: <CAPig+cRimDGFDk7A2p2rKpJ2GR27_R=BJdpyPK4xeyDU1TruWQ@mail.gmail.com>
In-Reply-To: <01020153c1dae323-b918e73f-f116-4760-9747-9aa8cafb2db5-000000@eu-west-1.amazonses.com>

On Tue, Mar 29, 2016 at 6:11 AM, Ray Zhang <zhanglei002@gmail.com> wrote:

Show 10 quoted lines

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

Show 32 quoted lines

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

Show 7 quoted lines

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

Show 10 quoted lines

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