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

[re-sending this to the list since it's relevant, and I somehow
accidentally dropped the list]

On Mon, Mar 28, 2016 at 1:13 PM, Eric Sunshine [off-list ref] wrote:

On Mon, Mar 28, 2016 at 6:52 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[v3]:
        Documentation/git-worktree.txt: HEAD --> `<branch>`
        t/t2025-worktree-add.sh: fix style

Thanks, this version addresses the minor issues raised with the previous one.

One observation below...

quoted

diff --git a/builtin/worktree.c b/builtin/worktree.c

@@ -284,11 +285,13 @@ 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 (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) {
                is_junk = 0;
                free(junk_work_tree);

In the no-checkout case, this code effectively becomes:

    ret = run_command("update-ref"/"symbolic-ref");
    if (ret)
        goto done;
    ...
    if (!ret)
        ...free stuff...
    done:
        ...

'ret' does not change value, so it's a bit odd to have an 'if (ret)'
conditional immediately followed by an 'if (!ret)'.

It might be cleaner to re-organize the code slightly so that 'if
(ret)' is used after all run_command()s, and then outdent the "...free
stuff..." code, like this:

    ret = run_command(...);
    if (ret)
        goto done;
    if (checkout) {
        ret = run_command(...)
        if (ret)
            goto done;
    }
    ...free stuff...
    done:
        ...

This is a very minor issue, and I'm not even convinced that the code
is any clearer this way, so it's not something which should hold up
this patch. If you try it and find it nicer, then it could be done as
a follow-on cleanup patch (or a preparatory cleanup patch if you want
to re-roll, but that's probably not necessary).

I should have mentioned this earlier since I specifically looked for
it with a previous version of the patch, but unfortunately didn't
quite spot it. This time I did spot it.