[PATCH] diff: treat -crlf files as binary

[PATCH] diff: treat -crlf files as binary

[Junio C Hamano]

The manual advertises that setting "crlf" attribute to false marks the
file as binary.  We should pay attention to this condition in addition
to the "do not diff" attribute (i.e. setting "diff" to false) when
deciding not to show the textual diff.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Strictly speaking any change is backward incompatible, and this is
   certainly one, but I do not think of a good use case to depend on the
   previous behaviour, which was reported as a bug by my coworker.

 diff.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/diff.c b/diff.c
index 18fa7a7..258b438 100644
--- a/diff.c
+++ b/diff.c
@@ -1314,36 +1314,46 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)
 static void setup_diff_attr_check(struct git_attr_check *check)
 {
 	static struct git_attr *attr_diff;
+	static struct git_attr *attr_crlf;
 
-	if (!attr_diff) {
+	if (!attr_diff)
 		attr_diff = git_attr("diff", 4);
-	}
+	if (!attr_crlf)
+		attr_crlf = git_attr("crlf", 4);
 	check[0].attr = attr_diff;
+	check[1].attr = attr_crlf;
 }
 
 static void diff_filespec_check_attr(struct diff_filespec *one)
 {
-	struct git_attr_check attr_diff_check;
+	struct git_attr_check attr_diff_check[2];
 	int check_from_data = 0;
 
 	if (one->checked_attr)
 		return;
 
-	setup_diff_attr_check(&attr_diff_check);
+	setup_diff_attr_check(attr_diff_check);
 	one->is_binary = 0;
 	one->funcname_pattern_ident = NULL;
 
-	if (!git_checkattr(one->path, 1, &attr_diff_check)) {
+	if (!git_checkattr(one->path, 2, attr_diff_check)) {
 		const char *value;
 
-		/* binaryness */
-		value = attr_diff_check.value;
+		/* binaryness; check both "diff" and "crlf" */
+		value = attr_diff_check[0].value;
 		if (ATTR_TRUE(value))
 			;
 		else if (ATTR_FALSE(value))
 			one->is_binary = 1;
-		else
-			check_from_data = 1;
+		else {
+			const char *crlf = attr_diff_check[1].value;
+			if (ATTR_TRUE(crlf))
+				;
+			else if (ATTR_FALSE(crlf))
+				one->is_binary = 1;
+			else
+				check_from_data = 1;
+		}
 
 		/* funcname pattern ident */
 		if (ATTR_TRUE(value) || ATTR_FALSE(value) || ATTR_UNSET(value))
-- 
1.6.0.1.90.g27a6e

Re: [PATCH] diff: treat -crlf files as binary

[Avery Pennarun]

On Fri, Aug 29, 2008 at 5:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> The manual advertises that setting "crlf" attribute to false marks the
> file as binary.  We should pay attention to this condition in addition
> to the "do not diff" attribute (i.e. setting "diff" to false) when
> deciding not to show the textual diff.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  * Strictly speaking any change is backward incompatible, and this is
>   certainly one, but I do not think of a good use case to depend on the
>   previous behaviour, which was reported as a bug by my coworker.

I'm not sure this is a good idea.  In our repository, for example, we
have a few files that are strictly speaking "text" but absolutely,
positively must be CRLF on all platforms because the idiotic
proprietary parsers and generators that manipulate them need it to be
that way.

I think the bug is that "crlf=false" should not be considered the same
as "binary=true", which seems to be a bug in the documentation, not
the program.

Have fun,

Avery

Re: [PATCH] diff: treat -crlf files as binary

[Junio C Hamano]

"Avery Pennarun" <apenwarr@gmail.com> writes:

> I think the bug is that "crlf=false" should not be considered the same
> as "binary=true", which seems to be a bug in the documentation, not
> the program.

Yeah, that's right.  How about doing something like this?

 Documentation/gitattributes.txt |   39 ++++++++++++++++++++++++++++++++++++---
 1 files changed, 36 insertions(+), 3 deletions(-)

diff --git i/Documentation/gitattributes.txt w/Documentation/gitattributes.txt
index db16b0c..ec8a860 100644
--- i/Documentation/gitattributes.txt
+++ w/Documentation/gitattributes.txt
@@ -105,9 +105,8 @@ Set::
 
 Unset::
 
-	Unsetting the `crlf` attribute on a path is meant to
-	mark the path as a "binary" file.  The path never goes
-	through line endings conversion upon checkin/checkout.
+	Unsetting the `crlf` attribute on a path is tells git
+	not to attempt any end-of-line conversion  upon checkin/checkout.
 
 Unspecified::
 
@@ -482,6 +481,40 @@ in the file.  E.g. the string `$Format:%H$` will be replaced by the
 commit hash.
 
 
+USING ATTRIBUTE MACROS
+----------------------
+
+You do not want any end-of-line conversions applied to, nor textual diffs
+produced for any binary file you track.  You would need to specify e.g.
+
+------------
+*.jpg -crlf -diff
+------------
+
+but that is cumbersome.  Using attribute macros, you can specify groups of
+attributes set or unset at the same time.  The system knows a built-in
+attribute macro, `binary`:
+
+------------
+*.jpg binary
+------------
+
+which is equivalent to the above.  Note that the attribute macros can only
+be "Set" (see the above example).
+
+
+DEFINING ATTRIBUTE MACROS
+-------------------------
+
+Custom attribute macros can be defined only in the `.gitattributes` file
+at the toplevel (i.e. not in any subdirectory).  The built-in attribute
+macro "binary" is equivalent to:
+
+------------
+[attr]binary -diff -crlf
+------------
+
+
 EXAMPLE
 -------
 

Re: [PATCH] diff: treat -crlf files as binary

[Avery Pennarun]

On Sat, Aug 30, 2008 at 5:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>  Unset::
>
> -       Unsetting the `crlf` attribute on a path is meant to
> -       mark the path as a "binary" file.  The path never goes
> -       through line endings conversion upon checkin/checkout.
> +       Unsetting the `crlf` attribute on a path is tells git
> +       not to attempt any end-of-line conversion  upon checkin/checkout.
[...]

Well, _I_ understand it :)

Avery

Re: [PATCH] diff: treat -crlf files as binary

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> +	Unsetting the `crlf` attribute on a path is tells git
> +	not to attempt any end-of-line conversion  upon checkin/checkout.

If you apply it, and to be picky: typo ("is tells") and strange
formatting (double space before upon).

Other than that, it seems to be a good change, yes.

-- 
Matthieu

Re: [PATCH] diff: treat -crlf files as binary

[Alex Riesen]

2008/8/30 Junio C Hamano <gitster@pobox.com>:
> diff --git i/Documentation/gitattributes.txt w/Documentation/gitattributes.txt
> index db16b0c..ec8a860 100644
> --- i/Documentation/gitattributes.txt
> +++ w/Documentation/gitattributes.txt
> @@ -105,9 +105,8 @@ Set::
>
>  Unset::
>
> -       Unsetting the `crlf` attribute on a path is meant to
> -       mark the path as a "binary" file.  The path never goes
> -       through line endings conversion upon checkin/checkout.
> +       Unsetting the `crlf` attribute on a path is tells git
> +       not to attempt any end-of-line conversion  upon checkin/checkout.

"+       Unsetting the `crlf` attribute on a path tells git"

(remove "is" before "tells")

Re: [PATCH] diff: treat -crlf files as binary

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> 2008/8/30 Junio C Hamano <gitster@pobox.com>:
>> diff --git i/Documentation/gitattributes.txt w/Documentation/gitattributes.txt
>> ...
>> -       through line endings conversion upon checkin/checkout.
>> +       Unsetting the `crlf` attribute on a path is tells git
>> ...
>
> "+       Unsetting the `crlf` attribute on a path tells git"
>
> (remove "is" before "tells")

Yeah, thanks.  I reworded the sentence number of times and somehow crufts
were left behind.  Fixed, together with the double space before "upon",
before committing.

I am actually more surprised by the lack of any surprised comment about
the unadvertised presense of "binary" (pseudo)attribute, though.  The
support has been there since f48fd68 (attribute macro support,
2007-04-14), two days after introduction of gitattributes mechanism.

Maybe somebody should re-read the log message of that commit, and refine
the attribute macros sections the patch under discussion has added.

Somebody who can write and spell English better than me, that is ;-)

Re: [PATCH] diff: treat -crlf files as binary

[Avery Pennarun]

On Sun, Aug 31, 2008 at 12:25 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I am actually more surprised by the lack of any surprised comment about
> the unadvertised presense of "binary" (pseudo)attribute, though.  The
> support has been there since f48fd68 (attribute macro support,
> 2007-04-14), two days after introduction of gitattributes mechanism.

Er, well, I was actually talking about a *hypothetical* "binary"
attribute when I commented on this in the first place.  But when there
then turned out to actually be one that worked exactly as I wanted, I
was hardly going to be the first to complain :)

Have fun,

Avery