threads / patch / 15276

patchdiff: treat -crlf files as binary

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

## tl;dr

8 messages between Aug 29, 2008 and Aug 31, 2008. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Junio C Hamano· Aug 29, 2008, 21:28 UTC · lore

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(-)
Show changes to diff.c +19 −9
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
Avery Pennarun· Aug 29, 2008, 21:56 UTC · re: Junio C Hamano · lore

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

On Fri, Aug 29, 2008 at 5:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> 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
Junio C Hamano· Aug 30, 2008, 21:34 UTC · re: Avery Pennarun · lore

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

"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(-)
Show changes to diff +36 −3
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
 -------
 
Avery Pennarun· Aug 31, 2008, 02:34 UTC · re: Junio C Hamano · lore

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

On Sat, Aug 30, 2008 at 5:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
>  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
Matthieu Moy· Aug 31, 2008, 08:27 UTC · re: Junio C Hamano · lore

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

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
Alex Riesen· Aug 31, 2008, 09:16 UTC · re: Junio C Hamano · lore

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

2008/8/30 Junio C Hamano <gitster@pobox.com>:
Show 13 quoted lines
> 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")
Junio C Hamano· Aug 31, 2008, 16:25 UTC · re: Alex Riesen · lore

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

"Alex Riesen" <raa.lkml@gmail.com> writes:
Show 10 quoted lines
> 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 ;-)
Avery Pennarun· Aug 31, 2008, 18:34 UTC · re: Junio C Hamano · lore

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

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

← back to recent threads