Re: [PATCH 3/4] diff: introduce diff.<driver>.binary

From: Jeff King <peff@peff.net>
Date: Oct 24, 2008, 02:46 UTC
Message-ID: <20081024024631.GA20365@coredump.intra.peff.net>
In-Reply-To: <20081005214336.GC21925@coredump.intra.peff.net>

On Sun, Oct 05, 2008 at 05:43:36PM -0400, Jeff King wrote:

Show 5 quoted lines

> However, there is at least one conflicting situation: there
> is no way to say "use the regular rules to determine whether
> this file is binary, but if we do diff it textually, use
> this funcname pattern." That is, currently setting diff=foo
> indicates that the file is definitely text.

Hrm. I don't know what crack I was smoking when I wrote this (and then argued about it for weeks afterward). It is actually the _opposite_ situation. That is, once you have said "diff=foo", there is no way to say "btw, foo files are _definitely_ text."

See, this is the old code:

Show 32 quoted lines

> -static void diff_filespec_check_attr(struct diff_filespec *one)
> +void diff_filespec_load_driver(struct diff_filespec *one)
>  {
> -	struct userdiff_driver *drv;
> -	int check_from_data = 0;
> -
> -	if (one->checked_attr)
> -		return;
> -
> -	drv = userdiff_find_by_path(one->path);
> -	one->is_binary = 0;
> -
> -	/* binaryness */
> -	if (drv == USERDIFF_ATTR_TRUE)
> -		;
> -	else if (drv == USERDIFF_ATTR_FALSE)
> -		one->is_binary = 1;
> -	else
> -		check_from_data = 1;
> -
> -	if (check_from_data) {
> -		if (!one->data && DIFF_FILE_VALID(one))
> -			diff_populate_filespec(one, 0);
> -
> -		if (one->data)
> -			one->is_binary = buffer_is_binary(one->data, one->size);
> -	}
> +	if (!one->driver)
> +		one->driver = userdiff_find_by_path(one->path);
> +	if (!one->driver)
> +		one->driver = userdiff_find_by_name("default");
>  }

You can clearly see that we use check_from_data as long as the value is not true or false. Meaning if it is unspecified _or_ if it has a string value (actually, this text is hacked up by my previous patch, but you can look at maint:diff.c and see that it is similar).

And this makes sense. Otherwise, plumbing like "git diff-tree" would get mightily confused by external diff commands. For example, consider if you had "foo diff=bar" in your attributes file, and defined "diff.bar.command". That external diff would be used for git-diff, but _not_ for diff-tree. But it would be stupid for diff-tree to look at the attribute and say "oh, we have a diff attr, it must be text."

So the patch is right to keep the binary value to "-1" except for the few cases where it has been specified explicitly. I found this out when I tried to "fix" it to the old behavior tonight and discovered lots of breakage.

And this also means that diff.*.binary really _does_ do something useful. If you have, for example, a ".foo" file that looks like binary, but really isn't, _and_ you want to set a custom hunk header for it, previously you were out of luck. You could do one or the other. Now you can do:

  git config diff.foo.xfuncname whatever
  git config diff.foo.binary false

and get the desired effect.

As for the fallout from this with regards to our discussion last week, there were two relevant points:

 - Junio suggested that anytime we use funcname, we always want text
   anyway. I think that is reasonable, but it has never been the case up
   until now (in fact, you were stuck with the _opposite_ until now, so
   my series at least makes it possible to say "always text", though
   you have to do it manually). So I will leave it for now, and
   if people feel strongly, my series provides a sane basis for a patch
   that does this automatically.

 - Johannes complained about having to set "diff.foo.binary = false"
   when we have set "diff.foo.textconv".  I agree that having to set it
   is cumbersome, but what's worse is that it is wrong. You are saying
   "this file is not binary" which is only _sometimes_ true. That is, it
   is only true if we are in a command which actually performs the text
   conversion. Plumbing sees _just_ the binary half, which is outright
   wrong (and which became painfully obvious once I wrote some tests).

   The solution is that textconv'd data should always be treated as
   text, and that takes some refactoring of my patches. I will post a
   series dealing with this in a minute.

Hopefully that explanation made sense. This turned out to be a lot trickier than I thought (in combination with my apparent crack habit), and I just spent several hours trying to figure out all of the niggling details. But I realize the rest of you haven't thought about it for at least a week. :)

-Peff