Fix ptrdiff_t vs. int

This trivial patch fixes an obvious ptrdiff_t vs. int mismatch. Which makes we wonder why Linus isn't hitting this on his ppc64 - maybe it's time to start using -Werror...

Markus Oberhumer, <[email protected]>, http://www.oberhumer.com/

This trivial patch fixes an obvious ptrdiff_t vs. int mismatch.

Index: diff-tree.c
===================================================================
@@ -274,7 +274,7 @@
 			for (cp = header; *cp; cp = ep) {
 				ep = strchr(cp, '\n');
 				if (ep == 0) ep = cp + strlen(cp);
-				printf("%.*s%c", ep-cp, cp, 0);
+				printf("%.*s%c", (int) (ep-cp), cp, 0);
 				if (*ep) ep++;
 			}
 		}

The best time to start using -Werror is "never". Different compilers (and versions)
warn about different things, often affected by, say, optimization switches. Different
system headers contain things that one compiler or another will issue a warning over.

Thus -Werror is solely for code that needs to work on one compiler, with one set of switches, with one libc version, and specific versions of included libraries.

It is tempting to submit a patch to the gcc people making -Werror issue a warning about the trouble using -Werror.

Morten

This doesn't affect CPUs with a register-based calling convention.

-hpa

My _kernel_ is 64-bit, but my user-space is all 32-bit because (a) I don't care about user-space and (b) I'm lazy and (c) all the distributions contain just 32-bit programs and a ppc64 kernel has no trouble running 32-bit programs.

I can compile a kernel with "-m64", but since I don't have any 64-bit libraries installed, user space doesn't work that well ;)

Linus

Btw, since this was the piece of code that I didn't bother simplifying last time it was discussed (then it was just "ugly"), I took a different approach instead, and committed the following..

Linus

---
diff-tree 84c1afd7e7c69c6c3c0677d5ee01925d4c70d318 (from a9c9cef161b26ca610783dd0b180d18956c7b119)

Author: Linus Torvalds <[email protected]>
Date:   Fri May 27 10:22:09 2005 -0700

git-diff-tree: simplify header output with '-z'

No need to make them multiple lines, in fact we explicitly don't want that.

This also fixes a 64-bit problem pointed out by Markus F.X.J. Oberhumer, where we gave "%.*s" a "ptrdiff_t" length argument instead of an "int".

@@ -269,18 +269,11 @@ static int call_diff_flush(void)
 		return 0;
 	}
 	if (header) {
-		if (diff_output_format == DIFF_FORMAT_MACHINE) {
-			const char *ep, *cp;
-			for (cp = header; *cp; cp = ep) {
-				ep = strchr(cp, '\n');
-				if (ep == 0) ep = cp + strlen(cp);
-				printf("%.*s%c", ep-cp, cp, 0);
-				if (*ep) ep++;
-			}
-		}
-		else {
-			printf("%s", header);
-		}
+		const char *fmt = "%s";
+		if (diff_output_format == DIFF_FORMAT_MACHINE)
+			fmt = "%s%c";
+		
+		printf(fmt, header, 0);
 		header = NULL;
 	}
 	diff_flush(diff_output_format, 1);

Many thanks. BTW, this is probably completely irrelevant on all real-world machines and calling conventions, but still, lying on the number of arguments in vararg-functions is not my favourite practice.

~Markus

Markus Oberhumer, <[email protected]>, http://www.oberhumer.com/

Bullshit.

Think about the way stdarg works, and you should quickly figure out that it is perfectly safe to pass extra arguments.

However, this code can also be rewritten:

	fputs(header, stdout);
	if (diff_output_format == DIFF_FORMAT_MACHINE)
		putchar('\0');

... which is probably faster anyway.

-hpa

Only using a subset is guaranteed to work - it's not about "real-world machines", it's about the fact that it's not a C compiler any more if it doesn't work.

Linus

LT> Btw, since this was the piece of code that I didn't bother simplifying LT> last time it was discussed (then it was just "ugly"), I took a different LT> approach instead, and committed the following..

I thought about this after getting your "Your solution is the yucky one" message, but make sure that you are not adding an extra blank line when this is combined with diff-helper. I think you currently do.

The latest change to diff-tree -z output adds an extra line termination after non diff-raw material (the header and the commit message). To compensate for this change, stop adding the output termination of our own. "diff-tree -v -z" piped to "diff-helper -z" would give different result from "diff-tree -v" piped to "diff-helper" without this change.

---

diff-helper.c | 5 ++++-

1 files changed, 4 insertions(+), 1 deletion(-)

@@ -15,6 +15,7 @@ static const char *diff_helper_usage =
 
 int main(int ac, const char **av) {
 	struct strbuf sb;
+	const char *garbage_flush_format;
 
 	strbuf_init(&sb);
 

@@ -30,6 +31,8 @@ int main(int ac, const char **av) {
 			usage(diff_helper_usage);
 		ac--; av++;
 	}
+	garbage_flush_format = (line_termination == 0) ? "%s" : "%s\n";
+
 	/* the remaining parameters are paths patterns */
 
 	diff_setup(0);

@@ -134,7 +137,7 @@ int main(int ac, const char **av) {
 		if (pickaxe)
 			diffcore_pickaxe(pickaxe, pickaxe_opts);
 		diff_flush(DIFF_FORMAT_PATCH, 0);
-		printf("%s\n", sb.buf);
+		printf(garbage_flush_format, sb.buf);
 	}
 	if (1 < ac)
 		diffcore_pathspec(av + 1);

------------------------------------------------

I think this is really a bug in your "read_line()" interface.

You should include the terminating character in the line count.

This also fixes and simplifies "EOF" handling.

Linus

LT> I think this is really a bug in your "read_line()" interface. LT> You should include the terminating character in the line count.

What the patch changed:

You give me a non diff-raw material "foo\nbar\n" when you are not doing -z. I read that without -z. read_line() drops the EOL so I get "foo" and "bar" on separate lines, both of which I spit out with my own '\n' using "%s\n".

You give me "foo\nbar\n\0" to express the same under -z, and I read that with -z. I get "foo\nbar\n" after read_line() drops the EOL. I should spit it out without my own '\n', i.e. not using "%s\n" but "%s".

If read_line() interface changes to include EOL, then...

You give me "foo\nbar\n" without -z. I read that without -z. Fixed read_line() retains the EOL and I get "foo\n" and "bar\n", and I do not have to add my own '\n' anymore; I just do fputs().

You give me "foo\nbar\n\0" under -z, and I read that with -z. Fixed read_line() retains the EOL so I get "foo\nbar\n\0". I just do fputs() and it would drop the '\0'.

What the last sentence does feels a bit hacky, but does the right thing. It's a good fix.

Please discard the patch you are responding to unless you already have applied it.

I think the PPC calling convention is to use registers for the first few parameters, so the bug probably will not surface on PPC64.

/Danjel