[PATCH] config.c: fix regression for core.safecrlf false

[PATCH] config.c: fix regression for core.safecrlf false

[Anthony Sottile]

A regression introduced in 8462ff43e42ab67cecd16fdfb59451a53cc8a945 caused
autocrlf rewrites to produce a warning message despite setting safecrlf=false.

Signed-off-by: Anthony Sottile <asottile@umich.edu>
---
 config.c        |  2 +-
 t/t0020-crlf.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/config.c b/config.c
index fbbf0f8..de24e90 100644
--- a/config.c
+++ b/config.c
@@ -1233,7 +1233,7 @@ static int git_default_core_config(const char *var, const char *value)
 		}
 		eol_rndtrp_die = git_config_bool(var, value);
 		global_conv_flags_eol = eol_rndtrp_die ?
-			CONV_EOL_RNDTRP_DIE : CONV_EOL_RNDTRP_WARN;
+			CONV_EOL_RNDTRP_DIE : 0;
 		return 0;
 	}
 
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 71350e0..5f05698 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -98,6 +98,16 @@ test_expect_success 'safecrlf: git diff demotes safecrlf=true to warn' '
 '
 
 
+test_expect_success 'safecrlf: no warning with safecrlf=false' '
+	git config core.autocrlf input &&
+	git config core.safecrlf false &&
+
+	for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
+	git add allcrlf 2>err &&
+	test_must_be_empty err
+'
+
+
 test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '
 	git config core.autocrlf false &&
 	git config core.safecrlf false &&
-- 
2.7.4

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Torsten Bögershausen]

On Mon, Jun 04, 2018 at 01:17:42PM -0700, Anthony Sottile wrote:
> A regression introduced in 8462ff43e42ab67cecd16fdfb59451a53cc8a945 caused
> autocrlf rewrites to produce a warning message despite setting safecrlf=false.
> 
> Signed-off-by: Anthony Sottile <asottile@umich.edu>
> ---
>  config.c        |  2 +-
>  t/t0020-crlf.sh | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/config.c b/config.c
> index fbbf0f8..de24e90 100644
> --- a/config.c
> +++ b/config.c
> @@ -1233,7 +1233,7 @@ static int git_default_core_config(const char *var, const char *value)
>  		}
>  		eol_rndtrp_die = git_config_bool(var, value);
>  		global_conv_flags_eol = eol_rndtrp_die ?
> -			CONV_EOL_RNDTRP_DIE : CONV_EOL_RNDTRP_WARN;
> +			CONV_EOL_RNDTRP_DIE : 0;
>  		return 0;
>  	}
>  
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> index 71350e0..5f05698 100755
> --- a/t/t0020-crlf.sh
> +++ b/t/t0020-crlf.sh
> @@ -98,6 +98,16 @@ test_expect_success 'safecrlf: git diff demotes safecrlf=true to warn' '
>  '
>  
>  
> +test_expect_success 'safecrlf: no warning with safecrlf=false' '
> +	git config core.autocrlf input &&
> +	git config core.safecrlf false &&
> +
> +	for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
> +	git add allcrlf 2>err &&
> +	test_must_be_empty err
> +'
> +
> +
>  test_expect_success 'switch off autocrlf, safecrlf, reset HEAD' '
>  	git config core.autocrlf false &&
>  	git config core.safecrlf false &&
> -- 
> 2.7.4
> 

Looks good to me, thanks for cleaning my mess.

Acked-By: Torsten Bögershausen <tboegi@web.de>

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Eric Sunshine]

On Mon, Jun 4, 2018 at 4:17 PM, Anthony Sottile <asottile@umich.edu> wrote:
> A regression introduced in 8462ff43e42ab67cecd16fdfb59451a53cc8a945 caused
> autocrlf rewrites to produce a warning message despite setting safecrlf=false.
>
> Signed-off-by: Anthony Sottile <asottile@umich.edu>
> ---
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> @@ -98,6 +98,16 @@ test_expect_success 'safecrlf: git diff demotes safecrlf=true to warn' '
> +test_expect_success 'safecrlf: no warning with safecrlf=false' '
> +       git config core.autocrlf input &&
> +       git config core.safecrlf false &&

I was going to suggest test_config() for these rather than bare
git-config, but I see other tests in this file already use the bare
form, so this is following existing practice.

> +       for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&

Simpler: printf "%s\n" I am all CRLF | append_cr >allcrlf &&

(probably not worth a re-roll)

> +       git add allcrlf 2>err &&
> +       test_must_be_empty err
> +'

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Eric Sunshine]

On Wed, Jun 6, 2018 at 1:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jun 4, 2018 at 4:17 PM, Anthony Sottile <asottile@umich.edu> wrote:
>> +       for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
>
> Simpler: printf "%s\n" I am all CRLF | append_cr >allcrlf &&

Or even simpler:

printf "%s\r\n" I am all CRLF >allcrlf &&

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Anthony Sottile]

On Wed, Jun 6, 2018 at 10:17 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jun 6, 2018 at 1:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Mon, Jun 4, 2018 at 4:17 PM, Anthony Sottile <asottile@umich.edu> wrote:
>>> +       for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
>>
>> Simpler: printf "%s\n" I am all CRLF | append_cr >allcrlf &&
>
> Or even simpler:
>
> printf "%s\r\n" I am all CRLF >allcrlf &&

Yeah, I just copied the line in my test from another test in this file
which was doing a ~similar thing.  My original bug report actually
uses `echo -en ...` to accomplish the same thing.

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Eric Sunshine]

On Wed, Jun 6, 2018 at 1:18 PM, Anthony Sottile <asottile@umich.edu> wrote:
> On Wed, Jun 6, 2018 at 10:17 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, Jun 6, 2018 at 1:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Mon, Jun 4, 2018 at 4:17 PM, Anthony Sottile <asottile@umich.edu> wrote:
>>>> +       for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
>>>
>>> Simpler: printf "%s\n" I am all CRLF | append_cr >allcrlf &&
>>
>> Or even simpler:
>>
>> printf "%s\r\n" I am all CRLF >allcrlf &&
>
> Yeah, I just copied the line in my test from another test in this file
> which was doing a ~similar thing. [...]

Thanks for pointing that out. In that case, it's following existing
practice, thus certainly not worth a re-roll.

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Anthony Sottile]

On Wed, Jun 6, 2018 at 10:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, Jun 6, 2018 at 1:18 PM, Anthony Sottile <asottile@umich.edu> wrote:
>> On Wed, Jun 6, 2018 at 10:17 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Wed, Jun 6, 2018 at 1:15 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>>> On Mon, Jun 4, 2018 at 4:17 PM, Anthony Sottile <asottile@umich.edu> wrote:
>>>>> +       for w in I am all CRLF; do echo $w; done | append_cr >allcrlf &&
>>>>
>>>> Simpler: printf "%s\n" I am all CRLF | append_cr >allcrlf &&
>>>
>>> Or even simpler:
>>>
>>> printf "%s\r\n" I am all CRLF >allcrlf &&
>>
>> Yeah, I just copied the line in my test from another test in this file
>> which was doing a ~similar thing. [...]
>
> Thanks for pointing that out. In that case, it's following existing
> practice, thus certainly not worth a re-roll.

Anything else for me to do here? (sorry! not super familiar with the process)

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Eric Sunshine]

[cc:+torsten]

On Mon, Jun 11, 2018 at 9:46 PM, Anthony Sottile <asottile@umich.edu> wrote:
> On Wed, Jun 6, 2018 at 10:22 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Thanks for pointing that out. In that case, it's following existing
>> practice, thus certainly not worth a re-roll.
>
> Anything else for me to do here? (sorry! not super familiar with the process)

I don't think so. Nothing in the review warranted a re-roll, and
(importantly) Torsten gave his Acked-by:, so the next step is to wait
for Junio to pick up the patch. He's been offline for a bit, so it
might take a some time for him to catch up since the list has been
plenty busy in his absence.

It's a good idea to check the "What's Cooking" summaries Junio sends
to the mailing list once in a while to check the progress of your
patch. If you don't see it show up in the summary within a couple
weeks or so, it wouldn't hurt to ping again (as you did here).

Re: [PATCH] config.c: fix regression for core.safecrlf false

[Torsten Bögershausen]

On Mon, Jun 11, 2018 at 06:46:34PM -0700, Anthony Sottile wrote:
[]
> Anything else for me to do here? (sorry! not super familiar with the process)

Your patch has been picked up by Junio, and is currently merged into the
"pu" branch (proposed updates):

  commit bc8ff8aec33836af3fefe1bcd3f533a1486b793f
  Merge: e69b544a38 6cb09125be
  Author: Junio C Hamano <gitster@pobox.com>
  Date:   Tue Jun 12 10:15:13 2018 -0700

      Merge branch 'as/safecrlf-quiet-fix' into jch
      
      Fix for 2.17-era regression.
      
      * as/safecrlf-quiet-fix:
        config.c: fix regression for core.safecrlf false

From there, it will typically progress into next and master,
unless reviewers come with comments and improvements.
You can watch out for "What's cooking in git" messages here on the list
to follow the progress.

From my experience it will end up in a Git release, but I don't know,
if it will be 2.18 or a later one.