[PATCH] Teach core.autocrlf to 'git apply'

[PATCH] Teach core.autocrlf to 'git apply'

[Junio C Hamano]

This teaches git-apply that the data read from and written to
the filesystem might need to get converted to adjust for local
line-ending convention.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * Comes on top of lt/crlf branch which is part of 'next'.

   One problem that hasn't been solved is that git-apply without
   --index nor --cached does not read the configuration even
   when in a git-controlled working tree.  This is not a new
   issue and needs to be addressed separately.  For now, just to
   make testing easier, I tentatively added GIT_AUTOCRLF
   environment variable as a hack, but it should be removed from
   the real version.

 builtin-apply.c |   37 +++++++++++++++++++++++++++++++------
 t/t0020-crlf.sh |   22 +++++++++++++++++-----
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 3fefdac..18f7307 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1393,28 +1393,39 @@ static void show_stats(struct patch *patch)
 	free(qname);
 }
 
-static int read_old_data(struct stat *st, const char *path, void *buf, unsigned long size)
+static int read_old_data(struct stat *st, const char *path, char **buf_p, unsigned long *alloc_p, unsigned long *size_p)
 {
 	int fd;
 	unsigned long got;
+	unsigned long nsize;
+	char *nbuf;
+	unsigned long size = *size_p;
+	char *buf = *buf_p;
 
 	switch (st->st_mode & S_IFMT) {
 	case S_IFLNK:
-		return readlink(path, buf, size);
+		return readlink(path, buf, size) != size;
 	case S_IFREG:
 		fd = open(path, O_RDONLY);
 		if (fd < 0)
 			return error("unable to open %s", path);
 		got = 0;
 		for (;;) {
-			int ret = xread(fd, (char *) buf + got, size - got);
+			int ret = xread(fd, buf + got, size - got);
 			if (ret <= 0)
 				break;
 			got += ret;
 		}
 		close(fd);
-		return got;
-
+		nsize = got;
+		nbuf = buf;
+		if (convert_to_git(path, &nbuf, &nsize)) {
+			free(buf);
+			*buf_p = nbuf;
+			*alloc_p = nsize;
+			*size_p = nsize;
+		}
+		return got != size;
 	default:
 		return -1;
 	}
@@ -1910,7 +1921,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *
 		size = st->st_size;
 		alloc = size + 8192;
 		buf = xmalloc(alloc);
-		if (read_old_data(st, patch->old_name, buf, alloc) != size)
+		if (read_old_data(st, patch->old_name, &buf, &alloc, &size))
 			return error("read of %s failed", patch->old_name);
 	}
 
@@ -2282,12 +2293,22 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 static int try_create_file(const char *path, unsigned int mode, const char *buf, unsigned long size)
 {
 	int fd;
+	char *nbuf;
+	unsigned long nsize;
 
 	if (S_ISLNK(mode))
 		/* Although buf:size is counted string, it also is NUL
 		 * terminated.
 		 */
 		return symlink(buf, path);
+	nsize = size;
+	nbuf = (char *) buf;
+	if (convert_to_working_tree(path, &nbuf, &nsize)) {
+		free((char *) buf);
+		buf = nbuf;
+		size = nsize;
+	}
+
 	fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 0666);
 	if (fd < 0)
 		return -1;
@@ -2598,6 +2619,10 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 
 	const char *whitespace_option = NULL;
 
+	/* This still is undesirable... */
+	if (getenv("GIT_AUTOCRLF"))
+		git_default_config("core.autocrlf", getenv("GIT_AUTOCRLF"));
+
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		char *end;
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 58a4d86..c64d48c 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -180,11 +180,9 @@ test_expect_success 'apply patch (autocrlf=true)' '
 	git repo-config core.autocrlf true &&
 	git read-tree --reset -u HEAD &&
 
-	# Sore thumb
-	remove_cr one >tmp && mv -f tmp one &&
-
-	git apply patch.file &&
-	test "$patched" = "`git hash-object --stdin <one`" || {
+	# This is still undesirable...
+	GIT_AUTOCRLF=true git apply patch.file &&
+	test "$patched" = "`remove_cr one | git hash-object --stdin`" || {
 		echo "Eh?  apply without index"
 		false
 	}
@@ -203,4 +201,18 @@ test_expect_success 'apply patch --cached (autocrlf=true)' '
 	}
 '
 
+test_expect_success 'apply patch --index (autocrlf=true)' '
+
+	rm -f tmp one dir/two &&
+	git repo-config core.autocrlf true &&
+	git read-tree --reset -u HEAD &&
+
+	git apply --index patch.file &&
+	test "$patched" = `git rev-parse :one` &&
+	test "$patched" = "`remove_cr one | git hash-object --stdin`" || {
+		echo "Eh?  apply with --index"
+		false
+	}
+'
+
 test_done

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

[Junio C Hamano]

When neither --index nor --cached was used, git-apply did not
try calling setup_git_directory(), which means it did not look
at configuration files at all.  This fixes it to call the setup
function but still allow the command to be run in a directory
not controlled by git.

The bug probably meant that 'git apply', not moving up to the
toplevel, did not apply properly formatted diffs from the
toplevel when you are inside a subdirectory, even though 'git
apply --index' would.  As a side effect, this patch fixes it as
well.

Signed-off-by: Junio C Hamano <junkio@cox.net>

---

 builtin-apply.c         |   21 +++++++----
 t/t4119-apply-config.sh |   90 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+), 8 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 3fefdac..fc1d673 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2595,9 +2595,18 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 	int read_stdin = 1;
 	int inaccurate_eof = 0;
 	int errs = 0;
+	int is_not_gitdir = 0;
 
 	const char *whitespace_option = NULL;
 
+	prefix = setup_git_directory_gently(&is_not_gitdir);
+	prefix_length = prefix ? strlen(prefix) : 0;
+	if (!is_not_gitdir) {
+		git_config(git_apply_config);
+		if (apply_default_whitespace)
+			parse_whitespace_option(apply_default_whitespace);
+	}
+
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 		char *end;
@@ -2648,10 +2657,14 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			continue;
 		}
 		if (!strcmp(arg, "--index")) {
+			if (is_not_gitdir)
+				die("--index outside a repository");
 			check_index = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--cached")) {
+			if (is_not_gitdir)
+				die("--cached outside a repository");
 			check_index = 1;
 			cached = 1;
 			continue;
@@ -2700,14 +2713,6 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 			inaccurate_eof = 1;
 			continue;
 		}
-
-		if (check_index && prefix_length < 0) {
-			prefix = setup_git_directory();
-			prefix_length = prefix ? strlen(prefix) : 0;
-			git_config(git_apply_config);
-			if (!whitespace_option && apply_default_whitespace)
-				parse_whitespace_option(apply_default_whitespace);
-		}
 		if (0 < prefix_length)
 			arg = prefix_filename(prefix, prefix_length, arg);
 
diff --git a/t/t4119-apply-config.sh b/t/t4119-apply-config.sh
new file mode 100755
index 0000000..0e8ea7e
--- /dev/null
+++ b/t/t4119-apply-config.sh
@@ -0,0 +1,90 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Junio C Hamano
+#
+
+test_description='git-apply --whitespace=strip and configuration file.
+
+'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo A >file1 &&
+	cp file1 saved &&
+	git add file1 &&
+	echo "B " >file1 &&
+	git diff >patch.file
+'
+
+test_expect_success 'apply --whitespace=strip' '
+
+	cp saved file1 &&
+	git update-index --refresh &&
+
+	git apply --whitespace=strip patch.file &&
+	if grep " " file1
+	then
+		echo "Eh?"
+		false
+	else
+		echo Happy
+	fi
+'
+
+test_expect_success 'apply --whitespace=strip from config' '
+
+	cp saved file1 &&
+	git update-index --refresh &&
+
+	git config apply.whitespace strip &&
+	git apply patch.file &&
+	if grep " " file1
+	then
+		echo "Eh?"
+		false
+	else
+		echo Happy
+	fi
+'
+
+mkdir sub
+D=`pwd`
+
+test_expect_success 'apply --whitespace=strip in subdir' '
+
+	cd "$D" &&
+	git config --unset-all apply.whitespace
+	cp saved file1 &&
+	git update-index --refresh &&
+
+	cd sub &&
+	git apply --whitespace=strip ../patch.file &&
+	if grep " " ../file1
+	then
+		echo "Eh?"
+		false
+	else
+		echo Happy
+	fi
+'
+
+test_expect_success 'apply --whitespace=strip from config in subdir' '
+
+	cd "$D" &&
+	git config apply.whitespace strip &&
+	cp saved file1 &&
+	git update-index --refresh &&
+
+	cd sub &&
+	git apply ../patch.file &&
+	if grep " " file1
+	then
+		echo "Eh?"
+		false
+	else
+		echo Happy
+	fi
+'
+
+test_done

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

[Jeff King]

On Sat, Feb 17, 2007 at 01:12:52PM -0800, Junio C Hamano wrote:

> +	prefix = setup_git_directory_gently(&is_not_gitdir);
> +	prefix_length = prefix ? strlen(prefix) : 0;
> +	if (!is_not_gitdir) {
> +		git_config(git_apply_config);
> +		if (apply_default_whitespace)
> +			parse_whitespace_option(apply_default_whitespace);
> +	}
> +

If I read this correctly, running 'git apply' inside a git repository
will parse $GIT_DIR/config and $HOME/.gitconfig. However, outside of a
repository it will parse neither. It would make more sense to me to
still parse $HOME/.gitconfig to pick up the user's global options.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Feb 17, 2007 at 01:12:52PM -0800, Junio C Hamano wrote:
>
>> +	prefix = setup_git_directory_gently(&is_not_gitdir);
>> +	prefix_length = prefix ? strlen(prefix) : 0;
>> +	if (!is_not_gitdir) {
>> +		git_config(git_apply_config);
>> +		if (apply_default_whitespace)
>> +			parse_whitespace_option(apply_default_whitespace);
>> +	}
>> +
>
> If I read this correctly, running 'git apply' inside a git repository
> will parse $GIT_DIR/config and $HOME/.gitconfig. However, outside of a
> repository it will parse neither. It would make more sense to me to
> still parse $HOME/.gitconfig to pick up the user's global options.

I thought about that, but decided against it.  If you are truly
operating outside a git managed repository, it does not feel
right to apply configuration user has for git.

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

[Jeff King]

On Sat, Feb 17, 2007 at 03:31:18PM -0800, Junio C Hamano wrote:

> I thought about that, but decided against it.  If you are truly
> operating outside a git managed repository, it does not feel
> right to apply configuration user has for git.

Then why are they using git-apply, and not patch?

-Peff

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

[Linus Torvalds]

On Sat, 17 Feb 2007, Jeff King wrote:
>
> On Sat, Feb 17, 2007 at 03:31:18PM -0800, Junio C Hamano wrote:
> 
> > I thought about that, but decided against it.  If you are truly
> > operating outside a git managed repository, it does not feel
> > right to apply configuration user has for git.
> 
> Then why are they using git-apply, and not patch?

I don't know about others, but I use "git apply" even outside git (*) 
simply because the defaults for it are a lot better than "patch".

I've always hated how patch has some _really_ unsafe default behaviour:

 - it will guess at filenames. As in *totally* guess. If you have a patch 
   that touches a Makefile in a subdirectory, but that subdirectory had 
   been renamed or removed, it's entirely possible that "patch" will 
   actually find *another* file called "Makefile" (most likely your 
   top-most one) and apply the patch to that.

   And yes, this has actually happened to me.

 - it defaults to various unsafe options, like allowing a big fuzz factor 
   (I think it defaults to --fuzz=2), which means that if you've already 
   applied the patch, but there was _another_ place that looks a bit like 
   the original place, "patch" will happily apply it *again* because the 
   default fuzz-factor is so permissive.

git-apply has much saner defaults (it defaults to something pretty safe, 
and you can then make it less safe if the patch doesn't apply).

It also knows about renames. I hope that some day people will start 
sending rename-patches around, just because they are *so* much more 
readable than delete/create patches.

		Linus

(*) Although I have also noticed that even more often than using "git 
apply" outside a git thing, I just import everything into git these days. 
So I may not have actually used git-apply outside of a git project in a 
long time any more. But I did, a few times.

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

[Shawn O. Pearce]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
> (*) Although I have also noticed that even more often than using "git 
> apply" outside a git thing, I just import everything into git these days. 
> So I may not have actually used git-apply outside of a git project in a 
> long time any more. But I did, a few times.

Which makes me wonder, have you looked at (or used)
contrib/fast-import/import-tars.perl ?

Its Perl, which I know is not your favorite language, but it should
be a faster way to get a tar into a Git repository, as we never
actually extract the files from the tar, or create loose objects.

-- 
Shawn.

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

[Junio C Hamano]

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

> I don't know about others, but I use "git apply" even outside git (*) 
> simply because the defaults for it are a lot better than "patch".
>
> I've always hated how patch has some _really_ unsafe default behaviour:
> ...
> git-apply has much saner defaults (it defaults to something pretty safe, 
> and you can then make it less safe if the patch doesn't apply).

All true.

> It also knows about renames. I hope that some day people will start 
> sending rename-patches around, just because they are *so* much more 
> readable than delete/create patches.

I've seen a few on git list and I think I saw a couple on kernel
list as well in the past.

By the way, do you want to veto a related change that makes
git-apply behave consistently between:

	$ cd sub/directory
        $ git apply patch.file

and

	$ cd sub/directory
        $ git apply --index patch.file

The issue is, that "patch.file" (typically) starts with:

	diff a/sub/directory/Makefile b/sub/directory/Makefile
	--- a/sub/directory/Makefile
	+++ b/sub/directory/Makefile

and the form without --index would say "What are you talking
about? I do not see sub/directory/Makefile".

I consider this is a bugfix, but it does change the behaviour,
so I am a bit worried about possible fallout.

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

[Linus Torvalds]

On Mon, 19 Feb 2007, Junio C Hamano wrote:
> > ...
> > git-apply has much saner defaults (it defaults to something pretty safe, 
> > and you can then make it less safe if the patch doesn't apply).
> 
> All true.

One thing I forgot to mention: "git apply" doesn't apply *anything* unless 
everything applies cleanly. In contrast, when "patch" fails in the middle, 
it will have done part of the job, and then leaves a reject file. I much 
prefer the "everything or nothing" approach of git-apply (again, obviously 
with "--reject" you can make it work the bad old way too).

> By the way, do you want to veto a related change that makes
> git-apply behave consistently between:
> 
> 	$ cd sub/directory
>         $ git apply patch.file
> 
> and
> 
> 	$ cd sub/directory
>         $ git apply --index patch.file
> 
> The issue is, that "patch.file" (typically) starts with:
> 
> 	diff a/sub/directory/Makefile b/sub/directory/Makefile
> 	--- a/sub/directory/Makefile
> 	+++ b/sub/directory/Makefile
> 
> and the form without --index would say "What are you talking
> about? I do not see sub/directory/Makefile".
> 
> I consider this is a bugfix, but it does change the behaviour,
> so I am a bit worried about possible fallout.

Ahh.. I'm not going to veto it, although I have to admit that I don't know 
what the "right answer" is, or if a "right answer" really exists.

I _think_ that the right answer is to (a) yes, make it be consistent, but 
(b) _not_ make it be the way we do "--index" now.

Right now, when we see "--index", we do the "setup_git_directory()" and 
the git_config() stuff - which is (I think) something we should always do, 
but then we do *not* prefix the patch itself with the prefix we got. And I 
think that's wrong. I think we should always do the "-p1" behaviour from 
where we started.

Then, if somebody is in a sub/directory/, maybe they need to add a "-p3" 
to indicate that, but at least that's better than having a patch that just 
says "Makefile", and applying the patch to the *wrong* "Makefile" 
(top-level one, rather than the one you were in).

Hmm?

		Linus

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

[Junio C Hamano]

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

> On Mon, 19 Feb 2007, Junio C Hamano wrote:
>> > ...
>> > git-apply has much saner defaults (it defaults to something pretty safe, 
>> > and you can then make it less safe if the patch doesn't apply).
>> 
>> All true.
>
> One thing I forgot to mention: "git apply" doesn't apply *anything* unless 
> everything applies cleanly. In contrast, when "patch" fails in the middle, 
> it will have done part of the job, and then leaves a reject file. I much 
> prefer the "everything or nothing" approach of git-apply (again, obviously 
> with "--reject" you can make it work the bad old way too).

Yup.

> I _think_ that the right answer is to (a) yes, make it be consistent, but 
> (b) _not_ make it be the way we do "--index" now.
>
> Right now, when we see "--index", we do the "setup_git_directory()" and 
> the git_config() stuff - which is (I think) something we should always do, 
> but then we do *not* prefix the patch itself with the prefix we got. And I 
> think that's wrong. I think we should always do the "-p1" behaviour from 
> where we started.

Hmm.  I am puzzled.  Are you suggesting to change behaviour of
"git apply" with --index?

git generated patch, or patches on the kernel list that are not
generated with git are always relative to the top-level, so I
think the current --index behaviour makes tons of sense.

> Then, if somebody is in a sub/directory/, maybe they need to add a "-p3" 
> to indicate that, but at least that's better than having a patch that just 
> says "Makefile", and applying the patch to the *wrong* "Makefile" 
> (top-level one, rather than the one you were in).
>
> Hmm?

I think it boils down to this question: when you have a patch on
hand that you are considering to apply to your tree, if the
patch talks about just "Makefile" (e.g. it says "a/Makefile
b/Makefile") which Makefile is more likely to be what the patch
is talking about -- the toplevel one or the one in the
subdirectory you happen to be in?

Both (1) diff generated by git are always relative to top, and
(2) the BCP on the kernel list (and I suspect many other
projects are run this way as well) is to have diff relative to
the toplevel, suggests that "a/Makefile b/Makefile" patch is
much more likely to be about the top-level Makefile no matter
where you happen to be.

Although the fact you *are* in the subdirectory when you are
considering that patch makes it a bit more plausible than
otherwise that the patch may be about sub/directory/Makefile, I
do not think that is strong enough hint to make it more
plausible to apply to the sub/directory one than to the
toplevel.

If the patch were what you made by running "GNU diff" inside a
corresponding subdirectory of another repository (perhaps you
wanted to feed uncommitted changes from there to this
repository), then you can always use "GNU patch" to apply.  If
you made such a one-shot patch using git-diff, it will tell you
the correct directory to apply to, so...

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

[Linus Torvalds]

On Mon, 19 Feb 2007, Junio C Hamano wrote:
> 
> Hmm.  I am puzzled.  Are you suggesting to change behaviour of
> "git apply" with --index?

Yeah. Or at least _verify_ that the patch is within the subdirectory we 
are in right now. Right now, we do neither, and it will actually change 
files outside that subdirectory, which I think is a bit dangerous.

> git generated patch, or patches on the kernel list that are not
> generated with git are always relative to the top-level, so I
> think the current --index behaviour makes tons of sense.

I agree that it is sensible, but it's sensible only within the context of 
purely self-generated patches, where the patch itself was generated not 
just with git, but with that exact project.

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.

(Again, "Makefile" is a common case, since you often have it at multiple 
levels - maybe it would try to apply the changes to the top-level 
Makefile, even though I was in the drivers/usb/ subdirectory).

Now, I personally actually always work in the top-level directory, so I 
don't actually much care. I'm so used to editing the patches themselves 
that that is what I always end up doing, but I do believe that I'm odd in 
that. Few enough people have worked with unified diffs for over 15 years, 
and are perfectly happy to edit not just the headers, but the actual 
contents too, and fixing up the line numbers to make it all come out 
right. I do that regularly (not daily, but I probably do it almost once a 
month - I decide that I like somebody's patch and want to apply it, but 
fix up some whitespace issue in the patch where he had the opening brace 
of an "if"-statement on the wrong line etc).

So while I *personally* would probably never be impacted, just because I 
alway work in the top directory anyway, I do that partly exactly because 
I'm the top-level maintainer. Somebody who maintains drivers/usb/ might 
well get patches to a driver that only edits the xyzzy.c and xyzzy.h files 
within that subdirectory, and it really does make sense that he be able to 
do "git apply < patch" in that situation.

		Linus

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

[Junio C Hamano]

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

> I agree that it is sensible, but it's sensible only within the context of 
> purely self-generated patches, where the patch itself was generated not 
> just with git, but with that exact project.

That makes sense, except that the patch could be made with any
other tools; it just has to follow the "patch is from the top of
the tree, with -p1" convention.

I think it might make sense to change (this might have to be
read "break", unfortunately) "git apply" for all three cases.

So the new rule, which would affect only when you run from a
subdirectory, would be that "diff -u a/foo b/foo" would be
parsed, 1 level (or -p <n> levels) of leading paths stripped,
and then prefix is added to form "new" and "old" filenames.

And I think Johannes is happy with that change as well.

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

[Johannes Schindelin]

Hi,

On Mon, 19 Feb 2007, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > I agree that it is sensible, but it's sensible only within the context of 
> > purely self-generated patches, where the patch itself was generated not 
> > just with git, but with that exact project.
> 
> That makes sense, except that the patch could be made with any
> other tools; it just has to follow the "patch is from the top of
> the tree, with -p1" convention.
> 
> I think it might make sense to change (this might have to be
> read "break", unfortunately) "git apply" for all three cases.

Just for the subdirectory case.

> So the new rule, which would affect only when you run from a 
> subdirectory, would be that "diff -u a/foo b/foo" would be parsed, 1 
> level (or -p <n> levels) of leading paths stripped, and then prefix is 
> added to form "new" and "old" filenames.

Wouldn't it be easier to just cd to the prefix?

> And I think Johannes is happy with that change as well.

Yes, thank you.

Ciao,
Dscho

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

[Junio C Hamano]

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

> Wouldn't it be easier to just cd to the prefix?

I do not think so.  Think of what you need to do to the index.

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

[Johannes Schindelin]

Hi,

On Mon, 19 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Wouldn't it be easier to just cd to the prefix?
> 
> I do not think so.  Think of what you need to do to the index.

Ouch. I did not even think of that. You're right.

Ciao,
Dscho

[PATCH] git-apply: require -p<n> when working in a subdirectory.

[Junio C Hamano]

git-apply running inside a subdirectory, with or without --index,
used to always assume that the patch is formatted in such a way
to apply with -p1 from the toplevel, but it is more useful and
consistent with the use of "GNU patch -p1" if it defaulted to
assume that its input is meant to apply at the level it is
invoked in.

This changes the behaviour.  It used to be that the patch
generated this way would apply without any trick:

	edit Documentation/Makefile
	git diff >patch.file
	cd Documentation
	git apply ../patch.file

You need to give an explicit -p2 to git-apply now.  On the other
hand, if you got a patch from somebody else who did not follow
"patch is to apply from the top with -p1" convention, the input
patch would start with:

	diff -u Makefile.old Makefile
	--- Makefile.old
	+++ Makefile

and in such a case, you can apply it with:

	git apply -p0 patch.file

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * This also makes "git apply" to honor -p<n> option even on git
   generated patches ("diff --git").

 builtin-apply.c         |   46 ++++++++++++++++++++++++++++++----------------
 t/t4119-apply-config.sh |   32 ++++++++++++++++++--------------
 2 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 6153791..53f286e 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -238,7 +238,7 @@ static int name_terminate(const char *name, int namelen, int c, int terminate)
 	return 1;
 }
 
-static char * find_name(const char *line, char *def, int p_value, int terminate)
+static char *find_name(const char *line, char *def, int p_value, int terminate)
 {
 	int len;
 	const char *start = line;
@@ -362,7 +362,7 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)
 static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, const char *oldnew)
 {
 	if (!orig_name && !isnull)
-		return find_name(line, NULL, 1, TERM_TAB);
+		return find_name(line, NULL, p_value, TERM_TAB);
 
 	if (orig_name) {
 		int len;
@@ -372,7 +372,7 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,
 		len = strlen(name);
 		if (isnull)
 			die("git-apply: bad git-diff - expected /dev/null, got %s on line %d", name, linenr);
-		another = find_name(line, NULL, 1, TERM_TAB);
+		another = find_name(line, NULL, p_value, TERM_TAB);
 		if (!another || memcmp(another, name, len))
 			die("git-apply: bad git-diff - inconsistent %s filename on line %d", oldnew, linenr);
 		free(another);
@@ -427,28 +427,28 @@ static int gitdiff_newfile(const char *line, struct patch *patch)
 static int gitdiff_copysrc(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
-	patch->old_name = find_name(line, NULL, 0, 0);
+	patch->old_name = find_name(line, NULL, p_value-1, 0);
 	return 0;
 }
 
 static int gitdiff_copydst(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
-	patch->new_name = find_name(line, NULL, 0, 0);
+	patch->new_name = find_name(line, NULL, p_value-1, 0);
 	return 0;
 }
 
 static int gitdiff_renamesrc(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
-	patch->old_name = find_name(line, NULL, 0, 0);
+	patch->old_name = find_name(line, NULL, p_value-1, 0);
 	return 0;
 }
 
 static int gitdiff_renamedst(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
-	patch->new_name = find_name(line, NULL, 0, 0);
+	patch->new_name = find_name(line, NULL, p_value-1, 0);
 	return 0;
 }
 
@@ -2394,7 +2394,7 @@ static void write_out_one_result(struct patch *patch, int phase)
 {
 	if (patch->is_delete > 0) {
 		if (phase == 0)
-			remove_file(patch);
+			remove_file(patch, 1);
 		return;
 	}
 	if (patch->is_new > 0 || patch->is_copy) {
@@ -2407,7 +2407,7 @@ static void write_out_one_result(struct patch *patch, int phase)
 	 * thing: remove the old, write the new
 	 */
 	if (phase == 0)
-		remove_file(patch);
+		remove_file(patch, 0);
 	if (phase == 1)
 		create_file(patch);
 }
@@ -2520,15 +2520,26 @@ static int use_patch(struct patch *p)
 			return 0;
 		x = x->next;
 	}
-	if (0 < prefix_length) {
-		int pathlen = strlen(pathname);
-		if (pathlen <= prefix_length ||
-		    memcmp(prefix, pathname, prefix_length))
-			return 0;
-	}
 	return 1;
 }
 
+static char *prefix_one(char *name)
+{
+	if (!name)
+		return name;
+	return xstrdup(prefix_filename(prefix, prefix_length, name));
+}
+
+static void prefix_patches(struct patch *p)
+{
+	if (!prefix)
+		return;
+	for ( ; p; p = p->next) {
+		p->new_name = prefix_one(p->new_name);
+		p->old_name = prefix_one(p->old_name);
+	}
+}
+
 static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 {
 	unsigned long offset, size;
@@ -2551,11 +2562,14 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 			break;
 		if (apply_in_reverse)
 			reverse_patches(patch);
+		if (prefix)
+			prefix_patches(patch);
 		if (use_patch(patch)) {
 			patch_stats(patch);
 			*listp = patch;
 			listp = &patch->next;
-		} else {
+		}
+		else {
 			/* perhaps free it a bit better? */
 			free(patch);
 			skipped_patch++;
diff --git a/t/t4119-apply-config.sh b/t/t4119-apply-config.sh
index 0e8ea7e..816b5b8 100755
--- a/t/t4119-apply-config.sh
+++ b/t/t4119-apply-config.sh
@@ -10,20 +10,22 @@ test_description='git-apply --whitespace=strip and configuration file.
 . ./test-lib.sh
 
 test_expect_success setup '
-	echo A >file1 &&
-	cp file1 saved &&
-	git add file1 &&
-	echo "B " >file1 &&
+	mkdir sub &&
+	echo A >sub/file1 &&
+	cp sub/file1 saved &&
+	git add sub/file1 &&
+	echo "B " >sub/file1 &&
 	git diff >patch.file
 '
 
 test_expect_success 'apply --whitespace=strip' '
 
-	cp saved file1 &&
+	rm -f sub/file1 &&
+	cp saved sub/file1 &&
 	git update-index --refresh &&
 
 	git apply --whitespace=strip patch.file &&
-	if grep " " file1
+	if grep " " sub/file1
 	then
 		echo "Eh?"
 		false
@@ -34,12 +36,13 @@ test_expect_success 'apply --whitespace=strip' '
 
 test_expect_success 'apply --whitespace=strip from config' '
 
-	cp saved file1 &&
+	rm -f sub/file1 &&
+	cp saved sub/file1 &&
 	git update-index --refresh &&
 
 	git config apply.whitespace strip &&
 	git apply patch.file &&
-	if grep " " file1
+	if grep " " sub/file1
 	then
 		echo "Eh?"
 		false
@@ -48,19 +51,19 @@ test_expect_success 'apply --whitespace=strip from config' '
 	fi
 '
 
-mkdir sub
 D=`pwd`
 
 test_expect_success 'apply --whitespace=strip in subdir' '
 
 	cd "$D" &&
 	git config --unset-all apply.whitespace
-	cp saved file1 &&
+	rm -f sub/file1 &&
+	cp saved sub/file1 &&
 	git update-index --refresh &&
 
 	cd sub &&
-	git apply --whitespace=strip ../patch.file &&
-	if grep " " ../file1
+	git apply --whitespace=strip -p2 ../patch.file &&
+	if grep " " file1
 	then
 		echo "Eh?"
 		false
@@ -73,11 +76,12 @@ test_expect_success 'apply --whitespace=strip from config in subdir' '
 
 	cd "$D" &&
 	git config apply.whitespace strip &&
-	cp saved file1 &&
+	rm -f sub/file1 &&
+	cp saved sub/file1 &&
 	git update-index --refresh &&
 
 	cd sub &&
-	git apply ../patch.file &&
+	git apply -p2 ../patch.file &&
 	if grep " " file1
 	then
 		echo "Eh?"
-- 
1.5.0.1.555.g13b30

[PATCH] apply: fix memory leak in prefix_one()

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---

	Thank you for your patches. (BTW they _are_ dependent, since
	the signature of remove_file() has changed.)

	This is on top of them.

 builtin-apply.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index e01969f..02b7ed9 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2516,11 +2516,13 @@ static int use_patch(struct patch *p)
 	return 1;
 }
 
-static char *prefix_one(char *name)
+static void prefix_one(char **name)
 {
-	if (!name)
-		return name;
-	return xstrdup(prefix_filename(prefix, prefix_length, name));
+	char *old_name = *name;
+	if (!old_name)
+		return;
+	*name = xstrdup(prefix_filename(prefix, prefix_length, *name));
+	free(old_name);
 }
 
 static void prefix_patches(struct patch *p)
@@ -2528,8 +2530,8 @@ static void prefix_patches(struct patch *p)
 	if (!prefix)
 		return;
 	for ( ; p; p = p->next) {
-		p->new_name = prefix_one(p->new_name);
-		p->old_name = prefix_one(p->old_name);
+		prefix_one(&p->new_name);
+		prefix_one(&p->old_name);
 	}
 }
 
-- 
1.5.0.1.2143.g4dac-dirty

Re: [PATCH] apply: fix memory leak in prefix_one()

[Junio C Hamano]

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

> 	This is on top of them.

I am not sure if this is correct.  Don't we do some point
new_name = old_name = blah?

Re: [PATCH] apply: fix memory leak in prefix_one()

[Johannes Schindelin]

Hi,

On Mon, 19 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 	This is on top of them.
> 
> I am not sure if this is correct.  Don't we do some point
> new_name = old_name = blah?

Yes, you're right. I missed that. The change is in the last hunk:

--

[PATCH] apply: fix memory leak in prefix_one()

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 builtin-apply.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index e01969f..2a23138 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2516,11 +2516,13 @@ static int use_patch(struct patch *p)
 	return 1;
 }
 
-static char *prefix_one(char *name)
+static void prefix_one(char **name)
 {
-	if (!name)
-		return name;
-	return xstrdup(prefix_filename(prefix, prefix_length, name));
+	char *old_name = *name;
+	if (!old_name)
+		return;
+	*name = xstrdup(prefix_filename(prefix, prefix_length, *name));
+	free(old_name);
 }
 
 static void prefix_patches(struct patch *p)
@@ -2528,8 +2530,9 @@ static void prefix_patches(struct patch *p)
 	if (!prefix)
 		return;
 	for ( ; p; p = p->next) {
-		p->new_name = prefix_one(p->new_name);
-		p->old_name = prefix_one(p->old_name);
+		if (p->new_name != p->old_name)
+			prefix_one(&p->new_name);
+		prefix_one(&p->old_name);
 	}
 }
 
-- 
1.5.0.1.2141.gc066

[PATCH] git-apply: do not lose cwd when run from a subdirectory.

[Junio C Hamano]

When a patch modifies (not deletes) the last file in a
directory, because we treat a modification just as deletion
followed by creation, and deleting the last file in a directory
automatically rmdir(2)'s that directory, we ended up removing
the directory, which can potentially be the cwd, and then
recreating the same directory to create the patch result.

Avoid the rmdir step when remove_file() is called only because
we are replacing it with the result by later calling
create_file().

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 * This is not related to the previous patch, but the updated
   test trigeers it ;-).

 builtin-apply.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 20fb5bd..6153791 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2243,7 +2243,7 @@ static void patch_stats(struct patch *patch)
 	}
 }
 
-static void remove_file(struct patch *patch)
+static void remove_file(struct patch *patch, int rmdir_empty)
 {
 	if (write_index) {
 		if (remove_file_from_cache(patch->old_name) < 0)
@@ -2251,7 +2251,7 @@ static void remove_file(struct patch *patch)
 		cache_tree_invalidate_path(active_cache_tree, patch->old_name);
 	}
 	if (!cached) {
-		if (!unlink(patch->old_name)) {
+		if (!unlink(patch->old_name) && rmdir_empty) {
 			char *name = xstrdup(patch->old_name);
 			char *end = strrchr(name, '/');
 			while (end) {
-- 
1.5.0.1.555.g13b30

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

[Junio C Hamano]

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

> On Mon, 19 Feb 2007, Junio C Hamano wrote:
>> 
>> Hmm.  I am puzzled.  Are you suggesting to change behaviour of
>> "git apply" with --index?
>
> Yeah. Or at least _verify_ that the patch is within the subdirectory we 
> are in right now. Right now, we do neither, and it will actually change 
> files outside that subdirectory, which I think is a bit dangerous.

I think use_patch() has always took care of that.

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

[Linus Torvalds]

On Mon, 19 Feb 2007, Junio C Hamano wrote:
> 
> I think use_patch() has always took care of that.

Ahh, right you are. 

		Linus

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

[Daniel Barkalow]

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.

Wouldn't the patch author have to do something like 
"cd drivers; diff -ur usb.orig usb > patch" (i.e., have old and new 
_directories_ in the _same_ source tree, rather than just files, or 
separate source trees) in order to generate a patch that would be confusing?

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.

	-Daniel
*This .sig left intentionally blank*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

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

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.

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

[Daniel Barkalow]

On Wed, 21 Feb 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > "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.
> 
> 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.

In this case you get:

--- Makefile~   2007-02-16 18:03:16.000000000 -0500
+++ Makefile    2007-02-19 20:13:18.000000000 -0500

Which means that the before and after filenames don't agree. And this 
isn't a patch where git can tell what it applies to, so I think (2) is 
giving the right conclusion in this case. I'm not seeing any case where 
(1), (2), and (3) all don't trigger, and the patch isn't BCP, except for 
the case I gave in my previous email, which I don't think anyone would 
actually do (if only because it would be confusing and annoying to prepare 
the change).

So my contention is that git can reliably detect non-git-formatted/BCP 
patches, and so convenient support for git-formatted/BCP patches is 
beneficial, because it won't cause git to do the wrong thing.

I think it's as simple as: if apply.c line 290 changes name, the patch 
shouldn't be assumed to be "absolute" (-p1 in the repository root). Other 
than that, I think the patch pretty much has to be "absolute", unless the 
author has done something really odd. And I think an author who does that 
will just be misunderstood in general, including by human reviewers.

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

I think there are three cases:

 - "absolute" patch in a git repository. We can tell it's "absolute", and 
   we know what the project root is, so we know what the correct -p<n> 
   level is. We should default to the right answer, and give an error if 
   the user specifies something else.

 - "absolute" patch in a non-git repository. We know the paths of the 
   files relative to the repository root, but we need -p<n> to determine 
   what the repository root is. We should default to -p1, and let the user
   specify they're running git-apply in a subdirectory with -p<n>.

 - non-"absolute" patch. We don't know what directory the patch author was 
   in, relative to the author's repository root. We should require that 
   the user figure it out and use -p<n> to specify the right answer. But 
   maybe we could still default to -p1, because it's relatively safe and 
   traditional (if it's not right, chances are there won't be any filename 
   left, and we'll just give an error).

I think that the only way this would go wrong is if the user is applying 
an "absolute" patch to the toplevel Makefile when in a subdirectory of a 
non-git tree, and expects to get an error message instead of having it try 
to patch the subdirectory Makefile.

I don't see any reason to match GNU patch, because that just means that 
we're doing something we know is wrong in the first case, and something we 
suspect is wrong in the third case. 

On the other hand, I agree that the old interpretation of -p<n> wasn't 
right. I think the first case above should change the *default* plevel, 
not (as it used to) change the interpretation of the default -p1. I.e., if 
you're in drivers/usb/, and you apply a BCP patch, it should work with -p3 
and give an error with -p1, but it should also work without a -p<n> 
option.

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

Thanks. :) I've been following the list somewhat, but I've been 
too distracted with other projects to get involved with discussions.

	-Daniel
*This .sig left intentionally blank*

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

[Linus Torvalds]

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

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

[Junio C Hamano]

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

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

I guess I took your suggestion too literally while being blinded
by the thought to make things consistent.

>> I _think_ that the right answer is to (a) yes, make it be consistent, but 
>> (b) _not_ make it be the way we do "--index" now.
>>
>> Right now, when we see "--index", we do the "setup_git_directory()" and 
>> the git_config() stuff - which is (I think) something we should always do, 
>> but then we do *not* prefix the patch itself with the prefix we got. And I 
>> think that's wrong. I think we should always do the "-p1" behaviour from 
>> where we started.
>
>Hmm.  I am puzzled.  Are you suggesting to change behaviour of
>"git apply" with --index?

We can (and should) obviously do different things depending on
"diff --git".  Let me see.

[PATCH] git-apply: notice "diff --git" patch again

[Junio C Hamano]

Earlier one that tried to be too consistent with GNU patch by
not stripping the leading path when we _know_ we are in a
subdirectory and the patch is relative to the toplevel was a
mistake.  This fixes it.

 - No change to behaviour when it is run from the toplevel of
   the repository.

 - When run from a subdirectory to apply a git-generated patch, 
   it uses the right -p<n> value automatically, with or without
   --index nor --cached option.

 - When run from a subdirectory to apply a randomly generated
   patch, it wants the right -p<n> value to be given by the
   user.

The second one is a pure improvement to correct inconsistency
between --index and non --index case, compared with 1.5.0.  The
third point could be further improved to guess what the right
value for -p<n> should be by looking at the patch, but should be
a topic of a separate patch.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-apply.c         |   23 ++++++++++++++++-------
 t/t4119-apply-config.sh |    4 ++--
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 1beebe5..12f00e3 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -144,6 +144,7 @@ struct patch {
 	unsigned long deflate_origlen;
 	int lines_added, lines_deleted;
 	int score;
+	unsigned int is_toplevel_relative:1;
 	unsigned int inaccurate_eof:1;
 	unsigned int is_binary:1;
 	unsigned int is_copy:1;
@@ -362,7 +363,7 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)
 static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, const char *oldnew)
 {
 	if (!orig_name && !isnull)
-		return find_name(line, NULL, p_value, TERM_TAB);
+		return find_name(line, NULL, 1, TERM_TAB);
 
 	if (orig_name) {
 		int len;
@@ -372,7 +373,7 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,
 		len = strlen(name);
 		if (isnull)
 			die("git-apply: bad git-diff - expected /dev/null, got %s on line %d", name, linenr);
-		another = find_name(line, NULL, p_value, TERM_TAB);
+		another = find_name(line, NULL, 1, TERM_TAB);
 		if (!another || memcmp(another, name, len))
 			die("git-apply: bad git-diff - inconsistent %s filename on line %d", oldnew, linenr);
 		free(another);
@@ -427,28 +428,28 @@ static int gitdiff_newfile(const char *line, struct patch *patch)
 static int gitdiff_copysrc(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
-	patch->old_name = find_name(line, NULL, p_value-1, 0);
+	patch->old_name = find_name(line, NULL, 0, 0);
 	return 0;
 }
 
 static int gitdiff_copydst(const char *line, struct patch *patch)
 {
 	patch->is_copy = 1;
-	patch->new_name = find_name(line, NULL, p_value-1, 0);
+	patch->new_name = find_name(line, NULL, 0, 0);
 	return 0;
 }
 
 static int gitdiff_renamesrc(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
-	patch->old_name = find_name(line, NULL, p_value-1, 0);
+	patch->old_name = find_name(line, NULL, 0, 0);
 	return 0;
 }
 
 static int gitdiff_renamedst(const char *line, struct patch *patch)
 {
 	patch->is_rename = 1;
-	patch->new_name = find_name(line, NULL, p_value-1, 0);
+	patch->new_name = find_name(line, NULL, 0, 0);
 	return 0;
 }
 
@@ -787,6 +788,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 {
 	unsigned long offset, len;
 
+	patch->is_toplevel_relative = 0;
 	patch->is_rename = patch->is_copy = 0;
 	patch->is_new = patch->is_delete = -1;
 	patch->old_mode = patch->new_mode = 0;
@@ -831,6 +833,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
 					die("git diff header lacks filename information (line %d)", linenr);
 				patch->old_name = patch->new_name = patch->def_name;
 			}
+			patch->is_toplevel_relative = 1;
 			*hdrsize = git_hdr_len;
 			return offset;
 		}
@@ -2499,6 +2502,12 @@ static int use_patch(struct patch *p)
 			return 0;
 		x = x->next;
 	}
+	if (0 < prefix_length) {
+		int pathlen = strlen(pathname);
+		if (pathlen <= prefix_length ||
+		    memcmp(prefix, pathname, prefix_length))
+			return 0;
+	}
 	return 1;
 }
 
@@ -2513,7 +2522,7 @@ static void prefix_one(char **name)
 
 static void prefix_patches(struct patch *p)
 {
-	if (!prefix)
+	if (!prefix || p->is_toplevel_relative)
 		return;
 	for ( ; p; p = p->next) {
 		if (p->new_name == p->old_name) {
diff --git a/t/t4119-apply-config.sh b/t/t4119-apply-config.sh
index f9b9425..32e0d71 100755
--- a/t/t4119-apply-config.sh
+++ b/t/t4119-apply-config.sh
@@ -78,7 +78,7 @@ test_expect_success 'apply --whitespace=strip in subdir' '
 	git update-index --refresh &&
 
 	cd sub &&
-	git apply --whitespace=strip -p2 ../patch.file &&
+	git apply --whitespace=strip ../patch.file &&
 	if grep " " file1
 	then
 		echo "Eh?"
@@ -101,7 +101,7 @@ test_expect_success 'apply --whitespace=strip from config in subdir' '
 	git update-index --refresh &&
 
 	cd sub &&
-	git apply -p2 ../patch.file &&
+	git apply ../patch.file &&
 	if grep " " file1
 	then
 		echo "Eh?"

[PATCH] git-apply: guess correct -p<n> value for non-git patches.

[Junio C Hamano]

This enhances the third point in the previous commit.  When
applying a non-git patch that begins like this:

	--- 2.6.orig/mm/slab.c
	+++ 2.6/mm/slab.c
	@@ -N,M +L,K @@@
	...

and if you are in 'mm' subdirectory, we notice that -p2 is the
right option to use to apply the patch in file slab.c in the
current directory (i.e. mm/slab.c)

The guess function also knows about this pattern, where you
would need to use -p0 if applying from the top-level:

	--- mm/slab.c
	+++ mm/slab.c
	@@ -N,M +L,K @@@
	...

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-apply.c         |   59 +++++++++++++++++++++++++++++++++++++++++++++-
 t/t4119-apply-config.sh |   56 +++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 111 insertions(+), 4 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 12f00e3..c7d4bdd 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -28,6 +28,7 @@ static int newfd = -1;
 
 static int unidiff_zero;
 static int p_value = 1;
+static int p_value_known;
 static int check_index;
 static int write_index;
 static int cached;
@@ -312,11 +313,54 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 	return name;
 }
 
+static int count_slashes(const char *cp)
+{
+	int cnt = 0;
+	char ch;
+
+	while ((ch = *cp++))
+		if (ch == '/')
+			cnt++;
+	return cnt;
+}
+
+/*
+ * Given the string after "--- " or "+++ ", guess the appropriate
+ * p_value for the given patch.
+ */
+static int guess_p_value(const char *nameline)
+{
+	char *name, *cp;
+	int val = -1;
+
+	if (is_dev_null(nameline))
+		return -1;
+	name = find_name(nameline, NULL, 0, TERM_SPACE | TERM_TAB);
+	if (!name)
+		return -1;
+	cp = strchr(name, '/');
+	if (!cp)
+		val = 0;
+	else if (prefix) {
+		/*
+		 * Does it begin with "a/$our-prefix" and such?  Then this is
+		 * very likely to apply to our directory.
+		 */
+		if (!strncmp(name, prefix, prefix_length))
+			val = count_slashes(prefix);
+		else {
+			cp++;
+			if (!strncmp(cp, prefix, prefix_length))
+				val = count_slashes(prefix) + 1;
+		}
+	}
+	free(name);
+	return val;
+}
+
 /*
  * Get the name etc info from the --/+++ lines of a traditional patch header
  *
- * NOTE! This hardcodes "-p1" behaviour in filename detection.
- *
  * FIXME! The end-of-filename heuristics are kind of screwy. For existing
  * files, we can happily check the index for a match, but for creating a
  * new file we should try to match whatever "patch" does. I have no idea.
@@ -327,6 +371,16 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 
 	first += 4;	/* skip "--- " */
 	second += 4;	/* skip "+++ " */
+	if (!p_value_known) {
+		int p, q;
+		p = guess_p_value(first);
+		q = guess_p_value(second);
+		if (p < 0) p = q;
+		if (0 <= p && p == q) {
+			p_value = p;
+			p_value_known = 1;
+		}
+	}
 	if (is_dev_null(first)) {
 		patch->is_new = 1;
 		patch->is_delete = 0;
@@ -2656,6 +2710,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 		}
 		if (!strncmp(arg, "-p", 2)) {
 			p_value = atoi(arg + 2);
+			p_value_known = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--no-add")) {
diff --git a/t/t4119-apply-config.sh b/t/t4119-apply-config.sh
index 32e0d71..55f4673 100755
--- a/t/t4119-apply-config.sh
+++ b/t/t4119-apply-config.sh
@@ -19,7 +19,7 @@ test_expect_success setup '
 '
 
 # Also handcraft GNU diff output; note this has trailing whitespace.
-cat >gpatch.file <<\EOF
+cat >gpatch.file <<\EOF &&
 --- file1	2007-02-21 01:04:24.000000000 -0800
 +++ file1+	2007-02-21 01:07:44.000000000 -0800
 @@ -1 +1 @@
@@ -27,6 +27,12 @@ cat >gpatch.file <<\EOF
 +B 
 EOF
 
+sed -e 's|file1|sub/&|' gpatch.file >gpatch-sub.file &&
+sed -e '
+	/^--- /s|file1|a/sub/&|
+	/^+++ /s|file1|b/sub/&|
+' gpatch.file >gpatch-ab-sub.file &&
+
 test_expect_success 'apply --whitespace=strip' '
 
 	rm -f sub/file1 &&
@@ -124,7 +130,53 @@ test_expect_success 'same in subdir but with traditional patch input' '
 	git update-index --refresh &&
 
 	cd sub &&
-	git apply -p0 ../gpatch.file &&
+	git apply ../gpatch.file &&
+	if grep " " file1
+	then
+		echo "Eh?"
+		false
+	elif grep B file1
+	then
+		echo Happy
+	else
+		echo "Huh?"
+		false
+	fi
+'
+
+test_expect_success 'same but with traditional patch input of depth 1' '
+
+	cd "$D" &&
+	git config apply.whitespace strip &&
+	rm -f sub/file1 &&
+	cp saved sub/file1 &&
+	git update-index --refresh &&
+
+	cd sub &&
+	git apply ../gpatch-sub.file &&
+	if grep " " file1
+	then
+		echo "Eh?"
+		false
+	elif grep B file1
+	then
+		echo Happy
+	else
+		echo "Huh?"
+		false
+	fi
+'
+
+test_expect_success 'same but with traditional patch input of depth 2' '
+
+	cd "$D" &&
+	git config apply.whitespace strip &&
+	rm -f sub/file1 &&
+	cp saved sub/file1 &&
+	git update-index --refresh &&
+
+	cd sub &&
+	git apply ../gpatch-ab-sub.file &&
 	if grep " " file1
 	then
 		echo "Eh?"
-- 
1.5.0.1.615.gfb51

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

[Johannes Schindelin]

Hi,

On Mon, 19 Feb 2007, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Mon, 19 Feb 2007, Junio C Hamano wrote:
> >> > ...
> >> > git-apply has much saner defaults (it defaults to something pretty safe, 
> >> > and you can then make it less safe if the patch doesn't apply).
> >> 
> >> All true.
> >
> > One thing I forgot to mention: "git apply" doesn't apply *anything* unless 
> > everything applies cleanly. In contrast, when "patch" fails in the middle, 
> > it will have done part of the job, and then leaves a reject file. I much 
> > prefer the "everything or nothing" approach of git-apply (again, obviously 
> > with "--reject" you can make it work the bad old way too).
> 
> Yup.

That is important to keep in mind for the following:

> > I _think_ that the right answer is to (a) yes, make it be consistent, but 
> > (b) _not_ make it be the way we do "--index" now.

BTW this is what I argued, too. But I am not hard minded.

> > Right now, when we see "--index", we do the "setup_git_directory()" 
> > and the git_config() stuff - which is (I think) something we should 
> > always do, but then we do *not* prefix the patch itself with the 
> > prefix we got. And I think that's wrong. I think we should always do 
> > the "-p1" behaviour from where we started.
> 
> Hmm.  I am puzzled.  Are you suggesting to change behaviour of "git 
> apply" with --index?
> 
> git generated patch, or patches on the kernel list that are not 
> generated with git are always relative to the top-level, so I think the 
> current --index behaviour makes tons of sense.

But not all patches are generated by git. Unfortunately.

> > Then, if somebody is in a sub/directory/, maybe they need to add a 
> > "-p3" to indicate that, but at least that's better than having a patch 
> > that just says "Makefile", and applying the patch to the *wrong* 
> > "Makefile" (top-level one, rather than the one you were in).
> 
> I think it boils down to this question: when you have a patch on
> hand that you are considering to apply to your tree, if the
> patch talks about just "Makefile" (e.g. it says "a/Makefile
> b/Makefile") which Makefile is more likely to be what the patch
> is talking about -- the toplevel one or the one in the
> subdirectory you happen to be in?
> 
> Both (1) diff generated by git are always relative to top, and
> (2) the BCP on the kernel list (and I suspect many other
> projects are run this way as well) is to have diff relative to
> the toplevel, suggests that "a/Makefile b/Makefile" patch is
> much more likely to be about the top-level Makefile no matter
> where you happen to be.
> 
> Although the fact you *are* in the subdirectory when you are
> considering that patch makes it a bit more plausible than
> otherwise that the patch may be about sub/directory/Makefile, I
> do not think that is strong enough hint to make it more
> plausible to apply to the sub/directory one than to the
> toplevel.
> 
> If the patch were what you made by running "GNU diff" inside a
> corresponding subdirectory of another repository (perhaps you
> wanted to feed uncommitted changes from there to this
> repository), then you can always use "GNU patch" to apply.  If
> you made such a one-shot patch using git-diff, it will tell you
> the correct directory to apply to, so...

... and here we come back to the argument that git-apply is so much saner 
than patch.

And yes, it happened to me a while ago that I used "patch", cursed for a 
little moment, said "git reset --hard" and cursed even more when I 
realized that I was not in a git-tracked working directory!

My vote is for "take the current subdirectory as root for the patch", 
since that is what I'd expect out of the box. But as I said: I am not 
_that_ hard minded, at least on this issue.

Ciao,
Dscho

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

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

Junio C Hamano wrote:
> If the patch were what you made by running "GNU diff" inside a
> corresponding subdirectory of another repository (perhaps you
> wanted to feed uncommitted changes from there to this
> repository), then you can always use "GNU patch" to apply.  If
> you made such a one-shot patch using git-diff, it will tell you
> the correct directory to apply to, so...

The difference there is that GNU/Larry Wall diff does not prepend a/ and b/ to the paths.  Maybe use this as an indicator for "this patch is relative to project root" vs. "this patch doesn't know where it wants to be applied".

A parameter --subdir, or even plain -pN could mean "apply this patch here, now."

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Sat, Feb 17, 2007 at 01:12:52PM -0800, Junio C Hamano wrote:
> >
> >> +	prefix = setup_git_directory_gently(&is_not_gitdir);
> >> +	prefix_length = prefix ? strlen(prefix) : 0;
> >> +	if (!is_not_gitdir) {
> >> +		git_config(git_apply_config);
> >> +		if (apply_default_whitespace)
> >> +			parse_whitespace_option(apply_default_whitespace);
> >> +	}
> >> +
> >
> > If I read this correctly, running 'git apply' inside a git repository
> > will parse $GIT_DIR/config and $HOME/.gitconfig. However, outside of a
> > repository it will parse neither. It would make more sense to me to
> > still parse $HOME/.gitconfig to pick up the user's global options.
> 
> I thought about that, but decided against it.  If you are truly
> operating outside a git managed repository, it does not feel
> right to apply configuration user has for git.

That is a good point. But in the same vein, why not have a flag to 
git-apply, and let it ignore the configuration altogether?

Ciao,
Dscho

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

[Junio C Hamano]

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

>> > If I read this correctly, running 'git apply' inside a git repository
>> > will parse $GIT_DIR/config and $HOME/.gitconfig. However, outside of a
>> > repository it will parse neither. It would make more sense to me to
>> > still parse $HOME/.gitconfig to pick up the user's global options.
>> 
>> I thought about that, but decided against it.  If you are truly
>> operating outside a git managed repository, it does not feel
>> right to apply configuration user has for git.
>
> That is a good point. But in the same vein, why not have a flag to 
> git-apply, and let it ignore the configuration altogether?

Do you mean --whitespace=strip option from the command line?

But I think Jeff is right.  It would make sense to let apply
and perhaps 'diff', if we can somehow merge 'diff2' into it,
still read from $HOME/.gitconfig if available.

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > If I read this correctly, running 'git apply' inside a git repository
> >> > will parse $GIT_DIR/config and $HOME/.gitconfig. However, outside of a
> >> > repository it will parse neither. It would make more sense to me to
> >> > still parse $HOME/.gitconfig to pick up the user's global options.
> >> 
> >> I thought about that, but decided against it.  If you are truly
> >> operating outside a git managed repository, it does not feel
> >> right to apply configuration user has for git.
> >
> > That is a good point. But in the same vein, why not have a flag to 
> > git-apply, and let it ignore the configuration altogether?
> 
> Do you mean --whitespace=strip option from the command line?

I meant something like 
"--whitespace=I-know-the-patch-is-sane-but-lets-add-cr-to-all-lfs".

> But I think Jeff is right.  It would make sense to let apply
> and perhaps 'diff', if we can somehow merge 'diff2' into it,
> still read from $HOME/.gitconfig if available.

Yes, especially if you are soo used to colours as I grew to be used to 
them. This was literally one of the reasons I wrote diff2 in the first 
place. Another was --color-words.

BTW any good ideas how to make diff fall back to diff2, so that no similar 
case falls back to diff-index?

I am really wondering if you can have a syntax which Does The Right Thing 
at all times. Maybe we can teach diff that exactly two arguments, which 
both exist in the filesystem, and at least one of them is not tracked, 
then please use diff2? I wonder how often you do something like

	git diff object.c object-refs.c

and how confusing it would be that it could mean two things, diff2 _and_ 
diff-index?

Ciao,
Dscho

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

[Junio C Hamano]

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

> On Sat, 17 Feb 2007, Junio C Hamano wrote:
> ...
>> But I think Jeff is right.  It would make sense to let apply
>> and perhaps 'diff', if we can somehow merge 'diff2' into it,
>> still read from $HOME/.gitconfig if available.
>
> Yes, especially if you are soo used to colours as I grew to be used to 
> them. This was literally one of the reasons I wrote diff2 in the first 
> place. Another was --color-words.

True.  Paches welcome ;-).

> BTW any good ideas how to make diff fall back to diff2, so that no similar 
> case falls back to diff-index?
>
> I am really wondering if you can have a syntax which Does The Right Thing 
> at all times. Maybe we can teach diff that exactly two arguments, which 
> both exist in the filesystem, and at least one of them is not tracked, 
> then please use diff2? I wonder how often you do something like
>
> 	git diff object.c object-refs.c
>
> and how confusing it would be that it could mean two things, diff2 _and_ 
> diff-index?

I _think_ the case I would want to use diff2 are:

 (1) I am totally outside a git repository.

or

 (2) I am inside a git repository but I want to compare two
     specific managed files, say GIT-VERSION-GEN and
     git-gui/GIT-VERSION-GEN.

Now, (1) can be had by moving the RUN_SETUP bit out of entry for
"diff" in git.c, and do it only when we know we are in a repo
(maybe using "setup gently").  If we are outside of a
repository, any of the existing diff-* brothers do not make much
sense --- we can always do diff2.

For (2), I _think_ it may be useful sometimes but not very
often, so how about a specific option that you require upfront?

	git diff --fs --color-words GIT-VERSION-GEN git-gui/GIT-VERSION-GEN

I can do almost that with

	git diff --color-words :GIT-VERSION-GEN :git-gui/GIT-VERSION-GEN

but, it compares indexed ones, not from the working tree, so it
is not exactly the same.  However, the difference may not make
practical difference in this particular example, though.  When I
want to know the differences between two tracked files, it is
usually becausel I want to see if there are similarities to be
consolidated, and I would do that before starting to alter
working tree files.

Also, strictly speaking, there is third one:

 (3) I am inside a git repository but I want to compare files
     that do not have anything to do with the project I am
     currently working on:

	git diff --fs /etc/skel/profile $HOME/.profile

while I do not think this usage makes any sense, an explicit
flag upfront saves you from wondering what the user meant.

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sat, 17 Feb 2007, Junio C Hamano wrote:
> > ...
> >> But I think Jeff is right.  It would make sense to let apply
> >> and perhaps 'diff', if we can somehow merge 'diff2' into it,
> >> still read from $HOME/.gitconfig if available.
> >
> > Yes, especially if you are soo used to colours as I grew to be used to 
> > them. This was literally one of the reasons I wrote diff2 in the first 
> > place. Another was --color-words.
> 
> True.  Paches welcome ;-).

I sent it out already... It was titled "[PATCH] Add `git diff2`, a GNU 
diff workalike" ;-)

> > BTW any good ideas how to make diff fall back to diff2, so that no similar 
> > case falls back to diff-index?
> >
> > I am really wondering if you can have a syntax which Does The Right Thing 
> > at all times. Maybe we can teach diff that exactly two arguments, which 
> > both exist in the filesystem, and at least one of them is not tracked, 
> > then please use diff2? I wonder how often you do something like
> >
> > 	git diff object.c object-refs.c
> >
> > and how confusing it would be that it could mean two things, diff2 _and_ 
> > diff-index?
> 
> I _think_ the case I would want to use diff2 are:
> 
>  (1) I am totally outside a git repository.
> 
> or
> 
>  (2) I am inside a git repository but I want to compare two
>      specific managed files, say GIT-VERSION-GEN and
>      git-gui/GIT-VERSION-GEN.
> 
> Now, (1) can be had by moving the RUN_SETUP bit out of entry for
> "diff" in git.c, and do it only when we know we are in a repo
> (maybe using "setup gently").  If we are outside of a
> repository, any of the existing diff-* brothers do not make much
> sense --- we can always do diff2.
> 
> For (2), I _think_ it may be useful sometimes but not very
> often, so how about a specific option that you require upfront?
> 
> 	git diff --fs --color-words GIT-VERSION-GEN git-gui/GIT-VERSION-GEN
> 
> I can do almost that with
> 
> 	git diff --color-words :GIT-VERSION-GEN :git-gui/GIT-VERSION-GEN
> 
> but, it compares indexed ones, not from the working tree, so it
> is not exactly the same.  However, the difference may not make
> practical difference in this particular example, though.  When I
> want to know the differences between two tracked files, it is
> usually becausel I want to see if there are similarities to be
> consolidated, and I would do that before starting to alter
> working tree files.
> 
> Also, strictly speaking, there is third one:
> 
>  (3) I am inside a git repository but I want to compare files
>      that do not have anything to do with the project I am
>      currently working on:
> 
> 	git diff --fs /etc/skel/profile $HOME/.profile
> 
> while I do not think this usage makes any sense, an explicit
> flag upfront saves you from wondering what the user meant.

And there is always

  (4) I am inside a git repository and want to compare one tracked file 
      with one non-tracked file, and
  (5) I am outside of a git repository, but want to compare one tracked 
      file (by absolute path) with an arbitrary other file.

The longer I think about it, the more I am convinced that trying to 
integrate diff2 into diff _will_ lead to errors due to my trained, but 
disobeying, fingers.

I also debated with myself calling it diff-two or diff-direct (both not 
really convincing), or even diff-file (too similar to diff-files), but I 
could not come up with a better name than diff2. Which is also very easy 
to write, I have to say.

Ciao,
Dscho

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

[Junio C Hamano]

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

>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > On Sat, 17 Feb 2007, Junio C Hamano wrote:
>> > ...
>> >> But I think Jeff is right.  It would make sense to let apply
>> >> and perhaps 'diff', if we can somehow merge 'diff2' into it,
>> >> still read from $HOME/.gitconfig if available.
>> >
>> > Yes, especially if you are soo used to colours as I grew to be used to 
>> > them. This was literally one of the reasons I wrote diff2 in the first 
>> > place. Another was --color-words.
>> 
>> True.  Paches welcome ;-).
>
> I sent it out already... It was titled "[PATCH] Add `git diff2`, a GNU 
> diff workalike" ;-)

I thought we were talking about making 'git apply' to read from
$HOME/.gitconfig even outside a git repository.

A funny thing is that I was suspecting that calling git_config()
would fail if you are not in a git repository after you called
setup_git_directory_gently() with no_git_ok).  The lower level
does return error because ".git/config" does not exist, but it
does not call error() to say why it failed.  Callers typically
ignore the return value from the function, so it ends up
working.

So this simple patch is the welcomed one for now ;-)

-- >8 --

diff --git a/builtin-apply.c b/builtin-apply.c
index 6ed257d..20fb5bd 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2622,11 +2622,9 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 
 	prefix = setup_git_directory_gently(&is_not_gitdir);
 	prefix_length = prefix ? strlen(prefix) : 0;
-	if (!is_not_gitdir) {
-		git_config(git_apply_config);
-		if (apply_default_whitespace)
-			parse_whitespace_option(apply_default_whitespace);
-	}
+	git_config(git_apply_config);
+	if (apply_default_whitespace)
+		parse_whitespace_option(apply_default_whitespace);
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];

-- 8< --

> The longer I think about it, the more I am convinced that trying to 
> integrate diff2 into diff _will_ lead to errors due to my trained, but 
> disobeying, fingers.
>
> I also debated with myself calling it diff-two or diff-direct (both not 
> really convincing), or even diff-file (too similar to diff-files), but I 
> could not come up with a better name than diff2. Which is also very easy 
> to write, I have to say.

Hmmmmmmmmmmmmmmmmmmmm.

I do agree diff-file is too confusing with diff-files, but...

In any case, I am done for the evening, and your patch did not
apply to any of the well-known versions, so perhaps I can take a
rest and have dinner and hope to find a respun one that
everybody on the list agrees with its name in my mailbox later?
;-)

I've parked the --color --check one at near the tip of 'pu', but
it appears its handling of whitespace warning is broken.

For example, from a patch that does this (I replaced a HT with =
and SP with _):

        @@ -1,6 +1,6 @@
         # The default target of this Makefile is...
         all:
        -
        +_=ab_
         # Define NO_OPENSSL
         # This also implies
         #

I get:

        Makefile:3: space before tab.white space at end: +=ab_
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~     ~

where ~ are colored in whitespace warning color.

	

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Junio C Hamano wrote:

> I do agree diff-file is too confusing with diff-files, but...

... but nobody writes out diff-files these days, right?

> In any case, I am done for the evening, and your patch did not apply to 
> any of the well-known versions, so perhaps I can take a rest and have 
> dinner and hope to find a respun one that everybody on the list agrees 
> with its name in my mailbox later? ;-)

;-)

Well, that'll have to wait a bit. I am way past bed-time. Same goes for 
the colour --check thing. Will work on that tomorrow or Monday.

Ciao,
Dscho

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

[Junio C Hamano]

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

> On Sat, 17 Feb 2007, Junio C Hamano wrote:
>
>> I do agree diff-file is too confusing with diff-files, but...
>
> ... but nobody writes out diff-files these days, right?

Actually, "diff-files -c -p" while looking at a merge conflict
is the only thing I found "git diff" does not let me do.   The
latter always gives condensed combined diff.

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sat, 17 Feb 2007, Junio C Hamano wrote:
> >
> >> I do agree diff-file is too confusing with diff-files, but...
> >
> > ... but nobody writes out diff-files these days, right?
> 
> Actually, "diff-files -c -p" while looking at a merge conflict
> is the only thing I found "git diff" does not let me do.   The
> latter always gives condensed combined diff.

So, diff2 is my preference. If anybody has better suggestions: go ahead.

Ciao,
Dscho

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

[Jakub Narebski]

Junio C Hamano wrote:

> I can do almost that with
> 
>         git diff --color-words :GIT-VERSION-GEN :git-gui/GIT-VERSION-GEN
> 
> but, it compares indexed ones, not from the working tree, so it
> is not exactly the same.

Not ::GIT-VERSION-GEN? 

We could use :-1: for filesystem (working directory, :-: in short, version.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Jeff King wrote:

> On Sat, Feb 17, 2007 at 01:12:52PM -0800, Junio C Hamano wrote:
> 
> > +	prefix = setup_git_directory_gently(&is_not_gitdir);
> > +	prefix_length = prefix ? strlen(prefix) : 0;
> > +	if (!is_not_gitdir) {
> > +		git_config(git_apply_config);
> > +		if (apply_default_whitespace)
> > +			parse_whitespace_option(apply_default_whitespace);
> > +	}
> > +
> 
> If I read this correctly, running 'git apply' inside a git repository
> will parse $GIT_DIR/config and $HOME/.gitconfig. However, outside of a
> repository it will parse neither. It would make more sense to me to
> still parse $HOME/.gitconfig to pick up the user's global options.

In git-config we solve the problem by running git_config regardless of 
is_not_gitdir. However, I suspect you have to 'cd' back to prefix, or else 
the patch gets applied in the repo root, right? (Disclaimer: I did not 
read the patch.)

Ciao,
Dscho

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

[Junio C Hamano]

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

> ... However, I suspect you have to 'cd' back to prefix, or else 
> the patch gets applied in the repo root, right? (Disclaimer: I did not 
> read the patch.)

Actually, not cd-ing up was a bug, since git diff is always
relative to root.  The behaviour to apply the same file was
inconsistent between with --index and without as far as I can
tell.

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > ... However, I suspect you have to 'cd' back to prefix, or else the 
> > patch gets applied in the repo root, right? (Disclaimer: I did not 
> > read the patch.)
> 
> Actually, not cd-ing up was a bug, since git diff is always relative to 
> root.  The behaviour to apply the same file was inconsistent between 
> with --index and without as far as I can tell.

For the same reason that I like git-merge-file, and git-diff2, namely to 
have a _sane_ tool with a lot of options, which works the same everywhere 
I have git, I also like git-apply.

And I use git-apply to apply patches way more often than "patch" these 
days. And I _think_ that it is a feature that it does not cd-up before 
trying to apply the stuff. In git.git, I cannot think of a reasonable use 
case for applying something not relative-to-root, but I had that use case 
in some other (git-tracked) project.

So my vote is to leave the cwd where it is in git-apply.

Ciao,
Dscho

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

[Junio C Hamano]

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

> And I use git-apply to apply patches way more often than "patch" these 
> days. And I _think_ that it is a feature that it does not cd-up before 
> trying to apply the stuff. In git.git, I cannot think of a reasonable use 
> case for applying something not relative-to-root, but I had that use case 
> in some other (git-tracked) project.
>
> So my vote is to leave the cwd where it is in git-apply.

I strongly disagree from my recent day-job experience.  I was
feeding some changes to my co-worker's repository from my
uncommitted changes (because it was incomplete but the part
needed to unstuck him was ready).  The day job project is much
deeper than git.git, and the changes were to two files in a
directory somewhat deep.

So I went there and said "git apply --index P.diff", which
applied cleanly.  But the other "git apply --index Q.diff"
didn't.

So naturally I said:

	$ git apply --reject Q.diff
        error: filfre/frotz/nitfol.c: No such file or directory

I ended up editing filfre/frotz/ out of Q.diff in his editor.
Explaining why P.diff and Q.diff, both of which were about the
files in the same dirrectly, behaved differently was not pretty
to git uninitiated.

Leaving --index case and working-tree-only case inconsistent is
bad.  We really should fix it (I really wish I found it out
before 1.5.0 went out).

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > And I use git-apply to apply patches way more often than "patch" these 
> > days. And I _think_ that it is a feature that it does not cd-up before 
> > trying to apply the stuff. In git.git, I cannot think of a reasonable use 
> > case for applying something not relative-to-root, but I had that use case 
> > in some other (git-tracked) project.
> >
> > So my vote is to leave the cwd where it is in git-apply.
> 
> I strongly disagree from my recent day-job experience. [Explains a 
> convincing use case in favour of cd-up.]

Hmm. I have to think about that. But a consequence of what you suggest 
would be to disallow git-apply outside of a repository, because then you 
would introduce _another_ inconsistency (git-apply without --index would 
behave differently when inside a repo than when outside of one).

Ciao,
Dscho

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

[Junio C Hamano]

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

>> I strongly disagree from my recent day-job experience. [Explains a 
>> convincing use case in favour of cd-up.]
>
> Hmm. I have to think about that. But a consequence of what you suggest 
> would be to disallow git-apply outside of a repository, because then you 
> would introduce _another_ inconsistency (git-apply without --index would 
> behave differently when inside a repo than when outside of one).

Why?  git-apply outside is like 'patch' isn't it?

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

[Johannes Schindelin]

Hi,

On Sat, 17 Feb 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> I strongly disagree from my recent day-job experience. [Explains a 
> >> convincing use case in favour of cd-up.]
> >
> > Hmm. I have to think about that. But a consequence of what you suggest 
> > would be to disallow git-apply outside of a repository, because then you 
> > would introduce _another_ inconsistency (git-apply without --index would 
> > behave differently when inside a repo than when outside of one).
> 
> Why?  git-apply outside is like 'patch' isn't it?

So, why shouldn't it be the same inside? That's the inconsistency I am 
alluding to.

Ciao,
Dscho

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

[Junio C Hamano]

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

> On Sat, 17 Feb 2007, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> >> I strongly disagree from my recent day-job experience. [Explains a 
>> >> convincing use case in favour of cd-up.]
>> >
>> > Hmm. I have to think about that. But a consequence of what you suggest 
>> > would be to disallow git-apply outside of a repository, because then you 
>> > would introduce _another_ inconsistency (git-apply without --index would 
>> > behave differently when inside a repo than when outside of one).
>> 
>> Why?  git-apply outside is like 'patch' isn't it?
>
> So, why shouldn't it be the same inside? That's the inconsistency I am 
> alluding to.

It appears that you are saying we cannot make it consistent in
both ways, and you are probably right.

I think git should cater first to git users who use git in git
repositories, which means consistency inside a git repository
between with --index and without is more important.

We can certainly teach 'git apply' without --index/--cache a new
"stay where you are" option to behave like patch (but do that
only without --index/--cache) inside a git repository for
special cases.