tboegi@web.de writes:
From: Torsten Bögershausen <redacted>
When core.autocrlf is set to false, and no attributes are set, the file
is treated as binary.
This, and also on the title, I know by "binary" you mean "no
conversion is attempted", and it is the word used in the code around
there, but it still makes my heart skip a beat every time I read
this sentence--it is not like we do not treat the contents as text
after all.
In any case, I take the above sentence the statement of the fact,
describing how the world currently is, not declaring a new world
order.
Simplify the logic and remove duplicated code when dealing with
(crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) by
setting crlf_action=CRLF_BINARY already in convert_attrs().
I looked at all the places where CRLF_BINARY is checked. The ones
that are in this patch are clearly where "Is it BINARY?" and "Is
AUTO_CRLF_FALSE and CRLF_GUESS both true?" mean the same thing, so
this is a correct simplification to these places.
It is not easy to see what the effect of this change to the other
places that use CRLF_BINARY, though.
* output_eol() used to return EOL_UNSET when auto_crlf is not in
effect and CRLF_GUESS is. The function will see CRLF_BINARY with
this patch in such a case, and returns EOL_UNSET. So there is no
change to the function and its callers.
* convert_attrs() has "If BINARY don't do anything and return".
Will the patch change behaviour for the "not-autocrlf,
CRLF_GUESS" case in this codepath? I think ca->crlf_action used
to be left as CRLF_GUESS here before the patch, and now by the
time the control flow reaches here it is already CRLF_BINARY.
Would it affect the callers, and if so how?
* get_convert_attr_ascii() would change the behaviour, right? It
runs convert_attrs(), and with this change a path without
attribute when autocrlf is not in effect would get BINARY and
would show "-text", while the code before this change would give
an empty string. Am I misreading the code, or is the change
intended?
Thanks.
On 02/08/2016 07:27 PM, Junio C Hamano wrote:
tboegi@web.de writes:
quoted
From: Torsten Bögershausen <redacted>
When core.autocrlf is set to false, and no attributes are set, the file
is treated as binary.
This, and also on the title, I know by "binary" you mean "no
conversion is attempted", and it is the word used in the code around
there, but it still makes my heart skip a beat every time I read
this sentence--it is not like we do not treat the contents as text
after all.
In any case, I take the above sentence the statement of the fact,
describing how the world currently is, not declaring a new world
order.
The word binary is indeed not ideal here: no eol conversion is done.
(And the commit message could use "-text")
quoted
Simplify the logic and remove duplicated code when dealing with
(crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE) by
setting crlf_action=CRLF_BINARY already in convert_attrs().
I looked at all the places where CRLF_BINARY is checked. The ones
that are in this patch are clearly where "Is it BINARY?" and "Is
AUTO_CRLF_FALSE and CRLF_GUESS both true?" mean the same thing, so
this is a correct simplification to these places.
It is not easy to see what the effect of this change to the other
places that use CRLF_BINARY, though.
* output_eol() used to return EOL_UNSET when auto_crlf is not in
effect and CRLF_GUESS is. The function will see CRLF_BINARY with
this patch in such a case, and returns EOL_UNSET. So there is no
change to the function and its callers.
OK
* convert_attrs() has "If BINARY don't do anything and return".
Will the patch change behaviour for the "not-autocrlf,
CRLF_GUESS" case in this codepath? I think ca->crlf_action used
to be left as CRLF_GUESS here before the patch, and now by the
time the control flow reaches here it is already CRLF_BINARY.
Would it affect the callers, and if so how?
Not sure if I fully understand the question:
The old CRLF_GUESS could mean (a) core.autocrlf=true,
(b) core.autocrlf=input or (c) core.autocrlf=false.
The callers had to look at the core.autocrlf them self.
This patch removes (c), the next (or over next) (a) and (b)
if (ca->crlf_action == CRLF_GUESS && auto_crlf == AUTO_CRLF_FALSE)
ca->crlf_action = CRLF_BINARY;
}
The next patch 6/7 removes "GUESS" completely.
* get_convert_attr_ascii() would change the behaviour, right? It
runs convert_attrs(), and with this change a path without
attribute when autocrlf is not in effect would get BINARY and
would show "-text", while the code before this change would give
an empty string. Am I misreading the code, or is the change
intended?
Thanks.
(That happened in my first (not published) version,
today we have this:)
struct conv_attrs {
struct convert_driver *drv;
enum crlf_action attr_action; /* What attr says */
enum crlf_action crlf_action; /* When no attr is set, use core.autocrlf */
int ident;
};
The idea is (after this and the next commit 6/7) is
that once we had run convert_attrs() there is a clear picture
what should be done with the eols, and it is stored in crlf_action.
No further check with core.autocrlf in any calling party should be needed.
In a future series it should be possible to set
*text=auto
*eol=clrf
to behave exactly as if core.autocrlf=true.
But that is a change in behavior, and needs a separate series.
This series should not change the behavior, so a critical review is appreciated.
Does this answers the questions ?
Is a local amend possible ?