Re: [PATCH 1/1] convert.c: correct attr_action

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2016, 08:20 UTC
Message-ID: <xmqq8u2d88ce.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1456117898-30357-1-git-send-email-tboegi@web.de>

tboegi@web.de writes:

Show 13 quoted lines

> diff --git a/convert.c b/convert.c
> index 18af685..0bc32ec 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -708,7 +708,7 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
>  	const char *value = check->value;
>  
>  	if (ATTR_TRUE(value))
> -		return text_eol_is_crlf() ? CRLF_TEXT_CRLF : CRLF_TEXT_INPUT;
> +		return CRLF_TEXT;
>  	else if (ATTR_FALSE(value))
>  		return CRLF_BINARY;
>  	else if (ATTR_UNSET(value))

Hmph, this function has exactly two callers, and they call it like this:

	if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
		enum eol eol_attr;
		ca->crlf_action = git_path_check_crlf(ccheck + 4);
		if (ca->crlf_action == CRLF_UNDEFINED)
			ca->crlf_action = git_path_check_crlf(ccheck + 0);
		ca->attr_action = ca->crlf_action;

where ccheck+0 refers to "crlf" and ccheck+4 refers to "text".

So, we say "first check 'text' attribute, and if it is true we say CRLF_TEXT and stop." We also say "first check 'text', and if it is not there, the first call returns undef, in which case we check 'crlf' attribute, and if it is true we say CRLF_TEXT".

And ca->attr_action retains this value.

However, immediately after this assignment to ca->attr_action, we have this:

		ca->ident = git_path_check_ident(ccheck + 1);
		ca->drv = git_path_check_convert(ccheck + 2);
		if (ca->crlf_action == CRLF_BINARY)
			return;
		eol_attr = git_path_check_eol(ccheck + 3);
		if (eol_attr == EOL_LF)
			ca->crlf_action = CRLF_TEXT_INPUT;
		else if (eol_attr == EOL_CRLF)
			ca->crlf_action = CRLF_TEXT_CRLF;
		ca->attr_action = ca->crlf_action;

We check ident (ccheck+1) and filter (ccheck+2); if the value of ca->crlf_action (which was determined by checking 'text' and then 'crlf') is not CRLF_BINARY, we further check eol (ccheck+3) and update ca->crlf_action based on it. And ca->attr_action gets updated again.

This feels unnecessarily convoluted. Perhaps if you get rid of the early return for CRLF_BINARY, the flow may become a lot clearer, i.e.

	ca->crlf_action = check 'text';
        if (ca->crlf_action == undef)
        	ca->crlf_action = check 'crlf';
	// DO NOT ASSIGN ca->attr_action HERE YET
        ca->ident = check 'ident';
        ca->drv = check 'filter';
        if (crlf_action != CRLF_BINARY) {
                eol_attr = check 'eol';
                if (eol == LF)
                        ca->crlf_action = ...
                else if (eol == EOL_CRLF)
                        ca->crlf_action = ...
	}
        ca->attr_action = ca->crlf_action;

Hmm?