Re: [PATCH 4/4] remote: check branch names
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 11, 2024, 17:03 UTC
- Message-ID
- <xmqqfrq686n5.fsf@gitster.g>
- In-Reply-To
- <dba31245607f85c48947da60fe0955a6ed3e2c43.1726067917.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 12 quoted lines
> +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.
Show 10 quoted lines
> 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.
Show 6 quoted lines
> @@ -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)