Hi Eric,
On Wed, 10 Jun 2020, Eric Sunshine wrote:
> 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...
I downcased it...
> > + if (from_config)
> > + branch_name = from_config;
> > + else
> > + branch_name = "master";
>
> Non-actionable nit: could be written:
>
> branch_name = from_config ? from_config : "master";
Good call.
> > + }
> > +
> > + 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.
I agree that it is a bit hard to follow, but then, the function is really
short, so I hoped it is okay.
> > + /* 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.
For such a tiny nuance, I'd rather keep it as one function...
Thank you,
Dscho
