remote: branch setting fixes

From: Phillip Wood <[email protected]>

To replace the list of branches to be fetched "git remote set-branches" first removes the fetch refspecs for the remote and then creates a new set of fetch refspecs based and the branches passed on the commandline. When deleting the existing refspecs git_config_set_multivar_gently() will return a non-zero result if there was nothing to delete. Unfortunately the calling code treats that as an error and bails out rather than setting up the new branches. Fix this by not treating a return value of CONFIG_NOTHING_SET as an error.

t/t5505-remote.sh +3 -1

index 08424e878e1..cfbd6139e00 100755
@@ -1131,7 +1131,9 @@ test_expect_success 'remote set-branches' '
 	+refs/heads/next:refs/remotes/scratch/next
 	+refs/heads/seen:refs/remotes/scratch/seen
 	EOF
-
+	cat  <<-\EOF >expect.replace-missing &&
+	+refs/heads/topic:refs/remotes/scratch/topic
+	EOF
 	git clone .git/ setbranches &&
 	(
 		cd setbranches &&

From: Phillip Wood <[email protected]>

To replace the list of branches to be fetched "git remote set-branches" first removes the fetch refspecs for the remote and then creates a new set of fetch refspecs based and the branches passed on the commandline.

When deleting the existing refspecs git_config_set_multivar_gently() will return a non-zero result if there was nothing to delete. Unfortunately the calling code treats that as an error and bails out rather than setting up the new branches. Fix this by not treating a return value of CONFIG_NOTHING_SET as an error.

Reported-by Han Jiang <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
 builtin/remote.c  |  8 ++++++--
 t/t5505-remote.sh | 14 +++++++++++---
 2 files changed, 17 insertions(+), 5 deletions(-)

builtin/remote.c +0 -0

index d1f9292ed2b..794396ba02f 100644
@@ -1567,8 +1567,12 @@ static int update(int argc, const char **argv, const char *prefix)
 
 static int remove_all_fetch_refspecs(const char *key)

t/t5505-remote.sh +2 -0

+	+refs/heads/topic:refs/remotes/scratch/topic
+	EOF
 	git clone .git/ setbranches &&
 	(
 		cd setbranches &&

From: Phillip Wood <[email protected]>

If the existing fetch refspecs cannot be removed when replacing the set of branches to fetch with "git remote set-branches" the command silently fails. Add an error message to tell the user what when wrong.

Signed-off-by Phillip Wood <[email protected]>

---
 builtin/remote.c | 1 +
 1 file changed, 1 insertion(+)

builtin/remote.c +0 -0

index 794396ba02f..4dbf7a4c506 100644

Quoted 16 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

If the existing fetch refspecs cannot be removed when replacing the set of branches to fetch with "git remote set-branches" the command silently fails. Add an error message to tell the user what when wrong.

Signed-off-by Phillip Wood <[email protected]>

---
 builtin/remote.c | 1 +
 1 file changed, 1 insertion(+)

builtin/remote.c +0 -0

index 794396ba02f..4dbf7a4c506 100644

builtin/remote.c static int set_remote_branches(const char *remotename, const char **branches, · line 1609

Quoted 7 lines

builtin/remote.c +1 -0

@@ -1603,6 +1603,7 @@ static int set_remote_branches(const char *remotename, const char **branches,
 	}
 
 	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {
+		error(_("could not remove existing fetch refspec"));
 		strbuf_release(&key);
 		return 1;
 	}

It is a minor point, but would it help to say what we tried to remove (e.g. "from remote X") or is it too obvious to the end user in the context they get this error?

The reason why I had the above question was because inserting error() before strbuf_release(&key) looked curious and I initially suspected that it was because key was used in the error message somehow, but it turns out that is not the case at all.

IOW, I would have expected something more like this:

I don't think we want to return the error code from error(), do we? set_branches() is wired up as a subcommand, so we'd ultimately do exit(-1) instead of exit(1) if we returned the error() code here.

Quoted 3 lines · Patrick Steinhardt

I don't think we want to return the error code from error(), do we? set_branches() is wired up as a subcommand, so we'd ultimately do exit(-1) instead of exit(1) if we returned the error() code here.

Hmph, I thought there was somebody doing !! to canonicalize the return value to exit status in the call chain.

... goes and looks again ...

After finding the subcommand in fn, cmd_remote() ends with

	if (fn) {
		return !!fn(argc, argv, prefix);
	} else {
		...
		return !!show_all();
	}

builtin/remote.c static int set_remote_branches(const char *remotename, const char **branches, · line 1609

Quoted 5 lines · Phillip Wood via GitGitGadget

  	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {

builtin/remote.c +1 -0

+		error(_("could not remove existing fetch refspec"));
  		strbuf_release(&key);
  		return 1;
  	}

It is a minor point, but would it help to say what we tried to remove (e.g. "from remote X") or is it too obvious to the end user in the context they get this error?

The reason why I had the above question was because inserting error() before strbuf_release(&key) looked curious and I initially suspected that it was because key was used in the error message somehow, but it turns out that is not the case at all.

IOW, I would have expected something more like this:

  	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {
  		strbuf_release(&key);

(quoted diff) +2 -0

+		return error(_("failed to remove fetch refspec from '%s'"),
+				remotename);
 
  	}

Quoted 9 lines · Junio C Hamano

builtin/remote.c static int set_remote_branches(const char *remotename, const char **branches, · line 1609

Quoted 5 lines · Phillip Wood via GitGitGadget

  	if (!add_mode && remove_all_fetch_refspecs(key.buf)) {

builtin/remote.c +1 -0

+		error(_("could not remove existing fetch refspec"));
  		strbuf_release(&key);
  		return 1;
  	}

It is a minor point, but would it help to say what we tried to remove (e.g. "from remote X") or is it too obvious to the end user in the context they get this error?

The user has to give the remote name on the command line so I think it should be obvious to the user.

From: Phillip Wood <[email protected]>

Store the list of branches to track in a ’struct strvec' instead of a 'struct string_list'. This in preparation for the next commit where it will be convenient to have them stored in a NULL terminated array. This means that we now duplicate the strings when storing them but the overhead is not significant.

Signed-off-by Phillip Wood <[email protected]>

---
 builtin/remote.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

From: Phillip Wood <[email protected]>

Store the list of branches to track in a ’struct strvec' instead of a 'struct string_list'. This in preparation for the next commit where it

will be convenient to have them stored in a NULL terminated array. This means that we now duplicate the strings when storing them but the overhead is not significant.

Review

Quoted 12 lines · Phillip Wood via GitGitGadget

(quoted diff) +12 -0

+static int check_branch_names(const char **branches)
+{
+	int ret = 0;
+
+	for (const char **b = branches; *b; b++) {
+		if (check_refname_format(*b, REFNAME_ALLOW_ONELEVEL |
+						REFNAME_REFSPEC_PATTERN))
+			ret = error(_("invalid branch name '%s'"), *b);
+	}
+
+	return ret;
+}

This implementation is inconsistent with what "git branch new HEAD" uses to check the validity of "new", which is in this call chain:

    builtin/branch.c:cmd_branch()
    -> branch.c:create_branch()
       -> branch.c:validate_new_branchname()
          -> branch.c:validate_branchname()
             -> object-name.c:strbuf_check_branch_ref()

At least, we should prepend "refs/heads/" to *b, so that we can reject "refs/heads/HEAD". The authoritative logic in the above however may further evolve, and we need to make sure that these two checks from drifting away from each other over time. We probably should refactor the leaf function in the above call chain so that both places can use it (the main difference is that you allow '*' in yours when calling check_refname_format()).

Side note: we *should* lose "strbuf_" from its name, as it is
not about string manipulation but the "strbuf'-ness of the function is merely that as the side effect of checking it computes a full refname and it happens to use strbuf as a mechanism to return it.

Something like the patch attached at the end.

Agreed. It's also kind of curious that the function lives in "object-name.c" and not in "refs.c".

"Phillip Wood via GitGitGadget" <[email protected]> writes:

The authoritative logic in the above
however may further evolve, and we need to make sure that these two checks from drifting away from each other over time. We probably should refactor the leaf function in the above call chain so that both places can use it (the main difference is that you allow '*' in yours when calling check_refname_format()).

Side note: we *should* lose "strbuf_" from its name, as it is
not about string manipulation but the "strbuf'-ness of the function is merely that as the side effect of checking it computes a full refname and it happens to use strbuf as a mechanism to return it.

Something like the patch attached at the end.

Review static int add(int argc, const char **argv, const char *prefix) · line 221

Quoted 10 lines

  static const char mirror_advice[] =
  N_("--mirror is dangerous and deprecated; please\n"
     "\t use --mirror=fetch or --mirror=push instead");

(quoted diff) +3 -0

@@ -203,6 +216,9 @@ static int add(int argc, const char **argv, const char *prefix)
  	if (!valid_remote_name(name))
  		die(_("'%s' is not a valid remote name"), name);
 
+	if (check_branch_names(track.v))
+		exit(128);
+

Seeing that the loop in check_branch_names() is brand new and you could have iterated over a string-list just as easily, I somehow doubt that step [3/4] was fully warranted.

Review static int set_remote_branches(const char *remotename, const char **branches, · line 1621

Quoted 6 lines

(quoted diff) +2 -0

@@ -1601,6 +1617,9 @@ static int set_remote_branches(const char *remotename, const char **branches,
  		exit(2);
  	}
 
+	if (check_branch_names(branches))
+		exit(128);

But here you are already passed "const char *branches[]" to this caller, and it would be hassle to turn it into string_list, so [3/4] is fine after all.

  object-name.h |  2 ++
  object-name.c | 23 +++++++++++++++++++++--
  2 files changed, 23 insertions(+), 2 deletions(-)

object-name.h +2 -0

index 8dba4a47a4..fa70d42044 100644
@@ -130,4 +130,6 @@ struct object *repo_peel_to_type(struct repository *r,
  /* used when the code does not know or care what the default abbrev is */
  #define FALLBACK_DEFAULT_ABBREV 7
 
+/* Check if "name" is allowed as a branch */
+int valid_branch_name(const char *name, int allow_wildcard);
  #endif /* OBJECT_NAME_H */

object-name.c +21 -2

index 09c1bd93a3..e3bed5a664 100644
@@ -1747,7 +1747,8 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
  	strbuf_add(sb, name + used, len - used);
  }
 
-int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+static int full_ref_from_branch_name_internal(struct strbuf *sb, const char *name,
+					      int crf_flags)
  {
  	if (startup_info->have_repository)
  		strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);

@@ -1766,7 +1767,25 @@ int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
  	    !strcmp(sb->buf, "refs/heads/HEAD"))
  		return -1;
 
-	return check_refname_format(sb->buf, 0);
+	return check_refname_format(sb->buf, crf_flags);
+}
+
+/* NEEDSWORK: rename this to full_ref_from_branch_name */
+int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
+{
+	return full_ref_from_branch_name_internal(sb, name, 0);
+}
+
+int valid_branch_name(const char *name, int allow_wildcard)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int ret;
+	int flags;
+
+	flags = allow_wildcard ? REFNAME_REFSPEC_PATTERN : 0;
+	ret = full_ref_from_branch_name_internal(&sb, name, flags);
+	strbuf_release(&sb);
+	return ret;
  }
 
  void object_context_release(struct object_context *ctx)

Thanks for the patch, I'll re-roll based on that. I wonder if we really want to support "@{-N}" when setting remote tracking branches though - should we be using INTERPRET_BRANCH_REMOTE instead when calling strbuf_branchname()?

Quoted 4 lines · [email protected] writes:

Thanks for the patch, I'll re-roll based on that. I wonder if we really want to support "@{-N}" when setting remote tracking branches though - should we be using INTERPRET_BRANCH_REMOTE instead when calling strbuf_branchname()?

Perhaps. Users try to use "-" in surprising places, though ;-)

... Given that "git remote
set-branches" has only ever supported "real" branch names and patterns on the command line and no-one has complained I wonder if we're better off doing something like

	if (strbuf_check_branch_ref(&buf, branch_name) ||
	    strcmp(buf.buf + 11, branch_name))
		error(_("invalid branch name '%s'", branch_name));

where the "buf.buf + 11" skips "refs/heads/"