diff(1): Add --color support

[me_cameronkatri.com]

Summary

Adds a --color flag to diff(1) that supports the same options as GNU's diff(1). The colors are customizable with the env var DIFFCOLORS in a format similar to grep(1)'s GREPCOLORS. An example would be 04;36:41 for additions to be underlined light blue, and deletions have a red background.

[me_cameronkatri.com]

Respect CLICOLOR and COLORTERM variables.

[me_cameronkatri.com]

Remove leftover printf from testing.

[pstef]

I want to test this soon, but for now I'll just make a style-related note.

usr.bin/diff/diffreg.c

1196

This binary operator and the few below will need spaces around it.

[me_cameronkatri.com]

Add spaces around binary operators.
Test color using operator.

[pstef]

Apart from inline comments I would like to ask you to consider adding at least one test case.

usr.bin/diff/diff.c

41

I would move these to diff.h.
In any case they should be defined to distinct values.

46

They are all initialized to 0 anyway.

603

What is the intention of this part? Would it be possible to avoid the strdup? This is not a hard requirement but maybe there's a more elegant way to do this.

usr.bin/diff/diffreg.c

1206

Purely for stylistic reasons I would rewrite this as \033[m

[me_cameronkatri.com]

usr.bin/diff/diff.c

603

getenv(3) says:

The application should not modify the string pointed to by the getenv() function.

So I'm copying it, because strsep(3) modifies the string passed to it.

[pstef]

usr.bin/diff/diff.c

602

This is unimportant so feel free to ignore, but I would parse both substrings with strtonum() and handle possible errors. Then store that as char in del_code and add_code. That change would of course require the printfs to use %c instead of %s.

usr.bin/diff/diff.h

94

I asked about this before so maybe I'm wrong and they need to be defined as the same value? If so, why?

[pstef]

usr.bin/diff/diff.c

602

Ah, I assumed the format is as simple as "A:B" where A and B are unsigned integers, but that's not (always) the case.

[me_cameronkatri.com]

Fix COLORFLAG_* values.
Fix init_code() to return proper value.

usr.bin/diff/diff.h

94

Whoops, that's a mistake, they should all be different.

[pstef]

usr.bin/diff/diff.1

343

DIFFCOLOR or DIFFCOLORS?

[pstef]

usr.bin/diff/diff.1

343

The format should be explained here if only shortly. I'd also make it clear that it's ok not to set it and in that case the colors will be set to default red and green.

[me_cameronkatri.com]

Describe DIFFCOLORS in the manpage.

[jrtc27]

usr.bin/diff/diff.1

341

Should this not be .It Fl -color= for consistency? Also style for this manpage seems to be to not include the =.

usr.bin/diff/diff.c

582

This case is only actually use for COLORFLAG_UNSET, right? Would be clearer to have this as a case below rather than almost always override the variable and make it look like dead code. Especially if you reorder the cases below you can have this and the COLORFLAG_AUTO case share the isatty call and make it clearer when things are equivalent.

583

Doesn't isatty already always return either 1 or 0?

592

return (ret) is the correct style (issue repeated throughout)

[pstef]

And I'm leaning towards never being the default here, like in ls(1).

[me_cameronkatri.com]

Fix return styling.
Default to never
auto only applies when COLORTERM or CLICOLOR is set (the same as ls(1)).

[me_cameronkatri.com]

usr.bin/diff/diff.1

341

Optional arguments don't support arguments without a =, so removing it from the manpage wouldn't make sense.

[pstef]

usr.bin/diff/diff.c

336–340

I would do it like this:

	if (do_color()) {
		char *p;
		char const *env;

		color = 1;
		add_code = "32";
		del_code = "31";
		env = getenv("DIFFCOLORS");
		if (env != NULL && *env != '\0' && (p = strdup(env))) {
			add_code = p;
			strsep(&p, ":");
			if (p != NULL)
				del_code = p;
		}
	}

and abandon init_code(). The function is not easy to write in a way that doesn't leak memory.

599

When I commit this, I really want to do return (0) and return (1) where appropriate, instead of returning this pascal-style variable ret.

[me_cameronkatri.com]

Remove init_code()
Don't use ret variable.

[pstef]

I made some stylistic changes and rebased this patch on top of it. I also modified it to clear color (especially background color) before newlines.
Are you OK with me committing your change like this?

diff-color.patch8 KBDownload

[me_cameronkatri.com]

In D30545#718242, @pstef wrote:

I made some stylistic changes and rebased this patch on top of it. I also modified it to clear color (especially background color) before newlines.
Are you OK with me committing your change like this?

diff-color.patch8 KBDownload

Yep.