From: Junio C Hamano <gitster@pobox•com>
To: tboegi@web•de
Cc: git@vger•kernel.org
Subject: Re: [PATCH 1/1] convert.c: correct attr_action
Date: Mon, 22 Feb 2016 00:20:17 -0800 [thread overview]
Message-ID: <xmqq8u2d88ce.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <1456117898-30357-1-git-send-email-tboegi@web.de> (tboegi@web.de's message of "Mon, 22 Feb 2016 06:11:38 +0100")
tboegi@web•de writes:
> 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?
next prev parent reply other threads:[~2016-02-22 8:20 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <Message-Id=1453558101-6858-1-git-send-email-tboegi@web.de>
2016-01-24 7:55 ` [PATCH v2] t0027: Add tests for get_stream_filter() tboegi
2016-01-27 6:34 ` Junio C Hamano
2016-01-27 9:05 ` Torsten Bögershausen
2016-01-27 15:15 ` [PATCH v1 1/6] " tboegi
2016-02-02 16:53 ` tboegi
2016-02-02 21:18 ` Junio C Hamano
2016-02-02 16:53 ` [PATCH v1 2/6] convert.c: Remove path when not needed tboegi
2016-02-02 21:32 ` Junio C Hamano
2016-02-02 16:53 ` [PATCH v1 3/6] convert.c: Remove input_crlf_action() tboegi
2016-02-02 21:44 ` Junio C Hamano
2016-02-02 16:53 ` [PATCH v1 4/6] convert.c: Use text_eol_is_crlf() tboegi
2016-02-02 16:53 ` [PATCH v1 5/6] convert: auto_crlf=false and no attributes set: same as binary tboegi
2016-02-02 16:53 ` [PATCH v1 6/6] convert.c: Refactor crlf_action tboegi
2016-02-04 17:49 ` [PATCH v2 1/7] t0027: Add tests for get_stream_filter() tboegi
2016-02-04 19:52 ` Junio C Hamano
2016-02-04 17:49 ` [PATCH v2 2/7] convert.c: remove unused parameter 'path' tboegi
2016-02-04 17:49 ` [PATCH v2 3/7] convert.c: Remove input_crlf_action() tboegi
2016-02-04 17:49 ` [PATCH v2 4/7] convert.c: Use text_eol_is_crlf() tboegi
2016-02-04 20:13 ` Junio C Hamano
2016-02-04 17:49 ` [PATCH v2 5/7] convert: auto_crlf=false and no attributes set: same as binary tboegi
2016-02-04 17:49 ` [PATCH v2 6/7] convert.c: Refactor crlf_action tboegi
2016-02-04 17:50 ` [PATCH v2 7/7] convert.c: simplify text_stat tboegi
2016-02-04 20:37 ` Junio C Hamano
2016-02-05 16:13 ` [PATCH v3 1/7] t0027: Add tests for get_stream_filter() tboegi
2016-02-08 17:59 ` Junio C Hamano
2016-02-05 16:13 ` [PATCH v3 2/7] convert.c: remove unused parameter 'path' tboegi
2016-02-05 16:13 ` [PATCH v3 3/7] convert.c: Remove input_crlf_action() tboegi
2016-02-05 16:13 ` [PATCH v3 4/7] convert.c: use text_eol_is_crlf() tboegi
2016-02-05 16:13 ` [PATCH v3 5/7] convert: auto_crlf=false and no attributes set: same as binary tboegi
2016-02-08 18:27 ` Junio C Hamano
2016-02-09 14:34 ` Torsten Bögershausen
2016-02-09 18:06 ` Junio C Hamano
2016-02-05 16:13 ` [PATCH v3 6/7] convert.c: refactor crlf_action tboegi
2016-02-05 16:13 ` [PATCH v3 7/7] convert.c: simplify text_stat tboegi
2016-02-10 16:24 ` [PATCH v4 1/6] t0027: add tests for get_stream_filter() tboegi
2016-02-10 16:24 ` [PATCH v4 2/6] convert.c: remove unused parameter 'path' tboegi
2016-02-10 16:24 ` [PATCH v4 3/6] convert.c: remove input_crlf_action() tboegi
2016-02-10 16:24 ` [PATCH v4 4/6] convert.c: use text_eol_is_crlf() tboegi
2016-02-10 16:24 ` [PATCH v4 5/6] convert.c: refactor crlf_action tboegi
2016-02-10 16:24 ` [PATCH v4 6/6] convert.c: simplify text_stat tboegi
2016-02-22 5:11 ` [PATCH 1/1] convert.c: correct attr_action tboegi
2016-02-22 5:34 ` Eric Sunshine
2016-02-22 8:04 ` Junio C Hamano
2016-02-22 8:20 ` Junio C Hamano [this message]
2016-02-23 5:26 ` Torsten Bögershausen
2016-02-23 17:07 ` [PATCH v2 " tboegi
2016-02-23 20:52 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq8u2d88ce.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox