Re: [PATCH] [v9] safecrlf: Add mechanism to warn about irreversible crlf conversions

From: Steffen Prohaska <prohaska@zib.de>
Date: Feb 6, 2008, 20:35 UTC
Message-ID: <E2F063EE-06C5-4B85-8511-5FD4EB6FFEC9@zib.de>
In-Reply-To: <7vk5lhyir6.fsf@gitster.siamese.dyndns.org>

On Feb 6, 2008, at 8:42 PM, Junio C Hamano wrote:

Show 11 quoted lines

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> This repaces v9 of the patch and should be applied.
>> It contains modifications as suggested by Junio in
>> <7vbq6u32mq.fsf@gitster.siamese.dyndns.org>
>>
>>     Steffen
>>
>> -- >8 --
>
> This v9 replaces itself, ah it repaces ;-)

Yeah, I recognized this, too; but was to lazy to send another correction ;)

Show 31 quoted lines

>
>> git diff, git add, and any other command that calls
>> index_fd(..., write_object=1) also behave as requested by
>> safecrlf.  The user still has the chance to save her data before
>> checkout or applying the diff.
>
> "git diff" writes by calling index_fd(..., write_object=1)???
>
> I think warning in "git diff" is probably a good thing to do but
> that is not because it writes.  The warning would trigger only
> when you are comparing what you have in the work tree with
> something else, and if you get such a warning, it means what you
> can potentially commit (i.e. what you have in the work tree) is
> not "autocrlf" safe.  You would get the same warning when you
> later runn "git add" on such a file anyway, but it is nicer to
> catch the potential problem earlier.
>
> I do not know if the above "why this command should behave this
> way with respect to safecrlf" needs to be in the commit log
> message.  But I think that information should be in the
> documentation to help the end users (the end users typically do
> not have access to the commit log of git.git).
>
> The rest of the patch looked sane.  Thanks.
>
> So I'd suggest the following change to the commit log message,
> and then another patch at the end squashed into the
> Documentation/ part.
>
> If you agree with these, you can just say "Ok, amend it like
> so".  Or you can say "Here is an even better replacement."

Ok, amend it as you proposed but also squash in the following ...

-- >8 --

diff --git a/builtin-apply.c b/builtin-apply.c
index d0b3586..3b5618d 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1430,7 +1430,7 @@ static int read_old_data(struct stat *st, const  
char *path, struct strbuf *buf)
  	case S_IFREG:
  		if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
  			return error("unable to open or read %s", path);
-		convert_to_git(path, buf->buf, buf->len, buf, safe_crlf);
+		convert_to_git(path, buf->buf, buf->len, buf, 0);
  		return 0;
  	default:
  		return -1;
-- >8 --

... because ...

[...]

> +
> +- "git apply" to update a text file with a patch does touch the files
> +  in the work tree, but the operation is about text files and CRLF
> +  conversion is about fixing the line ending inconsistencies, so the
> +  safety does not trigger;

... now after I read this I fully understand what you meant before.
But my v9 patch does not match your description without the change
to builtin-apply.c from above.  In v9 the mechanism was still activated
in "git apply".

	Steffen