Re: [PATCH 01/11] maintenance: create basic maintenance runner

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 12, 2020, 22:50 UTC
Message-ID: <20200812225054.GB104953@google.com>
In-Reply-To: <xmqqsgcr4ixo.fsf@gitster.c.googlers.com>

Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

Show 23 quoted lines

>>> +static int maintenance_run(struct maintenance_opts *opts)
>>> +{
>>> +	return maintenance_task_gc(opts);
>>> +}
>>> +
>>> +int cmd_maintenance(int argc, const char **argv, const char *prefix)
>>> +{
>>> +	static struct maintenance_opts opts;
>>> +	static struct option builtin_maintenance_options[] = {
>>> +		OPT_BOOL(0, "auto", &opts.auto_flag,
>>> +			 N_("run tasks based on the state of the repository")),
>>> +		OPT_END()
>>> +	};
>>
>> optional: these could be local instead of static
>
> Do we have preference?  I think it is more common in our codebase to
> have these non-static but I do not think we've chosen which is more
> preferable with technical reasoning behind it.  As the top-level
> function in any callchain, cmd_foo() does not have multiple instances
> running at the same time, or it does not have to be prepared to be
> called twice, so they can afford to be static, but is there any upside
> for them to be static?

Code size, execution time:

- benefit of static: can rely on automatic zero-initialization of
  pages instead of spending cycles and text size on explicit
  zero-initialization

- benefit of local: avoids wasting address space in bss when the
  function isn't called

Neither of those seems important enough to care about. :)

Maintainability:

- benefit of static: address is determined at compile time, so can build
  using C89 compilers that require struct initializers to be constant
  (but Git already cannot be built with such compilers)

- benefit of local: less likely to accidentally move the static var into
  a function that needs to be reentrant

- benefit of local: allows readers and reviewers to build the habit of
  seeing non-const "static" declarations within a function as the
  unusual case.  When a "static" is declared in a function body, this
  means we are caching something or have some other performance reason
  to sacrifice reentrancy

It's mostly that last aspect (saving readers from having to think about it) that motivates my suggestion above.

Older Git code used static more because it was needed when building using C89 compilers.

Thanks, Jonathan