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

On Fri, Feb 09 2018, Eric Sunshine jotted:

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

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 <redacted>
---

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'