Hi Junio,
On Tue, 23 Jun 2020, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> 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.
>
>
