On Wed, Jun 10, 2020 at 5:19 PM Don Goodman-Wilson via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> [...]
> 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@xxxxxxxxxxxxxxxxxx>
> ---
> 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.
> +/*
> + * 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.
