Re: [PATCH 3/3] submodule: implement `module_clone` as a builtin helper

From: Stefan Beller <sbeller@google.com>
Date: Sep 1, 2015, 17:49 UTC
Message-ID: <CAGZ79kYDYO-q6QEmRQ4YW4ED2HAmMjZNyRaY52ix6YU1=WVNHw@mail.gmail.com>
In-Reply-To: <CAPig+cQVe+qmTsqqdnTXDABxNkQoeEqz07pP2pYbY_n11XMDWA@mail.gmail.com>

took all suggestions except the one below.

Show 15 quoted lines

>
>     if (strbuf_getcwd(&sb))
>         die_errno(...);
>     strbuf_addf(&sb, "/%s, sm_gitdir);
>     free(sm_gitdir);
>     sm_gitdir = strbuf_detach(&sb, NULL);
>
>> +       }
>> +
>> +       if (strbuf_getcwd(&sb))
>> +               die_errno("unable to get current working directory");
>
> The conditional block just above here also gets 'cwd'. If you move
> this code above the !is_absolute_path(sm_gitdir) conditional block,
> then you can avoid the duplication.

I don't get it. We need to have strbuf_getcwd twice no matter how we arrange the code as we strbuf_detach will empty the strbuf.

Do you mean to call strbuf_getcwd just once and then xstrdup the value, then reset the strbuf to just contain the cwd and append the other string ?

Show 12 quoted lines

>
>> +       strbuf_addf(&sb, "/%s", path);
>> +
>> +       p = git_pathdup_submodule(path, "config");
>> +       if (!p)
>> +               die("Could not get submodule directory for '%s'", path);
>> +       git_config_set_in_file(p, "core.worktree",
>> +                              relative_path(sb.buf, sm_gitdir, &rel_path));
>> +       strbuf_release(&sb);
>> +       free((char *)sm_gitdir);
>> +       return 0;
>> +}