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

On Tue, Sep 1, 2015 at 3:05 PM, Jeff King [off-list ref] wrote:

On Tue, Sep 01, 2015 at 02:52:54PM -0400, Eric Sunshine wrote:

quoted

quoted

+       /* 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:

quoted

quoted

+               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.