{"thread":{"id":"20075","subject":"Re: What's happening with vr41xx_giu.c?","startedAt":"2009-07-10T10:47:43Z","lastAt":"2009-07-11T03:57:32Z","messageCount":7,"participants":["Ralf Baechle","Junio C Hamano","Linus Torvalds"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"117763","messageId":"20090710104743.GB1288@linux-mips.org","threadId":"20075","inReplyTo":"4A56B060.7090106@mips.com","subject":"Re: What's happening with vr41xx_giu.c?","fromName":"Ralf Baechle","fromEmail":"ralf@linux-mips.org","sentAt":"2009-07-10T10:47:43Z","receivedAt":"2009-07-10T10:47:43Z","isPatch":false,"sender":{"key":"ralf@linux-mips.org","avatar":null},"body":"On Thu, Jul 09, 2009 at 08:07:12PM -0700, Chris Dearman wrote:\n\nThis is smelling like a git issue so I'm adding git@vger.kernel.org to cc\nlist.\n\n> Shinya Kuribayashi wrote:\n>\n>> skuribay@ubuntu:linux.git$ make distclean\n>> skuribay@ubuntu:linux.git$\n>> skuribay@ubuntu:linux.git$\n>> skuribay@ubuntu:linux.git$ git status\n>> # On branch master\n>> # Changed but not updated:\n>> #   (use \"git add/rm <file>...\" to update what will be committed)\n>> #   (use \"git checkout -- <file>...\" to discard changes in working  \n>> directory)\n>> #\n>> #       deleted:    drivers/char/vr41xx_giu.c\n>> #\n>> no changes added to commit (use \"git add\" and/or \"git commit -a\")\n>> skuribay@ubuntu:linux.git$\n>>\n>\n> Commit 27fdd325dace4a1ebfa10e93ba6f3d25f25df674 turned  \n> drivers/char/vr41xx_giu.c into an empty file instead of deleting it when  \n> the file was moved to drivers/gpio\n>\n> \"make distclean\" deletes any 0 length .c files that it finds.\n>\n> Leaving drivers/char/vr41xx_giu.c as a zero length file may have been a  \n> git bug but was probably just an oversight. I'll send a patch to clean  \n> it up as a followup.\n\nAnd issue is reproducable.  When I go back to commit\n27fdd325dace4a1ebfa10e93ba6f3d25f25df674^ and apply Yoichi's patch using\ngit am or git apply this will leave a zero byte drivers/char/vr41xx_giu.c.\nPatch(1) otoh will remove that file as expected.  The patch file Yoichi\nsent looks perfectly ok; here the headers of the vr41xx_giu.c bit:\n\n[...]\ndiff -pruN -X /home/yuasa/Memo/dontdiff linux-orig/drivers/char/vr41xx_giu.c linux/drivers/char/vr41xx_giu.c\n--- linux-orig/drivers/char/vr41xx_giu.c        2009-06-29 10:06:58.329177629 +0900\n+++ linux/drivers/char/vr41xx_giu.c     1970-01-01 09:00:00.000000000 +0900\n@@ -1,680 +0,0 @@\n-/*\n[...]\n\nThis is with git 1.6.0.6 (git-1.6.0.6-4.fc10.x86_64 from Fedora 10).\n\nThe patch is available at http://www.linux-mips.org/cgi-bin/extract-mesg.cgi?a=linux-mips&m=2009-06&i=20090629111105.9ff024bf.yyuasa%40linux.com\nand the git tree in question is Linus' kernel tree available from\ngit://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.\n\n  Ralf\n"},{"id":"117776","messageId":"7vhbxkv7ax.fsf@alter.siamese.dyndns.org","threadId":"20075","inReplyTo":"20090710104743.GB1288@linux-mips.org","subject":"Re: What's happening with vr41xx_giu.c?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-10T16:20:22Z","receivedAt":"2009-07-10T16:20:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralf Baechle <ralf@linux-mips.org> writes:\n\n> 27fdd325dace4a1ebfa10e93ba6f3d25f25df674^ and apply Yoichi's patch using\n> git am or git apply this will leave a zero byte drivers/char/vr41xx_giu.c.\n> Patch(1) otoh will remove that file as expected.  The patch file Yoichi\n> sent looks perfectly ok; here the headers of the vr41xx_giu.c bit:\n>\n> [...]\n> diff -pruN -X /home/yuasa/Memo/dontdiff linux-orig/drivers/char/vr41xx_giu.c linux/drivers/char/vr41xx_giu.c\n> --- linux-orig/drivers/char/vr41xx_giu.c        2009-06-29 10:06:58.329177629 +0900\n> +++ linux/drivers/char/vr41xx_giu.c     1970-01-01 09:00:00.000000000 +0900\n> @@ -1,680 +0,0 @@\n> -/*\n\nIf you look for -E option in \"man patch\" and find that it says \"causes\npatch to remove output files that are empty after the patches have been\napplied.\", you will realize that your claim that \"patch(1) otoh ... as\nexpected\" does not match the reality for everybody.  It is true only if\nyou _are_ explicitly asking to remove such an empty file.\n\nThe recent diff specification (at least the one in POSIX.1) says that file\nremoval is marked by the UNIX epoch timestamp you see there, instead of a\nmore recent timestamp. IOW, you should _in theory_ be able to tell by\nlooking at the 1970-01-01 timestamp that the intention of this patch is\nnot to make the file empty, but is to remove.\n\nBut in practice, because traditionally GNU diff and other people's diff\nplaced pretty arbitrary garbage after the TAB that follows the filename,\npatch does not rely on that convention to detect a removal patch.  Notice\nthat even -E option does not pay attention to that timestamp line, but\nremoves files that become _empty_.\n\nNeither do we.  The patch application toolchain in \"git\" does not have -E\noption and the above patch is interpreted just like traditional patch does\nby default: the file goes empty.\n\nThe output from \"git diff\" is designed so that (1) it can distinguish the\nremoval case and the goes-empty case more clearly, and also that (2) it\ncan be safely used by patch(1).  A removal patch from git looks like:\n\n    diff --git a/file b/file\n    deleted file mode 100644\n    index 363ef61..0000000\n    --- a/file\n    +++ /dev/null\n    @@ -1 +0,0 @@\n    -original contents\n\nwhile \"goes empty\" patch looks like:\n\n    diff --git a/file b/file\n    index 363ef61..e69de29 100644\n    --- a/file\n    +++ b/file\n    @@ -1 +0,0 @@\n    -original contents\n\nand when applied with git, they both produce \"expected\" results.\n\nWe _could_ add -E option to \"git apply\" and pass that through \"git am\" to\nsupport projects like the kernel where 0-byte files are forbidden.  A\npatch to do that shouldn't be too involved.\n"},{"id":"117809","messageId":"7vk52frjv9.fsf@alter.siamese.dyndns.org","threadId":"20075","inReplyTo":"7vhbxkv7ax.fsf@alter.siamese.dyndns.org","subject":"Re: What's happening with vr41xx_giu.c?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-11T03:14:50Z","receivedAt":"2009-07-11T03:14:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> We _could_ add -E option to \"git apply\" and pass that through \"git am\" to\n> support projects like the kernel where 0-byte files are forbidden.  A\n> patch to do that shouldn't be too involved.\n\nI ended up doing something a bit more useful.  A two-patch series follows\nshortly.\n"},{"id":"117810","messageId":"7vfxd3rjmh.fsf_-_@alter.siamese.dyndns.org","threadId":"20075","inReplyTo":"7vk52frjv9.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] tm_to_time_t(): allow times in year 1969","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-11T03:20:06Z","receivedAt":"2009-07-11T03:20:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We used to reject anything older than January 1st 1970 regardless of the\ntimezone, but in order to parse UNIX epoch in zones west of GMT, we need\nto allow the time without zone to be a little bit into 1969.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n date.c |   16 ++++++++--------\n 1 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/date.c b/date.c\nindex 409a17d..5d259af 100644\n--- a/date.c\n+++ b/date.c\n@@ -6,6 +6,7 @@\n \n #include \"cache.h\"\n \n+#define BAD_STRUCT_TM (-400*24*60*60)\n /*\n  * This is like mktime, but without normalization of tm_wday and tm_yday.\n  */\n@@ -18,10 +19,10 @@ time_t tm_to_time_t(const struct tm *tm)\n \tint month = tm->tm_mon;\n \tint day = tm->tm_mday;\n \n-\tif (year < 0 || year > 129) /* algo only works for 1970-2099 */\n-\t\treturn -1;\n+\tif (year < -1 || year > 129) /* algo only works for 1969-2099 */\n+\t\treturn BAD_STRUCT_TM;\n \tif (month < 0 || month > 11) /* array bounds */\n-\t\treturn -1;\n+\t\treturn BAD_STRUCT_TM;\n \tif (month < 2 || (year + 2) % 4)\n \t\tday--;\n \treturn (year * 365 + (year + 1) / 4 + mdays[month] + day) * 24*60*60UL +\n@@ -337,7 +338,7 @@ static int is_date(int year, int month, int day, struct tm *now_tm, time_t now,\n \t\t\t\treturn 1;\n \t\t\tr->tm_year = now_tm->tm_year;\n \t\t}\n-\t\telse if (year >= 1970 && year < 2100)\n+\t\telse if (year >= 1969 && year < 2100)\n \t\t\tr->tm_year = year - 1900;\n \t\telse if (year > 70 && year < 100)\n \t\t\tr->tm_year = year;\n@@ -611,12 +612,11 @@ int parse_date(const char *date, char *result, int maxlen)\n \n \t/* mktime uses local timezone */\n \tthen = tm_to_time_t(&tm);\n-\tif (offset == -1)\n-\t\toffset = (then - mktime(&tm)) / 60;\n-\n-\tif (then == -1)\n+\tif (then == BAD_STRUCT_TM)\n \t\treturn -1;\n \n+\tif (offset == -1)\n+\t\toffset = (then - mktime(&tm)) / 60;\n \tif (!tm_gmt)\n \t\tthen -= offset * 60;\n \treturn date_string(then, offset, result, maxlen);\n-- \n1.6.3.3.412.gf581d\n"},{"id":"117811","messageId":"7vbpnrrjld.fsf_-_@alter.siamese.dyndns.org","threadId":"20075","inReplyTo":"7vk52frjv9.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] apply: notice creation/removal patches produced by GNU diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-11T03:20:46Z","receivedAt":"2009-07-11T03:20:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Unified context patch generated by GNU diff has UNIX epoch timestamp\non the side that does not exist when the patch is about a creation or\na deletion event.  Notice this convention when reading a non-git diff.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin-apply.c          |   63 ++++++++++++++++++++++++++++++++-\n t/t4132-apply-removal.sh |   88 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 150 insertions(+), 1 deletions(-)\n create mode 100755 t/t4132-apply-removal.sh\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex dc0ff5e..06e80e4 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -458,6 +458,57 @@ static int guess_p_value(const char *nameline)\n }\n \n /*\n+ * Does the ---/+++ line has the POSIX timestamp after the last HT?\n+ * GNU diff puts epoch there to signal a creation/deletion event.  Is\n+ * this such a timestamp?\n+ */\n+static int has_epoch_timestamp(const char *nameline)\n+{\n+\t/*\n+\t * We are only interested in epoch timestamp; any non-zero\n+\t * fraction cannot be one, hence \"(\\.0+)?\" in the regexp below.\n+\t */\n+\tconst char stamp_regexp[] =\n+\t\t\"^[0-9][0-9][0-9][0-9]-[01][0-9]-[0-3][0-9]\"\n+\t\t\" \"\n+\t\t\"[0-2][0-9]:[0-5][0-9]:[0-6][0-9](\\\\.0+)?\"\n+\t\t\" \"\n+\t\t\"[-+][0-2][0-9][0-5][0-9]\\n\";\n+\tconst char *timestamp = NULL, *cp;\n+\tstatic regex_t *stamp;\n+\tint status;\n+\tchar parsed[100];\n+\n+\tfor (cp = nameline; *cp != '\\n'; cp++) {\n+\t\tif (*cp == '\\t')\n+\t\t\ttimestamp = cp + 1;\n+\t}\n+\tif (!timestamp)\n+\t\treturn 0;\n+\tif (!stamp) {\n+\t\tstamp = xmalloc(sizeof(*stamp));\n+\t\tif (regcomp(stamp, stamp_regexp, REG_EXTENDED)) {\n+\t\t\twarning(\"Cannot prepare timestamp regexp %s\",\n+\t\t\t\tstamp_regexp);\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\tstatus = regexec(stamp, timestamp, 0, NULL, 0);\n+\tif (status) {\n+\t\tif (status != REG_NOMATCH)\n+\t\t\twarning(\"regexec returned %d for input: %s\",\n+\t\t\t\tstatus, timestamp);\n+\t\treturn 0;\n+\t}\n+\n+\tparse_date(timestamp, parsed, sizeof(parsed));\n+\tif (parsed[0] == '0' && parsed[1] == ' ')\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+/*\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@@ -493,7 +544,17 @@ static void parse_traditional_patch(const char *first, const char *second, struc\n \t} else {\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+\t\tif (has_epoch_timestamp(first)) {\n+\t\t\tpatch->is_new = 1;\n+\t\t\tpatch->is_delete = 0;\n+\t\t\tpatch->new_name = name;\n+\t\t} else if (has_epoch_timestamp(second)) {\n+\t\t\tpatch->is_new = 0;\n+\t\t\tpatch->is_delete = 1;\n+\t\t\tpatch->old_name = name;\n+\t\t} else {\n+\t\t\tpatch->old_name = patch->new_name = name;\n+\t\t}\n \t}\n \tif (!name)\n \t\tdie(\"unable to find filename in patch at line %d\", linenr);\ndiff --git a/t/t4132-apply-removal.sh b/t/t4132-apply-removal.sh\nnew file mode 100755\nindex 0000000..eb971f7\n--- /dev/null\n+++ b/t/t4132-apply-removal.sh\n@@ -0,0 +1,88 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2009 Junio C Hamano\n+\n+test_description='git-apply notices removal patches generated by GNU diff'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tcat <<-EOF >c &&\n+\tdiff -ruN a/file b/file\n+\t--- a/file\tTS0\n+\t+++ b/file\tTS1\n+\t@@ -0,0 +1 @@\n+\t+something\n+\tEOF\n+\n+\tcat <<-EOF >d &&\n+\tdiff -ruN a/file b/file\n+\t--- a/file\tTS0\n+\t+++ b/file\tTS1\n+\t@@ -1 +0,0 @@\n+\t-something\n+\tEOF\n+\n+\ttimeWest=\"1982-09-16 07:00:00.000000000 -0800\" &&\n+\ttimeEast=\"1982-09-17 00:00:00.000000000 +0900\" &&\n+\tepocWest=\"1969-12-31 16:00:00.000000000 -0800\" &&\n+\tepocEast=\"1970-01-01 09:00:00.000000000 +0900\" &&\n+\n+\tsed -e \"s/TS0/$epocWest/\" -e \"s/TS1/$timeWest/\" <c >createWest.patch &&\n+\tsed -e \"s/TS0/$epocEast/\" -e \"s/TS1/$timeEast/\" <c >createEast.patch &&\n+\n+\tsed -e \"s/TS0/$timeWest/\" -e \"s/TS1/$timeWest/\" <c >addWest.patch &&\n+\tsed -e \"s/TS0/$timeEast/\" -e \"s/TS1/$timeEast/\" <c >addEast.patch &&\n+\n+\tsed -e \"s/TS0/$timeWest/\" -e \"s/TS1/$timeWest/\" <d >emptyWest.patch &&\n+\tsed -e \"s/TS0/$timeEast/\" -e \"s/TS1/$timeEast/\" <d >emptyEast.patch &&\n+\n+\tsed -e \"s/TS0/$timeWest/\" -e \"s/TS1/$epocWest/\" <d >removeWest.patch &&\n+\tsed -e \"s/TS0/$timeEast/\" -e \"s/TS1/$epocEast/\" <d >removeEast.patch &&\n+\n+\techo something >something &&\n+\t>empty\n+'\n+\n+for patch in *.patch\n+do\n+\ttest_expect_success \"test $patch\" '\n+\t\trm -f file .git/index &&\n+\t\tcase \"$patch\" in\n+\t\tcreate*)\n+\t\t\t# must be able to create\n+\t\t\tgit apply --index $patch &&\n+\t\t\ttest_cmp file something &&\n+\t\t\t# must notice the file is already there\n+\t\t\t>file &&\n+\t\t\tgit add file &&\n+\t\t\ttest_must_fail git apply $patch\n+\t\t\t;;\n+\t\tadd*)\n+\t\t\t# must be able to create or patch\n+\t\t\tgit apply $patch &&\n+\t\t\ttest_cmp file something &&\n+\t\t\t>file &&\n+\t\t\tgit apply $patch &&\n+\t\t\ttest_cmp file something\n+\t\t\t;;\n+\t\tempty*)\n+\t\t\t# must leave an empty file\n+\t\t\tcat something >file &&\n+\t\t\tgit add file &&\n+\t\t\tgit apply --index $patch &&\n+\t\t\ttest -f file &&\n+\t\t\ttest_cmp empty file\n+\t\t\t;;\n+\t\tremove*)\n+\t\t\t# must remove the file\n+\t\t\tcat something >file &&\n+\t\t\tgit add file &&\n+\t\t\tgit apply --index $patch &&\n+\t\t\t! test -f file\n+\t\t\t;;\n+\t\tesac\n+\t'\n+done\n+\n+test_done\n-- \n1.6.3.3.412.gf581d\n"},{"id":"117812","messageId":"alpine.LFD.2.01.0907102029570.3552@localhost.localdomain","threadId":"20075","inReplyTo":"7vbpnrrjld.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] apply: notice creation/removal patches produced by GNU diff","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-07-11T03:32:01Z","receivedAt":"2009-07-11T03:32:01Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 10 Jul 2009, Junio C Hamano wrote:\n>\n> Unified context patch generated by GNU diff has UNIX epoch timestamp\n> on the side that does not exist when the patch is about a creation or\n> a deletion event.  Notice this convention when reading a non-git diff.\n\nHmm. Do you really want to do a regex here? That seems overkill. Why not \njust try to parse the date?\n\n> +\tconst char stamp_regexp[] =\n> +\t\t\"^[0-9][0-9][0-9][0-9]-[01][0-9]-[0-3][0-9]\"\n> +\t\t\" \"\n> +\t\t\"[0-2][0-9]:[0-5][0-9]:[0-6][0-9](\\\\.0+)?\"\n> +\t\t\" \"\n> +\t\t\"[-+][0-2][0-9][0-5][0-9]\\n\";\n\nAlso, why are you apparently expecting micro-seconds to always be all \nzeroes? Maybe that's the common case, but I'd expect that somebody has \nnon-zero microseconds on filesystems that support them..\n\n\t\tLinus\n"},{"id":"117813","messageId":"7vws6foor7.fsf@alter.siamese.dyndns.org","threadId":"20075","inReplyTo":"alpine.LFD.2.01.0907102029570.3552@localhost.localdomain","subject":"Re: [PATCH 2/2] apply: notice creation/removal patches produced by GNU diff","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-11T03:57:32Z","receivedAt":"2009-07-11T03:57:32Z","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 Fri, 10 Jul 2009, Junio C Hamano wrote:\n>>\n>> Unified context patch generated by GNU diff has UNIX epoch timestamp\n>> on the side that does not exist when the patch is about a creation or\n>> a deletion event.  Notice this convention when reading a non-git diff.\n>\n> Hmm. Do you really want to do a regex here? That seems overkill. Why not \n> just try to parse the date?\n\nIf parse_date() says \"It is a date and it is UNIX epoch\", we mark the\npatch as either creation or deletion, and that is used by various\nconsistency checks.  Deletion patch must remove the entire line, the file\nmust not exist if it is the target of a creation patch, etc.\n\nI found parse_date() to be a bit too forgiving for my taste for this\nparticular application; I wanted to be anal and only accept the format\nspecified by\n \n  http://www.opengroup.org/onlinepubs/9699919799/utilities/diff.html#tag_20_34_10_07\n\nBy the way, the above does not mention anything about marking\ncreation/deletion event with UNIX epoch; I am guessing it is a GNU\nextension.\n\n>> +\tconst char stamp_regexp[] =\n>> +\t\t\"^[0-9][0-9][0-9][0-9]-[01][0-9]-[0-3][0-9]\"\n>> +\t\t\" \"\n>> +\t\t\"[0-2][0-9]:[0-5][0-9]:[0-6][0-9](\\\\.0+)?\"\n>> +\t\t\" \"\n>> +\t\t\"[-+][0-2][0-9][0-5][0-9]\\n\";\n>\n> Also, why are you apparently expecting micro-seconds to always be all \n> zeroes? Maybe that's the common case, but I'd expect that somebody has \n> non-zero microseconds on filesystems that support them..\n\nAs the comment before the quoted part of the patch says, the point of this\nfunction is not about detecting the presense of a timestamp (and parsing\nthe time), but telling if the timestamp that represents UNIX epoch is\nthere.  The first iteration of my patch did not even use parse_date() but\naccepted only 1969-12-31 (west of GMT) or 1970-01-01 (east of GMT) and\nthen checked timestamp with timezone manually.  Perhaps that might be a\nbetter way to do this function?  I dunno.\n"}]}