Subject: Re: [PATCH v2 01/17] fetch: don't redundantly NULL something calloc() gave us

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Fri, 09 Feb 2018 20:06:53 +0100

On Fri, Feb 09 2018, Eric Sunshine jotted:

> On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
> <[email-masked]> wrote:
>> Stop redundantly NULL-ing the last element of the refs structure,
>> which was retrieved via calloc(), and is thus guaranteed to be
>> pre-NULL'd.
>> [...]
>> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
>> ---
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> @@ -1302,7 +1302,6 @@ static int fetch_one(struct remote *remote, int argc, const char **argv)
>>                         } else
>>                                 refs[j++] = argv[i];
>>                 }
>> -               refs[j] = NULL;
>>                 ref_nr = j;
>>         }
>
> This is purely subjective, and I neglected to mention it as early as
> v1, but I find that this change hurts readability. Specifically, as
> I'm scanning or reading code, explicit termination conditions, like
> this NULL assignment, are things I'm expecting to see; they're part of
> the idiom of the language. When they're missing, I have to stop, go
> back, and study the code more carefully to see if the "missing bit" is
> a bug or is intentional. And, it's easy to misread xcalloc() as
> xmalloc(), meaning that it's likely that one studying the code would
> conclude that the missing NULL assignment is a bug.
>
> If anything, if this patch wants to eliminate redundancy, I'd expect
> it to change xcalloc() to xmalloc() and keep the NULL assignment, thus
> leaving the idiom intact.

Thanks for all your review, really appreciate it.

I'm going to keep this as-is, reasons:

 * With calloc it's easier to look at the values in a debugger, you get
   NULLs instead of some random garbage for e.g. the ref src/dst. It
   makes it clear it's unset / the tail value.

 * Ditto fewer things to step through / inpect in a debugger. E.g. I
   have a dump of variables before/after in the debugger, with
   assignments like this it's just adding verbosity & something to
   eyeball for something that's never going to change.

 * If there's a bug in the code using calloc is likely to reveal it
   sooner, since you'll be deref-ing NULL instead of some stray
   (possibly still valid) pointer you got from malloc.

 * It looks more in line with established idioms in the codebase. I
   wasn't able to find other cases where we were double-NULL ing
   calloc'd data, but rather stuff like this:

       git grep -W '\bpattern\b' -- '*/ls-remote.c'