[v2,02/12] fmt-merge-msg: introduce a way to override the main branch name

Commit Message

John Passaro via GitGitGadget June 15, 2020, 12:50 p.m. UTC

From: Johannes Schindelin <[e-mail]>

There is a growing number of projects and companies desiring to change
the main branch name of their repositories (see e.g.
https://twitter.com/mislav/status/1270388510684598272 for background on
this).

However, there are a couple of hard-coded spots in Git's source code
that make this endeavor harder than necessary. For example, when
formatting the commit message for merge commits, Git appends "into
<branch-name>" unless the current branch is the `master` branch.

Clearly, this is not what one wants when already having gone through all
the steps to manually rename the main branch (and taking care of all the
fall-out such as re-targeting existing Pull Requests).

Let's introduce a way to override Git's hard-coded default:
`core.mainBranch`.

We will start supporting this config option in the `git fmt-merge-msg`
command and successively adjust all other places where the main branch
name is hard-coded.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 Documentation/config/core.txt |  5 +++++
 fmt-merge-msg.c               |  6 ++++--
 refs.c                        | 27 +++++++++++++++++++++++++++
 refs.h                        |  7 +++++++
 t/t6200-fmt-merge-msg.sh      |  7 +++++++
 5 files changed, 50 insertions(+), 2 deletions(-)

Comments

Phillip Wood June 15, 2020, 3 p.m. UTC | #1

Hi dscho

On 15/06/2020 13:50, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> There is a growing number of projects and companies desiring to change
> the main branch name of their repositories (see e.g.
> https://twitter.com/mislav/status/1270388510684598272 for background on
> this).

I think this is a good way of phrasing the rationale for the change

> However, there are a couple of hard-coded spots in Git's source code
> that make this endeavor harder than necessary. For example, when
> formatting the commit message for merge commits, Git appends "into
> <branch-name>" unless the current branch is the `master` branch.
> 
> Clearly, this is not what one wants when already having gone through all
> the steps to manually rename the main branch

This didn't quite scan for me maybe s/already having/one has already/ ?

> (and taking care of all the
> fall-out such as re-targeting existing Pull Requests).
> 
> Let's introduce a way to override Git's hard-coded default:
> `core.mainBranch`.
> 
> We will start supporting this config option in the `git fmt-merge-msg`
> command and successively adjust all other places where the main branch
> name is hard-coded.
> 
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>   Documentation/config/core.txt |  5 +++++
>   fmt-merge-msg.c               |  6 ++++--
>   refs.c                        | 27 +++++++++++++++++++++++++++
>   refs.h                        |  7 +++++++
>   t/t6200-fmt-merge-msg.sh      |  7 +++++++
>   5 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> index 74619a9c03b..32bb5368ebb 100644
> --- a/Documentation/config/core.txt
> +++ b/Documentation/config/core.txt
> @@ -626,3 +626,8 @@ core.abbrev::
>   	in your repository, which hopefully is enough for
>   	abbreviated object names to stay unique for some time.
>   	The minimum length is 4.
> +
> +core.mainBranch::
> +	The name of the main (or: primary) branch in the current repository.
> +	For historical reasons, `master` is used as the fall-back for this
> +	setting.
> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
> index 72d32bd73b1..43f4f829242 100644
> --- a/fmt-merge-msg.c
> +++ b/fmt-merge-msg.c
> @@ -407,7 +407,7 @@ static void fmt_merge_msg_title(struct strbuf *out,
>   				const char *current_branch)
>   {
>   	int i = 0;
> -	char *sep = "";
> +	char *sep = "", *main_branch;
>   
>   	strbuf_addstr(out, "Merge ");
>   	for (i = 0; i < srcs.nr; i++) {
> @@ -451,10 +451,12 @@ static void fmt_merge_msg_title(struct strbuf *out,
>   			strbuf_addf(out, " of %s", srcs.items[i].string);
>   	}
>   
> -	if (!strcmp("master", current_branch))
> +	main_branch = git_main_branch_name();
> +	if (!strcmp(main_branch, current_branch))
>   		strbuf_addch(out, '\n');
>   	else
>   		strbuf_addf(out, " into %s\n", current_branch);
> +	free(main_branch);
>   }
>   
>   static void fmt_tag_signature(struct strbuf *tagbuf,
> diff --git a/refs.c b/refs.c
> index 224ff66c7bb..f1854cffa2f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -560,6 +560,33 @@ void expand_ref_prefix(struct argv_array *prefixes, const char *prefix)
>   		argv_array_pushf(prefixes, *p, len, prefix);
>   }
>   
> +char *repo_main_branch_name(struct repository *r)
> +{
> +	const char *config_key = "core.mainbranch";
> +	const char *config_display_key = "core.mainBranch";
> +	const char *fall_back = "master";
> +	char *name = NULL, *ret;
> +
> +	if (repo_config_get_string(r, config_key, &name) < 0)
> +		die(_("could not retrieve `%s`"), config_display_key);
> +
> +	ret = name ? name : xstrdup(fall_back);
> +
> +	if (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))
> +		die(_("invalid branch name: %s = %s"),
> +		    config_display_key, name);
> +
> +	if (name != ret)
> +		free(name);

I'm struggling to come up with a scenario where name != NULL && name != 
ret here, however once we get to patch 4 that scenario definitely does 
exist.

> +
> +	return ret;
> +}
> +
> +char *git_main_branch_name(void)
> +{
> +	return repo_main_branch_name(the_repository);
> +}
> +
>   /*
>    * *string and *len will only be substituted, and *string returned (for
>    * later free()ing) if the string passed in is a magic short-hand form
> diff --git a/refs.h b/refs.h
> index a92d2c74c83..a207ef01348 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -154,6 +154,13 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_
>   int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);
>   int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
>   
> +/*
> + * Retrieves the name of the main (or: primary) branch of the given

nit pick, I'm confused by the ':'

Best Wishes

Phillip

> + * repository.
> + */
> +char *git_main_branch_name(void);
> +char *repo_main_branch_name(struct repository *r);
> +
>   /*
>    * A ref_transaction represents a collection of reference updates that
>    * should succeed or fail together.
> diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
> index e4c2a6eca43..7a873f4a05c 100755
> --- a/t/t6200-fmt-merge-msg.sh
> +++ b/t/t6200-fmt-merge-msg.sh
> @@ -158,6 +158,13 @@ test_expect_success 'setup FETCH_HEAD' '
>   	git fetch . left
>   '
>   
> +test_expect_success 'with overridden default branch name' '
> +	test_when_finished "git switch master" &&
> +	git switch -c default &&
> +	git -c core.mainBranch=default fmt-merge-msg <.git/FETCH_HEAD >actual &&
> +	! grep "into default" actual
> +'
> +
>   test_expect_success 'merge.log=3 limits shortlog length' '
>   	cat >expected <<-EOF &&
>   	Merge branch ${apos}left${apos}
>

Junio C Hamano June 15, 2020, 5:05 p.m. UTC | #2

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
> index 72d32bd73b1..43f4f829242 100644
> --- a/fmt-merge-msg.c
> +++ b/fmt-merge-msg.c
> @@ -407,7 +407,7 @@ static void fmt_merge_msg_title(struct strbuf *out,
>  				const char *current_branch)
>  {
>  	int i = 0;
> -	char *sep = "";
> +	char *sep = "", *main_branch;
>  
>  	strbuf_addstr(out, "Merge ");
>  	for (i = 0; i < srcs.nr; i++) {
> @@ -451,10 +451,12 @@ static void fmt_merge_msg_title(struct strbuf *out,
>  			strbuf_addf(out, " of %s", srcs.items[i].string);
>  	}
>  
> -	if (!strcmp("master", current_branch))
> +	main_branch = git_main_branch_name();
> +	if (!strcmp(main_branch, current_branch))
>  		strbuf_addch(out, '\n');
>  	else
>  		strbuf_addf(out, " into %s\n", current_branch);
> +	free(main_branch);

While you are at it, taking

https://lore.kernel.org/git/20200614211500.GA22505@dcvr/

and the response to it into consideration, I'd suggest we should
support the case where the user says "no single branch is special
here" by configuring it to an empty string.

> +core.mainBranch::
> +	The name of the main (or: primary) branch in the current repository.
> +	For historical reasons, `master` is used as the fall-back for this
> +	setting.

As to the naming of the configuration variable and the actual
fall-back value, I would strongly suggest making them DIFFERNT
(i.e. separate the concept from an actual value).

An instruction

    ... oh, if you want to do so, you can set the core.mainBranch
    configuration variable to 'main'

sounds strange than

    ... oh, if you want to do so, you can set the core.primaryBranch
    configuration variable to 'main'

at least to me, and since I am OK with your choice of 'main' as the
replacement for 'master', a separate word would be more appropriate
for the variable name.

Ævar Arnfjörð Bjarmason June 16, 2020, 8:46 a.m. UTC | #3

On Mon, Jun 15 2020, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>

> +core.mainBranch::
> +	The name of the main (or: primary) branch in the current repository.
> +	For historical reasons, `master` is used as the fall-back for this
> +	setting.

Everywhere else in git-config(1) we just say something to the effect of
the more brief:

    The name of the main (or: primary) branch in the current repository
    (`master` by default).

I think we should do the same here for consistency & ease of reading.

As you note at the start of this series we're not changing the default
yet, so referring to the current default as historical is putting the
cart before the horse as far as producing self-contained patch serieses
goes.

Jeff King June 16, 2020, 1:04 p.m. UTC | #4

On Mon, Jun 15, 2020 at 12:50:06PM +0000, Johannes Schindelin via GitGitGadget wrote:

> +char *repo_main_branch_name(struct repository *r)
> +{
> +	const char *config_key = "core.mainbranch";
> +	const char *config_display_key = "core.mainBranch";
> +	const char *fall_back = "master";
> +	char *name = NULL, *ret;
> +
> +	if (repo_config_get_string(r, config_key, &name) < 0)
> +		die(_("could not retrieve `%s`"), config_display_key);
> +
> +	ret = name ? name : xstrdup(fall_back);
> +
> +	if (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))
> +		die(_("invalid branch name: %s = %s"),
> +		    config_display_key, name);

Ah, this fixes the "we do not check the format of the short name" issue
I pointed out in v1 (sorry, I just realized that v2 existed so I'll
resume reviewing from there; I do still think this might make life
easier for callers by returning a const pointer).

I'm not sure if this check_refname_format() is valid, though. IIRC we've
had issues where "ONELEVEL" was used to check a branch name, but misses
some cases. The more full check done by strbuf_check_branch_ref()
actually creates the full refname and checks that. It also catches stuff
like refs/heads/HEAD.

I doubt that it matters too much for us to be completely thorough here
(unlike some other spots, we are not enforcing rules against potentially
malicious names, but rather just helping the user realize early that
their config is bogus). So I'm not sure how careful we want to be.

-Peff

Junio C Hamano June 17, 2020, 6:21 p.m. UTC | #5

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> On Mon, Jun 15 2020, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <[e-mail]>
>
>> +core.mainBranch::
>> +	The name of the main (or: primary) branch in the current repository.
>> +	For historical reasons, `master` is used as the fall-back for this
>> +	setting.
>
> Everywhere else in git-config(1) we just say something to the effect of
> the more brief:
>
>     The name of the main (or: primary) branch in the current repository
>     (`master` by default).
>
> I think we should do the same here for consistency & ease of reading.
>
> As you note at the start of this series we're not changing the default
> yet, so referring to the current default as historical is putting the
> cart before the horse as far as producing self-contained patch serieses
> goes.

Very good point.  

In [*1*], I gave a potential outline of how a transition plan might
look like (if we were to transition, that is), but what is written
as step 1. in there should be split into two: step 0, in which the
mechanisms (1) to change the default name used for the first branch
and (2) to specify the primary branch that is special-cased by a few
commands are introduced, without any future plan, and step 1, in
which warning and/or advice messages knudge the users and hint the
future direction.

Thanks.


[Reference]

*1* https://lore.kernel.org/git/[e-mail]

Junio C Hamano June 17, 2020, 6:23 p.m. UTC | #6

Jeff King <[e-mail]> writes:

> I'm not sure if this check_refname_format() is valid, though. IIRC we've
> had issues where "ONELEVEL" was used to check a branch name, but misses
> some cases. The more full check done by strbuf_check_branch_ref()
> actually creates the full refname and checks that. It also catches stuff
> like refs/heads/HEAD.

Yup.

I actually am in favor of removing special casing of a single branch
done by fmt-merge-msg and fast-export --anonymize, so this may not
matter.

We still need a mechanism to allow users specify the default name to
be given to the first branch "git init" creates and used by "git clone"
as a fallback name when it cannot infer what the other side uses, though.

Johannes Sixt June 17, 2020, 8:56 p.m. UTC | #7

Am 15.06.20 um 14:50 schrieb Johannes Schindelin via GitGitGadget:
> @@ -451,10 +451,12 @@ static void fmt_merge_msg_title(struct strbuf *out,
>  			strbuf_addf(out, " of %s", srcs.items[i].string);
>  	}
>  
> -	if (!strcmp("master", current_branch))
> +	main_branch = git_main_branch_name();
> +	if (!strcmp(main_branch, current_branch))
>  		strbuf_addch(out, '\n');
>  	else
>  		strbuf_addf(out, " into %s\n", current_branch);
> +	free(main_branch);
>  }

Now that the removal of this special case is on the plate, I would
prefer that the phrase "into foo" is never appended instead of always
appended.

For me, it was a always more of a hindrance than a help. The story goes
like this: A branch that I'm working on was named "edit-box-fix"
yesterday, but today it was renamed to "layout-fix" because the scope
changed. I had merged a topic "rename-buttons" yesterday, and now I have
to go back and rename that "into edit-box-fix" thing! Argh! And tomorrow
I'm going to branch off yet another feature "optional-reset" from
today's state that will be merged into upstream soon; "Merge branch
'rename-buttons' into layout-fix" will read strange in a history that
ends in "Merge branch 'optional-reset'".

And I haven't even mentioned this horrid "into HEAD", which you get
during a rebase operation.

To be clear, the branch name in "Merge branch 'option-reset'" is very
important and invaluable. But the "into foo" part is mostly noise.

-- Hannes

Junio C Hamano June 17, 2020, 9:16 p.m. UTC | #8

Johannes Sixt <[e-mail]> writes:

> Am 15.06.20 um 14:50 schrieb Johannes Schindelin via GitGitGadget:
>> @@ -451,10 +451,12 @@ static void fmt_merge_msg_title(struct strbuf *out,
>>  			strbuf_addf(out, " of %s", srcs.items[i].string);
>>  	}
>>  
>> -	if (!strcmp("master", current_branch))
>> +	main_branch = git_main_branch_name();
>> +	if (!strcmp(main_branch, current_branch))
>>  		strbuf_addch(out, '\n');
>>  	else
>>  		strbuf_addf(out, " into %s\n", current_branch);
>> +	free(main_branch);
>>  }
>
> Now that the removal of this special case is on the plate, I would
> prefer that the phrase "into foo" is never appended instead of always
> appended.

I do not mind such an optional feature.  I always find it useful
whenever I read "git log --oneline --first-parent master..pu" (of
course I have an alias for that) to see which topics are already in
my private "to be used in real life" edition, so I would oppose to
an unconditional removal, though.

Johannes Schindelin June 18, 2020, 1:15 p.m. UTC | #9

Hi,

On Wed, 17 Jun 2020, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
>
> > I'm not sure if this check_refname_format() is valid, though. IIRC we've
> > had issues where "ONELEVEL" was used to check a branch name, but misses
> > some cases. The more full check done by strbuf_check_branch_ref()
> > actually creates the full refname and checks that. It also catches stuff
> > like refs/heads/HEAD.
>
> Yup.
>
> I actually am in favor of removing special casing of a single branch
> done by fmt-merge-msg and fast-export --anonymize, so this may not
> matter.
>
> We still need a mechanism to allow users specify the default name to
> be given to the first branch "git init" creates and used by "git clone"
> as a fallback name when it cannot infer what the other side uses, though.

All right, `core.mainBranch` will go, then, and `init.defaultBranch` will
stay and I will check the full ref.

Thank you for your help improving the patch series,
Dscho

Johannes Schindelin June 23, 2020, 12:31 p.m. UTC | #10

Hi Phillip,

On Mon, 15 Jun 2020, Phillip Wood wrote:

> On 15/06/2020 13:50, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[e-mail]>
> >
> > There is a growing number of projects and companies desiring to change
> > the main branch name of their repositories (see e.g.
> > https://twitter.com/mislav/status/1270388510684598272 for background on
> > this).
>
> I think this is a good way of phrasing the rationale for the change

As I am replacing this patch in v3 with a version that simply drops the
special handling of the `master` branch, I moved that rationale into the
patch introducing support for `git init --initial-branch=<name>`.

> > However, there are a couple of hard-coded spots in Git's source code
> > that make this endeavor harder than necessary. For example, when
> > formatting the commit message for merge commits, Git appends "into
> > <branch-name>" unless the current branch is the `master` branch.
> >
> > Clearly, this is not what one wants when already having gone through all
> > the steps to manually rename the main branch
>
> This didn't quite scan for me maybe s/already having/one has already/ ?

Thank you! If I had not dropped that part of the commit message, I would
have taken your suggested fix.

> > (and taking care of all the
> > fall-out such as re-targeting existing Pull Requests).
> >
> > Let's introduce a way to override Git's hard-coded default:
> > `core.mainBranch`.
> >
> > We will start supporting this config option in the `git fmt-merge-msg`
> > command and successively adjust all other places where the main branch
> > name is hard-coded.
> >
> > Signed-off-by: Johannes Schindelin <[e-mail]>
> > ---
> >   Documentation/config/core.txt |  5 +++++
> >   fmt-merge-msg.c               |  6 ++++--
> >   refs.c                        | 27 +++++++++++++++++++++++++++
> >   refs.h                        |  7 +++++++
> >   t/t6200-fmt-merge-msg.sh      |  7 +++++++
> >   5 files changed, 50 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> > index 74619a9c03b..32bb5368ebb 100644
> > --- a/Documentation/config/core.txt
> > +++ b/Documentation/config/core.txt
> > @@ -626,3 +626,8 @@ core.abbrev::
> >    in your repository, which hopefully is enough for
> >    abbreviated object names to stay unique for some time.
> >    The minimum length is 4.
> > +
> > +core.mainBranch::
> > +	The name of the main (or: primary) branch in the current repository.
> > +	For historical reasons, `master` is used as the fall-back for this
> > +	setting.
> > diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
> > index 72d32bd73b1..43f4f829242 100644
> > --- a/fmt-merge-msg.c
> > +++ b/fmt-merge-msg.c
> > @@ -407,7 +407,7 @@ static void fmt_merge_msg_title(struct strbuf *out,
> >   				const char *current_branch)
> >   {
> >   	int i = 0;
> > -	char *sep = "";
> > +	char *sep = "", *main_branch;
> >
> >    strbuf_addstr(out, "Merge ");
> >    for (i = 0; i < srcs.nr; i++) {
> > @@ -451,10 +451,12 @@ static void fmt_merge_msg_title(struct strbuf *out,
> >    		strbuf_addf(out, " of %s", srcs.items[i].string);
> >    }
> >   -	if (!strcmp("master", current_branch))
> > +	main_branch = git_main_branch_name();
> > +	if (!strcmp(main_branch, current_branch))
> >    	strbuf_addch(out, '\n');
> >    else
> >   		strbuf_addf(out, " into %s\n", current_branch);
> > +	free(main_branch);
> >   }
> >
> >   static void fmt_tag_signature(struct strbuf *tagbuf,
> > diff --git a/refs.c b/refs.c
> > index 224ff66c7bb..f1854cffa2f 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -560,6 +560,33 @@ void expand_ref_prefix(struct argv_array *prefixes,
> > const char *prefix)
> >   		argv_array_pushf(prefixes, *p, len, prefix);
> >   }
> >
> > +char *repo_main_branch_name(struct repository *r)
> > +{
> > +	const char *config_key = "core.mainbranch";
> > +	const char *config_display_key = "core.mainBranch";
> > +	const char *fall_back = "master";
> > +	char *name = NULL, *ret;
> > +
> > +	if (repo_config_get_string(r, config_key, &name) < 0)
> > +		die(_("could not retrieve `%s`"), config_display_key);
> > +
> > +	ret = name ? name : xstrdup(fall_back);
> > +
> > +	if (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))
> > +		die(_("invalid branch name: %s = %s"),
> > +		    config_display_key, name);
> > +
> > +	if (name != ret)
> > +		free(name);
>
> I'm struggling to come up with a scenario where name != NULL && name != ret
> here, however once we get to patch 4 that scenario definitely does exist.

Right.

But as I am dropping the concept of `core.mainBranch` from v3, this won't
apply anymore.

>
> > +
> > +	return ret;
> > +}
> > +
> > +char *git_main_branch_name(void)
> > +{
> > +	return repo_main_branch_name(the_repository);
> > +}
> > +
> >   /*
> >    * *string and *len will only be substituted, and *string returned (for
> >    * later free()ing) if the string passed in is a magic short-hand form
> > diff --git a/refs.h b/refs.h
> > index a92d2c74c83..a207ef01348 100644
> > --- a/refs.h
> > +++ b/refs.h
> > @@ -154,6 +154,13 @@ int repo_dwim_log(struct repository *r, const char
> > *str, int len, struct object_
> >   int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);
> >   int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
> >
> > +/*
> > + * Retrieves the name of the main (or: primary) branch of the given
>
> nit pick, I'm confused by the ':'

Right, v3 won't have that peculiar construct.

Thank you for your review!
Dscho

>
> Best Wishes
>
> Phillip
>
> > + * repository.
> > + */
> > +char *git_main_branch_name(void);
> > +char *repo_main_branch_name(struct repository *r);
> > +
> >   /*
> >    * A ref_transaction represents a collection of reference updates that
> >    * should succeed or fail together.
> > diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
> > index e4c2a6eca43..7a873f4a05c 100755
> > --- a/t/t6200-fmt-merge-msg.sh
> > +++ b/t/t6200-fmt-merge-msg.sh
> > @@ -158,6 +158,13 @@ test_expect_success 'setup FETCH_HEAD' '
> >   	git fetch . left
> >   '
> >
> > +test_expect_success 'with overridden default branch name' '
> > +	test_when_finished "git switch master" &&
> > +	git switch -c default &&
> > +	git -c core.mainBranch=default fmt-merge-msg <.git/FETCH_HEAD >actual
> > &&
> > +	! grep "into default" actual
> > +'
> > +
> >   test_expect_success 'merge.log=3 limits shortlog length' '
> >    cat >expected <<-EOF &&
> >    Merge branch ${apos}left${apos}
> >
>
>

Johannes Schindelin June 23, 2020, 7:19 p.m. UTC | #11

Hi Junio,

On Mon, 15 Jun 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <[e-mail]>
> writes:
>
> > diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
> > index 72d32bd73b1..43f4f829242 100644
> > --- a/fmt-merge-msg.c
> > +++ b/fmt-merge-msg.c
> > @@ -407,7 +407,7 @@ static void fmt_merge_msg_title(struct strbuf *out,
> >  				const char *current_branch)
> >  {
> >  	int i = 0;
> > -	char *sep = "";
> > +	char *sep = "", *main_branch;
> >
> >  	strbuf_addstr(out, "Merge ");
> >  	for (i = 0; i < srcs.nr; i++) {
> > @@ -451,10 +451,12 @@ static void fmt_merge_msg_title(struct strbuf *out,
> >  			strbuf_addf(out, " of %s", srcs.items[i].string);
> >  	}
> >
> > -	if (!strcmp("master", current_branch))
> > +	main_branch = git_main_branch_name();
> > +	if (!strcmp(main_branch, current_branch))
> >  		strbuf_addch(out, '\n');
> >  	else
> >  		strbuf_addf(out, " into %s\n", current_branch);
> > +	free(main_branch);
>
> While you are at it, taking
>
> https://lore.kernel.org/git/20200614211500.GA22505@dcvr/
>
> and the response to it into consideration, I'd suggest we should
> support the case where the user says "no single branch is special
> here" by configuring it to an empty string.

Together with Peff's comments, I think we're even further than that: v3 of
this patch series will completely drop `core.mainBranch` and not
special-case *any* branch in `fmt-merge-msg`.

There is still merit in Hannes Sixt's wish to be able to turn off the
`into <branch>` suffix, but that is orthogonal to the purpose of this here
patch series.

Ciao,
Dscho

>
> > +core.mainBranch::
> > +	The name of the main (or: primary) branch in the current repository.
> > +	For historical reasons, `master` is used as the fall-back for this
> > +	setting.
>
> As to the naming of the configuration variable and the actual
> fall-back value, I would strongly suggest making them DIFFERNT
> (i.e. separate the concept from an actual value).
>
> An instruction
>
>     ... oh, if you want to do so, you can set the core.mainBranch
>     configuration variable to 'main'
>
> sounds strange than
>
>     ... oh, if you want to do so, you can set the core.primaryBranch
>     configuration variable to 'main'
>
> at least to me, and since I am OK with your choice of 'main' as the
> replacement for 'master', a separate word would be more appropriate
> for the variable name.
>
>

Johannes Schindelin June 23, 2020, 9:12 p.m. UTC | #12

Hi Junio & Hannes,


On Wed, 17 Jun 2020, Junio C Hamano wrote:

> Johannes Sixt <[e-mail]> writes:
>
> > Am 15.06.20 um 14:50 schrieb Johannes Schindelin via GitGitGadget:
> >> @@ -451,10 +451,12 @@ static void fmt_merge_msg_title(struct strbuf *out,
> >>  			strbuf_addf(out, " of %s", srcs.items[i].string);
> >>  	}
> >>
> >> -	if (!strcmp("master", current_branch))
> >> +	main_branch = git_main_branch_name();
> >> +	if (!strcmp(main_branch, current_branch))
> >>  		strbuf_addch(out, '\n');
> >>  	else
> >>  		strbuf_addf(out, " into %s\n", current_branch);
> >> +	free(main_branch);
> >>  }
> >
> > Now that the removal of this special case is on the plate, I would
> > prefer that the phrase "into foo" is never appended instead of always
> > appended.
>
> I do not mind such an optional feature.  I always find it useful
> whenever I read "git log --oneline --first-parent master..pu" (of
> course I have an alias for that) to see which topics are already in
> my private "to be used in real life" edition, so I would oppose to
> an unconditional removal, though.

I concur that this would make for a fine optional feature. Of course, that
is an issue that is separate from the goal to make the default branch name
used by `git init` configurable, so I will leave that feature to be
implemented later (and by somebody else).

Ciao,
Dscho

Patch

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 74619a9c03b..32bb5368ebb 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -626,3 +626,8 @@  core.abbrev::
 	in your repository, which hopefully is enough for
 	abbreviated object names to stay unique for some time.
 	The minimum length is 4.
+
+core.mainBranch::
+	The name of the main (or: primary) branch in the current repository.
+	For historical reasons, `master` is used as the fall-back for this
+	setting.
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 72d32bd73b1..43f4f829242 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -407,7 +407,7 @@  static void fmt_merge_msg_title(struct strbuf *out,
 				const char *current_branch)
 {
 	int i = 0;
-	char *sep = "";
+	char *sep = "", *main_branch;
 
 	strbuf_addstr(out, "Merge ");
 	for (i = 0; i < srcs.nr; i++) {
@@ -451,10 +451,12 @@  static void fmt_merge_msg_title(struct strbuf *out,
 			strbuf_addf(out, " of %s", srcs.items[i].string);
 	}
 
-	if (!strcmp("master", current_branch))
+	main_branch = git_main_branch_name();
+	if (!strcmp(main_branch, current_branch))
 		strbuf_addch(out, '\n');
 	else
 		strbuf_addf(out, " into %s\n", current_branch);
+	free(main_branch);
 }
 
 static void fmt_tag_signature(struct strbuf *tagbuf,
diff --git a/refs.c b/refs.c
index 224ff66c7bb..f1854cffa2f 100644
--- a/refs.c
+++ b/refs.c
@@ -560,6 +560,33 @@  void expand_ref_prefix(struct argv_array *prefixes, const char *prefix)
 		argv_array_pushf(prefixes, *p, len, prefix);
 }
 
+char *repo_main_branch_name(struct repository *r)
+{
+	const char *config_key = "core.mainbranch";
+	const char *config_display_key = "core.mainBranch";
+	const char *fall_back = "master";
+	char *name = NULL, *ret;
+
+	if (repo_config_get_string(r, config_key, &name) < 0)
+		die(_("could not retrieve `%s`"), config_display_key);
+
+	ret = name ? name : xstrdup(fall_back);
+
+	if (check_refname_format(ret, REFNAME_ALLOW_ONELEVEL))
+		die(_("invalid branch name: %s = %s"),
+		    config_display_key, name);
+
+	if (name != ret)
+		free(name);
+
+	return ret;
+}
+
+char *git_main_branch_name(void)
+{
+	return repo_main_branch_name(the_repository);
+}
+
 /*
  * *string and *len will only be substituted, and *string returned (for
  * later free()ing) if the string passed in is a magic short-hand form
diff --git a/refs.h b/refs.h
index a92d2c74c83..a207ef01348 100644
--- a/refs.h
+++ b/refs.h
@@ -154,6 +154,13 @@  int repo_dwim_log(struct repository *r, const char *str, int len, struct object_
 int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);
 int dwim_log(const char *str, int len, struct object_id *oid, char **ref);
 
+/*
+ * Retrieves the name of the main (or: primary) branch of the given
+ * repository.
+ */
+char *git_main_branch_name(void);
+char *repo_main_branch_name(struct repository *r);
+
 /*
  * A ref_transaction represents a collection of reference updates that
  * should succeed or fail together.
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index e4c2a6eca43..7a873f4a05c 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -158,6 +158,13 @@  test_expect_success 'setup FETCH_HEAD' '
 	git fetch . left
 '
 
+test_expect_success 'with overridden default branch name' '
+	test_when_finished "git switch master" &&
+	git switch -c default &&
+	git -c core.mainBranch=default fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	! grep "into default" actual
+'
+
 test_expect_success 'merge.log=3 limits shortlog length' '
 	cat >expected <<-EOF &&
 	Merge branch ${apos}left${apos}