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

"Johannes Schindelin via GitGitGadget" [off-list ref]
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.

OK.

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?

+		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.

+		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 ...

... 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"));

That's much better ;-)

Other than that, looks good to me.

Thanks.

Hi Junio,

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

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

 [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").

quoted

+	  [-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.

quoted

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.

quoted

+		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.

quoted

+		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 ...

quoted

@@ -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.