* [PATCH] apply: Recognize epoch timestamps with : in the timezone @ 2010-09-29 20:49 Anders Kaseorg 2010-09-29 21:41 ` Jonathan Nieder 0 siblings, 1 reply; 7+ messages in thread From: Anders Kaseorg @ 2010-09-29 20:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Some patches have a timezone formatted like ‘-08:00’ instead of ‘-0800’ (e.g. http://lwn.net/Articles/131729/), so git apply would fail to recognize the epoch timestamp of deleted files and would create empty files instead. Teach it to support both formats, and add a test case. Signed-off-by: Anders Kaseorg <andersk@mit.edu> --- builtin/apply.c | 6 ++++-- t/t4132-apply-removal.sh | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 23c18c5..0fa9a8d 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -733,7 +733,7 @@ static int has_epoch_timestamp(const char *nameline) " " "[0-2][0-9]:[0-5][0-9]:00(\\.0+)?" " " - "([-+][0-2][0-9][0-5][0-9])\n"; + "([-+][0-2][0-9]):?([0-5][0-9])\n"; const char *timestamp = NULL, *cp; static regex_t *stamp; regmatch_t m[10]; @@ -765,7 +765,9 @@ static int has_epoch_timestamp(const char *nameline) } zoneoffset = strtol(timestamp + m[3].rm_so + 1, NULL, 10); - zoneoffset = (zoneoffset / 100) * 60 + (zoneoffset % 100); + if (m[4].rm_so == m[3].rm_so + 3) + zoneoffset /= 100; + zoneoffset = zoneoffset * 60 + strtol(timestamp + m[4].rm_so, NULL, 10); if (timestamp[m[3].rm_so] == '-') zoneoffset = -zoneoffset; diff --git a/t/t4132-apply-removal.sh b/t/t4132-apply-removal.sh index bb1ffe3..a2bc1cd 100755 --- a/t/t4132-apply-removal.sh +++ b/t/t4132-apply-removal.sh @@ -30,6 +30,7 @@ test_expect_success setup ' epocWest="1969-12-31 16:00:00.000000000 -0800" && epocGMT="1970-01-01 00:00:00.000000000 +0000" && epocEast="1970-01-01 09:00:00.000000000 +0900" && + epocWest2="1969-12-31 16:00:00 -08:00" && sed -e "s/TS0/$epocWest/" -e "s/TS1/$timeWest/" <c >createWest.patch && sed -e "s/TS0/$epocEast/" -e "s/TS1/$timeEast/" <c >createEast.patch && @@ -46,6 +47,7 @@ test_expect_success setup ' sed -e "s/TS0/$timeWest/" -e "s/TS1/$epocWest/" <d >removeWest.patch && sed -e "s/TS0/$timeEast/" -e "s/TS1/$epocEast/" <d >removeEast.patch && sed -e "s/TS0/$timeGMT/" -e "s/TS1/$epocGMT/" <d >removeGMT.patch && + sed -e "s/TS0/$timeWest/" -e "s/TS1/$epocWest2/" <d >removeWest2.patch && echo something >something && >empty -- 1.7.3 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] apply: Recognize epoch timestamps with : in the timezone 2010-09-29 20:49 [PATCH] apply: Recognize epoch timestamps with : in the timezone Anders Kaseorg @ 2010-09-29 21:41 ` Jonathan Nieder 2010-09-29 21:49 ` Jonathan Nieder 2010-10-13 18:59 ` Junio C Hamano 0 siblings, 2 replies; 7+ messages in thread From: Jonathan Nieder @ 2010-09-29 21:41 UTC (permalink / raw) To: Anders Kaseorg; +Cc: Junio C Hamano, git Hi Anders, Anders Kaseorg wrote: > Some patches have a timezone formatted like '-08:00' instead of > '-0800' (e.g. http://lwn.net/Articles/131729/) Odd. Any idea what tool generates these patches? > --- a/builtin/apply.c > +++ b/builtin/apply.c [...] > @@ -765,7 +765,9 @@ static int has_epoch_timestamp(const char *nameline) > } > > zoneoffset = strtol(timestamp + m[3].rm_so + 1, NULL, 10); > - zoneoffset = (zoneoffset / 100) * 60 + (zoneoffset % 100); > + if (m[4].rm_so == m[3].rm_so + 3) > + zoneoffset /= 100; > + zoneoffset = zoneoffset * 60 + strtol(timestamp + m[4].rm_so, NULL, 10); Might be clearer to write if (timestamp[m[3].rm_so + 3] != ':') With or without that change, Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Maybe something like this on top? -- 8< -- Subject: apply: handle patches with funny filename and colon in timezone Some patches have a timezone formatted like '-08:00' instead of '-0800' in their ---/+++ lines (e.g. http://lwn.net/Articles/131729/). Take this into account when searching for the start of the timezone (which is the end of the filename). This does not actually affect the outcome of patching unless (1) a file being patched has a non-' ' whitespace character (e.g., tab) in its filename, or (2) the patch is whitespace-damaged, so the tab between filename and timestamp has been replaced with spaces. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- builtin/apply.c | 24 ++++++++++++++++++++++-- t/t4135-apply-weird-filenames.sh | 16 ++++++++++++++++ t/t4135/damaged-tz.diff | 5 +++++ t/t4135/funny-tz.diff | 5 +++++ 4 files changed, 48 insertions(+), 2 deletions(-) create mode 100644 t/t4135/damaged-tz.diff create mode 100644 t/t4135/funny-tz.diff diff --git a/builtin/apply.c b/builtin/apply.c index 0fa9a8d..7d91d8f 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -449,7 +449,7 @@ static char *find_name_gnu(const char *line, char *def, int p_value) return squash_slash(strbuf_detach(&name, NULL)); } -static size_t tz_len(const char *line, size_t len) +static size_t sane_tz_len(const char *line, size_t len) { const char *tz, *p; @@ -467,6 +467,24 @@ static size_t tz_len(const char *line, size_t len) return line + len - tz; } +static size_t tz_with_colon_len(const char *line, size_t len) +{ + const char *tz, *p; + + if (len < strlen(" +08:00") || line[len - strlen(":00")] != ':') + return 0; + tz = line + len - strlen(" +08:00"); + + if (tz[0] != ' ' || (tz[1] != '+' && tz[1] != '-')) + return 0; + p = tz + 2; + if (!isdigit(*p++) || !isdigit(*p++) || *p++ != ':' || + !isdigit(*p++) || !isdigit(*p++)) + return 0; + + return line + len - tz; +} + static size_t date_len(const char *line, size_t len) { const char *date, *p; @@ -561,7 +579,9 @@ static size_t diff_timestamp_len(const char *line, size_t len) if (!isdigit(end[-1])) return 0; - n = tz_len(line, end - line); + n = sane_tz_len(line, end - line); + if (!n) + n = tz_with_colon_len(line, end - line); end -= n; n = short_time_len(line, end - line); diff --git a/t/t4135-apply-weird-filenames.sh b/t/t4135-apply-weird-filenames.sh index 1e5aad5..bf5dc57 100755 --- a/t/t4135-apply-weird-filenames.sh +++ b/t/t4135-apply-weird-filenames.sh @@ -72,4 +72,20 @@ test_expect_success 'whitespace-damaged traditional patch' ' test_cmp expected postimage.txt ' +test_expect_success 'traditional patch with colon in timezone' ' + echo postimage >expected && + reset_preimage && + rm -f "post image.txt" && + git apply "$vector/funny-tz.diff" && + test_cmp expected "post image.txt" +' + +test_expect_success 'traditional, whitespace-damaged, colon in timezone' ' + echo postimage >expected && + reset_preimage && + rm -f "post image.txt" && + git apply "$vector/damaged-tz.diff" && + test_cmp expected "post image.txt" +' + test_done diff --git a/t/t4135/damaged-tz.diff b/t/t4135/damaged-tz.diff new file mode 100644 index 0000000..07aaf08 --- /dev/null +++ b/t/t4135/damaged-tz.diff @@ -0,0 +1,5 @@ +diff -urN -X /usr/people/jes/exclude-linux linux-2.6.12-rc2-mm3-vanilla/post image.txt linux-2.6.12-rc2-mm3/post image.txt +--- linux-2.6.12-rc2-mm3-vanilla/post image.txt 1969-12-31 16:00:00 -08:00 ++++ linux-2.6.12-rc2-mm3/post image.txt 2005-04-12 02:14:06 -07:00 +@@ -0,0 +1 @@ ++postimage diff --git a/t/t4135/funny-tz.diff b/t/t4135/funny-tz.diff new file mode 100644 index 0000000..998e3a8 --- /dev/null +++ b/t/t4135/funny-tz.diff @@ -0,0 +1,5 @@ +diff -urN -X /usr/people/jes/exclude-linux linux-2.6.12-rc2-mm3-vanilla/post image.txt linux-2.6.12-rc2-mm3/post image.txt +--- linux-2.6.12-rc2-mm3-vanilla/post image.txt 1969-12-31 16:00:00 -08:00 ++++ linux-2.6.12-rc2-mm3/post image.txt 2005-04-12 02:14:06 -07:00 +@@ -0,0 +1 @@ ++postimage -- 1.6.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] apply: Recognize epoch timestamps with : in the timezone 2010-09-29 21:41 ` Jonathan Nieder @ 2010-09-29 21:49 ` Jonathan Nieder 2010-10-13 18:59 ` Junio C Hamano 1 sibling, 0 replies; 7+ messages in thread From: Jonathan Nieder @ 2010-09-29 21:49 UTC (permalink / raw) To: Anders Kaseorg; +Cc: Junio C Hamano, git Jonathan Nieder wrote: > Some patches have a timezone formatted like '-08:00' instead of > '-0800' in their ---/+++ lines (e.g. http://lwn.net/Articles/131729/). > Take this into account when searching for the start of the timezone s/timezone/timestamp/ > (which is the end of the filename). Sorry for the noise. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] apply: Recognize epoch timestamps with : in the timezone 2010-09-29 21:41 ` Jonathan Nieder 2010-09-29 21:49 ` Jonathan Nieder @ 2010-10-13 18:59 ` Junio C Hamano 2010-10-13 19:31 ` Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2010-10-13 18:59 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Anders Kaseorg, git Jonathan Nieder <jrnieder@gmail.com> writes: > Hi Anders, > > Anders Kaseorg wrote: >> Some patches have a timezone formatted like '-08:00' instead of >> '-0800' (e.g. http://lwn.net/Articles/131729/) > > Odd. Any idea what tool generates these patches? > >> --- a/builtin/apply.c >> +++ b/builtin/apply.c > [...] >> @@ -765,7 +765,9 @@ static int has_epoch_timestamp(const char *nameline) >> } >> >> zoneoffset = strtol(timestamp + m[3].rm_so + 1, NULL, 10); >> - zoneoffset = (zoneoffset / 100) * 60 + (zoneoffset % 100); >> + if (m[4].rm_so == m[3].rm_so + 3) >> + zoneoffset /= 100; >> + zoneoffset = zoneoffset * 60 + strtol(timestamp + m[4].rm_so, NULL, 10); > > Might be clearer to write > > if (timestamp[m[3].rm_so + 3] != ':') Neither the patch nor your suggestion makes much sense to me. With the patch, the regexp is now ^(1969-12-31|1970-01-01) <time>(\.0+)? ([-+][0-2][0-9]):?([0-5][0-9]) so $3 is always 3 letters long (i.e. hour with sign), no? IOW, zoneoffset is never divided by 100 by the original patch. What am I missing? ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] apply: Recognize epoch timestamps with : in the timezone 2010-10-13 18:59 ` Junio C Hamano @ 2010-10-13 19:31 ` Junio C Hamano 2010-10-13 22:50 ` Jonathan Nieder 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2010-10-13 19:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Anders Kaseorg, git Junio C Hamano <gitster@pobox.com> writes: > Neither the patch nor your suggestion makes much sense to me. With the > patch, the regexp is now > > ^(1969-12-31|1970-01-01) <time>(\.0+)? ([-+][0-2][0-9]):?([0-5][0-9]) > > so $3 is always 3 letters long (i.e. hour with sign), no? IOW, zoneoffset > is never divided by 100 by the original patch. > > What am I missing? Well, I was missing that without ':' strtol() goes through to parse $3$4 as a single integer, and the division was done to discard the minute part and parse it again. Calling strtol() twice only because some unusual input may have ':' there feels ugly, but now I understand why the patch is written that way, at least. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] apply: Recognize epoch timestamps with : in the timezone 2010-10-13 19:31 ` Junio C Hamano @ 2010-10-13 22:50 ` Jonathan Nieder 2010-10-13 23:37 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Jonathan Nieder @ 2010-10-13 22:50 UTC (permalink / raw) To: Junio C Hamano; +Cc: Anders Kaseorg, git Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: >> Neither the patch nor your suggestion makes much sense to me. With the >> patch, the regexp is now >> >> ^(1969-12-31|1970-01-01) <time>(\.0+)? ([-+][0-2][0-9]):?([0-5][0-9]) [...] > Well, I was missing that without ':' strtol() goes through to parse $3$4 > as a single integer So maybe something like the following would make this easier to follow. diff --git a/builtin/apply.c b/builtin/apply.c index 0fa9a8d..000d3e5 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -733,8 +733,8 @@ static int has_epoch_timestamp(const char *nameline) " " "[0-2][0-9]:[0-5][0-9]:00(\\.0+)?" " " - "([-+][0-2][0-9]):?([0-5][0-9])\n"; + "([-+][0-2][0-9]:?[0-5][0-9])\n"; - const char *timestamp = NULL, *cp; + const char *timestamp = NULL, *cp, *colon; static regex_t *stamp; regmatch_t m[10]; int zoneoffset; @@ -764,10 +764,11 @@ static int has_epoch_timestamp(const char *nameline) return 0; } - zoneoffset = strtol(timestamp + m[3].rm_so + 1, NULL, 10); + zoneoffset = strtol(timestamp + m[3].rm_so + 1, (char **) &colon, 10); - if (m[4].rm_so == m[3].rm_so + 3) - zoneoffset /= 100; - zoneoffset = zoneoffset * 60 + strtol(timestamp + m[4].rm_so, NULL, 10); + if (*colon == ':') + zoneoffset = zoneoffset * 60 + strtol(colon + 1, NULL, 10); + else + zoneoffset = (zoneoffset / 100) * 60 + (zoneoffset % 100); if (timestamp[m[3].rm_so] == '-') zoneoffset = -zoneoffset; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] apply: Recognize epoch timestamps with : in the timezone 2010-10-13 22:50 ` Jonathan Nieder @ 2010-10-13 23:37 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2010-10-13 23:37 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Anders Kaseorg, git Jonathan Nieder <jrnieder@gmail.com> writes: > Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: > >>> Neither the patch nor your suggestion makes much sense to me. With the >>> patch, the regexp is now >>> >>> ^(1969-12-31|1970-01-01) <time>(\.0+)? ([-+][0-2][0-9]):?([0-5][0-9]) > [...] >> Well, I was missing that without ':' strtol() goes through to parse $3$4 >> as a single integer > > So maybe something like the following would make this easier to follow. Yeah, this feels much more natural. > diff --git a/builtin/apply.c b/builtin/apply.c > index 0fa9a8d..000d3e5 100644 > --- a/builtin/apply.c > +++ b/builtin/apply.c > @@ -733,8 +733,8 @@ static int has_epoch_timestamp(const char *nameline) > " " > "[0-2][0-9]:[0-5][0-9]:00(\\.0+)?" > " " > - "([-+][0-2][0-9]):?([0-5][0-9])\n"; > + "([-+][0-2][0-9]:?[0-5][0-9])\n"; > - const char *timestamp = NULL, *cp; > + const char *timestamp = NULL, *cp, *colon; > static regex_t *stamp; > regmatch_t m[10]; > int zoneoffset; > @@ -764,10 +764,11 @@ static int has_epoch_timestamp(const char *nameline) > return 0; > } > > - zoneoffset = strtol(timestamp + m[3].rm_so + 1, NULL, 10); > + zoneoffset = strtol(timestamp + m[3].rm_so + 1, (char **) &colon, 10); > - if (m[4].rm_so == m[3].rm_so + 3) > - zoneoffset /= 100; > - zoneoffset = zoneoffset * 60 + strtol(timestamp + m[4].rm_so, NULL, 10); > + if (*colon == ':') > + zoneoffset = zoneoffset * 60 + strtol(colon + 1, NULL, 10); > + else > + zoneoffset = (zoneoffset / 100) * 60 + (zoneoffset % 100); > if (timestamp[m[3].rm_so] == '-') > zoneoffset = -zoneoffset; > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-10-13 23:38 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-09-29 20:49 [PATCH] apply: Recognize epoch timestamps with : in the timezone Anders Kaseorg 2010-09-29 21:41 ` Jonathan Nieder 2010-09-29 21:49 ` Jonathan Nieder 2010-10-13 18:59 ` Junio C Hamano 2010-10-13 19:31 ` Junio C Hamano 2010-10-13 22:50 ` Jonathan Nieder 2010-10-13 23:37 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).