threads / patch / 1679

patch, 9 partsFix detection of files with only one line in git-apply.

Subject: [PATCH 2/9] Fix detection of files with only one line in git-apply.

## tl;dr

11 messages between Aug 28, 2005 and Aug 30, 2005. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Robert Fitzsimons· Aug 28, 2005, 15:24 UTC · lore

[PATCH 1/9] Fix git patch header processing in git-apply.

Stop processing and return NULL if we encounter a '\n' character before we have two matching names in the git header.

Signed-off-by: Robert Fitzsimons <robfitz@273k.net>
---
 apply.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
289e53f770e37dfe25c740788256d24c19c2e16d
Show changes to apply.c +1 −1
diff --git a/apply.c b/apply.c
--- a/apply.c
+++ b/apply.c
@@ -387,7 +387,7 @@ static char *git_header_name(char *line)
 		default:
 			continue;
 		case '\n':
-			break;
+			return NULL;
 		case '\t': case ' ':
 			second = name+len;
 			for (;;) {
Robert Fitzsimons· Aug 28, 2005, 15:24 UTC · re: Robert Fitzsimons · lore

A patch which added one line to an empty file or removed the only line from a one line file would be incorrectly detected as an invalid new or delete patch. Added a new test case.

Signed-off-by: Robert Fitzsimons <robfitz@273k.net>
---
 apply.c                  |    4 ++--
 t/t4103-apply-oneline.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)
 create mode 100644 t/t4103-apply-oneline.sh
babd40afef54b06775b11a877b347685edbedcd2
Show changes to 2 files +45 −2

apply.c, t/t4103-apply-oneline.sh

diff --git a/apply.c b/apply.c
--- a/apply.c
+++ b/apply.c
@@ -638,9 +638,9 @@ static int parse_fragment(char *line, un
 			patch->new_name = NULL;
 	}
 
-	if (patch->is_new != !oldlines)
+	if (patch->is_new && (patch->is_new != !oldlines))
 		return error("new file depends on old contents");
-	if (patch->is_delete != !newlines) {
+	if (patch->is_delete && (patch->is_delete != !newlines)) {
 		if (newlines)
 			return error("deleted file still has contents");
 		fprintf(stderr, "** warning: file %s becomes empty but is not deleted\n", patch->new_name);
diff --git a/t/t4103-apply-oneline.sh b/t/t4103-apply-oneline.sh
new file mode 100644
--- /dev/null
+++ b/t/t4103-apply-oneline.sh
@@ -0,0 +1,46 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+# Copyright (c) 2005 Robert Fitzsimons
+#
+
+test_description='git-apply files with one line.
+
+'
+. ./test-lib.sh
+
+# setup
+
+cat > patch1.patch <<\EOF
+diff --git a/main.c b/main.c
+new file mode 100644
+--- /dev/null
++++ b/main.c
+@@ -0,0 +0,0 @@
+EOF
+cat > patch2.patch <<\EOF
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -0,0 +1,1 @@
++#include <stdio.h>
+EOF
+cat > patch3.patch <<\EOF
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -1,1 +0,0 @@
+-#include <stdio.h>
+EOF
+
+test_expect_success "S = patch1 (new file with no lines)" \
+    'git-apply patch1.patch'
+
+test_expect_success "S = patch2 (add one line)" \
+    'git-apply patch2.patch'
+
+test_expect_success "S = patch3 (delete one line)" \
+    'git-apply patch3.patch'
+
+test_done
+
Linus Torvalds· Aug 28, 2005, 16:55 UTC · re: Robert Fitzsimons · lore

Re: [PATCH 2/9] Fix detection of files with only one line in git-apply.

On Sun, 28 Aug 2005, Robert Fitzsimons wrote:
>
> A patch which added one line to an empty file or removed the only line
> from a one line file would be incorrectly detected as an invalid new
> or delete patch.  Added a new test case.

This patch looks wrong or at least needs some more thought. The test was done the way it was done for a reason. For example:

	if (patch->is_new != !oldlines)
		return error("new file depends on old contents");

actually has two error cases (admittedly it re-uses the same error message for both, which is a bit confusing):

 - is_new, and oldlines != 0
	This is the obvious error, and the one the message talk about.
 - !is_new, and oldlines == 0
	In kernel usage, empty files are not legal, and empty files get 
	deleted. So this actually was intentional, even if the error 
	message ends up being less than perfect.

The "is_delete" thing is the same. At least for the kernel, I want a patch that removes all content to also always _delete_ the file, because otherwise old-style patches are ambiguous. Is it a delete, or is it a "leave an empty file"?

Now, git patches aren't ambigious, so while git patches can clearly say "this is a patch to an existing empty file" vs "this patch creates this file", I wanted to have the behaviour where we maintain the principle that empty files are wrong.

Now, that's a kernel-specific thing, so it may be that enabling this behaviour this should be a command line flag, but the point is that I don't want to entirely lose the test.

		Linus
Junio C Hamano· Aug 28, 2005, 23:39 UTC · re: Robert Fitzsimons · lore

Re: [PATCH 1/9] Fix git patch header processing in git-apply.

Thanks for "apply" patches. I have merged some of them and pushed out.

I further wanted to apply the following to the proposed updates branch, but even with only the first one applied, it fails its own test, t4104-apply-complex.sh:

* Fix processing of a patch file which modifies the same file in git-apply.
* Fix the processing of multiple patch files with --check in git-apply.
* New option --ignore-whitespace for git-apply.
* New option --ignore-applied for git-apply.
*** t4104-apply-complex.sh ***
* FAIL 5: S = Nx Px1
	cat Nx Px1 | git-apply -
* FAIL 6: S = Nx Px1 Px2
	cat Nx Px1 Px2 | git-apply -
* FAIL 7: S = Nx Px1 Px2 Px3
	cat Nx Px1 Px2 Px3 | git-apply -
* FAIL 8: S = Nx Px1 Px2 Px3 Dx
	cat Nx Px1 Px2 Px3 Dx | git-apply -
* FAIL 13: S = Ny Ryx Px1 Px2 Cxy Px3 Dx
	cat Ny Ryx Px1 Px2 Cxy Px3 Dx | git-apply -
* FAIL 14: S = Ny Ryx Px1 Px2 Cxy Px3 Dx Cyx
	cat Ny Ryx Px1 Px2 Cxy Px3 Dx Cyx | git-apply -
* FAIL 15: S = Ny Cyx Px1 Px2 Px3 Dy Cxy Dx Cyx Dy Rxy Dy
	cat Ny Cyx Px1 Px2 Px3 Dy Cxy Dx Cyx Dy Rxy Dy| git-apply -
Robert Fitzsimons· Aug 29, 2005, 23:58 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/9] Fix git patch header processing in git-apply.

I should have mentioned before, these changes are being used in a SourcePuller dump file to git import script I've been working on.

> I further wanted to apply the following to the proposed updates
> branch, but even with only the first one applied, it fails its
> own test, t4104-apply-complex.sh:
The test was the problem it tried to work with empty files.
> * Fix processing of a patch file which modifies the same file in git-apply.
> * Fix the processing of multiple patch files with --check in git-apply.
> * New option --ignore-applied for git-apply.
I've fixed up the problems with these patches and rebased them.
> * New option --ignore-whitespace for git-apply.

This change was just wrong and could cause file corruption, I've got another version in the works which fixes the problem and adds some of the suggested features.

Robert Fitzsimons robfitz@273k.net

Linus Torvalds· Aug 30, 2005, 00:47 UTC · re: Robert Fitzsimons · lore

Re: [PATCH 1/9] Fix git patch header processing in git-apply.

On Mon, 29 Aug 2005, Robert Fitzsimons wrote:
>
> I should have mentioned before, these changes are being used in a
> SourcePuller dump file to git import script I've been working on.

Hmm. Anything that depends on "--ignore-applied" is fundamentally broken as an import tool. There's no way something like that can be valid, since it inhently means that somebody is trying to apply a patch that simple isn't applicable.

So it seems like you're trying to work around some other serious problems in the import, and as a result the import is clearly not trustworthy. 

Don't do this.
			Linus
Junio C Hamano· Aug 30, 2005, 01:09 UTC · re: Linus Torvalds · lore

Re: [PATCH 1/9] Fix git patch header processing in git-apply.

Linus Torvalds <torvalds@osdl.org> writes:
Show 6 quoted lines
> Hmm. Anything that depends on "--ignore-applied" is fundamentally broken
> as an import tool. There's no way something like that can be valid, since
> it inhently means that somebody is trying to apply a patch that simple
> isn't applicable.
>
> Don't do this.

I suspect that it is exported data that is broken, if this --ignore-applied flag is needed for importing something somebody exported.

I, however, am not 100% convinced --ignore-applied is too dangerous to be useful in any conceivable use cases, at least not yet. For example, you might be cherry-picking a change from a foreign branch with 'git-diff-tree -p other~45 | git apply', where the current branch may have already aquired part of the changes that foreign branch contains since 45 generations ago.

Linus Torvalds· Aug 30, 2005, 01:24 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/9] Fix git patch header processing in git-apply.

On Mon, 29 Aug 2005, Junio C Hamano wrote:
Show 7 quoted lines
> 
> I, however, am not 100% convinced --ignore-applied is too
> dangerous to be useful in any conceivable use cases, at least
> not yet.  For example, you might be cherry-picking a change from
> a foreign branch with 'git-diff-tree -p other~45 | git apply',
> where the current branch may have already aquired part of the
> changes that foreign branch contains since 45 generations ago.

.. and that's why we use a three-way merge, and not diffs, to merge two branches.

--ignore-applied really is a pretty dangerous thing. I can see that it might do the right thing in many cases, but definitely not in a scripted environment, much less in an "import".

By definition, importing stuff must apply, since it clearly applied in whatever original source control tool you are importing from. So if an importer needs --ignore-applied, it is fundamentally doing something wrong.

And I just realized what "SourcePuller" is. Gaah. Trust me, that export is bad, and the original source control tool did everything very well indeed.

			Linus
Junio C Hamano· Aug 30, 2005, 01:34 UTC · re: Linus Torvalds · lore

Re: [PATCH 1/9] Fix git patch header processing in git-apply.

Linus Torvalds <torvalds@osdl.org> writes:
Show 11 quoted lines
> On Mon, 29 Aug 2005, Junio C Hamano wrote:
>> 
>> I, however, am not 100% convinced --ignore-applied is too
>> dangerous to be useful in any conceivable use cases, at least
>> not yet.  For example, you might be cherry-picking a change from
>> a foreign branch with 'git-diff-tree -p other~45 | git apply',
>> where the current branch may have already aquired part of the
>> changes that foreign branch contains since 45 generations ago.
>
> .. and that's why we use a three-way merge, and not diffs, to merge two 
> branches.

.. and that's why I recently rewrote revert, cherry-pick and rebase to use three-way merge as well ;-).

The code is simple enough and I see some beauty in it, but I honor your veto, at least for now.

Linus Torvalds· Aug 30, 2005, 02:00 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/9] Fix git patch header processing in git-apply.

On Mon, 29 Aug 2005, Junio C Hamano wrote:
> 
> The code is simple enough and I see some beauty in it, but I
> honor your veto, at least for now.

Well, I didn't actually veto it when I saw the feature the first time, because I think it's a valid thing to do when applying patches by hand.

It was only when I realized that Robert was trying to use it to import from somewhere else that it became broken. An import should use the most anal patches possible.

Actually, an import should preferably never use patches at all, since that never really works for binary data. This is why the CVS importer actually checks out full files and imports them that way. The patch-based ones will fundamentally always have problems.

		Linus
Martin Langhoff· Aug 30, 2005, 07:36 UTC · re: Linus Torvalds · lore

Re: [PATCH 1/9] Fix git patch header processing in git-apply.

On 8/30/05, Linus Torvalds <torvalds@osdl.org> wrote:
> Actually, an import should preferably never use patches at all, since that
> never really works for binary data. This is why the CVS importer actually
> checks out full files and imports them that way. The patch-based ones will
> fundamentally always have problems.

<AOL>. Patches can be useful during the process, but you should (a) prefer more solid manifestations of the file/tree if possible, and (b) be extremely strict with them. Any hint that they don't match means that something is borked.

However, if you're importing from patch-oriented SCMs that tend to 'replay' patch sequences from one branch to the other, asking git to skip applied patches *could* be a shortcut. Not 100% correctness guarantee, but if the resulting history makes sense, it can be useful.

The other alternative is to do what I'll start to do soon on the archimport script: keep a tally of what was merged where according to the records that the source SCM has.

cheers,
martin

← back to recent threads