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

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 09 2018, Eric Sunshine jotted:

> On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
>  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 
>> ---
>> 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'

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

[Eric Sunshine]

On Thu, Feb 8, 2018 at 11:19 AM, Ævar Arnfjörð Bjarmason
 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 
> ---
> 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.

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

[Ævar Arnfjörð Bjarmason]

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.

This code dates back to b888d61c83 ("Make fetch a builtin",
2007-09-10), where wasn't any reason to do this back then either, it's
just boilerplate left over from when git-fetch was initially
introduced.

The motivation for this change was to make a subsequent change which
would also modify the refs variable smaller, since it won't have to
copy this redundant "NULL the last + 1 item" pattern.

We may not end up keeping that change, but as this pattern is still
pointless, so let's fix it.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/fetch.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 7bbcd26faf..b34665db9e 100644
--- 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;
}
 
-- 
2.15.1.424.g9478a66081