Re: [PATCH v3 2/6] sequencer: add NULL checks under read_author_script

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 23, 2019, 15:20 UTC
Message-ID: <xmqqsgprnc1u.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190820034536.13071-3-rohit.ashiwal265@gmail.com>

Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

> read_author_script reads name, email and author date from the author
> script. However, it does not check if the arguments are NULL. Adding
> NULL checks will allow us to selectively get the required value,...

I had a hard time understanding the argument here without knowing why I had trouble, and I think I figured it out. What you wrote may not be incorrect per-se, but the logic is backwards.

The function has been about reading all three, and it always took and required the callers to pass three valid pointers to locations to store these three. It did not check for NULL; passing NULL was simply a bug in the caller who deserved a segfault.

This series, however, wants to allow new callers of the function to selectively read some among the three and ignore the rest, and you happened to choose "pass NULL for an uninteresting field" as the new calling convention.

That choice is where "checking for NULL" comes in.

In other words, "checking for NULL" is merely an implementation detail for a more important change this patch brings in: We now can read some and ignore the rest, while requiring that the input file is well formed even for the fields we do not care about.

    sequencer: allow callers of read_author_script() to ignore fields

    The current callers of the read_author_script() function read
    name, email and date from the author script.  Allow callers to
    signal that they are not interested in some among these three
    fields by passing NULL.

    Note that fields that are ignored still must exist and be
    formatted correctly in the author script.

or something like that, perhaps.