patchAdd a test-case for git-apply trying to add an ending line

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(-)

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.

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

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.

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

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.

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

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

On Tue, 23 May 2006, Linus Torvalds wrote:

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

Linus Torvalds <torvalds@osdl.org> writes:

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

On 24/05/06, Junio C Hamano <junkio@cox.net> wrote:

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.

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

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