Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2013, 16:44 UTC
Message-ID: <7vli96p34f.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130329130230.GA25861@lanh>

Duy Nguyen <pclouds@gmail.com> writes:

Show 7 quoted lines

>> So we would want to do any adjustment to the fix when we merge up to
>> maint.
>
> OK. Then Junio, you may need to resolve the conflict with something
> like this. Originally match_basename uses fnmatch, not wildmatch. But
> using wildmatch there too should be fine, now that both
> match_{base,path}name share fnmatch_icase_mem().

Thanks.

The result still smells somewhat funny, though.

fnmatch_icase_mem() is meant to be a wrapper of fnmatch_icase() for counted strings and its matching semantics should be the same as fnmatch_icase().

With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but fnmatch_icase() still calls into fnmatch().

The latter's flags are meant to be taken from FNM_* family, but the former takes flags from WM_* family of bits, no?

I think you are running with USE_WILDMATCH which may make the differences harder to notice, but the name fnmatch_icase_mem() that is not in the same family as fnmatch but is from the wildmatch() family smells like an accident waiting to happen.

I tend to think in the longer term it may be a good idea to build with USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so in the end this may not matter that much, but before that happens, soon after we merge the regression fix with this merge-fix, we may want to update the codebase as if we applied a series that were based on 'maint' as you suggested, i.e. using raw wildmatch() consistently in the match_{base,path}name() codepath.

Opinions?

Show 27 quoted lines

>
> -- 8< --
> diff --git a/dir.c b/dir.c
> index 73a08af..84744df 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -81,7 +81,9 @@ static int fnmatch_icase_mem(const char *pattern, int patternlen,
>  		use_str = str_buf.buf;
>  	}
>  
> -	match_status = fnmatch_icase(use_pat, use_str, flags);
> +	if (ignore_case)
> +		flags |= WM_CASEFOLD;
> +	match_status = wildmatch(use_pat, use_str, flags, NULL);
>  
>  	strbuf_release(&pat_buf);
>  	strbuf_release(&str_buf);
> @@ -564,7 +566,7 @@ int match_pathname(const char *pathname, int pathlen,
>  
>  	return fnmatch_icase_mem(pattern, patternlen,
>  				 name, namelen,
> -				 FNM_PATHNAME) == 0;
> +				 WM_PATHNAME) == 0;
>  }
>  
>  /*
> -- 8< --