Hi Peff,
On Tue, 16 Jun 2020, Jeff King wrote:
> On Tue, Jun 16, 2020 at 08:45:02AM -0400, 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?
> >
> > 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());
> > }
>
> Actually, one small hiccup is that the config option specifies the
> branch name, not the ref name. So you really would have to prepare a
> static-owned copy of it to turn "foo" into "refs/heads/foo" to get the
> refname.
>
> On the other hand, that would also be a good time to run
> check_ref_format(). In the patch as-is, the "short" return does not
> check that the branch is a valid name.
Legit.
I will work on this.
Thanks,
Dscho
