Hi Peff,
On Tue, 16 Jun 2020, Jeff King wrote:
> On Wed, Jun 10, 2020 at 08:16:38PM -0400, Eric Sunshine wrote:
>
> > > +/*
> > > + * 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.
>
> Having seen one of the callers, might it be worth avoiding handing off
> ownership of the string entirely?
For `git_default_branch_name()`: yes. For `repo_default_branch_name()`,
not really, as that is potentially repository-specific.
(Side note: while I cannot really think of a use case where you would want
to set `init.defaultBranch` in a repository-local config, there _might_ be
use cases for that out there, and it _is_ how our config machinery works.)
> I.e., this comes from a string that's already owned for the lifetime of
> the process (either the environment, or a string stored by the config
> machinery). Could we just pass that back (or if we want to be more
> careful about getenv() lifetimes, we can copy it into a static owned by
> this function)?
>
> Then all of the callers can stop dealing with the extra free(), and you
> can do:
>
> const char *git_default_branch_name(void)
> {
> return skip_prefix("refs/heads/", git_default_ref_name());
> }
For ease of use, I decided to only ever return the branch name (but check
the full ref).
Those callers that actually need the full ref usually also need the branch
name, and it is easy enough to call `xstrfmt("refs/heads/%s", ...)`.
It might make the code a bit less efficient (but who cares, it's not like
we're setting up a gazillion repositories per second all the time), but
quite a bit easier to reason about.
Ciao,
Dscho
