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

Daniel Barkalow <barkalow@iabervon.org> writes:

I would sort-of agree with (1) and (3) but people can do

	cd drivers && diff -u Makefile~ Makefile

so (2) is not a good clue and (3) is not really either.

Especially files like 'Makefile' are everywhere that dwimming and getting it wrong is worse than being stupid but consistent.

We do -p1 by default because the BCP on the kernel list (which is where git came from -- but I suspect many projects follow the convention) is to use "diff a/dir/file b/dir/file". This is also a convenient setup for people who apply git generated (or BCP formatted) patches from the toplevel of the tree.

But when you are dealing with handcrafted patches in a subdirectory, such as the one you would get by:

    $ cd some/dir && edit file.c && diff -u file.c~ file.c >G.patch

you do not have the "a/some/dir b/some/dir" prefix, so -p1 to strip "a/" and "b/" is a wrong thing to begin with, but looking at G.patch you would not even have "some/dir", so no "correct" value exists if we refuse to DWIM (and I refuse to). We support path strip levels with -p<n> option ever since you added it with e36f8b60, so that is Ok.

However, the current version (in v1.5.0.1) of git-apply has a few inconsistencies.

 * When --index (or --cached) is given, and when the patch is a
   git generated (or BCP formatted) patch, it finds out where in
   the working tree you are, and strips the right prefix
   automatically.

 * When there is no --index nor --cached, however, it does not
   bother to find out where in the working tree your $cwd is,
   and assumes you are at the toplevel (you may not even be in a
   git managed tree but are using git-apply as a better "patch",
   in which case there is no way to find out where the toplevel
   is).  You need to give -p<n> to explicitly strip the leading
   path.  This is not a problem but happens to be a feature to
   help applying handcrafted patches, like G.patch above.  Also,
   it is consistent with how GNU patch would behave.  However,
   -p<n> is ignored when the patch is a git generated one, which
   is a minor additional problem.

I was initially in favor of correcting this inconsistency by matching the behaviour of non --index/--cached case to that of the behaviour of --index/--cached case (in other words, making things more convenient for people who apply git generated and/or BCP formatted patches). But Linus talked me out of it --- and I think he is right.

Inconsistency is fixed by making the behaviour more similar to "GNU patch". The behaviour of --index/--cached case is changed so that it does not automatically strip "some/dir" part when you are in a subdirectory, which is how git-apply without these options operates. Of course, ignoring -p<n> for git generated patches no longer makes sense with this change, which is also fixed, so you can use the same -p<n> as you would give to "GNU patch".

What is cooking in 'next' should behave the way described above. If not, patches to fix it is very much welcome ;-).

P.S. I think the list hasn't heard from you for quite some time. Welcome back.