[PATCH] Add a test-case for git-apply trying to add an ending line

[PATCH] Add a test-case for git-apply trying to add an ending line

[Catalin Marinas]

From: Catalin Marinas <catalin.marinas@gmail.com>

git-apply adding an ending line doesn't seem to fail if the ending line is
already present in the patched file.

Signed-off-by: Catalin Marinas <catalin.marinas@gmail.com>
---

 t/t4113-apply-ending.sh |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
new file mode 100755
index 0000000..d021ae8
--- /dev/null
+++ b/t/t4113-apply-ending.sh
@@ -0,0 +1,35 @@
+#!/bin/sh
+#
+# Copyright (c) 2006 Catalin Marinas
+#
+
+test_description='git-apply trying to add an ending line.
+
+'
+. ./test-lib.sh
+
+# setup
+
+cat >test-patch <<\EOF
+diff --git a/file b/file
+--- a/file
++++ b/file
+@@ -1,2 +1,3 @@
+ a
+ b
++c
+EOF
+
+echo 'a' >file
+echo 'b' >>file
+echo 'c' >>file
+
+test_expect_success setup \
+    'git-update-index --add file'
+
+# test
+
+test_expect_failure apply \
+    'git-apply --index test-patch'
+
+test_done

Re: [PATCH] Add a test-case for git-apply trying to add an ending line

[Junio C Hamano]

Hmmmmm.

To git-apply, this patch:

        diff --git a/file b/file
        --- a/file
        +++ b/file
        @@ -1,2 +1,3 @@
         a
         b
        +c

currently means "I want to append a line c immediately after the
lines that have a and then b".  Nothing else.  And applying it
to

	a
        b
        c

produces

	a
        b
        c
        c

The first c is what your patch added, and the second c is what
was originally there.

I do not think this is necessarily a bug.  You _could_ make the
EOF a special case (i.e. you _could_ interpret the patch that it
also says, with "@@ -1,2", that "the result of this patch _must_
end with this line I just added"), and if you are going to do
that, you would also need a symmetric special case for the
beginning of the file, but I do not think it is the right thing
to do in general.

Realistically, you would have something like this:

        diff --git a/apply.c b/apply.c
        index 0ed9d13..f99c6fe 100644
        --- a/apply.c
        +++ b/apply.c
        @@ -2297,3 +2297,8 @@ int main(int argc, char **argv)

                return 0; /* end of main */
         }
        +
        +static void this_function_is_unused(void)
        +{
        +	printf("hello, world\n");
        +}

You added a useless function at the end of the file.

While you prepared the above patch, the upstream updated the
same file to end like this:

                return 0; /* end of main */
        }

        static int some_new_function(void)
        {
                return 314;
        }

Now, git-apply would produce this file if you apply the above
patch:

                return 0; /* end of main */
        }

        static void this_function_is_unused(void)
        {
                printf("hello, world\n");
        }

        static int some_new_function(void)
        {
                return 314;
        }

I think this current behaviour is more desirable than special
casing the end of file and refusing to apply.

In this particular case, expecting failure like your new test
does is somewhat understandable, but if you change the test case
to start with this file, you would realize that your expectation
is not the only valid understanding of what is really happening:

	echo 'a' >file
	echo 'b' >>file
	echo 'd' >>file

Applying test-patch to this would result in

	a
        b
        c
        d

which I think is more useful behaviour.

Re: [PATCH] Add a test-case for git-apply trying to add an ending line

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> To git-apply, this patch:
>
>         diff --git a/file b/file
>         --- a/file
>         +++ b/file
>         @@ -1,2 +1,3 @@
>          a
>          b
>         +c
>
> currently means "I want to append a line c immediately after the
> lines that have a and then b".
>...
> I do not think this is necessarily a bug.  You _could_ make the
> EOF a special case (i.e. you _could_ interpret the patch that it
> also says, with "@@ -1,2", that "the result of this patch _must_
> end with this line I just added"), and if you are going to do
> that, you would also need a symmetric special case for the
> beginning of the file, but I do not think it is the right thing
> to do in general.

Come to think of it, the above argument is bogus.  We _would_
want to make EOF just like any other context lines.

The issue is if we can reliably tell if there is such an EOF
context by looking at the diff.  Not having the same number of
lines that starts with ' ' in the hunk is not really a nice way
of doing so (you could make a unified diff that does not have
trailing context at all), and I do not offhand think of a good
way to do so.

Re: [PATCH] Add a test-case for git-apply trying to add an ending line

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> To git-apply, this patch:
>
>         diff --git a/file b/file
>         --- a/file
>         +++ b/file
>         @@ -1,2 +1,3 @@
>          a
>          b
>         +c
>
> currently means "I want to append a line c immediately after the
> lines that have a and then b".
>...
> I do not think this is necessarily a bug.  You _could_ make the
> EOF a special case (i.e. you _could_ interpret the patch that it
> also says, with "@@ -1,2", that "the result of this patch _must_
> end with this line I just added"), and if you are going to do
> that, you would also need a symmetric special case for the
> beginning of the file, but I do not think it is the right thing
> to do in general.

Come to think of it, the above argument is bogus.  We _would_
want to make EOF just like any other context lines.

The issue is if we can reliably tell if there is such an EOF
context by looking at the diff.  Not having the same number of
lines that starts with ' ' in the hunk is not really a nice way
of doing so (you could make a unified diff that does not have
trailing context at all), and I do not offhand think of a good
way to do so.

Re: [PATCH] Add a test-case for git-apply trying to add an ending line

[Linus Torvalds]

On Tue, 23 May 2006, Junio C Hamano wrote:
> 
> Come to think of it, the above argument is bogus.  We _would_
> want to make EOF just like any other context lines.
> 
> The issue is if we can reliably tell if there is such an EOF
> context by looking at the diff.  Not having the same number of
> lines that starts with ' ' in the hunk is not really a nice way
> of doing so (you could make a unified diff that does not have
> trailing context at all), and I do not offhand think of a good
> way to do so.

We can. Something like this should do it.

(The same thing could be done for "match_beginning", perhaps).

Totally untested, of course.

(It might be better to pass in "match_end" to find_offset(), so that it 
could do the "look forwards" pass to see if it finds a better line offset 
that is at the end - as it is, this will _fail_ the patch if it could 
apply better at a non-end thing, even if it would _also_ have applied at 
the end of the file).

		Linus
---
diff --git a/apply.c b/apply.c
index 0ed9d13..905bf34 100644
--- a/apply.c
+++ b/apply.c
@@ -1333,6 +1333,7 @@ static int apply_line(char *output, cons
 
 static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag)
 {
+	int match_end;
 	char *buf = desc->buffer;
 	const char *patch = frag->patch;
 	int offset, size = frag->size;
@@ -1395,10 +1396,20 @@ #endif
 	newlines = new;
 	leading = frag->leading;
 	trailing = frag->trailing;
+
+	/*
+	 * If we don't have any trailing data in the patch,
+	 * we want to match the final ending '\0' byte in
+	 * the file too..
+	 */
+	match_end = !trailing;
+
 	lines = 0;
 	pos = frag->newpos;
 	for (;;) {
 		offset = find_offset(buf, desc->size, oldlines, oldsize, pos, &lines);
+		if (match_end && offset + oldsize != desc->size)
+			offset = -1;
 		if (offset >= 0) {
 			int diff = newsize - oldsize;
 			unsigned long size = desc->size + diff;
@@ -1428,6 +1439,10 @@ #endif
 		/* Am I at my context limits? */
 		if ((leading <= p_context) && (trailing <= p_context))
 			break;
+		if (match_end) {
+			match_end = 0;
+			continue;
+		}
 		/* Reduce the number of context lines
 		 * Reduce both leading and trailing if they are equal
 		 * otherwise just reduce the larger context.

Re: [PATCH] Add a test-case for git-apply trying to add an ending line

[Linus Torvalds]

On Tue, 23 May 2006, Linus Torvalds wrote:
>
> +	/*
> +	 * If we don't have any trailing data in the patch,
> +	 * we want to match the final ending '\0' byte in
> +	 * the file too..
> +	 */

Btw, ignore the comment. I was thinking of doing the matching differently 
(just make the source buffer include a '\0' at the end, and forcing that 
to match), but once I actually wrote it, it ended up being much easier to 
just check the offset/size difference.

So that "final ending '\0' byte in the file" part of the comment is just 
nonsense.

		Linus

Re: [PATCH] Add a test-case for git-apply trying to add an ending line

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Tue, 23 May 2006, Junio C Hamano wrote:
>
>> The issue is if we can reliably tell if there is such an EOF
>> context by looking at the diff.  Not having the same number of
>> lines that starts with ' ' in the hunk is not really a nice way
>> of doing so (you could make a unified diff that does not have
>> trailing context at all), and I do not offhand think of a good
>> way to do so.
>
> We can. Something like this should do it.
>
> (The same thing could be done for "match_beginning", perhaps).

But this is exactly what I said I had trouble with in the above.

In the extreme case, wouldn't this make git apply to refuse to
apply a self generated patch with 0-line context?  IOW,

        $ git checkout -- foo ;# reset to indexed version
	$ edit foo
        $ git diff -U0 >P.diff
        $ git checkout -- foo ;# reset to indexed version
        $ git apply <P.diff

would fail, even though it _should_ cleanly apply.

I'd admit that trying to apply a patch without context like the
above example _is_ insane, and I realize that I am making this
problem unsolvable by refusing the heuristics to consider that
the patch is anchored at the end when we do not see any trailing
context.  But somehow it feels wrong...

Re: [PATCH] Add a test-case for git-apply trying to add an ending line

[Catalin Marinas]

On 24/05/06, Junio C Hamano <junkio@cox.net> wrote:
> I'd admit that trying to apply a patch without context like the
> above example _is_ insane, and I realize that I am making this
> problem unsolvable by refusing the heuristics to consider that
> the patch is anchored at the end when we do not see any trailing
> context.  But somehow it feels wrong...

The reason I sent you this test is that GNU patch fails to apply the
diff but git-apply succeeds (and I thought git-apply is more
restrictive).

When there are context lines either before or after the "+" line, it
should be OK to assume that the diff has context and therefore the EOF
should be considered.

If there are no context lines at all, the diff is either without
context or it is meant to patch an empty file. The latter is safer and
probably valid for most of the cases but if you have a patch without
context, you could explicitely pass the -C0 option to git-apply.

-- 
Catalin

Re: [PATCH] Add a test-case for git-apply trying to add an ending line

[Linus Torvalds]

On Tue, 23 May 2006, Junio C Hamano wrote:

> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > On Tue, 23 May 2006, Junio C Hamano wrote:
> >
> >> The issue is if we can reliably tell if there is such an EOF
> >> context by looking at the diff.  Not having the same number of
> >> lines that starts with ' ' in the hunk is not really a nice way
> >> of doing so (you could make a unified diff that does not have
> >> trailing context at all), and I do not offhand think of a good
> >> way to do so.
> >
> > We can. Something like this should do it.
> >
> > (The same thing could be done for "match_beginning", perhaps).
> 
> But this is exactly what I said I had trouble with in the above.

Well, not quite. You said "not the same number of lines", and I say "no 
ending context". Very different.

My patch actually is totally self-consistent: not having any context at 
the end of a unified diff really means that it is the end of the file (ie, 
the "end of file" there _is_ the context). And if you want to apply files 
without context, you should use "-Cx", and my patch does that too - if you 
asked for "relaxed context checking", it will re-try without the "only at 
end" check thanks to the

	if (match_end) {
		match_end = 0;
		continue;
	}

so it all should work.

Not that I _tested_ it, of course ;)

		Linus