Re: [PATCH 1/9] init: allow overriding the default branch name for new repositories

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 11, 2020, 00:16 UTC
Message-ID: <CAPig+cSnEvVB5vsffFXidG1-XNxDX10u2XhD9NqV3pwh8zyxxw@mail.gmail.com>
In-Reply-To: <90912e32da1192cfc3b39a18cb606caa46e85b1c.1591823971.git.gitgitgadget@gmail.com>

On Wed, Jun 10, 2020 at 5:19 PM Don Goodman-Wilson via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 11 quoted lines

> [...]
> To make this process much less cumbersome, let's introduce support for
> `core.defaultBranchName`. That way, users won't need to keep their
> copied template files up to date, and won't interfere with default hooks
> installed by their administrators.
> [...]
> Signed-off-by: Don Goodman-Wilson <don@goodman-wilson.com>
> ---
> diff --git a/refs.c b/refs.c
> @@ -560,6 +560,40 @@ void expand_ref_prefix(struct argv_array *prefixes, const char *prefix)
> +                       die(_("Could not retrieve `core.defaultBranchName`"));

Nit: here the error message is capitalized...

> +               if (from_config)
> +                       branch_name = from_config;
> +               else
> +                       branch_name = "master";

Non-actionable nit: could be written:

    branch_name = from_config ? from_config : "master";

> +       }
> +
> +       if (short_name)
> +               return from_config ? from_config : xstrdup(branch_name);

The logic overall is a bit difficult to follow when trying to understand when and when not to duplicate the string and when and when not to free(), but seems to be correct.

> +       /* prepend "refs/heads/" to the branch name */
> +       prefixed = xstrfmt("refs/heads/%s", branch_name);
> +       if (check_refname_format(prefixed, 0))
> +               die(_("invalid default branch name: '%s'"), branch_name);

Here, the error message is not capitalized. It would be nice for both messages to share a common capitalization scheme. These days, we tend to favor _not_ capitalizing error messages, so perhaps remove capitalization from the earlier one.

Show 5 quoted lines

> +/*
> + * Retrieves the name of the default branch. If `short_name` is non-zero, the
> + * branch name will be prefixed with "refs/heads/".
> + */
> +char *git_default_branch_name(int short_name);

Overall, the internal logic regarding duplicating/freeing strings would probably be easier to grok if there were two separate functions:

    char *git_default_branch_name(void);
    char *git_default_ref_name(void);

but that's subjective.