Re: [PATCH 3/3] blame,shortlog: don't make local option variables static

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 14, 2016, 04:32 UTC
Message-ID: <CAPig+cQudTwT6H35YgEvxdK9AAn0BwvUC8bi8-DxGYNh9rb_Lw@mail.gmail.com>
In-Reply-To: <20160613053928.GC23880@sigill.intra.peff.net>

On Mon, Jun 13, 2016 at 1:39 AM, Jeff King <peff@peff.net> wrote:

Show 23 quoted lines

> There's no need for these option variables to be static,
> except that they are referenced by the options array itself,
> which is static. But having all of this static is simply
> unnecessary and confusing (and inconsistent with most other
> commands, which either use a static global option list or a
> true function-local one).
>
> Note that in some cases we may need to actually initialize
> the variables (since we cannot rely on BSS to do so). This
> is a net improvement to readability, though, as we can use
> the more verbose initializers for our string_lists.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -2522,12 +2522,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> -       static struct string_list range_list;
> -       static int output_option = 0, opt = 0;
> -       static int show_stats = 0;
> -       static const char *revs_file = NULL;
> -       static const char *contents_from = NULL;
> -       static const struct option options[] = {
> +       struct string_list range_list = STRING_LIST_INIT_NODUP;

Related to this series, there's an additional "fix" which ought to be made, probably as a separate patch. In particular, in cmd_blame():

    if (lno && !range_list.nr)
        string_list_append(&range_list, xstrdup("1"));

which supplies a default range ("line 1 through end of file") if -L was not specified. I used xstrdup() on the literal "1" in 58dbfa2 (blame: accept multiple -L ranges, 2013-08-06) to be consistent with parse_opt_string_list() which was unconditionally xstrdup'ing the argument (but no longer does as of patch 1/3 of this series).

Show 8 quoted lines

> +       int output_option = 0, opt = 0;
> +       int show_stats = 0;
> +       const char *revs_file = NULL;
> +       const char *contents_from = NULL;
> +       const struct option options[] = {
>                 OPT_BOOL(0, "incremental", &incremental, N_("Show blame entries as we find them, incrementally")),
>                 OPT_BOOL('b', NULL, &blank_boundary, N_("Show blank SHA-1 for boundary commits (Default: off)")),
>                 OPT_BOOL(0, "root", &show_root, N_("Do not treat root commits as boundaries (Default: off)")),