Re: [PATCHv4 3/3] submodule: implement `clone` as a builtin helper

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 1, 2015, 19:15 UTC
Message-ID: <CAPig+cQUKVSmt6KNpo01wo8Wzq4YxoX130ROWEri7pVXv1YLqg@mail.gmail.com>
In-Reply-To: <20150901190506.GA8254@sigill.intra.peff.net>

On Tue, Sep 1, 2015 at 3:05 PM, Jeff King <peff@peff.net> wrote:

Show 25 quoted lines

> On Tue, Sep 01, 2015 at 02:52:54PM -0400, Eric Sunshine wrote:
>> > +       /* Redirect the worktree of the submodule in the superproject's config */
>> > +       if (strbuf_getcwd(&sb))
>> > +               die_errno(_("unable to get current working directory"));
>> > +
>> > +       if (!is_absolute_path(sm_gitdir)) {
>> > +               if (strbuf_getcwd(&sb))
>> > +                       die_errno(_("unable to get current working directory"));
>>
>> Why does this need to call getcwd() on 'sb' when it was called
>> immediately above the conditional and its value hasn't changed?
>
> Even weirder, the next code is:
>
>> > +               strbuf_addf(&sb, "/%s", sm_gitdir);
>> > +               free(sm_gitdir);
>> > +               sm_gitdir = strbuf_detach(&sb, NULL);
>> > +       }
>> > +
>> > +
>> > +       strbuf_addf(&sb, "/%s", path);
>
> So if it _was_ an absolute path, we are adding "/$path" to nothing
> (having just detached the strbuf in the conditional above). That seems
> weird.

Indeed, I saw that too, but didn't mention it since this appears to be an incomplete refactoring from the previous round, and my hope was that mentioning the getcwd() oddity would be enough to trigger a thorough check of the code before sending the next version.