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.
