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

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

Steffen Prohaska [off-list ref] writes:

quoted

This repaces v9 of the patch and should be applied.
It contains modifications as suggested by Junio in
[ref]

    Steffen

-- >8 --

This v9 replaces itself, ah it repaces ;-)

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

quoted

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