Re: [PATCH 4/4] remote: check branch names

[Junio C Hamano <gitster@pobox.com>]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

>  static const char mirror_advice[] =
>  N_("--mirror is dangerous and deprecated; please\n"
>     "\t use --mirror=fetch or --mirror=push instead");
> @@ -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.

> @@ -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(-)

diff --git i/object-name.h w/object-name.h
index 8dba4a47a4..fa70d42044 100644
--- i/object-name.h
+++ w/object-name.h
@@ -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 */
diff --git i/object-name.c w/object-name.c
index 09c1bd93a3..e3bed5a664 100644
--- i/object-name.c
+++ w/object-name.c
@@ -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)