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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 1, 2015, 18:35 UTC
Message-ID: <CAPig+cSZ9et=7DNF15MJo-Ef5o1bjbLxAoTgxJV6q1BEUiBGcA@mail.gmail.com>
In-Reply-To: <CAGZ79kYDYO-q6QEmRQ4YW4ED2HAmMjZNyRaY52ix6YU1=WVNHw@mail.gmail.com>

On Tue, Sep 1, 2015 at 1:49 PM, Stefan Beller <sbeller@google.com> wrote:

Show 24 quoted lines

> took all suggestions except the one below.
>
>>
>>     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 ?

Sorry, I have no idea what you are asking.

All I am saying is that you're unnecessarily invoking getcwd() twice. It's value doesn't change between invocations, so the second call is entirely redundant and wasteful. You can instead call it just once and use the result in both places.