On Fri, Feb 09 2018, Eric Sunshine jotted:
> On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
> <[email protected]> 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 protected]>
>> ---
>> 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'
