Re: [PATCH 4/4] submodule--helper: use relative path if possible

From: Stefan Beller <sbeller@google.com>
Date: Mar 31, 2016, 19:22 UTC
Message-ID: <CAGZ79kb9sRDuCvJw=S8_BLy7hej_YqE9Y-NbnQhQmafsZuEpwg@mail.gmail.com>
In-Reply-To: <xmqqd1qapore.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 31, 2016 at 9:59 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 40 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> As submodules have working directory and their git directory far apart
>> relative_path(gitdir, work_dir) will not produce a relative path when
>> git_dir is absolute. When the gitdir is absolute, we need to convert
>> the workdir to an absolute path as well to compute the relative path.
>>
>> (e.g. gitdir=/home/user/project/.git/modules/submodule,
>> workdir=submodule/, relative_dir doesn't take the current working directory
>> into account, so there is no way for it to know that the correct answer
>> is indeed "../.git/modules/submodule", if the workdir was given as
>> /home/user/project/submodule, the answer is obvious.)
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  builtin/submodule--helper.c | 7 +++++++
>>  t/t7400-submodule-basic.sh  | 2 +-
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 914e561..0b0fad3 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>>       FILE *submodule_dot_git;
>>       char *sm_gitdir, *cwd, *p;
>>       struct strbuf rel_path = STRBUF_INIT;
>> +     struct strbuf abs_path = STRBUF_INIT;
>>       struct strbuf sb = STRBUF_INIT;
>>
>>       struct option module_clone_options[] = {
>> @@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>>       if (!submodule_dot_git)
>>               die_errno(_("cannot open file '%s'"), sb.buf);
>>
>> +     strbuf_addf(&abs_path, "%s/%s",
>> +                 get_git_work_tree(),
>> +                 path);
>
> The "path" is assumed to be _always_ relative to work tree?

In the code prior to this patch, that was assumed, yes. (e.g. later in the code:

    /* Redirect the worktree of the submodule in the superproject's config */
    cwd = xgetcwd();
    strbuf_addf(&sb, "%s/%s", cwd, path);
    ....
    relative_path(sb.buf, ...
)

Show 35 quoted lines

>
> I am wondering if it would be prudent to have an assert for that
> before doing this, just like I suggested assert(path) for [2/4]
> earlier [*1*].
>
> On the other hand, if we allow path to be absolute, this would need
> to become something like:
>
>         if (is_absolute_path(path))
>                 strbuf_addstr(&abs_path, path);
>         else
>                 strbuf_addf(&abs_path, "%s/%s", get_git_work_tree(), path);
>
>>       fprintf(submodule_dot_git, "gitdir: %s\n",
>> +             is_absolute_path(sm_gitdir) ?
>> +             relative_path(sm_gitdir, abs_path.buf, &rel_path) :
>>               relative_path(sm_gitdir, path, &rel_path));
>
> It seems that the abs_path computation is not needed at all if
> sm_gitdir is relative to begin with.  I wonder if the code gets
> easier to read and can avoid unnecessary strbuf manipulation
> if this entire hunk is structured more like so:
>
>         if (is_absolute_path(sm_gitdir)) {
>                 ...
>         } else {
>                 ...
>         }
>         fprintf(submodule_dot_git, "gitdir: %s\n",
>                 relative_path(sm_gitdir, base_path, &rel_path));
>
>>       if (fclose(submodule_dot_git))
>>               die(_("could not close file %s"), sb.buf);
>
> [Footnote]

I just simplified the code to be

  if (!is_absolute(path))
    path=make_absolute(...);
  if (!is_absolute(sm_gitdir))
    sm_gitdir=make_absolute(...);
  ...
 /* rest operates under absolute path only, no
   conditions any more! */

And I'd think that is cleanest to read and understand.

Having absolute path for both path and gitdir all the time leaves no exceptions for relative_path to spew errors because one of both is relative and not connected to the other absolute.

Show 5 quoted lines

>
> *1* BTW, I tightened the assert for 2/4 to "assert(path && *path)"
> to match the assumption in its log message, i.e. "The calling shell
> code makes sure that path is non-NULL and non empty".
>