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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Feb 21, 2007, 16:44 UTC
Message-ID: <Pine.LNX.4.64.0702210836210.4043@woody.linux-foundation.org>
In-Reply-To: <Pine.LNX.4.64.0702210014140.6485@iabervon.org>

On Wed, 21 Feb 2007, Daniel Barkalow wrote:

Show 20 quoted lines

> 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