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

phillip.wood123@xxxxxxxxx · Fri, 13 Sep 2024 16:09:58 +0100

On 11/09/2024 18:03, Junio C Hamano wrote:
"Phillip Wood via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.

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

Best Wishes

Phillip

  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)







