Re: [PATCH v5 1/2] refs.c: optimize check_refname_component()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 3, 2014, 18:08 UTC
Message-ID: <xmqqzjhtx3vr.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1401690015-19191-1-git-send-email-dturner@twitter.com>

David Turner <dturner@twopensource.com> writes:

Show 9 quoted lines

>  static int check_refname_component(const char *refname, int flags)
>  {
>  	const char *cp;
>  	char last = '\0';
>  
>  	for (cp = refname; ; cp++) {
> -		char ch = *cp;
> -		if (ch == '\0' || ch == '/')
> +		unsigned char ch = (unsigned char) *cp;

Hmph, this cast bothers me.  I am fine with either of these two, though.

	int ch = *cp & 0377;
        unsigned char ch = *((unsigned char *)cp);

Show 44 quoted lines

> +		unsigned char disp = refname_disposition[ch];
> +		switch(disp) {
> +		case 1:
> +			goto out;
> +		case 2:
> +			if (last == '.')
> +				return -1; /* Refname contains "..". */
> +			break;
> +		case 3:
> +			if (last == '@')
> +				return -1; /* Refname contains "@{". */
>  			break;
> -		if (bad_ref_char(ch))
> -			return -1; /* Illegal character in refname. */
> -		if (last == '.' && ch == '.')
> -			return -1; /* Refname contains "..". */
> -		if (last == '@' && ch == '{')
> -			return -1; /* Refname contains "@{". */
> +		case 4:
> +			return -1;
> +		}
>  		last = ch;
>  	}
> +out:
>  	if (cp == refname)
>  		return 0; /* Component has zero length. */
>  	if (refname[0] == '.') {
> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
> index c289322..1571176 100755
> --- a/t/t5511-refspec.sh
> +++ b/t/t5511-refspec.sh
> @@ -5,7 +5,6 @@ test_description='refspec parsing'
>  . ./test-lib.sh
>  
>  test_refspec () {
> -
>  	kind=$1 refspec=$2 expect=$3
>  	git config remote.frotz.url "." &&
>  	git config --remove-section remote.frotz &&
> @@ -84,4 +83,9 @@ test_refspec push 'refs/heads/*/*/for-linus:refs/remotes/mine/*' invalid
>  test_refspec fetch 'refs/heads/*/for-linus:refs/remotes/mine/*'
>  test_refspec push 'refs/heads/*/for-linus:refs/remotes/mine/*'
>  
> +good=$(echo -n '\0377')

I think we avoid "echo -n" and use "printf" to be portable across different echo implementations.

Use of \0377, which most likely to be just half-a-character, does not feel a particularly good example, by the way.

> +test_refspec fetch "refs/heads/${good}"
> +bad=$(echo -n '\011')

Likewise.

> +test_refspec fetch "refs/heads/${bad}"				invalid
> +
>  test_done