Re: [PATCH v3 4/8] init: allow specifying the initial branch name for the new repository

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 23 Jun 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> >  [verse]
> >  'git init' [-q | --quiet] [--bare] [--template=<template_directory>]
> >  	  [--separate-git-dir <git dir>] [--object-format=<format]
>
> Completely offtopic.  We lack the closing ket> here.

Not so off-topic: the fix for this would cause conflicts. In the interest
of avoiding merge conflicts, I incorporated a patch to fix that (`git
grep` found two more instances, which I fixed "while at it").

> > +	  [-b <branch-name> | --initial-branch=<branch-name>]
> >  	  [--shared[=<permissions>]] [directory]
> >
> >
> > @@ -67,6 +68,12 @@ repository.
> >  +
> >  If this is reinitialization, the repository will be moved to the specified path.
> >
> > +-b <branch-name::
> > +--initial-branch=<branch-name>::
> > +
> > +Use the specified name for the initial branch in the newly created repository.
> > +If not specified, fall back to the default name: `master`.
>
> OK.
>
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 2a8e3aaaed..b751bdf13e 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -1111,7 +1111,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >  		}
> >  	}
> >
> > -	init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, INIT_DB_QUIET);
> > +	init_db(git_dir, real_git_dir, option_template, GIT_HASH_UNKNOWN, NULL,
> > +		INIT_DB_QUIET);
> >
> >  	if (real_git_dir)
> >  		git_dir = real_git_dir;
> > diff --git a/builtin/init-db.c b/builtin/init-db.c
> > index 0b7222e718..a898153901 100644
> > --- a/builtin/init-db.c
> > +++ b/builtin/init-db.c
> > @@ -203,6 +203,7 @@ void initialize_repository_version(int hash_algo)
> >
> >  static int create_default_files(const char *template_path,
> >  				const char *original_git_dir,
> > +				const char *initial_branch,
> >  				const struct repository_format *fmt)
> >  {
> >  	struct stat st1;
> > @@ -258,16 +259,29 @@ static int create_default_files(const char *template_path,
> >  		die("failed to set up refs db: %s", err.buf);
> >
> >  	/*
> > -	 * Create the default symlink from ".git/HEAD" to the "master"
> > -	 * branch, if it does not exist yet.
> > +	 * Create the default symlink from ".git/HEAD" to the default
> > +	 * branch name, if it does not exist yet.
> >  	 */
>
> To the caller of this helper, it may be "the default", but as far as
> this helper is concerned, it is not "default" but the initial branch
> that was given by the caller.  How about...
>
> 	/*
> 	 * Point the initial branch with HEAD symref, if HEAD does
> 	 * not exist yet.
> 	 */
>
> ... to modernize the reference to symlink and replace it with
> symref?

Good point. I massaged your proposed comment and replaced the old one with
it.

> > +		if (!initial_branch)
> > +			initial_branch = "master";
> > +
> > +		ref = xstrfmt("refs/heads/%s", initial_branch);
> > +		if (check_refname_format(ref, 0) < 0)
> > +			die(_("invalid initial branch name: '%s'"),
> > +			    initial_branch);
>
> Good.  We make sure to prefix with "refs/heads/" so the callers
> cannot abuse us to point HEAD outside the local branches.

Yes, Peff offered that concern, and I agree.

> > +		if (create_symref("HEAD", ref, NULL) < 0)
> >  			exit(1);
> > -	}
> > +		free(ref);
> > +	} else if (initial_branch)
> > +		warning(_("re-init: ignoring --initial-branch=%s"),
> > +			initial_branch);
>
> Somehow the error checking convention feels uneven in this API.  It
> is a warning-worthy offense for the caller to give initial_branch
> when we are re-initializing, but it is not an error for the caller
> not to supply the initial branch name on the other side of if/else.
> Worse yet, this helper function even knows the command line option
> name that resulted in the parameter coming to it.
>
> That unevenness ultimately comes from the fact that the caller does
> not know if we are dealing with a repository that already has HEAD
> before calling, but at least we should be able to tell the caller
> if we initialized or not with our return value and allow the caller
> to issue this warning---that way we can lose the warning from here
> and get rid of the uneven feeling.  Oh, and ...
>
> > @@ -383,7 +397,8 @@ static void validate_hash_algorithm(struct repository_format *repo_fmt, int hash
> >  }
> >
> >  int init_db(const char *git_dir, const char *real_git_dir,
> > -	    const char *template_dir, int hash, unsigned int flags)
> > +	    const char *template_dir, int hash, const char *initial_branch,
> > +	    unsigned int flags)
> >  {
> >  	int reinit;
> >  	int exist_ok = flags & INIT_DB_EXIST_OK;
> > @@ -425,7 +440,8 @@ int init_db(const char *git_dir, const char *real_git_dir,
> >
> >  	validate_hash_algorithm(&repo_fmt, hash);
> >
> > -	reinit = create_default_files(template_dir, original_git_dir, &repo_fmt);
> > +	reinit = create_default_files(template_dir, original_git_dir,
> > +				      initial_branch, &repo_fmt);
>
> ... we are telling the caller if we are in reinit situation, so we
> can afford to do exactly that.
>
> 	reinit = create_default_files...
>  	if (reinit && initial_branch)
> 		warning(_("re-init: ignored --initial-branch"));

I changed it accordingly.

Thanks,
Dscho

>
> That's much better ;-)
>
> Other than that, looks good to me.
>
> Thanks.
>
>