Re: [PATCHv5 3/3] submodule: Reimplement `module_clone` shell function in C

[Stefan Beller <sbeller@google.com>]

On Thu, Sep 3, 2015 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +
>> +     cp.no_stdin = 1;
>> +     cp.no_stdout = 1;
>> +     cp.no_stderr = 1;
>
> Output from "git clone" is not shown, regardless of --quiet option?

Removed that.

>> +     argc = parse_options(argc, argv, prefix, module_clone_options,
>> +                          git_submodule_helper_usage, 0);
>> +
>> +     strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
>
> The original says
>
>         base_name=$(dirname "$name")
>
> before doing this %s/modules/%s concatenation.  I do not think we
> intended to allow a slash in submodule name, so this difference may
> be showing that the original was doing an unnecessary thing.

The way I understood the code, this was a workaround of now having
safe_create_leading_directories, which takes the base directory of
a given path and creates that directory. (base_name is only used as an
argument for mkdir -p).

Slashes are already in use for submodule names as the name defaults
to the path if no explicit name is given. And the path may contain slashes
as it may be nested. In Gerrit we have a .gitmodules:
[submodule "plugins/commit-message-length-validator"]
    path = plugins/commit-message-length-validator
    url = ../plugins/commit-message-length-validator
[...

>
>> +     sm_gitdir = strbuf_detach(&sb, NULL);
>> +
>> +     if (!file_exists(sm_gitdir)) {
>> +             if (safe_create_leading_directories_const(sm_gitdir) < 0)
>> +                     die(_("could not create directory '%s'"), sm_gitdir);
>> +             if (clone_submodule(path, sm_gitdir, url, depth, reference, quiet))
>> +                     die(_("clone of '%s' into submodule path '%s' failed"),
>> +                         url, path);
>> +     } else {
>> +             if (safe_create_leading_directories_const(path) < 0)
>> +                     die(_("could not create directory '%s'"), path);
>> +             strbuf_addf(&sb, "%s/index", sm_gitdir);
>> +             if (unlink(sb.buf) < 0)
>> +                     die_errno(_("failed to delete '%s'"), sm_gitdir);
>
> The original says "we do not care if it failed" with
>
>         rm -f "$gitdir/index"
>
> I think the intention of the original is "we do not care if it
> failed because it did not exist." in which case unconditional
> die_errno() here may be something we do not want?

Right, this was a short-circuit reaction from me on Erics comment
to check for the return value of unlink. I think we can use
unlink_or_warn here as that only warns if we cannot remove
an existing file. non existent files are not warned about.

>
>> +             strbuf_reset(&sb);
>> +     }
>> +
>> +     /* Write a .git file in the submodule to redirect to the superproject. */
>> +     if (safe_create_leading_directories_const(path) < 0)
>> +             die(_("could not create directory '%s'"), path);
>> +
>> +     if (path && *path)
>> +             strbuf_addf(&sb, "%s/.git", path);
>> +     else
>> +             strbuf_addf(&sb, ".git");
>> +
>> +     if (safe_create_leading_directories_const(sb.buf) < 0)
>> +             die(_("could not create leading directories of '%s'"), sb.buf);
>> +     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))
>> +             die(_("could not close file %s"), sb.buf);
>> +     strbuf_reset(&sb);
>> +     strbuf_reset(&rel_path);
>
> The original seems to go quite a length to make sure symbolic links
> do not fool the comparison between $gitdir and $sm_path, and also it
> makes sure one is not a subpath of the other.  Do we need the same
> level of carefulness, or is textual relative_path() enough?

I think the original was doing an "optimized" version of relative_path()
as we know that they have an anchor at the superproject root directory.

relative_path() seems to deal with the symbolic links just fine, as all
tests pass. (6eafa6d096ce, "submodules: don't stumble over symbolic
links when cloning recursively" did add code as well as testing for symbolic
link problems, Thanks Jens!)

>>               --depth=*)
>> -                     depth=$1
>> +                     depth="$1"
>
> This seems to be an unrelated change.

A leftover from earlier, when I mucked around with --depth and
--reference in the shell scipt.


On Thu, Sep 3, 2015 at 4:17 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> +       if (path && *path)
>> +               strbuf_addf(&sb, "%s/.git", path);
>> +       else
>> +               strbuf_addf(&sb, ".git");
>
> Minor: strbuf_addstr(...);

done