From: phillip.wood123@gmail.com
Date: Fri, 13 Sep 2024 15:09:58 GMT
Subject: Re: [PATCH 4/4] remote: check branch names
Message-ID: <4915a1ba-eda9-435b-b615-4f78c7fe25f7@gmail.com>
In-Reply-To: <xmqqfrq686n5.fsf@gitster.g>

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