On Thu, Mar 31, 2016 at 9:59 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@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 <sbel...@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, ...
)
>
> 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.
>
> *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".
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
