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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 6, 2008, 19:42 UTC
Message-ID: <7vk5lhyir6.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <12022971582185-git-send-email-prohaska@zib.de>

Steffen Prohaska <prohaska@zib.de> writes:

Show 7 quoted lines

> 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 ;-)

> 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."

---------------------------------------------------------------- [proposed change to the commit log message]

--- /var/tmp/1 2008-02-06 11:36:28.000000000 -0800 +++ /var/tmp/2 2008-02-06 11:36:19.000000000 -0800

@@ -49,28 +49,29 @@
     leave core.autocrlf unset, so users will not see warnings unless
     they deliberately chose to activate the autocrlf mechanism.
     
-    The safecrlf mechanism's details depend on the git command.  If
-    safecrlf is active (not false), files in the work tree must not
-    be modified in an irreversible way without giving the user a
-    chance to backup the original file.  However, for read-only
-    operations that do not modify files in the work tree git should
-    not print annoying warnings.
-    
-    git apply behaves as requested by by safecrlf.  Even though git
-    apply writes changes directly back to the work tree, we assume
-    that the user still can get the original files back by checking
-    out from the index or HEAD.  git apply is about applying patches,
-    which means we are talking about "text" and therefore it is
-    safe to write the results directly back to the work tree even
-    if CRLFs were converted to LFs.
-    
-    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.
+    The safecrlf mechanism's details depend on the git command.  The
+    general principles when safecrlf is active (not false) are:
     
-    git blame will never warn nor fail because it never writes to the
-    work tree.
+     - we warn/error out if files in the work tree can modified in an
+       irreversible way without giving the user a chance to backup the
+       original file.
+    
+     - for read-only operations that do not modify files in the work tree
+       we do not not print annoying warnings.
+    
+    There are exceptions.  Even though...
+    
+     - "git add" itself does not touch the files in the work tree, the
+       next checkout would, so the safety triggers;
+    
+     - "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;
+    
+     - "git diff" itself does not touch the files in the work tree, it is
+       often run to inspect the changes you intend to next "git add".  To
+       catch potential problems early, safety triggers.
     
     The concept of a safety check was originally proposed in a similar
     way by Linus Torvalds.  Thanks to Dimitry Potapov for insisting

----------------------------------------------------------------
[adding the general rule and per-command behaviour to the doc]

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2ffe1fb..84ec962 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -137,7 +137,22 @@ If `core.safecrlf` is set to "true" or "warn", git verifies if
 the conversion is reversible for the current setting of
 `core.autocrlf`.  For "true", git rejects irreversible
 conversions; for "warn", git only prints a warning but accepts
-an irreversible conversion.
+an irreversible conversion.  The safety triggers to prevent such
+a conversion done to the files in the work tree, but there are a
+few exceptions.  Even though...
+
+- "git add" itself does not touch the files in the work tree, the
+  next checkout would, so the safety triggers;
+
+- "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;
+
+- "git diff" itself does not touch the files in the work tree, it is
+  often run to inspect the changes you intend to next "git add".  To
+  catch potential problems early, safety triggers.
+
 
 `ident`
 ^^^^^^^