{"thread":{"id":"13433","subject":"[PATCH] builtin-apply: check for empty files when detecting creation patch","startedAt":"2008-05-08T14:39:39Z","lastAt":"2008-05-17T09:19:13Z","messageCount":12,"participants":["Imre Deak","Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"76374","messageId":"1210257579-30975-1-git-send-email-imre.deak@gmail.com","threadId":"13433","inReplyTo":null,"subject":"[PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Imre Deak","fromEmail":"imre.deak@gmail.com","sentAt":"2008-05-08T14:39:39Z","receivedAt":"2008-05-08T14:39:39Z","isPatch":true,"sender":{"key":"imre.deak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1049948?v=4"},"body":"When we can only guess if we have a creation patch, we do\nthis by treating the patch as such if there weren't any old\nlines. Zero length files can be patched without old lines\nthough, so do an extra check for file size.\n\nSigned-off-by: Imre Deak <imre.deak@gmail.com>\n---\n builtin-apply.c |   34 +++++++++++++++++++++++++++++++++-\n 1 files changed, 33 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex caa3f2a..80d0779 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1096,6 +1096,33 @@ static int parse_fragment(char *line, unsigned long size,\n \treturn offset;\n }\n \n+static size_t existing_file_size(const char *file)\n+{\n+\tsize_t st_size = -1;\n+\n+\tif (file == NULL)\n+\t\treturn -1;\n+\n+\tif (cached) {\n+\t\tstruct cache_entry *ce;\n+\t\tint pos;\n+\n+\t\tpos = cache_name_pos(file, strlen(file));\n+\t\tif (pos < 0)\n+\t\t\treturn -1;\n+\t\tce = active_cache[pos];\n+\t\tst_size = ntohl(ce->ce_size);\n+\t} else {\n+\t\tstruct stat st;\n+\n+\t\tif (lstat(file, &st) < 0)\n+\t\t\treturn -1;\n+\t\tst_size = st.st_size;\n+\t}\n+\n+\treturn st_size;\n+}\n+\n static int parse_single_patch(char *line, unsigned long size, struct patch *patch)\n {\n \tunsigned long offset = 0;\n@@ -1143,13 +1170,18 @@ static int parse_single_patch(char *line, unsigned long size, struct patch *patc\n \tif (patch->is_delete < 0 &&\n \t    (newlines || (patch->fragments && patch->fragments->next)))\n \t\tpatch->is_delete = 0;\n+\t/* FIXME: How can be there context if it's a creation / deletion? */\n \tif (!unidiff_zero || context) {\n \t\t/* If the user says the patch is not generated with\n \t\t * --unified=0, or if we have seen context lines,\n \t\t * then not having oldlines means the patch is creation,\n \t\t * and not having newlines means the patch is deletion.\n+\t\t *\n+\t\t * It's also possible that a zero length file is added\n+\t\t * to.\n \t\t */\n-\t\tif (patch->is_new < 0 && !oldlines) {\n+\t\tif (patch->is_new < 0 && !oldlines &&\n+\t\t    existing_file_size(patch->old_name) != 0) {\n \t\t\tpatch->is_new = 1;\n \t\t\tpatch->old_name = NULL;\n \t\t}\n-- \n1.5.4.2.dirty\n"},{"id":"76568","messageId":"7vlk2h8t4d.fsf@gitster.siamese.dyndns.org","threadId":"13433","inReplyTo":"1210257579-30975-1-git-send-email-imre.deak@gmail.com","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-11T02:36:02Z","receivedAt":"2008-05-11T02:36:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Imre Deak <imre.deak@gmail.com> writes:\n\n> When we can only guess if we have a creation patch, we do\n> this by treating the patch as such if there weren't any old\n> lines. Zero length files can be patched without old lines\n> though, so do an extra check for file size.\n\nYou described what your patch does, but you did not explain why it is a\ngood addition.  One way to do so is to illustrate in what occasion what\nthe existing code does is insufficient.\n\n> +static size_t existing_file_size(const char *file)\n> +{\n> +\tsize_t st_size = -1;\n> +\n> +\tif (file == NULL)\n> +\t\treturn -1;\n> +\tif (cached) {\n> +\t\tstruct cache_entry *ce;\n> +\t\tint pos;\n> +\n> +\t\tpos = cache_name_pos(file, strlen(file));\n> +\t\tif (pos < 0)\n> +\t\t\treturn -1;\n> +\t\tce = active_cache[pos];\n> +\t\tst_size = ntohl(ce->ce_size);\n\nntohl()?  I thought ce->ce_* are host-native byte order these days...\n\n> +\t} else {\n> +\t\tstruct stat st;\n> +\n> +\t\tif (lstat(file, &st) < 0)\n> +\t\t\treturn -1;\n\nDoesn't this break the use case where \"git-apply --stat\" is used as an\nimproved diffstat outside a git repository?\n\n> @@ -1143,13 +1170,18 @@ static int parse_single_patch(char *line, unsigned long size, struct patch *patc\n>  \tif (patch->is_delete < 0 &&\n>  \t    (newlines || (patch->fragments && patch->fragments->next)))\n>  \t\tpatch->is_delete = 0;\n> +\t/* FIXME: How can be there context if it's a creation / deletion? */\n>  \tif (!unidiff_zero || context) {\n>  \t\t/* If the user says the patch is not generated with\n>  \t\t * --unified=0, or if we have seen context lines,\n>  \t\t * then not having oldlines means the patch is creation,\n>  \t\t * and not having newlines means the patch is deletion.\n> +\t\t *\n> +\t\t * It's also possible that a zero length file is added\n> +\t\t * to.\n>  \t\t */\n> -\t\tif (patch->is_new < 0 && !oldlines) {\n> +\t\tif (patch->is_new < 0 && !oldlines &&\n> +\t\t    existing_file_size(patch->old_name) != 0) {\n>  \t\t\tpatch->is_new = 1;\n>  \t\t\tpatch->old_name = NULL;\n>  \t\t}\n\nThe user did not say the patch was produced without context, or we do have\ncontext.  The latter cannot be a creation patch so the new logic is not\nappropriate.  But let's forget that problem for now and look at the case\nwhere the patch did _not_ have any context, i.e. only added and deleted\nlines.\n\nIf the patch did not have context, and the user did not ask for -u0 patch\nwhen it was produced, it could be a creation patch, but if there are\ndeleted lines it cannot be.  That is the original logic.\n\nAfter your patch, the original logic is allowed to decide that the patch\nis a creation _only if_ you happen to already have a file that is _to be\ncreated_ in the work tree with some existing contents, or the file does\nnot exist.  I do not see a sane logic behind that.  If you were making\nsure that the work tree does _not_ have the file, then I would understand,\neven though I think it is wrong for \"apply --stat\" case.  If you see a\nfile in the work tree, and if you assume the patch would apply to the\nwork tree, then the patch cannot be creation!\n\nIn general, it is not right to look at the work tree to decide how to\ninterpret what the patch means to begin with, but maybe you are trying to\nuse work tree status as a hint to disambiguate a corner case that the\ninformation in a patch we are reading is insufficient, in which case it\nmight be Ok.  But I cannot tell what that corner case is.\n\nI am lost.  Please explain what you are trying to fix first before\nexplaining how you attempted to fix it.\n"},{"id":"76881","messageId":"500f3d130805131316m59898392l46e0dbf7cb352981@mail.gmail.com","threadId":"13433","inReplyTo":"7vlk2h8t4d.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Imre Deak","fromEmail":"imre.deak@gmail.com","sentAt":"2008-05-13T20:16:11Z","receivedAt":"2008-05-13T20:16:11Z","isPatch":true,"sender":{"key":"imre.deak@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1049948?v=4"},"body":"Hi,\n\nthanks for your answer.\n\nOn Sun, May 11, 2008 at 5:36 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Imre Deak <imre.deak@gmail.com> writes:\n>\n>  > When we can only guess if we have a creation patch, we do\n>  > this by treating the patch as such if there weren't any old\n>  > lines. Zero length files can be patched without old lines\n>  > though, so do an extra check for file size.\n>\n>  You described what your patch does, but you did not explain why it is a\n>  good addition.  One way to do so is to illustrate in what occasion what\n>  the existing code does is insufficient.\n\nThe patch makes it possible to apply foreign patches (not created with\ngit diff) to zero length files already existing in the index. The problem:\n\n$ git init\nInitialized empty Git repository in .git/\n$ rm -rf a\n$ touch a\n$ git add a\n$ git commit -madd\nCreated initial commit 818f2b7: add\n 0 files changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 a\n$ echo 123 > a.new\n$ diff -u a a.new > patch\n$ git apply patch\nerror: a: already exists in working directory\n\nThe error happens because git guesses that `patch` is a creation patch\nand since `a` already exists in the index it will bail out. With the\nmodification\nforeign patches won't be guessed to be a creation patch if the file to be\npatch already exists in the index or the working directory.\n\n>\n>\n>  > +static size_t existing_file_size(const char *file)\n>  > +{\n>  > +     size_t st_size = -1;\n>  > +\n>  > +     if (file == NULL)\n>  > +             return -1;\n>  > +     if (cached) {\n>  > +             struct cache_entry *ce;\n>  > +             int pos;\n>  > +\n>  > +             pos = cache_name_pos(file, strlen(file));\n>  > +             if (pos < 0)\n>  > +                     return -1;\n>  > +             ce = active_cache[pos];\n>  > +             st_size = ntohl(ce->ce_size);\n>\n>  ntohl()?  I thought ce->ce_* are host-native byte order these days...\n\nSorry, I actually created the patch against an older version. It should be\nwithout ntohl().\n\n>\n>\n>  > +     } else {\n>  > +             struct stat st;\n>  > +\n>  > +             if (lstat(file, &st) < 0)\n>  > +                     return -1;\n>\n>  Doesn't this break the use case where \"git-apply --stat\" is used as an\n>  improved diffstat outside a git repository?\n\nIn that case we'll return error (file doesn't exist) which will leave\nthe possibility\nopen to guess the patch to be a creation patch. With the change when\ndoing --stat the patch might or might not be changed to a creation patch\nbased on the current working directory content. Though I think this doesn't\nresult in any difference in the --stat output one could argue that --stat's\noperation shouldn't depend in any way on the working directory content\nexcept when --check is specified in addition. So probably we should special\ncase --stat here...\n\n>\n>\n>  > @@ -1143,13 +1170,18 @@ static int parse_single_patch(char *line, unsigned long size, struct patch *patc\n>  >       if (patch->is_delete < 0 &&\n>  >           (newlines || (patch->fragments && patch->fragments->next)))\n>  >               patch->is_delete = 0;\n>  > +     /* FIXME: How can be there context if it's a creation / deletion? */\n>  >       if (!unidiff_zero || context) {\n>  >               /* If the user says the patch is not generated with\n>  >                * --unified=0, or if we have seen context lines,\n>  >                * then not having oldlines means the patch is creation,\n>  >                * and not having newlines means the patch is deletion.\n>  > +              *\n>  > +              * It's also possible that a zero length file is added\n>  > +              * to.\n>  >                */\n>  > -             if (patch->is_new < 0 && !oldlines) {\n>  > +             if (patch->is_new < 0 && !oldlines &&\n>  > +                 existing_file_size(patch->old_name) != 0) {\n>  >                       patch->is_new = 1;\n>  >                       patch->old_name = NULL;\n>  >               }\n>\n>  The user did not say the patch was produced without context, or we do have\n>  context.  The latter cannot be a creation patch so the new logic is not\n>  appropriate.  But let's forget that problem for now and look at the case\n>  where the patch did _not_ have any context, i.e. only added and deleted\n>  lines.\n>\n>  If the patch did not have context, and the user did not ask for -u0 patch\n>  when it was produced, it could be a creation patch, but if there are\n>  deleted lines it cannot be.  That is the original logic.\n\nSo as far as I understand creation or deletion patches cannot have any context\nlines. If that's correct shouldn't the if clause\n\nif (!unidiff_zero || context)\n\nread instead\n\nif (!unidiff_zero)\n\n?\n\n>\n>  After your patch, the original logic is allowed to decide that the patch\n>  is a creation _only if_ you happen to already have a file that is _to be\n>  created_ in the work tree with some existing contents, or the file does\n>  not exist.  I do not see a sane logic behind that.  If you were making\n>  sure that the work tree does _not_ have the file, then I would understand,\n>  even though I think it is wrong for \"apply --stat\" case.  If you see a\n>  file in the work tree, and if you assume the patch would apply to the\n>  work tree, then the patch cannot be creation!\n\nYou are right, that condition is wrong. The right one then should be:\n\nexisting_file_size(patch->old_name) < 0\n\n>\n>  In general, it is not right to look at the work tree to decide how to\n>  interpret what the patch means to begin with, but maybe you are trying to\n>  use work tree status as a hint to disambiguate a corner case that the\n>  information in a patch we are reading is insufficient, in which case it\n>  might be Ok.  But I cannot tell what that corner case is.\n\nHm, I agree with that if it's only a --stat, but if it's --check or\n--apply I think\nthere is no other way to guess if we really have a creation patch or only an\nadd-to-a-zero-length-file patch.\n\n--Imre\n\n>\n>  I am lost.  Please explain what you are trying to fix first before\n>  explaining how you attempted to fix it.\n"},{"id":"76889","messageId":"7vprrpswof.fsf@gitster.siamese.dyndns.org","threadId":"13433","inReplyTo":"500f3d130805131316m59898392l46e0dbf7cb352981@mail.gmail.com","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-13T21:48:00Z","receivedAt":"2008-05-13T21:48:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Imre Deak\" <imre.deak@gmail.com> writes:\n\n> On Sun, May 11, 2008 at 5:36 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Imre Deak <imre.deak@gmail.com> writes:\n>>\n>>  > When we can only guess if we have a creation patch, we do\n>>  > this by treating the patch as such if there weren't any old\n>>  > lines. Zero length files can be patched without old lines\n>>  > though, so do an extra check for file size.\n>>\n>>  You described what your patch does, but you did not explain why it is a\n>>  good addition.  One way to do so is to illustrate in what occasion what\n>>  the existing code does is insufficient.\n>\n> The patch makes it possible to apply foreign patches (not created with\n> git diff) to zero length files already existing in the index. The problem:\n>\n> $ git init\n> Initialized empty Git repository in .git/\n> $ rm -rf a\n> $ touch a\n> $ git add a\n> $ git commit -madd\n> Created initial commit 818f2b7: add\n>  0 files changed, 0 insertions(+), 0 deletions(-)\n>  create mode 100644 a\n> $ echo 123 > a.new\n> $ diff -u a a.new > patch\n> $ git apply patch\n> error: a: already exists in working directory\n>\n> The error happens because git guesses that `patch` is a creation patch\n> and since `a` already exists in the index it will bail out.\n\nOk, that is much clearer.  How about this two-liner instead, then (the\nfirst hunk is just an unreleated typofix)?\n\nOriginally when Linus wrote \"git-apply\" he was trying to be very cautious\nand used the /dev/null cue only in a positive way (i.e. if the patch is\nfrom /dev/null it is a creation).  But the preimage being something other\nthan /dev/null was not used as a statement that the patch is not a\ncreation.\n\nNon SCM patches of any nontrivial size would be created by comparing two\ntrees with \"diff -ru\" (with some more combination of options).  We would\nreliably use /dev/null cue in this case --- if the patch is from /dev/null\nit is creation but more importantly if it is not from /dev/null it is not\ncreation but modification.\n\nPatches from foreign SCMs also follow the /dev/null convention (e.g. SVN\nand CVS --- I did not check Hg but I would be surprised if it didn't\nfollow suit), and we can reliably use the lack of /dev/null as a cue that\nit is not a creation patch.\n\nA single-file non SCM patches are done by comparing $a.orig and $a, $a~\nand $a, etc., i.e. a pair of files the original and the modified with some\nfile suffixes.  What would people do to represent a creation in such a\ncase?  --- You are right.  By doing \"diff -u /dev/null $a\" (it is more\ncumbersome to do \"touch $a.empty; diff -u $a.empty $a\").\n\nSo I think it is reasonable to use non-/dev/null-ness of first as a cue\nthat it is not a creation patch.\n\nLinus, what do you think?  FYR, the original patch that led to this\nconversation was:\n\n    http://thread.gmane.org/gmane.comp.version-control.git/81531\n\n---\n\n builtin-apply.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 1103625..9d7cb05 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -418,7 +418,7 @@ static int guess_p_value(const char *nameline)\n }\n \n /*\n- * Get the name etc info from the --/+++ lines of a traditional patch header\n+ * Get the name etc info from the ---/+++ lines of a traditional patch header\n  *\n  * FIXME! The end-of-filename heuristics are kind of screwy. For existing\n  * files, we can happily check the index for a match, but for creating a\n@@ -451,6 +451,8 @@ static void parse_traditional_patch(const char *first, const char *second, struc\n \t\tname = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);\n \t\tpatch->old_name = name;\n \t} else {\n+\t\tpatch->is_new = 0;\n+\t\tpatch->is_delete = 0;\n \t\tname = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);\n \t\tname = find_name(second, name, p_value, TERM_SPACE | TERM_TAB);\n \t\tpatch->old_name = patch->new_name = name;\n"},{"id":"76890","messageId":"alpine.LFD.1.10.0805131514300.3019@woody.linux-foundation.org","threadId":"13433","inReplyTo":"7vprrpswof.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-05-13T22:24:45Z","receivedAt":"2008-05-13T22:24:45Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 13 May 2008, Junio C Hamano wrote:\n> \n> So I think it is reasonable to use non-/dev/null-ness of first as a cue\n> that it is not a creation patch.\n\nI disagree.\n\nThe fact is, /dev/null means that for patches generated by GNU diff, but a \nlot of other systems you'll find that it means no such thing.\n\nLook at CVS-generated patches, or SVN for that matter. The diffs look like \nthis:\n\n\tIndex: file\n\t===================================================================\n\t--- file (revision 0)\n\t+++ file (working copy)\n\t@@ -0,0 +1 @@\n\t+test\n\nand there is no /dev/null there.\n\nThe thing is, git-apply is careful, and it's very much careful with \nrespect to *knowing* that there are lots of different versions of \"diff\" \nfloating around, and lots of different SCM systems that generate odd diff \nheaders. We should absolutely NOT start expecting that diffs are only \ngenerated with GNU diff.\n\nSo non-/dev/null'ness means absolutely nothing. It means \"don't know\", and \nwe should leave is_new and is_delete as -1.\n\n\t\tLinus\n"},{"id":"76892","messageId":"7vlk2dsujm.fsf@gitster.siamese.dyndns.org","threadId":"13433","inReplyTo":"alpine.LFD.1.10.0805131514300.3019@woody.linux-foundation.org","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-13T22:34:05Z","receivedAt":"2008-05-13T22:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> Look at CVS-generated patches, or SVN for that matter. The diffs look like \n> this:\n>\n> \tIndex: file\n> \t===================================================================\n> \t--- file (revision 0)\n> \t+++ file (working copy)\n> \t@@ -0,0 +1 @@\n> \t+test\n>\n> and there is no /dev/null there.\n>\n> The thing is, git-apply is careful, and it's very much careful with \n> respect to *knowing* that there are lots of different versions of \"diff\" \n> floating around, and lots of different SCM systems that generate odd diff \n> headers. We should absolutely NOT start expecting that diffs are only \n> generated with GNU diff.\n>\n> So non-/dev/null'ness means absolutely nothing. It means \"don't know\", and \n> we should leave is_new and is_delete as -1.\n\nOk, then what's the judgement for the original issue?  Is it a user error\nto have a tracked absolutely empty file in the index?\n"},{"id":"76894","messageId":"alpine.LFD.1.10.0805131552410.3019@woody.linux-foundation.org","threadId":"13433","inReplyTo":"7vlk2dsujm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-05-13T22:58:44Z","receivedAt":"2008-05-13T22:58:44Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 13 May 2008, Junio C Hamano wrote:\n\n> Linus Torvalds <torvalds@linux-foundation.org> writes:\n> >\n> > So non-/dev/null'ness means absolutely nothing. It means \"don't know\", and \n> > we should leave is_new and is_delete as -1.\n> \n> Ok, then what's the judgement for the original issue?  Is it a user error\n> to have a tracked absolutely empty file in the index?\n\nI think this is the fundamental problem:\n\n\t..\n\tif (patch->is_new < 0 && !oldlines) {\n\t\tpatch->is_new = 1;\n\t..\n\nbecause that logic simply isn't right. (is_new < 0 && !oldlines) does \n*not* mean that it must be new.\n\nWe can say it the other way around, of course:\n\n\tif (patch->is_new < 0 && oldlines)\n\t\tpatch->is_new = 0;\n\nand that's a valid rule, but I think we already would never set \"is_new\" \nto -1 if we had old lines, so that would probably be a pointless thing to \ndo.\n\nSo: remove the check for (is_new < 0 && !oldlines) because it doesn't \nactually add any information, and leave \"is_new\" as unknown until later \nwhen we actually *see* that file or not. Hmm?\n\n\t\tLinus\n"},{"id":"76897","messageId":"7vve1hrbct.fsf@gitster.siamese.dyndns.org","threadId":"13433","inReplyTo":"alpine.LFD.1.10.0805131552410.3019@woody.linux-foundation.org","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-14T00:13:54Z","receivedAt":"2008-05-14T00:13:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> I think this is the fundamental problem:\n>\n> \t..\n> \tif (patch->is_new < 0 && !oldlines) {\n> \t\tpatch->is_new = 1;\n> \t..\n>\n> because that logic simply isn't right. (is_new < 0 && !oldlines) does \n> *not* mean that it must be new.\n>\n> We can say it the other way around, of course:\n>\n> \tif (patch->is_new < 0 && oldlines)\n> \t\tpatch->is_new = 0;\n>\n> and that's a valid rule, but I think we already would never set \"is_new\" \n> to -1 if we had old lines, so that would probably be a pointless thing to \n> do.\n\nYeah, in fact we have this:\n\n\tif (patch->is_new < 0 &&\n\t    (oldlines || (patch->fragments && patch->fragments->next)))\n\t\tpatch->is_new = 0;\n\na several lines above the piece you quoted in parse_single_patch().\n\n\n> So: remove the check for (is_new < 0 && !oldlines) because it doesn't \n> actually add any information, and leave \"is_new\" as unknown until later \n> when we actually *see* that file or not. Hmm?\n\nThat leads to the similar logic to remove \"If we do not know if it is\ndelete or not, not adding any lines (or any replacement lines) means\ndelete\" logic as well, which in turn means the whole if (!unidiff_zero...)\nblock would go.\n\nWhich actually is a good thing, I think.  As Imre mentioned, I do not\nthink \n\n\tif (!unidiff_zero || context)\n\nmakes sense.  I cannot guess what the intention of that was.\n\n    commit 4be609625e48e908f2b76d35bfeb61a8ba3a83a0\n    Author: Junio C Hamano <junkio@cox.net>\n    Date:   Sun Sep 17 01:04:24 2006 -0700\n\n        apply --unidiff-zero: loosen sanity checks for --unidiff=0 patches\n\n        In \"git-apply\", we have a few sanity checks and heuristics that\n        expects that the patch fed to us is a unified diff with at least\n        one line of context.\n\n         ...description of good tests)...\n\n        These sanity checks are good safety measures, but breaks down\n        when people feed a diff generated with --unified=0.  This was\n        recently noticed first by Matthew Wilcox and Gerrit Pape.\n\n        This adds a new flag, --unified-zero, to allow bypassing these\n        checks.  If you are in control of the patch generation process,\n        you should not use --unified=0 patch and fix it up with this\n        flag; rather you should try work with a patch with context.  But\n        if all you have to work with is a patch without context, this\n        flag may come handy as the last resort.\n\n        Signed-off-by: Junio C Hamano <junkio@cox.net>\n\nSo unless unidiff-zero is given, we would want extra checks inside that\nblock.  Also if we have context in the patch anywhere, that means the\npatch cannot be coming from \"diff -u0\", so we go into that block.\n\nEven though other checks that are guarded with \"if (!unidiff_zero)\"\nelsewhere in the code do make sense, however, this block may not do\nanything useful, as you pointed out.\n\nWith the change to remove the whole block, all tests still passes, and a\nlimited test with this:\n\n        --- empty\t2008-05-13 16:56:57.000000000 -0700\n        +++ empty.1\t2008-05-13 16:57:07.000000000 -0700\n        @@ -0,0 +1 @@\n        +foo\n\nto update an originally empty file \"empty\" also seems to work.\n\nHowever, with this change, it no longer allows you to accept such a patch\nand treat it as a creation of \"empty\".  Instead we barf with \"error:\nempty: No such file or directory\", if you do not have an empty \"empty\"\nfile in the work tree when you run \"git apply\" on the above patch.\n\nWhen \"diff\" was run with flags that could produce context (that is what we\nget from !unidiff_zero), if we do not see any lines that begin with '-',\nthe only case that it is not a creation patch is if you compared the\npostimage with a preimage of size 0.  Because we do not have usable\n/dev/null cue, we cannot tell that case from a creation patch.\n\nI am having a strong suspicion that doing this change is robbing Peter to\npay Imre.  We simply cannot have it both ways, methinks.\n\n---\n\n builtin-apply.c |   17 +----------------\n 1 files changed, 1 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 1103625..395f16b 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -418,7 +418,7 @@ static int guess_p_value(const char *nameline)\n }\n \n /*\n- * Get the name etc info from the --/+++ lines of a traditional patch header\n+ * Get the name etc info from the ---/+++ lines of a traditional patch header\n  *\n  * FIXME! The end-of-filename heuristics are kind of screwy. For existing\n  * files, we can happily check the index for a match, but for creating a\n@@ -1143,21 +1143,6 @@ static int parse_single_patch(char *line, unsigned long size, struct patch *patc\n \tif (patch->is_delete < 0 &&\n \t    (newlines || (patch->fragments && patch->fragments->next)))\n \t\tpatch->is_delete = 0;\n-\tif (!unidiff_zero || context) {\n-\t\t/* If the user says the patch is not generated with\n-\t\t * --unified=0, or if we have seen context lines,\n-\t\t * then not having oldlines means the patch is creation,\n-\t\t * and not having newlines means the patch is deletion.\n-\t\t */\n-\t\tif (patch->is_new < 0 && !oldlines) {\n-\t\t\tpatch->is_new = 1;\n-\t\t\tpatch->old_name = NULL;\n-\t\t}\n-\t\tif (patch->is_delete < 0 && !newlines) {\n-\t\t\tpatch->is_delete = 1;\n-\t\t\tpatch->new_name = NULL;\n-\t\t}\n-\t}\n \n \tif (0 < patch->is_new && oldlines)\n \t\tdie(\"new file %s depends on old contents\", patch->new_name);\n"},{"id":"76901","messageId":"alpine.LFD.1.10.0805131811540.3019@woody.linux-foundation.org","threadId":"13433","inReplyTo":"7vve1hrbct.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-05-14T01:14:32Z","receivedAt":"2008-05-14T01:14:32Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 13 May 2008, Junio C Hamano wrote:\n> \n> With the change to remove the whole block, all tests still passes, and a\n> limited test with this:\n> \n>         --- empty\t2008-05-13 16:56:57.000000000 -0700\n>         +++ empty.1\t2008-05-13 16:57:07.000000000 -0700\n>         @@ -0,0 +1 @@\n>         +foo\n> \n> to update an originally empty file \"empty\" also seems to work.\n> \n> However, with this change, it no longer allows you to accept such a patch\n> and treat it as a creation of \"empty\".  Instead we barf with \"error:\n> empty: No such file or directory\", if you do not have an empty \"empty\"\n> file in the work tree when you run \"git apply\" on the above patch.\n\nOk, that's a bug. It should *not* require that existing empty file, since \n\"is_new\" is -1. That's what -1 means: we don't know if it is new or not.\n\nSo I think your patch is correct, but we need to fix the thing that barfs \nto not barf if we don't know the status of \"is_new\"\n\n\t\tLinus\n"},{"id":"77178","messageId":"7vve1dcn0m.fsf@gitster.siamese.dyndns.org","threadId":"13433","inReplyTo":"alpine.LFD.1.10.0805131811540.3019@woody.linux-foundation.org","subject":"Re: [PATCH] builtin-apply: check for empty files when detecting creation patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-17T09:12:25Z","receivedAt":"2008-05-17T09:12:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> writes:\n\n> On Tue, 13 May 2008, Junio C Hamano wrote:\n>> \n>> With the change to remove the whole block, all tests still passes, and a\n>> limited test with this:\n>> \n>>         --- empty\t2008-05-13 16:56:57.000000000 -0700\n>>         +++ empty.1\t2008-05-13 16:57:07.000000000 -0700\n>>         @@ -0,0 +1 @@\n>>         +foo\n>> \n>> to update an originally empty file \"empty\" also seems to work.\n>> \n>> However, with this change, it no longer allows you to accept such a patch\n>> and treat it as a creation of \"empty\".  Instead we barf with \"error:\n>> empty: No such file or directory\", if you do not have an empty \"empty\"\n>> file in the work tree when you run \"git apply\" on the above patch.\n>\n> Ok, that's a bug. It should *not* require that existing empty file, since \n> \"is_new\" is -1. That's what -1 means: we don't know if it is new or not.\n>\n> So I think your patch is correct, but we need to fix the thing that barfs \n> to not barf if we don't know the status of \"is_new\"\n\nSorry for taking some time to follow this through (I've been busy with day\njob).  Two patches follow this message to address this issue.\n"},{"id":"77179","messageId":"7vr6c1cmpy.fsf_-_@gitster.siamese.dyndns.org","threadId":"13433","inReplyTo":"7vve1dcn0m.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/2] builtin-apply: accept patch to an empty file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-17T09:18:49Z","receivedAt":"2008-05-17T09:18:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"A patch from a foreign SCM (or plain \"diff\" output) often have both\npreimage and postimage filename on ---/+++ lines even for a patch that\ncreates a new file.  However, when there is a filename for preimage, we\nused to insist the file to exist (either in the work tree and/or in the\nindex).  When we cannot be sure by parsing the patch that it is not a\ncreation patch, we shouldn't complain when if there is no such a file.\nThis commit fixes the logic.\n\nRefactor the code that validates the preimage file into a separate\nfunction while we are at it, as it is getting rather big.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-apply.c |  133 ++++++++++++++++++++++++++++++++-----------------------\n 1 files changed, 77 insertions(+), 56 deletions(-)\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 776e596..10b1f88 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -2267,16 +2267,11 @@ static int verify_index_match(struct cache_entry *ce, struct stat *st)\n \treturn ce_match_stat(ce, st, CE_MATCH_IGNORE_VALID);\n }\n \n-static int check_patch(struct patch *patch, struct patch *prev_patch)\n+static int check_preimage(struct patch *patch, struct cache_entry **ce, struct stat *st)\n {\n-\tstruct stat st;\n \tconst char *old_name = patch->old_name;\n-\tconst char *new_name = patch->new_name;\n-\tconst char *name = old_name ? old_name : new_name;\n-\tstruct cache_entry *ce = NULL;\n-\tint ok_if_exists;\n-\n-\tpatch->rejected = 1; /* we will drop this after we succeed */\n+\tint stat_ret = 0;\n+\tunsigned st_mode = 0;\n \n \t/*\n \t * Make sure that we do not have local modifications from the\n@@ -2284,58 +2279,84 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)\n \t * we have the preimage file to be patched in the work tree,\n \t * unless --cached, which tells git to apply only in the index.\n \t */\n-\tif (old_name) {\n-\t\tint stat_ret = 0;\n-\t\tunsigned st_mode = 0;\n-\n-\t\tif (!cached)\n-\t\t\tstat_ret = lstat(old_name, &st);\n-\t\tif (check_index) {\n-\t\t\tint pos = cache_name_pos(old_name, strlen(old_name));\n-\t\t\tif (pos < 0)\n-\t\t\t\treturn error(\"%s: does not exist in index\",\n-\t\t\t\t\t     old_name);\n-\t\t\tce = active_cache[pos];\n-\t\t\tif (stat_ret < 0) {\n-\t\t\t\tstruct checkout costate;\n-\t\t\t\tif (errno != ENOENT)\n-\t\t\t\t\treturn error(\"%s: %s\", old_name,\n-\t\t\t\t\t\t     strerror(errno));\n-\t\t\t\t/* checkout */\n-\t\t\t\tcostate.base_dir = \"\";\n-\t\t\t\tcostate.base_dir_len = 0;\n-\t\t\t\tcostate.force = 0;\n-\t\t\t\tcostate.quiet = 0;\n-\t\t\t\tcostate.not_new = 0;\n-\t\t\t\tcostate.refresh_cache = 1;\n-\t\t\t\tif (checkout_entry(ce,\n-\t\t\t\t\t\t   &costate,\n-\t\t\t\t\t\t   NULL) ||\n-\t\t\t\t    lstat(old_name, &st))\n-\t\t\t\t\treturn -1;\n-\t\t\t}\n-\t\t\tif (!cached && verify_index_match(ce, &st))\n-\t\t\t\treturn error(\"%s: does not match index\",\n-\t\t\t\t\t     old_name);\n-\t\t\tif (cached)\n-\t\t\t\tst_mode = ce->ce_mode;\n-\t\t} else if (stat_ret < 0)\n-\t\t\treturn error(\"%s: %s\", old_name, strerror(errno));\n-\n-\t\tif (!cached)\n-\t\t\tst_mode = ce_mode_from_stat(ce, st.st_mode);\n+\tif (!old_name)\n+\t\treturn 0;\n \n+\tassert(patch->is_new <= 0);\n+\tif (!cached) {\n+\t\tstat_ret = lstat(old_name, st);\n+\t\tif (stat_ret && errno != ENOENT)\n+\t\t\treturn error(\"%s: %s\", old_name, strerror(errno));\n+\t}\n+\tif (check_index) {\n+\t\tint pos = cache_name_pos(old_name, strlen(old_name));\n+\t\tif (pos < 0) {\n+\t\t\tif (patch->is_new < 0)\n+\t\t\t\tgoto is_new;\n+\t\t\treturn error(\"%s: does not exist in index\", old_name);\n+\t\t}\n+\t\t*ce = active_cache[pos];\n+\t\tif (stat_ret < 0) {\n+\t\t\tstruct checkout costate;\n+\t\t\t/* checkout */\n+\t\t\tcostate.base_dir = \"\";\n+\t\t\tcostate.base_dir_len = 0;\n+\t\t\tcostate.force = 0;\n+\t\t\tcostate.quiet = 0;\n+\t\t\tcostate.not_new = 0;\n+\t\t\tcostate.refresh_cache = 1;\n+\t\t\tif (checkout_entry(*ce, &costate, NULL) ||\n+\t\t\t    lstat(old_name, st))\n+\t\t\t\treturn -1;\n+\t\t}\n+\t\tif (!cached && verify_index_match(*ce, st))\n+\t\t\treturn error(\"%s: does not match index\", old_name);\n+\t\tif (cached)\n+\t\t\tst_mode = (*ce)->ce_mode;\n+\t} else if (stat_ret < 0) {\n \t\tif (patch->is_new < 0)\n-\t\t\tpatch->is_new = 0;\n-\t\tif (!patch->old_mode)\n-\t\t\tpatch->old_mode = st_mode;\n-\t\tif ((st_mode ^ patch->old_mode) & S_IFMT)\n-\t\t\treturn error(\"%s: wrong type\", old_name);\n-\t\tif (st_mode != patch->old_mode)\n-\t\t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n-\t\t\t\told_name, st_mode, patch->old_mode);\n+\t\t\tgoto is_new;\n+\t\treturn error(\"%s: %s\", old_name, strerror(errno));\n \t}\n \n+\tif (!cached)\n+\t\tst_mode = ce_mode_from_stat(*ce, st->st_mode);\n+\n+\tif (patch->is_new < 0)\n+\t\tpatch->is_new = 0;\n+\tif (!patch->old_mode)\n+\t\tpatch->old_mode = st_mode;\n+\tif ((st_mode ^ patch->old_mode) & S_IFMT)\n+\t\treturn error(\"%s: wrong type\", old_name);\n+\tif (st_mode != patch->old_mode)\n+\t\tfprintf(stderr, \"warning: %s has type %o, expected %o\\n\",\n+\t\t\told_name, st_mode, patch->old_mode);\n+\treturn 0;\n+\n+ is_new:\n+\tpatch->is_new = 1;\n+\tpatch->is_delete = 0;\n+\tpatch->old_name = NULL;\n+\treturn 0;\n+}\n+\n+static int check_patch(struct patch *patch, struct patch *prev_patch)\n+{\n+\tstruct stat st;\n+\tconst char *old_name = patch->old_name;\n+\tconst char *new_name = patch->new_name;\n+\tconst char *name = old_name ? old_name : new_name;\n+\tstruct cache_entry *ce = NULL;\n+\tint ok_if_exists;\n+\tint status;\n+\n+\tpatch->rejected = 1; /* we will drop this after we succeed */\n+\n+\tstatus = check_preimage(patch, &ce, &st);\n+\tif (status)\n+\t\treturn status;\n+\told_name = patch->old_name;\n+\n \tif (new_name && prev_patch && 0 < prev_patch->is_delete &&\n \t    !strcmp(prev_patch->old_name, new_name))\n \t\t/*\n-- \n1.5.5.1.443.g123e3\n"},{"id":"77180","messageId":"7vmympcmpa.fsf_-_@gitster.siamese.dyndns.org","threadId":"13433","inReplyTo":"7vve1dcn0m.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 2/2] builtin-apply: do not declare patch is creation when we do not know it","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-05-17T09:19:13Z","receivedAt":"2008-05-17T09:19:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we see no context nor deleted line in the patch, we used to declare\nthat the patch creates a new file.  But some people create an empty file\nand then apply a patch to it.  Similarly, a patch that delete everything\nis not a deletion patch either.\n\nThis commit corrects these two issues.  Together with the previous commit,\nit allows a diff between an empty file and a line-ful file to be treated\nas both creation patch and \"add stuff to an existing empty file\",\ndepending on the context.  A new test t4126 demonstrates the fix.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-apply.c        |   15 ------------\n t/t4126-apply-empty.sh |   60 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 60 insertions(+), 15 deletions(-)\n create mode 100755 t/t4126-apply-empty.sh\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex 10b1f88..1540f28 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -1143,21 +1143,6 @@ static int parse_single_patch(char *line, unsigned long size, struct patch *patc\n \tif (patch->is_delete < 0 &&\n \t    (newlines || (patch->fragments && patch->fragments->next)))\n \t\tpatch->is_delete = 0;\n-\tif (!unidiff_zero || context) {\n-\t\t/* If the user says the patch is not generated with\n-\t\t * --unified=0, or if we have seen context lines,\n-\t\t * then not having oldlines means the patch is creation,\n-\t\t * and not having newlines means the patch is deletion.\n-\t\t */\n-\t\tif (patch->is_new < 0 && !oldlines) {\n-\t\t\tpatch->is_new = 1;\n-\t\t\tpatch->old_name = NULL;\n-\t\t}\n-\t\tif (patch->is_delete < 0 && !newlines) {\n-\t\t\tpatch->is_delete = 1;\n-\t\t\tpatch->new_name = NULL;\n-\t\t}\n-\t}\n \n \tif (0 < patch->is_new && oldlines)\n \t\tdie(\"new file %s depends on old contents\", patch->new_name);\ndiff --git a/t/t4126-apply-empty.sh b/t/t4126-apply-empty.sh\nnew file mode 100755\nindex 0000000..6641571\n--- /dev/null\n+++ b/t/t4126-apply-empty.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='apply empty'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\t>empty &&\n+\tgit add empty &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\tfor i in a b c d e\n+\tdo\n+\t\techo $i\n+\tdone >empty &&\n+\tcat empty >expect &&\n+\tgit diff |\n+\tsed -e \"/^diff --git/d\" \\\n+\t    -e \"/^index /d\" \\\n+\t    -e \"s|a/empty|empty.orig|\" \\\n+\t    -e \"s|b/empty|empty|\" >patch0 &&\n+\tsed -e \"s|empty|missing|\" patch0 >patch1 &&\n+\t>empty &&\n+\tgit update-index --refresh\n+'\n+\n+test_expect_success 'apply empty' '\n+\tgit reset --hard &&\n+\t>empty &&\n+\trm -f missing &&\n+\tgit apply patch0 &&\n+\ttest_cmp expect empty\n+'\n+\n+test_expect_success 'apply --index empty' '\n+\tgit reset --hard &&\n+\t>empty &&\n+\trm -f missing &&\n+\tgit apply --index patch0 &&\n+\ttest_cmp expect empty &&\n+\tgit diff --exit-code\n+'\n+\n+test_expect_success 'apply create' '\n+\tgit reset --hard &&\n+\t>empty &&\n+\trm -f missing &&\n+\tgit apply patch1 &&\n+\ttest_cmp expect missing\n+'\n+\n+test_expect_success 'apply --index create' '\n+\tgit reset --hard &&\n+\t>empty &&\n+\trm -f missing &&\n+\tgit apply --index patch1 &&\n+\ttest_cmp expect missing &&\n+\tgit diff --exit-code\n+'\n+\n-- \n1.5.5.1.443.g123e3\n"}]}