Re: [PATCH/RFC] Do not show "diff --git" metainfo with --no-prefix

Johannes Schindelin [off-list ref] writes:

quoted

diff --git a/diff.c b/diff.c
index b18c140..8126a74 100644
--- a/diff.c
+++ b/diff.c

@@ -1246,30 +1258,46 @@ static void builtin_diff(const char *name_a,
 	char *a_one, *b_two;
 	const char *set = diff_get_color_opt(o, DIFF_METAINFO);
 	const char *reset = diff_get_color_opt(o, DIFF_RESET);
+	int is_git_diff = with_standard_prefix(o);
 
 	a_one = quote_two(o->a_prefix, name_a + (*name_a == '/'));
 	b_two = quote_two(o->b_prefix, name_b + (*name_b == '/'));
 	lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
 	lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
-	printf("%sdiff --git %s %s%s\n", set, a_one, b_two, reset);
+
+	if (!is_git_diff)
+		printf("%sIndex: %s%s\n", set, b_two, reset);
+	else
+		printf("%sdiff --git %s %s%s\n", set, a_one, b_two, reset);
+

Hmm.  AFAICT plain diff outputs "diff ...", not "Index: ...".  IMHO doing 
half of what SVN does, and half what GNU diff does, but not completely 
what something else does, does not help anybody.

So I'm mildly negative on this hunk.

You misread the intention of the patch.

This whole point of this RFC patch is about not labelling a
non-git patch that results from --no-prefix with "diff --git".
As I said in my reply to Daniel, I do not like "Index:" myself,
and doing printf("diff %s %s\n", a_one, b_two) instead would be
perfectly fine by me.

I do not mind keeping the metainformation such as rename/deleted
labels in the output of non-git case (iow, dropping all the
other hunks that pay attention to is_git_diff in the RFC patch).
As long as the patch is not labelled with "diff --git", stricter
checks in git-apply will not trigger, and it knows to skip these
non-patch lines.  Also a plain GNU patch would ignore those
metainformation lines, so there is no strong reason to remove
them from the output, unless somebody wants to use non patch non
git tool that is stricter for no good reason (and I'd agree with
you that the solution to such a tool is a postprocessing filter
outside of git).

Hi,

On Tue, 15 Jan 2008, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

quoted

diff --git a/diff.c b/diff.c
index b18c140..8126a74 100644
--- a/diff.c
+++ b/diff.c

@@ -1246,30 +1258,46 @@ static void builtin_diff(const char *name_a,
 	char *a_one, *b_two;
 	const char *set = diff_get_color_opt(o, DIFF_METAINFO);
 	const char *reset = diff_get_color_opt(o, DIFF_RESET);
+	int is_git_diff = with_standard_prefix(o);
 
 	a_one = quote_two(o->a_prefix, name_a + (*name_a == '/'));
 	b_two = quote_two(o->b_prefix, name_b + (*name_b == '/'));
 	lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
 	lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
-	printf("%sdiff --git %s %s%s\n", set, a_one, b_two, reset);
+
+	if (!is_git_diff)
+		printf("%sIndex: %s%s\n", set, b_two, reset);
+	else
+		printf("%sdiff --git %s %s%s\n", set, a_one, b_two, reset);
+

Hmm.  AFAICT plain diff outputs "diff ...", not "Index: ...".  IMHO doing 
half of what SVN does, and half what GNU diff does, but not completely 
what something else does, does not help anybody.

So I'm mildly negative on this hunk.

You misread the intention of the patch.

This whole point of this RFC patch is about not labelling a non-git 
patch that results from --no-prefix with "diff --git". As I said in my 
reply to Daniel, I do not like "Index:" myself, and doing printf("diff 
%s %s\n", a_one, b_two) instead would be perfectly fine by me.

Well, I commented on this hunk specifically, and think that the intention 
of the patch would be better served by just conditionally omitting 
"--git", and nothing else.

I do not mind keeping the metainformation such as rename/deleted labels 
in the output of non-git case (iow, dropping all the other hunks that 
pay attention to is_git_diff in the RFC patch). As long as the patch is 
not labelled with "diff --git", stricter checks in git-apply will not 
trigger, and it knows to skip these non-patch lines.  Also a plain GNU 
patch would ignore those metainformation lines, so there is no strong 
reason to remove them from the output, unless somebody wants to use non 
patch non git tool that is stricter for no good reason (and I'd agree 
with you that the solution to such a tool is a postprocessing filter 
outside of git).

Fair enough.

Ciao,
Dscho

On Wed, 16 Jan 2008, Johannes Schindelin wrote:

Hi,

On Tue, 15 Jan 2008, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

quoted

diff --git a/diff.c b/diff.c
index b18c140..8126a74 100644
--- a/diff.c
+++ b/diff.c

@@ -1246,30 +1258,46 @@ static void builtin_diff(const char *name_a,
 	char *a_one, *b_two;
 	const char *set = diff_get_color_opt(o, DIFF_METAINFO);
 	const char *reset = diff_get_color_opt(o, DIFF_RESET);
+	int is_git_diff = with_standard_prefix(o);
 
 	a_one = quote_two(o->a_prefix, name_a + (*name_a == '/'));
 	b_two = quote_two(o->b_prefix, name_b + (*name_b == '/'));
 	lbl[0] = DIFF_FILE_VALID(one) ? a_one : "/dev/null";
 	lbl[1] = DIFF_FILE_VALID(two) ? b_two : "/dev/null";
-	printf("%sdiff --git %s %s%s\n", set, a_one, b_two, reset);
+
+	if (!is_git_diff)
+		printf("%sIndex: %s%s\n", set, b_two, reset);
+	else
+		printf("%sdiff --git %s %s%s\n", set, a_one, b_two, reset);
+

Hmm.  AFAICT plain diff outputs "diff ...", not "Index: ...".  IMHO doing 
half of what SVN does, and half what GNU diff does, but not completely 
what something else does, does not help anybody.

So I'm mildly negative on this hunk.

You misread the intention of the patch.

This whole point of this RFC patch is about not labelling a non-git 
patch that results from --no-prefix with "diff --git". As I said in my 
reply to Daniel, I do not like "Index:" myself, and doing printf("diff 
%s %s\n", a_one, b_two) instead would be perfectly fine by me.

Well, I commented on this hunk specifically, and think that the intention 
of the patch would be better served by just conditionally omitting 
"--git", and nothing else.

At most, I think, if a_one and b_two are identical, we could use the 
"Index:" form, since "diff -ur something something" is weird (how can 
"something" be different from itself?). If they're different, definitely 
use "diff %s %s".

	-Daniel
*This .sig left intentionally blank*