On Fri, Feb 09 2018, Eric Sunshine jotted: > On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason > <ava...@gmail.com> 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 <ava...@gmail.com> >> --- >> 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'