Re: [PATCH] Teach 'git apply' to look at $GIT_DIR/config

[Linus Torvalds <torvalds@linux-foundation.org>]

On Wed, 21 Feb 2007, Daniel Barkalow wrote:

> On Mon, 19 Feb 2007, Linus Torvalds wrote:
> > Imagine somebody sending you a patch to a set of files, and they didn't 
> > use git to generate that patch. What would it look right? Right, it might 
> > well look like
> > 
> > 	diff -u file.c.orig file.c
> > 	--- file.c.orig
> > 	+++ file.c
> > 	@@ -29,6 +29,7 @@
> > 	...
> > 
> > and it happens to be in some subdirectory. What would you do?
> > 
> > I'd use "git apply". And I would be really upset *if* git-apply actually 
> > applied the patch to some *other* subdirectory than the one I was in.
> 
> "git apply" should be able to notice the many clues that this patch 
> doesn't go at the root: (1) it's not -r; (2) it's not a rename, but the 
> filenames aren't the same; (3) there isn't an extra path element to 
> remove.

(4): it doesn't say "diff --git" with all the git-specific info.

We actually already *do* act differently for native git patches and for 
"traditional unified diffs", and yeah, we could certainly extend that to 
the whole "what to do about subdirectories" thing.

For traditional unified diffs, we already have extra rules about guessing 
the pathname, so this isn't even really a "new" rule, it's just an 
extension of the old ones. With traditional diffs, you *have* to guess at 
the pathnames, because the pathnames aren't well-defined (never mind 
renames, it's true for file create/delete events too, and it's true 
exactly because you often have two different filenames for the *same* 
file, like in my example).

> I think "git apply" should just know that if the filenames don't match, 
> and it's not a rename, and the --- filename isn't /dev/null, then add the 
> current directory and use -p0.

Well, "-p1" is still the saner default - even unified diff people tend to 
use that (and if there is no path at all, it's still safe - there's 
nothing to remove).

As to the traditional vs git patches: we already have totally separate 
functions for parsing what they are:

 - parse_git_header() parses git-only patches and understands all the 
   extended syntax, and has sanity checks

 - parse_traditional_patch() has this magic heuristic which knows about 
   the "/dev/null" special cases, and uses a magic "find_name()" function 
   that will choose whichever name makes more sense.

We could just make "parse_traditional_patch()" try to take the prefix 
information into account instead. That would be a better choice than doing 
it unconditionally even for native git patches.

Junio?

		Linus