From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sun, 15 Apr 2007 15:54:13 GMT
Subject: Re: [PATCH 2/3] Define 'crlf' attribute.
Message-ID: <Pine.LNX.4.64.0704151141090.18846@racer.site>
In-Reply-To: <7vps68d4wf.fsf@assigned-by-dhcp.cox.net>

Hi,

On Fri, 13 Apr 2007, Junio C Hamano wrote:

> diff --git a/attr.c b/attr.c
> index bdbc4a3..d35ae9e 100644
> --- a/attr.c
> +++ b/attr.c
> [...]
> +
> +int git_path_is_binary(const char *path)
> +{
> +	struct git_attr_check attr_binary_check;
> +
> +	setup_binary_check(&attr_binary_check);
> +	return (!git_checkattr(path, 1, &attr_binary_check) &&
> +		(0 < attr_binary_check.isset));
> +}

This function is not declared in a header file, and ...

> diff --git a/convert.c b/convert.c
> index 898bfe3..20c744a 100644
> --- a/convert.c
> +++ b/convert.c
> [...]
> +
> +static int git_path_is_binary(const char *path)
> +{
> +	struct git_attr_check attr_crlf_check;
> +
> +	setup_crlf_check(&attr_crlf_check);
> +
> +	/*
> +	 * If crlf is not mentioned, default to autocrlf;
> +	 * disable autocrlf only when crlf attribute is explicitly
> +	 * unset.
> +	 */
> +	return (!git_checkattr(path, 1, &attr_crlf_check) &&
> +		(0 == attr_crlf_check.isset));
> +}

... here you have a function of the _same_ name, which does something 
subtly different: it checks for "crlf" instead of "binary".

Perhaps you wanted the change to attr.c at a later stage, and the function 
in convert.c should be renamed to "path_wants_crlf()"?

[Reading 3/3 revealed that indeed, the 18 lines in attr.c were unwanted.]

Ciao,
Dscho