Re: [PATCH 2/4] submodule--helper clone: simplify path check

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 31, 2016, 17:04 UTC
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:

Show 15 quoted lines

> 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"));