[FEATURE REQUEST] git-svn format-patch

[FEATURE REQUEST] git-svn format-patch

[Chris Ortman]

Something that would really benefit the folks who use git to manage a
subversion repository (such as myself) would be a special format-patch
command for git-svn that creates a tortoise svn compatible diff file.

Thanks.

Re: [FEATURE REQUEST] git-svn format-patch

[Johannes Schindelin]

Hi,

On Tue, 15 Jan 2008, Chris Ortman wrote:

> Something that would really benefit the folks who use git to manage a 
> subversion repository (such as myself) would be a special format-patch 
> command for git-svn that creates a tortoise svn compatible diff file.

How does the output of "git format-patch" differ from that?  (I do not use 
TortoiseSVN, and I guess a lot of people on this list don't, either...)

Ciao,
Dscho

Re: [FEATURE REQUEST] git-svn format-patch

[Chris Ortman]

The format that TortoiseSVN expects is the same as the format of svn diff.
The most apparent differences are

diff --git a/Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj
b/Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj

becomes

Index: Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj

and

index a0a0d38..9676e16 100644

becomes

===================================================================

and

--- a/Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj
+++ b/Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj

becomes

--- Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj	(revision
4715)
+++ Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj	(working
copy)

The rest is pretty much the same.

Thanks

Re: [FEATURE REQUEST] git-svn format-patch

[Johannes Schindelin]

Hi,

On Tue, 15 Jan 2008, Chris Ortman wrote:

> The format that TortoiseSVN expects is the same as the format of svn diff.
> The most apparent differences are
>
> [...]

Nothing a simple perl script cannot do.  Wanna give it a try?

Ciao,
Dscho

Re: [FEATURE REQUEST] git-svn format-patch

[Chris Ortman]

Sure, but I will probably need some guidance.

Are you thinking to just create the standard patch but then regex
replace the necessary lines or something different?

Thanks

Re: [FEATURE REQUEST] git-svn format-patch

[Johannes Schindelin]

Hi,

On Tue, 15 Jan 2008, Chris Ortman wrote:

> Are you thinking to just create the standard patch but then regex 
> replace the necessary lines or something different?

I was thinking exactly that.  TortoiseSVN's diff format is not important 
enough to git to merit a core-level change (in git), but it should be easy 
enough even to write a "sed" command line with three expressions to effect 
the transformation you desire.

Ciao,
Dscho

Re: [FEATURE REQUEST] git-svn format-patch

[Chris Ortman]

Should this be a new command in git-svn.perl? or something in contrib?

Re: [FEATURE REQUEST] git-svn format-patch

[Johannes Schindelin]

Him

On Tue, 15 Jan 2008, Chris Ortman wrote:

> Should this be a new command in git-svn.perl? or something in contrib?

I'd just start with an alias at first, and if it develops into something 
you're happy with, send it here and let others comment on it -- also where 
it should go.

Possibly the best place really would be git-svn, but it might also be less 
interesting for other people, given that git-svn does not work in msysGit 
yet (and your use case was for TortoiseSVN, which is Windows-only).

Ciao,
Dscho

Re: [FEATURE REQUEST] git-svn format-patch

[Chris Ortman]

Myself and many others have excellent luck with the cygwin version.
But the reasoning behind wanting this isn't so much for the developer
that is creating the patch as it is for the person receiving it. Most
of the projects I work on use tortoise to apply the patches and don't
typically have patch.exe

If something like this was to be accepted and become part of standard
git is there a requirement that it be written in perl or is some other
scripting language fine?
Thanks

Re: [FEATURE REQUEST] git-svn format-patch

[Jan Hudec]

On Tue, Jan 15, 2008 at 13:04:23 -0600, Chris Ortman wrote:
> Myself and many others have excellent luck with the cygwin version.
> But the reasoning behind wanting this isn't so much for the developer
> that is creating the patch as it is for the person receiving it. Most
> of the projects I work on use tortoise to apply the patches and don't
> typically have patch.exe

Note, that tortoise might actually use the version numbers, so bonus points
for actually finding them (where applicable -- if the patch is not based on
subversion revision, you can't get them).

> If something like this was to be accepted and become part of standard
> git is there a requirement that it be written in perl or is some other
> scripting language fine?

Git currently uses C, shell, perl and tcl/tk. There would probably be some
resistance to adding more dependencies, but that would not apply to the
contrib directory (so useful things written in something else are likely to
end up there).

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>

Re: [FEATURE REQUEST] git-svn format-patch

[Miles Bader]

Jan Hudec <bulb@ucw.cz> writes:
> Git currently uses C, shell, perl and tcl/tk. There would probably be some
> resistance to adding more dependencies, but that would not apply to the
> contrib directory (so useful things written in something else are likely to
> end up there).

Is tcl/tk restricted to the GUI stuff?

-Miles

-- 
A zen-buddhist walked into a pizza shop and
said, "Make me one with everything."

Re: [FEATURE REQUEST] git-svn format-patch

[Shawn O. Pearce]

Miles Bader <miles.bader@necel.com> wrote:
> Jan Hudec <bulb@ucw.cz> writes:
> > Git currently uses C, shell, perl and tcl/tk. There would probably be some
> > resistance to adding more dependencies, but that would not apply to the
> > contrib directory (so useful things written in something else are likely to
> > end up there).
> 
> Is tcl/tk restricted to the GUI stuff?

Yes.  Currently the only users of Tcl (or Tk) within the core
Git distribution is gitk and git-gui.

-- 
Shawn.

Re: [FEATURE REQUEST] git-svn format-patch

[Pascal Obry]

Chris,

You want to use format-patch's --no-prefix option.

Pascal.

-- 

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Re: [FEATURE REQUEST] git-svn format-patch

[Daniel Barkalow]

On Tue, 15 Jan 2008, Chris Ortman wrote:

> The format that TortoiseSVN expects is the same as the format of svn diff.
> The most apparent differences are
> 
> diff --git a/Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj
> b/Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj
> 
> becomes
> 
> Index: Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj

When --no-prefix is used, we should probably do:

Index: <filename>

instead of

diff --git <filename> <filename>

If nothing else, --no-prefix generates patches that git-apply can't apply 
but thinks that it should be able to because of the "diff --git" line.

> and
> 
> index a0a0d38..9676e16 100644
> 
> becomes
> 
> ===================================================================

Can't tell if this matters, or if this is meant to underline the Index 
line, and if we can leave some extra info after it. The source link you 
sent requires a login; is this line actually important to recognition, or 
is it just different in the generated patches?

> and
> 
> --- a/Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj
> +++ b/Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj
> 
> becomes
> 
> --- Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj	(revision 4715)
> +++ Facilities/EventWiring/Castle.Facilities.EventWiring.Tests/Castle.Facilities.EventWiring.Tests-vs2005.csproj	(working copy)

This (putting a description of the revision at the end) would be nice in 
general for those of us who can't remember what arguments we gave to git 
diff and can't get back to them without quitting less and no longer having 
the diff.

Of course, it would take a lot of magic to get git to describe things with 
the svn revision info in a non-svn-specific command, but that may not be 
necessary if tortoise is willing to apply patches where the base revision 
is unknown. Or git-svn could just make a lot of tags like "revision 4715".

	-Daniel
*This .sig left intentionally blank*

Re: [FEATURE REQUEST] git-svn format-patch

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> When --no-prefix is used, we should probably do:
>
> Index: <filename>
>
> instead of
>
> diff --git <filename> <filename>
>
> If nothing else, --no-prefix generates patches that git-apply can't apply 
> but thinks that it should be able to because of the "diff --git" line.

While I do not necessarily agree with that "Index: <filename>"
thing, I think dropping "--git" from there is probably a good
idea, as that is clearly not "--git" patch meant to be fed to
git-apply.

Actually I vaguely recall somebody suggested that we drop
"--git" if any nonstandard --src-prefix and --dst-prefix, but
sorry I do not recall the details (I am a bit sick today).  I
guess somehow we did not heed that wise advise and accepted the
change as-is, and that new feature ended up with a half-baked
hack that did not consider its consequences X-<.

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

[Junio C Hamano]

If a non-standard prefix is used by --no-prefix, --src-prefix,
or --dst-prefix options, the resulting diff becomes something
git-apply would not grok.  In such a case, we should not trigger
the more strict check git-apply does for "diff --git" format. 

This checks the prefix specified when generating diff and if src
and dst prefix are not one-level of directory name followed by a
slash (i.e. the standard "diff --git a/foo b/foo" is fine, a
custom "diff --git l/foo k/foo" is Ok, but "diff --git foo foo"
is NOT Ok).

---
 diff.c |   52 ++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/diff.c b/diff.c
index b18c140..8126a74 100644
--- a/diff.c
+++ b/diff.c
@@ -1233,6 +1233,18 @@ static const char *diff_funcname_pattern(struct diff_filespec *one)
 	return NULL;
 }
 
+static int with_standard_prefix(struct diff_options *o)
+{
+	const char *slash;
+	slash = strchr(o->a_prefix, '/');
+	if (!slash || slash[1])
+		return 0;
+	slash = strchr(o->b_prefix, '/');
+	if (!slash || slash[1])
+		return 0;
+	return 1;
+}
+
 static void builtin_diff(const char *name_a,
 			 const char *name_b,
 			 struct diff_filespec *one,
@@ -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);
+
 	if (lbl[0][0] == '/') {
 		/* /dev/null */
-		printf("%snew file mode %06o%s\n", set, two->mode, reset);
-		if (xfrm_msg && xfrm_msg[0])
-			printf("%s%s%s\n", set, xfrm_msg, reset);
+		if (is_git_diff) {
+			printf("%snew file mode %06o%s\n",
+			       set, two->mode, reset);
+			if (xfrm_msg && xfrm_msg[0])
+				printf("%s%s%s\n", set, xfrm_msg, reset);
+		}
 	}
 	else if (lbl[1][0] == '/') {
-		printf("%sdeleted file mode %06o%s\n", set, one->mode, reset);
-		if (xfrm_msg && xfrm_msg[0])
-			printf("%s%s%s\n", set, xfrm_msg, reset);
+		if (is_git_diff) {
+			printf("%sdeleted file mode %06o%s\n",
+			       set, one->mode, reset);
+			if (xfrm_msg && xfrm_msg[0])
+				printf("%s%s%s\n", set, xfrm_msg, reset);
+		}
 	}
 	else {
-		if (one->mode != two->mode) {
-			printf("%sold mode %06o%s\n", set, one->mode, reset);
-			printf("%snew mode %06o%s\n", set, two->mode, reset);
+		if (is_git_diff) {
+			if (one->mode != two->mode) {
+				printf("%sold mode %06o%s\n",
+				       set, one->mode, reset);
+				printf("%snew mode %06o%s\n",
+				       set, two->mode, reset);
+			}
+			if (xfrm_msg && xfrm_msg[0])
+				printf("%s%s%s\n", set, xfrm_msg, reset);
 		}
-		if (xfrm_msg && xfrm_msg[0])
-			printf("%s%s%s\n", set, xfrm_msg, reset);
 		/*
 		 * we do not run diff between different kind
 		 * of objects.

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

[Johannes Schindelin]

Hi,

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

> 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.

Also, I am not quite sure what to do about the "rename from" and "copy 
from" headers... The "--git" was always an indication that this patch may 
contain something like these headers.

All in all, I think this would be too much.  Let's just keep our patch 
format, and if anybody else is not able to grok unified diffs as we output 
them, have a transformer.  Let's not have git core affected.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> 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).

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

[Johannes Schindelin]

Hi,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> 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

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

[Daniel Barkalow]

On Wed, 16 Jan 2008, Johannes Schindelin wrote:

> Hi,
> 
> On Tue, 15 Jan 2008, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > >> 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*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> 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?).

I think you can have --src-prefix=a- --dst-prefix=b- and see
"diff a-foo b-foo" ;-).  I really do not care much either way,
as long as it does not say "diff --git".

This is also an offtopic remark but I have been wondering how
safe those fake "diff --git" output people seem to be able to
get out of recent-enough Hg.

[PATCH v2] Do not show "diff --git" metainfo with --no-prefix

[Junio C Hamano]

If a non-standard prefix is used by --no-prefix, --src-prefix,
or --dst-prefix options, the resulting diff becomes something
git-apply would not grok.  In such a case, we should not trigger
the more strict check git-apply does for patches in "diff --git"
format.

This checks the prefix specified when generating diff.  If src
and dst prefix are not one-level of directory name followed by a
slash (i.e. the standard "diff --git a/foo b/foo" is fine, a
custom "diff --git l/foo k/foo" is Ok, but "diff --git foo foo"
is NOT Ok), we are generating with a custom prefix that would
fail git-apply's stricter check.  In such a case, we do not say
"diff --git" but just say "diff" in the header.

Metainformation (e.g. "index", "similarity", etc.) lines will
safely be ignored by patch and git-apply (even when the latter
parses a non-git diff output), so this patch does not bother
stripping them away.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I am signing this off, but I am not thinking straight today
   and did not test it, so I will not commit it for now and
   leave it in the list archive, to be commented on.

 diff.c |   17 ++++++++++++++++-
 1 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/diff.c b/diff.c
index b18c140..8321492 100644
--- a/diff.c
+++ b/diff.c
@@ -1233,6 +1233,18 @@ static const char *diff_funcname_pattern(struct diff_filespec *one)
 	return NULL;
 }
 
+static int with_standard_prefix(struct diff_options *o)
+{
+	const char *slash;
+	slash = strchr(o->a_prefix, '/');
+	if (!slash || slash[1])
+		return 0;
+	slash = strchr(o->b_prefix, '/');
+	if (!slash || slash[1])
+		return 0;
+	return 1;
+}
+
 static void builtin_diff(const char *name_a,
 			 const char *name_b,
 			 struct diff_filespec *one,
@@ -1246,12 +1258,15 @@ 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);
+	const char *gitdiff;
+
+	gitdiff = with_standard_prefix(o) ? " --git" : "";
 
 	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);
+	printf("%sdiff%s %s %s%s\n", set, gitdiff, a_one, b_two, reset);
 	if (lbl[0][0] == '/') {
 		/* /dev/null */
 		printf("%snew file mode %06o%s\n", set, two->mode, reset);

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

[Linus Torvalds]

On Tue, 15 Jan 2008, Junio C Hamano wrote:
>
> If a non-standard prefix is used by --no-prefix, --src-prefix,
> or --dst-prefix options, the resulting diff becomes something
> git-apply would not grok.  In such a case, we should not trigger
> the more strict check git-apply does for patches in "diff --git"
> format.

I think this is wrong.

If we do any git-specific stuff, we need to have that "--git" thing there. 
That is *not* just limited to the prefix, but to all the other things git 
diffs can do: renames, mode changes, etc.

> Metainformation (e.g. "index", "similarity", etc.) lines will
> safely be ignored by patch and git-apply (even when the latter
> parses a non-git diff output), so this patch does not bother
> stripping them away.

It's not necessarily safe to ignore some of them, like the rename info. If 
you see a rename patch and don't understand it as a rename, it's 
pointless.

So I would argue that you need something stronger to say "don't do a git 
diff", and that should also disallow rename detection at a minimum. Quite 
frankly, any program that is so stupid as to not accept current git 
patches (ie TortoiseSVN), then we damn well shouldn't just disable the 
most trivial part of it. We should make sure that we do not enable *any* 
of the rather important extensions: even if ToirtoiseSVN would ignore 
them, if ignoring them means that it mis-understands the diff, it 
shouldn't be allowed at all.

So maybe a --standard-diff option that removes the "--git" part, but also 
removes everything else.

And add a "--index-header" to enable the (totally *idiotic*) "Index:" 
prefix that is such a total waste of time that it's not even funny (ie it 
cannot do renames, which makes it entirely pointless). 

We do not want to make it particularly easy for people to create 
mind-bogglingly stupid diff output. 

		Linus

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

[Linus Torvalds]

On Tue, 15 Jan 2008, Linus Torvalds wrote:
> 
> If we do any git-specific stuff, we need to have that "--git" thing there. 
> That is *not* just limited to the prefix, but to all the other things git 
> diffs can do: renames, mode changes, etc.

Side note: the fact that git-apply itself might have issues with a 
"--no-prefix" patch is really a red herring, because while it's true that 
you would normally not do it for git, it's even more true that we haven't 
actually started teaching git about it and the cases where you *would* use 
it (eg recursive subproject diffs etc).

So I do not think it's true that "--no-prefix" (or --src/dst-prefix) 
necessarily implies "no-git" at all. It *can* do so, but it's not a given 
thing, and almost certainly isn't in the long run with submodule support. 

So it would be kind of sad if we mixed it up with the prefix decision, 
when it really is something totally separate. Many other SCM's may want a 
simple "-p1" patch (BK did, for example), and that doesn't make them 
particularly "git-like". And conversely, git itself will want more than a 
simple "-p1" patch for subproject handling.

		Linus

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

[Daniel Barkalow]

On Tue, 15 Jan 2008, Linus Torvalds wrote:

> On Tue, 15 Jan 2008, Linus Torvalds wrote:
> > 
> > If we do any git-specific stuff, we need to have that "--git" thing there. 
> > That is *not* just limited to the prefix, but to all the other things git 
> > diffs can do: renames, mode changes, etc.
> 
> Side note: the fact that git-apply itself might have issues with a 
> "--no-prefix" patch is really a red herring, because while it's true that 
> you would normally not do it for git, it's even more true that we haven't 
> actually started teaching git about it and the cases where you *would* use 
> it (eg recursive subproject diffs etc).
>
> So I do not think it's true that "--no-prefix" (or --src/dst-prefix) 
> necessarily implies "no-git" at all. It *can* do so, but it's not a given 
> thing, and almost certainly isn't in the long run with submodule support. 

I don't think --no-prefix is sufficient for submodules; it means that 
git-apply will accidentally remove exactly one level, but if your 
submodule is two directory levels down, it won't work, and having the 
effective prefixes be "gitweb" and "gitweb" is a little hackish. You'd 
really want to generate a -p1 patch whose root is shifted from the actual 
project root, not a -p0 patch or -p2 patch or something.

	-Daniel
*This .sig left intentionally blank*

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

[Linus Torvalds]

On Tue, 15 Jan 2008, Daniel Barkalow wrote:
> 
> I don't think --no-prefix is sufficient for submodules; it means that 
> git-apply will accidentally remove exactly one level, but if your 
> submodule is two directory levels down, it won't work, and having the 
> effective prefixes be "gitweb" and "gitweb" is a little hackish. You'd 
> really want to generate a -p1 patch whose root is shifted from the actual 
> project root, not a -p0 patch or -p2 patch or something.

.. and this is *exactly* what

	cd gitweb
	git diff --src-prefix=a/gitweb/ --dst-prefix=b/gitweb/ 

would do (obviously people wouldn't do this by hand - it would be 
something that is done by the "git diff" hitting a subproject).

The point is, Junio's patch suggestion tied that prefix together with the 
"gitness" of the patch, so Junio's patch would have broken the above: git 
would decide that since it's not a standard -p1 prefix, it's not a git 
diff, so it shouldn't have "--git" in it.

That's why tying "--git" together with any prefix handling is wrong: 
because it's a totally different issue. It's true that "git-apply" right 
now doesn't understand these things, but assuming we want to teach 
git-apply to apply to subprojects eventually (we do, don't we?) we'll 
eventually have to teach it.

			Linus

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> That's why tying "--git" together with any prefix handling is wrong: 
> because it's a totally different issue. It's true that "git-apply" right 
> now doesn't understand these things, but assuming we want to teach 
> git-apply to apply to subprojects eventually (we do, don't we?) we'll 
> eventually have to teach it.

That's all correct but

 * currently diff does not recurse, nor apply does not apply
   recursively;

 * "git diff" that comes with 1.5.4, if we do not do anything,
   can produce a diff that will be rejected by the stricter
   check "git apply" has when used with --no-prefix and friends;

 * submodule aware versions of "git diff" can be told to add
   "--mark-as-git-diff" when it passes "--src-prefix=a/git-gui"
   and "--dst-prefix=b/git-gui" when it recurses internally, to
   defeat what my proposed patch does.

So I think it makes more sense to mark output as a non-git diff
when custom prefix is used in the version we are going to ship
as part of 1.5.4.

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

[Daniel Barkalow]

On Wed, 16 Jan 2008, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > That's why tying "--git" together with any prefix handling is wrong: 
> > because it's a totally different issue. It's true that "git-apply" right 
> > now doesn't understand these things, but assuming we want to teach 
> > git-apply to apply to subprojects eventually (we do, don't we?) we'll 
> > eventually have to teach it.
> 
> That's all correct but
> 
>  * currently diff does not recurse, nor apply does not apply
>    recursively;
> 
>  * "git diff" that comes with 1.5.4, if we do not do anything,
>    can produce a diff that will be rejected by the stricter
>    check "git apply" has when used with --no-prefix and friends;
> 
>  * submodule aware versions of "git diff" can be told to add
>    "--mark-as-git-diff" when it passes "--src-prefix=a/git-gui"
>    and "--dst-prefix=b/git-gui" when it recurses internally, to
>    defeat what my proposed patch does.

Or it could pass an option to include the intermediate portion as part of 
the name rather than as part of the prefixes. And git-apply would probably 
be a lot happier to have confirmation that certain files are supposed to 
be from a submodule, which could be handled by including that option in 
the header after --git.

	-Daniel
*This .sig left intentionally blank*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Wed, 16 Jan 2008, Junio C Hamano wrote:
>
>> Linus Torvalds <torvalds@linux-foundation.org> writes:
>> 
>> > That's why tying "--git" together with any prefix handling is wrong: 
>> > because it's a totally different issue. It's true that "git-apply" right 
>> > now doesn't understand these things, but assuming we want to teach 
>> > git-apply to apply to subprojects eventually (we do, don't we?) we'll 
>> > eventually have to teach it.
>> 
>> That's all correct but
>> 
>>  * currently diff does not recurse, nor apply does not apply
>>    recursively;
>> 
>>  * "git diff" that comes with 1.5.4, if we do not do anything,
>>    can produce a diff that will be rejected by the stricter
>>    check "git apply" has when used with --no-prefix and friends;
>> 
>>  * submodule aware versions of "git diff" can be told to add
>>    "--mark-as-git-diff" when it passes "--src-prefix=a/git-gui"
>>    and "--dst-prefix=b/git-gui" when it recurses internally, to
>>    defeat what my proposed patch does.
>
> Or it could pass an option to include the intermediate portion as part of 
> the name rather than as part of the prefixes. And git-apply would probably 
> be a lot happier to have confirmation that certain files are supposed to 
> be from a submodule, which could be handled by including that option in 
> the header after --git.

Yeah, I guess we can solve it that way.  In either case that's a
future thing.

An important point for me in this discussion is to agree that
the current --no-prefix that claims to be "diff --git" is not
safe for release and come to consensus that we need a fix.

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

[Johannes Schindelin]

Hi,

On Wed, 16 Jan 2008, Junio C Hamano wrote:

> An important point for me in this discussion is to agree that the 
> current --no-prefix that claims to be "diff --git" is not safe for 
> release and come to consensus that we need a fix.

Having had time to think about it for a while, I think that the 
--no-prefix still can make sense with --git.  For example, if I want to 
submit a gitk patch, but only have git.git (and consequently, made the fix 
in that repository), I could use "git diff --no-prefix" to make it easier 
for Paul, no?

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Having had time to think about it for a while, I think that the 
> --no-prefix still can make sense with --git.  For example, if I want to 
> submit a gitk patch, but only have git.git (and consequently, made the fix 
> in that repository), I could use "git diff --no-prefix" to make it easier 
> for Paul, no?

No, what you are talking about is a need of negative prefix,
which you did not implement in that no/src/dst-prefix patch.

Using --no-prefix is a _hack_ that may happen to work only when
the subtree-merged project is one level down.  You would need
negative prefix of two level _and_ a/ and b/ prefix, when gitk
is moved to modules/gitk subdirectory.

Incidentally I am planning to do such a move of gitk and git-gui
to one level down (modules/gitk and modules/git-gui) sometime in
the future when I convert git.git to use submodules.  Privately
I already have such a tree based on -rc3 but for obvious reasons
I cannot push it out even to a preview branch in git.git
repository for the time being.

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

[Johannes Schindelin]

Hi,

On Wed, 16 Jan 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Having had time to think about it for a while, I think that the 
> > --no-prefix still can make sense with --git.  For example, if I want 
> > to submit a gitk patch, but only have git.git (and consequently, made 
> > the fix in that repository), I could use "git diff --no-prefix" to 
> > make it easier for Paul, no?
> 
> No, what you are talking about is a need of negative prefix, which you 
> did not implement in that no/src/dst-prefix patch.

I'm probably missing something, but wouldn't a "diff --git gitk-git/gitk 
gitk-git/gitk" instead of "diff --git a/gitk-git/gitk b/gitk-git/gitk" in 
mbox format be directly grokkable by git-am?

> Using --no-prefix is a _hack_ that may happen to work only when
> the subtree-merged project is one level down.

Yep.  But my point was more to show that it is still a valid git diff.  
With all the niceties that come with it, like "rename from", "rename to".  
So "--no-prefix" is not that good a reason to strip the "--git" away.

Probably I missed something, though.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> No, what you are talking about is a need of negative prefix, which you 
>> did not implement in that no/src/dst-prefix patch.
>
> I'm probably missing something, but wouldn't a "diff --git gitk-git/gitk 
> gitk-git/gitk" instead of "diff --git a/gitk-git/gitk b/gitk-git/gitk" in 
> mbox format be directly grokkable by git-am?
>
>> Using --no-prefix is a _hack_ that may happen to work only when
>> the subtree-merged project is one level down.
>
> Yep.  But my point was more to show that it is still a valid git diff.  

My point was that the validness you mentined above is a
happenstance, and not a result of a good design.

After I move gitk-git one level down to modules/gitk but before
making it as a submodule, the output with --no-prefix will say
"diff --git modules/gitk/gitk modules/gitk/gitk", and that will
not be a suitable diff for Paul to apply to his tree.

I think he needs "-p2", but then he can already do that to diffs
produced without using your --no-prefix that talks about "diff
--git a/gitk-git/gitk b/gitk-git/gitk".  IOW, --no-prefix is not
a solution to anything.

And that is why I keep calling your "--no-prefix happens to work
if you are only talking about a project that is subtree-merged
one level down" argument a _hack_.

If we were to do this properly in "git diff", we would:

 - introduce a separate --strip-paths=1 (or whatever number of
   levels of leading prefix);

 - not use --{src,dst,no}-prefix

and you would do:

	$ git diff --strip-paths=1 gitk-git

in the current tree, which would first strip one path component
and then do the usual opt->a_prefix/b_prefix thing to show:

	diff --git a/gitk b/gitk

Similarly you would run:

        $ git diff --strip-paths=2 modules/gitk

after I move gitk-git down one level.

An alternative would be to use the jc/diff-relative topic
currently parked in 'offcuts' branch, and run:

	$ cd gitk-git && git diff .

or

	$ cd modules/gitk && git diff .

which would give diffs in relative paths.

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 16 Jan 2008, Junio C Hamano wrote:
>
>> An important point for me in this discussion is to agree that the 
>> current --no-prefix that claims to be "diff --git" is not safe for 
>> release and come to consensus that we need a fix.
>
> Having had time to think about it for a while, I think that...

While we are discussing about diff, there is one thing that has
been bugging me occasionally, but the annoyance factor has not
motivated me enough to look into it myself, because I do not use
it often: --color-words.  It appears that it shows lines that do
not have any word differences in bold (whatever diff.color.meta
is configured) and I think it should use plain color instead.

Was this intentional, or just a simple plain bug?

I noticed when I was reviewing documentation patch I received
from Dave Peticolas today (the feature is great for "apply the
patch as received, review the --color-words output, and undo the
uncalled-for line wrapping" workflow).

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

[Johannes Schindelin]

Hi,

On Wed, 16 Jan 2008, Junio C Hamano wrote:

> While we are discussing about diff, there is one thing that has been 
> bugging me occasionally, but the annoyance factor has not motivated me 
> enough to look into it myself, because I do not use it often: 
> --color-words.  It appears that it shows lines that do not have any word 
> differences in bold (whatever diff.color.meta is configured) and I think 
> it should use plain color instead.
> 
> Was this intentional, or just a simple plain bug?

Plain bug.  I even meant to implement your suggestion of having a variable 
set of non-word characters, but never came around to work on it.  
Hopefully this weekend...

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> While we are discussing about diff, there is one thing that has been 
>> bugging me occasionally, but the annoyance factor has not motivated me 
>> enough to look into it myself, because I do not use it often: 
>> --color-words.  It appears that it shows lines that do not have any word 
>> differences in bold (whatever diff.color.meta is configured) and I think 
>> it should use plain color instead.
>> 
>> Was this intentional, or just a simple plain bug?
>
> Plain bug.  I even meant to implement your suggestion of having a variable 
> set of non-word characters, but never came around to work on it.  
> Hopefully this weekend...

I am not sure what that variable is about, but in the code you
have fn_out_diff_words_aux() that uses OLD/NEW/PLAIN and I do
not see where  you try to color anything to METAINFO color.

Perhaps you are talking about a different problem?  I am a bit
confused...

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

[Johannes Schindelin]

Hi,

On Wed, 16 Jan 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> While we are discussing about diff, there is one thing that has been 
> >> bugging me occasionally, but the annoyance factor has not motivated 
> >> me enough to look into it myself, because I do not use it often: 
> >> --color-words.  It appears that it shows lines that do not have any 
> >> word differences in bold (whatever diff.color.meta is configured) and 
> >> I think it should use plain color instead.
> >> 
> >> Was this intentional, or just a simple plain bug?
> >
> > Plain bug.  I even meant to implement your suggestion of having a 
> > variable set of non-word characters, but never came around to work on 
> > it.  Hopefully this weekend...
> 
> I am not sure what that variable is about, but in the code you have 
> fn_out_diff_words_aux() that uses OLD/NEW/PLAIN and I do not see where 
> you try to color anything to METAINFO color.
> 
> Perhaps you are talking about a different problem?  I am a bit 
> confused...

Yes, I was talking about another problem, which I want to look at while 
working on --color-diff.

Ciao,
Dscho

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

[Jeff King]

On Thu, Jan 17, 2008 at 01:48:54AM +0000, Johannes Schindelin wrote:

> > While we are discussing about diff, there is one thing that has been 
> > bugging me occasionally, but the annoyance factor has not motivated me 
> > enough to look into it myself, because I do not use it often: 
> > --color-words.  It appears that it shows lines that do not have any word 
> > differences in bold (whatever diff.color.meta is configured) and I think 
> > it should use plain color instead.
> > 
> > Was this intentional, or just a simple plain bug?
> 
> Plain bug.  I even meant to implement your suggestion of having a variable 
> set of non-word characters, but never came around to work on it.  

Hmm. I happen to set my "meta" color to something a little less
attention-grabbing (magenta), and I find the alternate coloring to be a
nice visual indicator of "nothing happened on this line". I can see how
bold would be very distracting, though. Perhaps there should be a
color.diff.unimportant?

-Peff

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

[Jeff King]

On Thu, Jan 17, 2008 at 09:49:14AM -0500, Jeff King wrote:

> Hmm. I happen to set my "meta" color to something a little less
> attention-grabbing (magenta), and I find the alternate coloring to be a
> nice visual indicator of "nothing happened on this line". I can see how
> bold would be very distracting, though. Perhaps there should be a
> color.diff.unimportant?

BTW, here is the fix to at least color it as plain (it is a little
larger than the one line it needs to be because it cleans up the
variable name "set", which is what caused this confusion in the first
place).

-- >8 --
color unchanged lines as "plain" in "diff --color-words"

These were mistakenly being colored in "meta" color.

---
diff --git a/diff.c b/diff.c
index b18c140..9b02e79 100644
--- a/diff.c
+++ b/diff.c
@@ -552,7 +552,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	int i;
 	int color;
 	struct emit_callback *ecbdata = priv;
-	const char *set = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
+	const char *meta = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
+	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
 	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
 
 	*(ecbdata->found_changesp) = 1;
@@ -564,9 +565,9 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 		name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
 
 		printf("%s--- %s%s%s\n",
-		       set, ecbdata->label_path[0], reset, name_a_tab);
+		       meta, ecbdata->label_path[0], reset, name_a_tab);
 		printf("%s+++ %s%s%s\n",
-		       set, ecbdata->label_path[1], reset, name_b_tab);
+		       meta, ecbdata->label_path[1], reset, name_b_tab);
 		ecbdata->label_path[0] = ecbdata->label_path[1] = NULL;
 	}
 
@@ -586,7 +587,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	}
 
 	if (len < ecbdata->nparents) {
-		set = reset;
 		emit_line(reset, reset, line, len);
 		return;
 	}
@@ -610,7 +610,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 			diff_words_show(ecbdata->diff_words);
 		line++;
 		len--;
-		emit_line(set, reset, line, len);
+		emit_line(plain, reset, line, len);
 		return;
 	}
 	for (i = 0; i < ecbdata->nparents && len; i++) {

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

[Johannes Schindelin]

Hi,

On Thu, 17 Jan 2008, Jeff King wrote:

> On Thu, Jan 17, 2008 at 09:49:14AM -0500, Jeff King wrote:
> 
> > Hmm. I happen to set my "meta" color to something a little less 
> > attention-grabbing (magenta), and I find the alternate coloring to be 
> > a nice visual indicator of "nothing happened on this line". I can see 
> > how bold would be very distracting, though. Perhaps there should be a 
> > color.diff.unimportant?
> 
> BTW, here is the fix to at least color it as plain (it is a little 
> larger than the one line it needs to be because it cleans up the 
> variable name "set", which is what caused this confusion in the first 
> place).

Ah, that explains it!  Your patch looks good to me.

Thanks,
Dscho

P.S.:

> @@ -586,7 +587,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
>  	}
>  
>  	if (len < ecbdata->nparents) {
> -		set = reset;
>  		emit_line(reset, reset, line, len);
>  		return;
>  	}

I wonder what I wanted to achieve with that ;-)

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

[Jeff King]

On Thu, Jan 17, 2008 at 03:12:34PM +0000, Johannes Schindelin wrote:

> > @@ -586,7 +587,6 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
> >  	}
> >  
> >  	if (len < ecbdata->nparents) {
> > -		set = reset;
> >  		emit_line(reset, reset, line, len);
> >  		return;
> >  	}
> 
> I wonder what I wanted to achieve with that ;-)

Heh. I'm guessing it was supposed to be

  set = reset;
  emit_line(set, reset, line, len);

and you optimized the first line out. :)

-Peff

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> That's why tying "--git" together with any prefix handling is wrong: 
>> because it's a totally different issue. It's true that "git-apply" right 
>> now doesn't understand these things, but assuming we want to teach 
>> git-apply to apply to subprojects eventually (we do, don't we?) we'll 
>> eventually have to teach it.
>
> That's all correct but
>
>  * currently diff does not recurse, nor apply does not apply
>    recursively;
>
>  * "git diff" that comes with 1.5.4, if we do not do anything,
>    can produce a diff that will be rejected by the stricter
>    check "git apply" has when used with --no-prefix and friends;
>
>  * submodule aware versions of "git diff" can be told to add
>    "--mark-as-git-diff" when it passes "--src-prefix=a/git-gui"
>    and "--dst-prefix=b/git-gui" when it recurses internally, to
>    defeat what my proposed patch does.
>
> So I think it makes more sense to mark output as a non-git diff
> when custom prefix is used in the version we are going to ship
> as part of 1.5.4.

Do you still have objections to the patch?

I do not think it matters _too much_, but I think starting
stricter and then making things more relaxed later is easier
than the other way around.

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So I do not think it's true that "--no-prefix" (or --src/dst-prefix) 
> necessarily implies "no-git" at all. It *can* do so, but it's not a given 
> thing, and almost certainly isn't in the long run with submodule support. 
>
> So it would be kind of sad if we mixed it up with the prefix decision, 
> when it really is something totally separate. Many other SCM's may want a 
> simple "-p1" patch (BK did, for example), and that doesn't make them 
> particularly "git-like". And conversely, git itself will want more than a 
> simple "-p1" patch for subproject handling.

Ok.  That's a sensible argument.

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

[Daniel Barkalow]

On Tue, 15 Jan 2008, Linus Torvalds wrote:

> On Tue, 15 Jan 2008, Junio C Hamano wrote:
> >
> > If a non-standard prefix is used by --no-prefix, --src-prefix,
> > or --dst-prefix options, the resulting diff becomes something
> > git-apply would not grok.  In such a case, we should not trigger
> > the more strict check git-apply does for patches in "diff --git"
> > format.
> 
> I think this is wrong.
> 
> If we do any git-specific stuff, we need to have that "--git" thing there. 
> That is *not* just limited to the prefix, but to all the other things git 
> diffs can do: renames, mode changes, etc.

Well, part of the issue is that, if you drop the prefix, then *git* can't 
understand the resulting patch (because --git causes git-apply to use 
open-coded -p1 handling of names, which won't be right).

I suppose the other option is to have the header in this case be:

diff --git --src-prefix= --dst-prefix= filename filename

so that apply can figure out what diff did correctly.

> > Metainformation (e.g. "index", "similarity", etc.) lines will
> > safely be ignored by patch and git-apply (even when the latter
> > parses a non-git diff output), so this patch does not bother
> > stripping them away.
> 
> It's not necessarily safe to ignore some of them, like the rename info. If 
> you see a rename patch and don't understand it as a rename, it's 
> pointless.
> 
> So I would argue that you need something stronger to say "don't do a git 
> diff", and that should also disallow rename detection at a minimum. Quite 
> frankly, any program that is so stupid as to not accept current git 
> patches (ie TortoiseSVN), then we damn well shouldn't just disable the 
> most trivial part of it. We should make sure that we do not enable *any* 
> of the rather important extensions: even if ToirtoiseSVN would ignore 
> them, if ignoring them means that it mis-understands the diff, it 
> shouldn't be allowed at all.
> 
> So maybe a --standard-diff option that removes the "--git" part, but also 
> removes everything else.

That seems wise to me. We should be able to generate patches that are 
accessible to programs that can't follow any clever instructions. I think 
the point of the "Index:" header is that these programs will freak out if 
two filenames don't match (or, more likely, break in some way), and it 
means you can't sensibly generate patches that upset them for deletes or 
creates.

	-Daniel
*This .sig left intentionally blank*

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

[Jan Hudec]

On Tue, Jan 15, 2008 at 22:56:14 -0500, Daniel Barkalow wrote:
> On Tue, 15 Jan 2008, Linus Torvalds wrote:
> > [...]
> > So maybe a --standard-diff option that removes the "--git" part, but also 
> > removes everything else.
> 
> That seems wise to me. We should be able to generate patches that are 
> accessible to programs that can't follow any clever instructions. I think 
> the point of the "Index:" header is that these programs will freak out if 
> two filenames don't match (or, more likely, break in some way), and it 
> means you can't sensibly generate patches that upset them for deletes or 
> creates.

Funny, Subversion has support for explicit renames in core. So I would have
thought it can represent them in it's diffs. Might be worth checking what it
generates for copies and deletes (rename being a copy+delete in SVN) before
creating the converter from git to subversion diffs.

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> We do not want to make it particularly easy for people to create 
> mind-bogglingly stupid diff output. 

Although the discussion was triggered by that Tortoise thing,
the RFC patch was not about helping that.  That's a new feature
asked long after we went into rc freeze, and I am not interested
in discussing such a feature, especially when I am sick.

The only objective was to make sure that a patch that is not
kosher in git-apply's eyes is not marked with "diff --git";
otherwise, its output will confuse git-apply.  As --no-prefix
will be a new feature in 1.5.4, we would be shipping with a
known-to-be-bad new feature that is --no-prefix, unless we do
something about it.  Fixing that breakage was the sole
motivation behind my patch.

I think a reasonable short-term solution might be to disable any
git specific stuff (renames, rewrites, etc) when --no-prefix and
its friends are used, along with the patch you are commenting on
to remove " --git" from the header.  That would at least make
sure that the --no-prefix feature is safe.

Re: [FEATURE REQUEST] git-svn format-patch

[Chris Ortman]

I'm sorry I completely forgot there was username / password on that link
username: guest
password: ''

Tortoise does care about the line of equals signs, although it seems
like an unecessary one from my understanding.

>From the best I can tell it doesn't look like tortoise actually cares
that the svn revision be something valid, just that something is there
as a placeholder

Re: [FEATURE REQUEST] git-svn format-patch

[Jean-Luc Herren]

Chris Ortman wrote:
> Something that would really benefit the folks who use git to manage a
> subversion repository (such as myself) would be a special format-patch
> command for git-svn that creates a tortoise svn compatible diff file.

Isn't it that TortoiseSVN is simply being too strict about the
diff format it accepts?  Since even GNU patch reads and applies
them fine (I didn't test it thoroughly though), I would assume git
diffs follow some sort of standard (couldn't find it though) for
the unified diff format, or at least was designed to not break
patch.  So in the long term, I think this is rather or at least
also something to be addressed in TortoiseSVN.

jlh

Re: [FEATURE REQUEST] git-svn format-patch

[Chris Ortman]

You are correct about Tortoise in that it is too strict.
I looked through their code and they have written their own patch
program which keys off these Index: lines
http://tortoisesvn.tigris.org/svn/tortoisesvn/trunk/src/TortoiseMerge/Patch.cpp

I think it could go either way as to if git-svn creates a different
format patch or tsvn accepts multiple formats, but I anticipated
git-svn would be easier to extend so I started here.

Re: [FEATURE REQUEST] git-svn format-patch

[Daniel Barkalow]

On Tue, 15 Jan 2008, Chris Ortman wrote:

> You are correct about Tortoise in that it is too strict.
> I looked through their code and they have written their own patch
> program which keys off these Index: lines
> http://tortoisesvn.tigris.org/svn/tortoisesvn/trunk/src/TortoiseMerge/Patch.cpp
> 
> I think it could go either way as to if git-svn creates a different
> format patch or tsvn accepts multiple formats, but I anticipated
> git-svn would be easier to extend so I started here.

I think it would be worthwhile for tsvn to be less picky in some ways. It 
should at least be able to accept GNU diff, since sometimes people send 
maintainers patches prepared by hand (diff -u file.c.orig file.c), and 
there are comments in there that suggest that they're trying to support 
non-svn-generated diffs, although they seem to think that such diffs look 
like:

Index: filename
============
@@ -xxx,xxx +xxx,xxx @@
...

which isn't anything I've ever seen. You're much more likely to get:

...junk...
--- junk
+++ filename	junk
@@ -xxx,xxx +xxx,xxx @@

And that should be easy enough to parse as an alternative format in tsvn. 
(I'd send them a patch to do it, but they wouldn't be able to apply it...)

	-Daniel
*This .sig left intentionally blank*

Re: [FEATURE REQUEST] git-svn format-patch

[Nigel Magnay]

Did there ever become a way of generating svn format diffs from git?

A project is having a hard time applying my format-patch --no-prefix
diffs, but I don't have a tortoiseSVN machine to figure out why..

On Wed, Jan 16, 2008 at 2:20 AM, Daniel Barkalow <barkalow@iabervon.org> wrote:
> On Tue, 15 Jan 2008, Chris Ortman wrote:
>
>
> > You are correct about Tortoise in that it is too strict.
>  > I looked through their code and they have written their own patch
>  > program which keys off these Index: lines
>  > http://tortoisesvn.tigris.org/svn/tortoisesvn/trunk/src/TortoiseMerge/Patch.cpp
>  >
>  > I think it could go either way as to if git-svn creates a different
>  > format patch or tsvn accepts multiple formats, but I anticipated
>  > git-svn would be easier to extend so I started here.
>
>  I think it would be worthwhile for tsvn to be less picky in some ways. It
>  should at least be able to accept GNU diff, since sometimes people send
>  maintainers patches prepared by hand (diff -u file.c.orig file.c), and
>  there are comments in there that suggest that they're trying to support
>  non-svn-generated diffs, although they seem to think that such diffs look
>  like:
>
>  Index: filename
>  ============
>  @@ -xxx,xxx +xxx,xxx @@
>  ...
>
>  which isn't anything I've ever seen. You're much more likely to get:
>
>  ...junk...
>  --- junk
>  +++ filename    junk
>  @@ -xxx,xxx +xxx,xxx @@
>
>  And that should be easy enough to parse as an alternative format in tsvn.
>  (I'd send them a patch to do it, but they wouldn't be able to apply it...)
>
>
>         -Daniel
>  *This .sig left intentionally blank*
>  -
>
>
> To unsubscribe from this list: send the line "unsubscribe git" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [FEATURE REQUEST] git-svn format-patch

[Jan Hudec]

On Tue, Mar 11, 2008 at 17:38:22 +0000, Nigel Magnay wrote:
> Did there ever become a way of generating svn format diffs from git?

There was a talk about it, but I am not sure anything was actually written.
Would be quite easy to add the Index: and equals line with a few lines of
perl.

> A project is having a hard time applying my format-patch --no-prefix
> diffs, but I don't have a tortoiseSVN machine to figure out why..

You quoted quite precise explanation below.

> On Wed, Jan 16, 2008 at 2:20 AM, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > On Tue, 15 Jan 2008, Chris Ortman wrote:
> >
> >
> > > You are correct about Tortoise in that it is too strict.
> >  > I looked through their code and they have written their own patch
> >  > program which keys off these Index: lines
> >  > http://tortoisesvn.tigris.org/svn/tortoisesvn/trunk/src/TortoiseMerge/Patch.cpp
> >  >
> >  > I think it could go either way as to if git-svn creates a different
> >  > format patch or tsvn accepts multiple formats, but I anticipated
> >  > git-svn would be easier to extend so I started here.
> >
> >  I think it would be worthwhile for tsvn to be less picky in some ways. It
> >  should at least be able to accept GNU diff, since sometimes people send
> >  maintainers patches prepared by hand (diff -u file.c.orig file.c), and
> >  there are comments in there that suggest that they're trying to support
> >  non-svn-generated diffs, although they seem to think that such diffs look
> >  like:
> >
> >  Index: filename
> >  ============
> >  @@ -xxx,xxx +xxx,xxx @@
> >  ...
> >
> >  which isn't anything I've ever seen. You're much more likely to get:
> >
> >  ...junk...
> >  --- junk
> >  +++ filename    junk
> >  @@ -xxx,xxx +xxx,xxx @@
> >
> >  And that should be easy enough to parse as an alternative format in tsvn.
> >  (I'd send them a patch to do it, but they wouldn't be able to apply it...)
> >
> >
> >         -Daniel
> >  *This .sig left intentionally blank*
> >  -
> >
> >
> > To unsubscribe from this list: send the line "unsubscribe git" in
> >  the body of a message to majordomo@vger.kernel.org
> >  More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>

Re: [FEATURE REQUEST] git-svn format-patch

[Daniel Barkalow]

On Tue, 11 Mar 2008, Nigel Magnay wrote:

> Did there ever become a way of generating svn format diffs from git?
> 
> A project is having a hard time applying my format-patch --no-prefix
> diffs, but I don't have a tortoiseSVN machine to figure out why..

Not really, so far as I know. I looked at it a bit, but didn't get too 
far.

	-Daniel
*This .sig left intentionally blank*