From: Eric Sunshine <sunshine@sunshineco.com>
Date: Thu, 31 Mar 2016 17:04:32 GMT
Subject: Re: [PATCH 2/4] submodule--helper clone: simplify path check
Message-ID: <CAPig+cRq3CoE1mH9=TeKnujQYVb0+tURpYNSsRKtnK_FTDyBAQ@mail.gmail.com>
In-Reply-To: <xmqqlh4yppsq.fsf@gitster.mtv.corp.google.com>

On Thu, Mar 31, 2016 at 12:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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).
>
> This is not Java so let's spell that thing NULL.
>
> Perhaps it would be cheap-enough prudence to do this on top?
>
>         argc = parse_options(argc, argv, prefix, module_clone_options,
>                              git_submodule_helper_usage, 0);
>
> +       assert(path);

Hmm, from the user perspective, this is still a "crash", just as the
existing segfault is a crash. While one can argue that this is an
internal command only invoked in a controlled fashion, it's not hard
to imagine someone running it manually to understand its behavior or
to debug some problem. This function already presents proper error
messages for other problems it encounters, so it seems reasonable for
it do so for this problem, as well.

    if (!path || !*path)
        die(_("submodule--helper: unspecified or empty --path"));