Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths

From: Stefan Beller <sbeller@google.com>
Date: Mar 31, 2016, 22:59 UTC
Message-ID: <CAGZ79kaAs_ZwxfhLSTw3z8JY33vGToms2z6zYEiCmRpfS6SA+Q@mail.gmail.com>
In-Reply-To: <xmqqfuv6l1xf.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 31, 2016 at 3:26 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> +     if (!is_absolute_path(sm_gitdir)) {
>> +             char *cwd = xgetcwd();
>> +             strbuf_addf(&sb, "%s/%s", cwd, sm_gitdir);
>> +             sm_gitdir = strbuf_detach(&sb, 0);
>> +             free(cwd);
>
> It would be surprising that this would be the first codepath that
> needs to get an absolute pathname in the codebase that is more than
> 10 years old, wouldn't it?  Don't we have a reasonable API helper
> function to do this kind of thing already?
>
>     ... goes and looks ...
>
> Doesn't absolute_path() work here?

*reads absolute_path(...)*

    Yes that is great. But why is it written the way it is?
    I would have expected:

 const char *absolute_path(const char *path)
 {
        static struct strbuf sb = STRBUF_INIT;
-       strbuf_reset(&sb);
        strbuf_add_absolute_path(&sb, path);
-       return sb.buf;
+       return strbuf_detach(&sb);
 }

Further checking reveals any caller uses it with

    desired= xstrdup(absolute_path(my_argument));

We are loosing memory of the strbuf here. so if I we'd take the diff above we can also get rid of all the xstrdups at the callers. For now I will adhere to all other callers and use xstrdup(absolute_path(...) here too.

I'll remove the unrelated changes as well in a resend.

Show 55 quoted lines

>
>> @@ -221,7 +240,6 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>>       submodule_dot_git = fopen(sb.buf, "w");
>>       if (!submodule_dot_git)
>>               die_errno(_("cannot open file '%s'"), sb.buf);
>> -
>>       fprintf(submodule_dot_git, "gitdir: %s\n",
>>               relative_path(sm_gitdir, path, &rel_path));
>>       if (fclose(submodule_dot_git))
>
> Looks like an unrelated change to me.
>
>> @@ -229,24 +247,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>>       strbuf_reset(&sb);
>>       strbuf_reset(&rel_path);
>>
>> -     cwd = xgetcwd();
>>       /* Redirect the worktree of the submodule in the superproject's config */
>> -     if (!is_absolute_path(sm_gitdir)) {
>> -             strbuf_addf(&sb, "%s/%s", cwd, sm_gitdir);
>> -             free(sm_gitdir);
>> -             sm_gitdir = strbuf_detach(&sb, NULL);
>> -     }
>> -
>> -     strbuf_addf(&sb, "%s/%s", cwd, 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));
>> +                            relative_path(path, sm_gitdir, &rel_path));
>>       strbuf_release(&sb);
>>       strbuf_release(&rel_path);
>>       free(sm_gitdir);
>> -     free(cwd);
>> +
>
> This addition of blank, too.
>
>>       free(p);
>>       return 0;
>>  }
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index fc11809..ea3fabb 100755
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>> @@ -818,7 +818,7 @@ test_expect_success 'submodule add --name allows to replace a submodule with ano
>>       )
>>  '
>>
>> -test_expect_failure 'recursive relative submodules stay relative' '
>> +test_expect_success 'recursive relative submodules stay relative' '
>>       test_when_finished "rm -rf super clone2 subsub sub3" &&
>>       mkdir subsub &&
>>       (