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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Sep 1, 2015 at 3:05 PM, Jeff King <peff@peff.net> wrote:
> 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.