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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jun 13, 2016 at 1:39 AM, Jeff King <peff@xxxxxxxx> wrote:
> 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@xxxxxxxx>
> ---
> 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).

> +       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)")),
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]