Re: [PATCHv2 2/5] submodule--helper clone: simplify path check

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller <sbeller@google.com> wrote:
> submodule--helper clone: simplify path check

I don't see anything in the patch which simplifies a path check.
Instead, this version of the patch is now fixing a potential
NULL-dereference.

> The calling shell code makes sure that `path` is non null and non empty.
> Side note: it cannot be null as just three lines before it is passed
> to safe_create_leading_directories_const which would crash as you feed
> it null. That means the `else` part is dead code, so remove it.

The above description has very little if anything to do with what this
patch is addressing considering that this is now a crash-fix patch.
While it's true that the (current) shell code won't trigger this
crash, that's not particularly interesting or relevant.

> To make the code futureproof, add a check for path being NULL or empty
> and report the error accordingly.

Selling this as a future-proofing change is misleading; it's a crash
fix, plain and simple. I think the entire commit message could be
collapsed to something like this:

    submodule--helper: fix potential NULL-dereference

    Don't dereference NULL 'path' if it was never assigned. While
    here also protect against empty --path argument.

You don't even need to mention removal of the conditional in the
second hunk since, for anyone reading the patch, that's an obvious
consequence of adding the new check in the first hunk.

The patch itself looks fine.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -194,6 +194,9 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>         argc = parse_options(argc, argv, prefix, module_clone_options,
>                              git_submodule_helper_usage, 0);
>
> +       if (!path || !*path)
> +               die(_("submodule--helper: unspecified or empty --path"));
> +
>         strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
>         sm_gitdir = strbuf_detach(&sb, NULL);
>
> @@ -215,10 +218,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>         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_addstr(&sb, ".git");
> +       strbuf_addf(&sb, "%s/.git", path);
>
>         if (safe_create_leading_directories_const(sb.buf) < 0)
>                 die(_("could not create leading directories of '%s'"), sb.buf);
> --
> 2.5.0.264.g39f00fe