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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 9, 2018, 04:36 UTC
Message-ID: <CAPig+cTwQj0UPvRy9xAF1kbyR8vx4WCsvb0KXsS5GQcDLiqcuQ@mail.gmail.com>
In-Reply-To: <20180208161936.8196-2-avarab@gmail.com>

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 14 quoted lines

> 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 <avarab@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.