# [PATCH 2/9] Fix detection of files with only one line in git-apply. 11 messages from 2005-08-28 to 2005-08-30. Participants: Robert Fitzsimons, Linus Torvalds, Junio C Hamano, Martin Langhoff. Thread: https://gitlist.dev/t/1679 ## Robert Fitzsimons, 2005-08-28 15:24 Subject: [PATCH 2/9] Fix detection of files with only one line in git-apply. Message-ID: <1125242667404-git-send-email-robfitz@273k.net> URL: https://gitlist.dev/e/1125242667404-git-send-email-robfitz%40273k.net In-Reply-To: <11252426672473-git-send-email-robfitz@273k.net> ``` 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 --- 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 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 +EOF +cat > patch3.patch <<\EOF +diff --git a/main.c b/main.c +--- a/main.c ++++ b/main.c +@@ -1,1 +0,0 @@ +-#include +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 + ``` ## Robert Fitzsimons, 2005-08-28 15:24 Subject: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: <11252426672473-git-send-email-robfitz@273k.net> URL: https://gitlist.dev/e/11252426672473-git-send-email-robfitz%40273k.net ``` 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 --- apply.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) 289e53f770e37dfe25c740788256d24c19c2e16d 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 (;;) { ``` ## Linus Torvalds, 2005-08-28 16:55 Subject: Re: [PATCH 2/9] Fix detection of files with only one line in git-apply. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0508280946180.3317%40g5.osdl.org In-Reply-To: <1125242667404-git-send-email-robfitz@273k.net> ``` 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, 2005-08-28 23:39 Subject: Re: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: <7vslwtein3.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vslwtein3.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <11252426672473-git-send-email-robfitz@273k.net> ``` 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, 2005-08-29 23:58 Subject: Re: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: <20050829235823.GA19351@localhost> URL: https://gitlist.dev/e/20050829235823.GA19351%40localhost In-Reply-To: <7vslwtein3.fsf@assigned-by-dhcp.cox.net> ``` 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, 2005-08-30 00:47 Subject: Re: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0508291744400.3243%40g5.osdl.org In-Reply-To: <20050829235823.GA19351@localhost> ``` 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, 2005-08-30 01:09 Subject: Re: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: <7vacj06xi6.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vacj06xi6.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > 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, 2005-08-30 01:24 Subject: Re: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0508291821010.3243%40g5.osdl.org In-Reply-To: <7vacj06xi6.fsf@assigned-by-dhcp.cox.net> ``` 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. --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, 2005-08-30 01:34 Subject: Re: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: <7vbr3g5hsb.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vbr3g5hsb.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > 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, 2005-08-30 02:00 Subject: Re: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.58.0508291856080.3243%40g5.osdl.org In-Reply-To: <7vbr3g5hsb.fsf@assigned-by-dhcp.cox.net> ``` 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, 2005-08-30 07:36 Subject: Re: [PATCH 1/9] Fix git patch header processing in git-apply. Message-ID: <46a038f905083000362ae0d9fc@mail.gmail.com> URL: https://gitlist.dev/e/46a038f905083000362ae0d9fc%40mail.gmail.com In-Reply-To: ``` On 8/30/05, Linus Torvalds 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. . 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 ```