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

Thanks for the review.
Sorry for the patch churn, I wasn't quite familiar with working with
mailing list.

2016-03-30 3:20 GMT+08:00 Eric Sunshine [off-list ref]:

On Tue, Mar 29, 2016 at 6:11 AM, Ray Zhang [off-list ref] wrote:

quoted

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 <redacted>
Signed-off-by: Ray Zhang <redacted>
---
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 [off-list ref]

A couple comments below...

quoted

---

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.

quoted

 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.

quoted

+       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