Re: [PATCH] convert: clarify line ending conversion warning

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 4, 2022, 16:53 UTC
Message-ID: <xmqqtub8rdrw.fsf@gitster.g>
In-Reply-To: <20220404055151.160184-1-alexhenrie24@gmail.com>

Alex Henrie <alexhenrie24@gmail.com> writes:

Show 11 quoted lines

> Rephrase the warning to be clear that line endings have not been changed
> in the working directory but will be changed on the next checkout, and
> explicitly say which line endings the file currently has in the working
> directory.
>
> Example commands to trigger the warning on Linux:
>
> git config core.autocrlf true
> echo 'Hello world!' > hello.txt
> git add hello.txt
> git commit -m "Add hello.txt"

While the "example" does not hurt, because the log message is not executable, it would not help to its potential unless you add its expected output to go with it.

    On a platform whose native line endings are not CRLF
    (e.g. Linux), the "git add" step in the sequence triggers the
    waring in question.

    $ git config core.autocrlf true
    $ echo 'Hello world!' >hello.txt
    $ git add hello.txt
    warning: LF will be replaced by CRLF in hello.txt
    The file will have its original line endings in your working directory

or something like that.

I think the recent trend is to enclose end-user supplied strings (like misspelt option names, arguments to options, and pathnames) inside single quote, so

    warning: LF will be replaced by CRLF in 'hello.txt'.

would probably be a good idea, on top of what you are aiming for. Also, in a multi-sentence warning message like this, I do not think it makes sense to omit the end-of-sentence full-stop.

Show 17 quoted lines

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> diff --git a/convert.c b/convert.c
> index 8e39731efb..b024d74222 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -195,9 +195,10 @@ static void check_global_conv_flags_eol(const char *path,
>  		if (conv_flags & CONV_EOL_RNDTRP_DIE)
>  			die(_("CRLF would be replaced by LF in %s"), path);
>  		else if (conv_flags & CONV_EOL_RNDTRP_WARN)
> -			warning(_("CRLF will be replaced by LF in %s.\n"
> -				  "The file will have its original line"
> -				  " endings in your working directory"), path);
> +			warning(_("CRLF will be replaced by LF in %s the next"
> +				  " time you check it out.\n"
> +				  "For now, the file still has CRLF line"
> +				  " endings in your working directory."), path);

I have mixed feelings with this change, even though I agree that the original is not good. The first sentence of the updated text already says that right now, the file ends with CRLF, and that the conversion happen the next time you check out the file to the working tree. And that makes "For now ... still" totally redundant.

Perhaps a single sentence, nothing more than

	warning: in '%s', CRLF will be replaced by LF the next time
	you check it out

is sufficient?  I dunno.

Thanks.