threads / discuss / 9151

Re: diffutils: C labels misdetected as functions

Subject: Re: diffutils: C labels misdetected as functions

## tl;dr

2 messages between Jul 22, 2007 and Jul 22, 2007.

replies: 1people: 2as markdown or json

Simon Arlott· Jul 22, 2007, 19:16 UTC · lore

On 22/07/07 19:38, Paul Eggert wrote:

Show 7 quoted lines

> Simon Arlott <simon@fire.lp0.eu> writes:
> 
>> The patch below stops diffutils treating C labels starting in column 1 as function names.
> 
> That patch alone wouldn't suffice, since -p is documented as implying
> -F '^[[:alpha:]$_]'.  If the behavior changes, we'd also have to
> change the documentation to match.

Ok.

> I'm not sure the change is a good idea.  It would complicate the
> documentation and therefore the user interface, without that much

Complicated documentation means a bug shouldn't be fixed?

> benefit.  And it wouldn't suffice in general, since it would still
> mishandle labels followed by comments or by white space.

Yes, the regexp needs changing further to ignore comments...

"[[:alpha:]$_][^:]*$" perhaps? Although that ignores functions with comments on the same line containing a :.

> Also, for long C functions some people might rather see a top-level
> (unindented) label highlighted than the function name highlighted, so
> they might prefer the current behavior.

"-p  --show-c-function  Show which C function each change is in."

It's not "--show-c-function-or-label".

> Anyway, to work around your problem without changing "diff", you can
> use "diff -u -F '^[[:alpha:]$_](|.*[^:])$'" instead of "diff -u -p".

Perhaps git could do this (or similar) as a workaround? (Cc:ed)

> Or you can put a single space before the labels in question: they'll
> still stand out plenty.

The code should definitely not need to be modified to work around a diff bug.

-- 
Simon Arlott

Junio C Hamano· Jul 22, 2007, 21:30 UTC · re: Simon Arlott · lore

Simon Arlott <simon@fire.lp0.eu> writes:

Show 12 quoted lines

> On 22/07/07 19:38, Paul Eggert wrote:
>> 
>> Anyway, to work around your problem without changing "diff", you can
>> use "diff -u -F '^[[:alpha:]$_](|.*[^:])$'" instead of "diff -u -p".
>
> Perhaps git could do this (or similar) as a workaround? (Cc:ed)
>
>> Or you can put a single space before the labels in question: they'll
>> still stand out plenty.
>
> The code should definitely not need to be modified to work around a
> diff bug.

I think the word "bug" is just about documentation. Promising "show C function" and including labels and/or not showing all functions would be a bug, but given that diff is about showing textual differences between two text files with arbitrary kind of contents, I do not think it is reasonable to take that "show C function" promise too literally.

To avoid this, the description of the feature could be reworded to "shows lines that match pattern '^[[:alpha:]$_]' on context lines, intent of the option being that the pattern tends to match the beginning of function in C language and help the reader to locate the context more easily, with a caveat that the pattern matching is not meant to capture all functions nor only functions --- there will be false matches."

But that would be too long. And as Paul showed, you can narrow or widen the set of lines that are shown with the -F option. I do not think this deserves to be called a "bug".

You can define your own customized "function name" pattern in your .git/config (or $HOME/.gitconfig) file to be used by the git-diff family with recent enough git via the attributes mechanism, but I do not think we should change the default, unless GNU diff changes its default. Personally I'd rather not see it change, though.