[2/9] remote: respect `core.defaultBranchName`

Commit Message

Johannes Schindelin via GitGitGadget June 10, 2020, 9:19 p.m. UTC

From: Johannes Schindelin <[e-mail]>

When guessing the default branch name of a remote, and there are no refs
to guess from, we want to go with the preference specified by the user
for the fall-back.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 remote.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

Comments

Jeff King June 16, 2020, 12:35 p.m. UTC | #1

On Wed, Jun 10, 2020 at 09:19:23PM +0000, Johannes Schindelin via GitGitGadget wrote:

> @@ -2099,7 +2100,10 @@ struct ref *guess_remote_head(const struct ref *head,
>  
>  	/* If refs/heads/master could be right, it is. */
>  	if (!all) {
> -		r = find_ref_by_name(refs, "refs/heads/master");
> +		char *name = git_default_branch_name(0);
> +
> +		r = find_ref_by_name(refs, name);
> +		free(name);
>  		if (r && oideq(&r->old_oid, &head->old_oid))
>  			return copy_ref(r);
>  	}

You'd perhaps want to update the comment above, too.

However, I think we should be a bit more lenient on the "reading" side
default names. Just because "foo" is _my_ default branch name, does not
mean it is the default on the remote side. We cannot know what the other
side's default preference is, but in a world where we have 15 years of
repos that may have been created with "master", it is probably still a
good guess.

I.e., I think this probably ought to check the preferred name, and then
fall back to the existing behavior, like:

  if (!all) {
	  char *name;

          /* try the user's preferred default branch name */
	  name = git_default_branch_name(0);
	  r = find_ref_by_name(refs, name);
	  free(name);
	  if (r && oideq(&r->old_oid, &head->old_oid))
	          return copy_ref(r);

	  /* otherwise, try "master", which is the historical default */
	  r = find_ref_by_name(refs, "refs/heads/master");
	  if (r && oideq(&r->old_oid, &head->old_oid))
	          return copy_ref(r);
  }

That will help minimize fallout when git_default_branch_name() changes,
either by user config or if we switch the baked-in default. In the
latter case, we might also consider hard-coding that as a guess between
the user's preferred name and the historical "master".

Hopefully this would not matter _too_ much either way, as most servers
would support the symref extension these days. But I still think we
should do our best to minimize spots where the user may see a
regression.

-Peff

Johannes Schindelin June 18, 2020, 10:21 a.m. UTC | #2

Hi Peff,

On Tue, 16 Jun 2020, Jeff King wrote:

> On Wed, Jun 10, 2020 at 09:19:23PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > @@ -2099,7 +2100,10 @@ struct ref *guess_remote_head(const struct ref *head,
> >
> >  	/* If refs/heads/master could be right, it is. */
> >  	if (!all) {
> > -		r = find_ref_by_name(refs, "refs/heads/master");
> > +		char *name = git_default_branch_name(0);
> > +
> > +		r = find_ref_by_name(refs, name);
> > +		free(name);
> >  		if (r && oideq(&r->old_oid, &head->old_oid))
> >  			return copy_ref(r);
> >  	}
>
> You'd perhaps want to update the comment above, too.
>
> However, I think we should be a bit more lenient on the "reading" side
> default names. Just because "foo" is _my_ default branch name, does not
> mean it is the default on the remote side. We cannot know what the other
> side's default preference is, but in a world where we have 15 years of
> repos that may have been created with "master", it is probably still a
> good guess.
>
> I.e., I think this probably ought to check the preferred name, and then
> fall back to the existing behavior, like:
>
>   if (!all) {
> 	  char *name;
>
>           /* try the user's preferred default branch name */
> 	  name = git_default_branch_name(0);
> 	  r = find_ref_by_name(refs, name);
> 	  free(name);
> 	  if (r && oideq(&r->old_oid, &head->old_oid))
> 	          return copy_ref(r);
>
> 	  /* otherwise, try "master", which is the historical default */
> 	  r = find_ref_by_name(refs, "refs/heads/master");
> 	  if (r && oideq(&r->old_oid, &head->old_oid))
> 	          return copy_ref(r);
>   }
>
> That will help minimize fallout when git_default_branch_name() changes,
> either by user config or if we switch the baked-in default. In the
> latter case, we might also consider hard-coding that as a guess between
> the user's preferred name and the historical "master".
>
> Hopefully this would not matter _too_ much either way, as most servers
> would support the symref extension these days. But I still think we
> should do our best to minimize spots where the user may see a
> regression.

Sure, we could just leave this alone, or we can just ditch the
special-casing of `master` here.

As you say, this does not affect any modern Git version, and IIRC the code
after that special-casing tries to find any remote ref that matches the
remote `HEAD`.

So it's not like we _need_ this special-casing, anyway.

Ciao,
Dscho

Jeff King June 18, 2020, 11:50 a.m. UTC | #3

On Thu, Jun 18, 2020 at 12:21:30PM +0200, Johannes Schindelin wrote:

> > Hopefully this would not matter _too_ much either way, as most servers
> > would support the symref extension these days. But I still think we
> > should do our best to minimize spots where the user may see a
> > regression.
> 
> Sure, we could just leave this alone, or we can just ditch the
> special-casing of `master` here.
> 
> As you say, this does not affect any modern Git version, and IIRC the code
> after that special-casing tries to find any remote ref that matches the
> remote `HEAD`.

I think we need to be a little careful with "any modern Git", because a
modern client against an old (or perhaps an alternative implementation)
server might still use it. I have to imagine it's pretty rare, but I
think it's still useful to return _some_ value.

But as you note, even without a symref extension, we already try to
guess based on a unique branch. Probably even choosing the first one
alphabetically would be reasonable. But I'd rather err on the side of
historical compatibility if we can do so easily. Looking for
init.mainBranch, followed by master, accomplishes that and isn't many
lines of code.

-Peff

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

Hi Peff,

On Thu, 18 Jun 2020, Jeff King wrote:

> On Thu, Jun 18, 2020 at 12:21:30PM +0200, Johannes Schindelin wrote:
>
> > > Hopefully this would not matter _too_ much either way, as most servers
> > > would support the symref extension these days. But I still think we
> > > should do our best to minimize spots where the user may see a
> > > regression.
> >
> > Sure, we could just leave this alone, or we can just ditch the
> > special-casing of `master` here.
> >
> > As you say, this does not affect any modern Git version, and IIRC the code
> > after that special-casing tries to find any remote ref that matches the
> > remote `HEAD`.
>
> I think we need to be a little careful with "any modern Git", because a
> modern client against an old (or perhaps an alternative implementation)
> server might still use it. I have to imagine it's pretty rare, but I
> think it's still useful to return _some_ value.
>
> But as you note, even without a symref extension, we already try to
> guess based on a unique branch. Probably even choosing the first one
> alphabetically would be reasonable. But I'd rather err on the side of
> historical compatibility if we can do so easily. Looking for
> init.mainBranch, followed by master, accomplishes that and isn't many
> lines of code.

I ended up using `init.defaultBranch` as preference, falling back to
`master`, and then falling back to the first ref matching the given commit
hash.

That should be safe enough.

Ciao,
Dscho

Patch

diff --git a/remote.c b/remote.c
index 534c6426f1e..95fa8cc78e0 100644
--- a/remote.c
+++ b/remote.c
@@ -256,7 +256,7 @@  static void read_remotes_file(struct remote *remote)
 
 static void read_branches_file(struct remote *remote)
 {
-	char *frag;
+	char *frag, *default_branch_name = NULL;
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f = fopen_or_warn(git_path("branches/%s", remote->name), "r");
 
@@ -276,7 +276,7 @@  static void read_branches_file(struct remote *remote)
 
 	/*
 	 * The branches file would have URL and optionally
-	 * #branch specified.  The "master" (or specified) branch is
+	 * #branch specified.  The default (or specified) branch is
 	 * fetched and stored in the local branch matching the
 	 * remote name.
 	 */
@@ -284,7 +284,7 @@  static void read_branches_file(struct remote *remote)
 	if (frag)
 		*(frag++) = '\0';
 	else
-		frag = "master";
+		frag = default_branch_name = git_default_branch_name(1);
 
 	add_url_alias(remote, strbuf_detach(&buf, NULL));
 	strbuf_addf(&buf, "refs/heads/%s:refs/heads/%s",
@@ -299,6 +299,7 @@  static void read_branches_file(struct remote *remote)
 	strbuf_addf(&buf, "HEAD:refs/heads/%s", frag);
 	refspec_append(&remote->push, buf.buf);
 	remote->fetch_tags = 1; /* always auto-follow */
+	free(default_branch_name);
 	strbuf_release(&buf);
 }
 
@@ -2099,7 +2100,10 @@  struct ref *guess_remote_head(const struct ref *head,
 
 	/* If refs/heads/master could be right, it is. */
 	if (!all) {
-		r = find_ref_by_name(refs, "refs/heads/master");
+		char *name = git_default_branch_name(0);
+
+		r = find_ref_by_name(refs, name);
+		free(name);
 		if (r && oideq(&r->old_oid, &head->old_oid))
 			return copy_ref(r);
 	}