Skip to contentSkip to conversation

What's happening with vr41xx_giu.c?

PATCH In review Shinya Kuribayashi 6 participants 10 messages Jul 9, 2009 – Jun 15, 2016

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

Does anyone have a similar symptom?

---

skuribay@ubuntu:linux.git$ git checkout -f master
Already on 'master'
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$ git log HEAD^..
commit 4eebdebd71325d0814b1c8e093fd0d1f191da9aa
Author: Kurt Martin <[email protected]>
Date:   Wed Jul 8 19:22:35 2009 -0700

MIPS: SMTC: Move cross VPE writes to after a TC is assigned to VPE.

skuribay@ubuntu:linux.git$

skuribay@ubuntu:linux.git$ git status
# On branch master
nothing to commit (working directory clean)
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$ make distclean
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$ git status
# On branch master
# Changed but not updated:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       deleted:    drivers/char/vr41xx_giu.c
#
no changes added to commit (use "git add" and/or "git commit -a")
skuribay@ubuntu:linux.git$
Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
#1 lore raw

The same to me.

is there a bug in git?

Quoted 36 lines · Shinya Kuribayashi

Does anyone have a similar symptom?

---

skuribay@ubuntu:linux.git$ git checkout -f master
Already on 'master'
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$ git log HEAD^..
commit 4eebdebd71325d0814b1c8e093fd0d1f191da9aa
Author: Kurt Martin <[email protected]>
Date:   Wed Jul 8 19:22:35 2009 -0700

MIPS: SMTC: Move cross VPE writes to after a TC is assigned to VPE.

skuribay@ubuntu:linux.git$

skuribay@ubuntu:linux.git$ git status
# On branch master
nothing to commit (working directory clean)
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$ make distclean
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$ git status
# On branch master
# Changed but not updated:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       deleted:    drivers/char/vr41xx_giu.c
#
no changes added to commit (use "git add" and/or "git commit -a")
skuribay@ubuntu:linux.git$
#2 lore raw
In reply to Shinya Kuribayashi
Quoted 5 lines · Shinya Kuribayashi
skuribay@ubuntu:linux.git$ make distclean
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$ git status
# On branch master
Quoted 9 lines
# Changed but not updated:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       deleted:    drivers/char/vr41xx_giu.c
#
no changes added to commit (use "git add" and/or "git commit -a")
skuribay@ubuntu:linux.git$

Commit 27fdd325dace4a1ebfa10e93ba6f3d25f25df674 turned drivers/char/vr41xx_giu.c into an empty file instead of deleting it when the file was moved to drivers/gpio

"make distclean" deletes any 0 length .c files that it finds.

Leaving drivers/char/vr41xx_giu.c as a zero length file may have been a git bug but was probably just an oversight. I'll send a patch to clean it up as a followup.

Chris

Signature · 2 lines
Chris Dearman
MIPS Technologies Inc            955 East Arques Ave, Sunnyvale CA 94085
#3 lore raw

On Thu, Jul 09, 2009 at 08:07:12PM -0700, Chris Dearman wrote:

This is smelling like a git issue so I'm adding [email protected] to cc list.

Quoted 22 lines
Quoted 6 lines · Shinya Kuribayashi
skuribay@ubuntu:linux.git$ make distclean
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$
skuribay@ubuntu:linux.git$ git status
# On branch master
# Changed but not updated:
Quoted 9 lines
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working  
directory)
#
#       deleted:    drivers/char/vr41xx_giu.c
#
no changes added to commit (use "git add" and/or "git commit -a")
skuribay@ubuntu:linux.git$

Commit 27fdd325dace4a1ebfa10e93ba6f3d25f25df674 turned drivers/char/vr41xx_giu.c into an empty file instead of deleting it when the file was moved to drivers/gpio

Quoted 5 lines

"make distclean" deletes any 0 length .c files that it finds.

Leaving drivers/char/vr41xx_giu.c as a zero length file may have been a git bug but was probably just an oversight. I'll send a patch to clean it up as a followup.

And issue is reproducable. When I go back to commit 27fdd325dace4a1ebfa10e93ba6f3d25f25df674^ and apply Yoichi's patch using git am or git apply this will leave a zero byte drivers/char/vr41xx_giu.c. Patch(1) otoh will remove that file as expected. The patch file Yoichi sent looks perfectly ok; here the headers of the vr41xx_giu.c bit:

[...]

drivers/char/vr41xx_giu.c 1970-01-01 09:00:00.000000000 +0900 renamed from drivers/char/vr41xx_giu.c 2009-06-29 10:06:58.329177629 +0900 +0 -1
diff -pruN -X /home/yuasa/Memo/dontdiff linux-orig/drivers/char/vr41xx_giu.c linux/drivers/char/vr41xx_giu.c
@@ -1,680 +0,0 @@
-/*

[...]

This is with git 1.6.0.6 (git-1.6.0.6-4.fc10.x86_64 from Fedora 10).

The 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
and the git tree in question is Linus' kernel tree available from
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.

Ralf

#4 lore raw

drivers/char/vr41xx_giu.c 1970-01-01 09:00:00.000000000 +0900 line 1

Quoted 11 lines · Ralf Baechle

27fdd325dace4a1ebfa10e93ba6f3d25f25df674^ and apply Yoichi's patch using git am or git apply this will leave a zero byte drivers/char/vr41xx_giu.c. Patch(1) otoh will remove that file as expected. The patch file Yoichi sent looks perfectly ok; here the headers of the vr41xx_giu.c bit:

[...]
diff -pruN -X /home/yuasa/Memo/dontdiff linux-orig/drivers/char/vr41xx_giu.c linux/drivers/char/vr41xx_giu.c

drivers/char/vr41xx_giu.c 1970-01-01 09:00:00.000000000 +0900 renamed from drivers/char/vr41xx_giu.c 2009-06-29 10:06:58.329177629 +0900 +0 -1
@@ -1,680 +0,0 @@
-/*

If you look for -E option in "man patch" and find that it says "causes patch to remove output files that are empty after the patches have been applied.", you will realize that your claim that "patch(1) otoh ... as expected" does not match the reality for everybody. It is true only if you _are_ explicitly asking to remove such an empty file.

The recent diff specification (at least the one in POSIX.1) says that file removal is marked by the UNIX epoch timestamp you see there, instead of a more recent timestamp. IOW, you should _in theory_ be able to tell by looking at the 1970-01-01 timestamp that the intention of this patch is not to make the file empty, but is to remove.

But in practice, because traditionally GNU diff and other people's diff placed pretty arbitrary garbage after the TAB that follows the filename, patch does not rely on that convention to detect a removal patch. Notice that even -E option does not pay attention to that timestamp line, but removes files that become _empty_.

Neither do we. The patch application toolchain in "git" does not have -E option and the above patch is interpreted just like traditional patch does by default: the file goes empty.

The output from "git diff" is designed so that (1) it can distinguish the removal case and the goes-empty case more clearly, and also that (2) it can be safely used by patch(1). A removal patch from git looks like:

    diff --git a/file b/file
    deleted file mode 100644
    index 363ef61..0000000
    --- a/file
    +++ /dev/null
    @@ -1 +0,0 @@
    -original contents

while "goes empty" patch looks like:

    diff --git a/file b/file
    index 363ef61..e69de29 100644
    --- a/file
    +++ b/file
    @@ -1 +0,0 @@
    -original contents

and when applied with git, they both produce "expected" results.

We _could_ add -E option to "git apply" and pass that through "git am" to support projects like the kernel where 0-byte files are forbidden. A patch to do that shouldn't be too involved.

#5 lore raw
Quoted 3 lines · Junio C Hamano

We _could_ add -E option to "git apply" and pass that through "git am" to support projects like the kernel where 0-byte files are forbidden. A patch to do that shouldn't be too involved.

I ended up doing something a bit more useful. A two-patch series follows shortly.

#6 lore raw

PATCH 1/2 tm_to_time_t(): allow times in year 1969

We used to reject anything older than January 1st 1970 regardless of the timezone, but in order to parse UNIX epoch in zones west of GMT, we need to allow the time without zone to be a little bit into 1969.

---
 date.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)
date.c +8 -8
index 409a17d..5d259af 100644
@@ -6,6 +6,7 @@
 
 #include "cache.h"
 
+#define BAD_STRUCT_TM (-400*24*60*60)
 /*
  * This is like mktime, but without normalization of tm_wday and tm_yday.
  */
@@ -18,10 +19,10 @@ time_t tm_to_time_t(const struct tm *tm)
 	int month = tm->tm_mon;
 	int day = tm->tm_mday;
 
-	if (year < 0 || year > 129) /* algo only works for 1970-2099 */
-		return -1;
+	if (year < -1 || year > 129) /* algo only works for 1969-2099 */
+		return BAD_STRUCT_TM;
 	if (month < 0 || month > 11) /* array bounds */
-		return -1;
+		return BAD_STRUCT_TM;
 	if (month < 2 || (year + 2) % 4)
 		day--;
 	return (year * 365 + (year + 1) / 4 + mdays[month] + day) * 24*60*60UL +
@@ -337,7 +338,7 @@ static int is_date(int year, int month, int day, struct tm *now_tm, time_t now,
 				return 1;
 			r->tm_year = now_tm->tm_year;
 		}
-		else if (year >= 1970 && year < 2100)
+		else if (year >= 1969 && year < 2100)
 			r->tm_year = year - 1900;
 		else if (year > 70 && year < 100)
 			r->tm_year = year;
@@ -611,12 +612,11 @@ int parse_date(const char *date, char *result, int maxlen)
 
 	/* mktime uses local timezone */
 	then = tm_to_time_t(&tm);
-	if (offset == -1)
-		offset = (then - mktime(&tm)) / 60;
-
-	if (then == -1)
+	if (then == BAD_STRUCT_TM)
 		return -1;
 
+	if (offset == -1)
+		offset = (then - mktime(&tm)) / 60;
 	if (!tm_gmt)
 		then -= offset * 60;
 	return date_string(then, offset, result, maxlen);
Signature · 1 line
1.6.3.3.412.gf581d
#7 lore raw

PATCH 2/2 apply: notice creation/removal patches produced by GNU diff

In reply to Junio C Hamano

Unified context patch generated by GNU diff has UNIX epoch timestamp on the side that does not exist when the patch is about a creation or a deletion event. Notice this convention when reading a non-git diff.

---
 builtin-apply.c          |   63 ++++++++++++++++++++++++++++++++-
 t/t4132-apply-removal.sh |   88 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 150 insertions(+), 1 deletions(-)
 create mode 100755 t/t4132-apply-removal.sh
builtin-apply.c +62 -1
index dc0ff5e..06e80e4 100644
@@ -458,6 +458,57 @@ static int guess_p_value(const char *nameline)
 }
 
 /*
+ * Does the ---/+++ line has the POSIX timestamp after the last HT?
+ * GNU diff puts epoch there to signal a creation/deletion event.  Is
+ * this such a timestamp?
+ */
+static int has_epoch_timestamp(const char *nameline)
+{
+	/*
+	 * We are only interested in epoch timestamp; any non-zero
+	 * fraction cannot be one, hence "(\.0+)?" in the regexp below.
+	 */
+	const char stamp_regexp[] =
+		"^[0-9][0-9][0-9][0-9]-[01][0-9]-[0-3][0-9]"
+		" "
+		"[0-2][0-9]:[0-5][0-9]:[0-6][0-9](\\.0+)?"
+		" "
+		"[-+][0-2][0-9][0-5][0-9]\n";
+	const char *timestamp = NULL, *cp;
+	static regex_t *stamp;
+	int status;
+	char parsed[100];
+
+	for (cp = nameline; *cp != '\n'; cp++) {
+		if (*cp == '\t')
+			timestamp = cp + 1;
+	}
+	if (!timestamp)
+		return 0;
+	if (!stamp) {
+		stamp = xmalloc(sizeof(*stamp));
+		if (regcomp(stamp, stamp_regexp, REG_EXTENDED)) {
+			warning("Cannot prepare timestamp regexp %s",
+				stamp_regexp);
+			return 0;
+		}
+	}
+
+	status = regexec(stamp, timestamp, 0, NULL, 0);
+	if (status) {
+		if (status != REG_NOMATCH)
+			warning("regexec returned %d for input: %s",
+				status, timestamp);
+		return 0;
+	}
+
+	parse_date(timestamp, parsed, sizeof(parsed));
+	if (parsed[0] == '0' && parsed[1] == ' ')
+		return 1;
+	return 0;
+}
+
+/*
  * Get the name etc info from the ---/+++ lines of a traditional patch header
  *
  * FIXME! The end-of-filename heuristics are kind of screwy. For existing
@@ -493,7 +544,17 @@ static void parse_traditional_patch(const char *first, const char *second, struc
 	} else {
 		name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB);
 		name = find_name(second, name, p_value, TERM_SPACE | TERM_TAB);
-		patch->old_name = patch->new_name = name;
+		if (has_epoch_timestamp(first)) {
+			patch->is_new = 1;
+			patch->is_delete = 0;
+			patch->new_name = name;
+		} else if (has_epoch_timestamp(second)) {
+			patch->is_new = 0;
+			patch->is_delete = 1;
+			patch->old_name = name;
+		} else {
+			patch->old_name = patch->new_name = name;
+		}
 	}
 	if (!name)
 		die("unable to find filename in patch at line %d", linenr);
t/t4132-apply-removal.sh new file +88 -0

88 lines not on this page · raw message

Signature · 1 line
1.6.3.3.412.gf581d
#8 lore raw

Re: [PATCH 2/2] apply: notice creation/removal patches produced by GNU diff

Quoted 4 lines · Junio C Hamano

Unified context patch generated by GNU diff has UNIX epoch timestamp on the side that does not exist when the patch is about a creation or a deletion event. Notice this convention when reading a non-git diff.

Hmm. Do you really want to do a regex here? That seems overkill. Why not just try to parse the date?

builtin-apply.c static int guess_p_value(const char *nameline) · line 476

Quoted 6 lines
builtin-apply.c +6 -0
+	const char stamp_regexp[] =
+		"^[0-9][0-9][0-9][0-9]-[01][0-9]-[0-3][0-9]"
+		" "
+		"[0-2][0-9]:[0-5][0-9]:[0-6][0-9](\\.0+)?"
+		" "
+		"[-+][0-2][0-9][0-5][0-9]\n";

Also, why are you apparently expecting micro-seconds to always be all zeroes? Maybe that's the common case, but I'd expect that somebody has non-zero microseconds on filesystems that support them..

Linus

#9 lore raw

Re: [PATCH 2/2] apply: notice creation/removal patches produced by GNU diff

Quoted 8 lines · Linus Torvalds
Quoted 4 lines · Junio C Hamano

Unified context patch generated by GNU diff has UNIX epoch timestamp on the side that does not exist when the patch is about a creation or a deletion event. Notice this convention when reading a non-git diff.

Hmm. Do you really want to do a regex here? That seems overkill. Why not just try to parse the date?

If parse_date() says "It is a date and it is UNIX epoch", we mark the patch as either creation or deletion, and that is used by various consistency checks. Deletion patch must remove the entire line, the file must not exist if it is the target of a creation patch, etc.

I found parse_date() to be a bit too forgiving for my taste for this particular application; I wanted to be anal and only accept the format specified by

http://www.opengroup.org/onlinepubs/9699919799/utilities/diff.html#tag_20_34_10_07

By the way, the above does not mention anything about marking creation/deletion event with UNIX epoch; I am guessing it is a GNU extension.

Quoted 10 lines

builtin-apply.c static int guess_p_value(const char *nameline) · line 476

Quoted 6 lines
builtin-apply.c +6 -0
+	const char stamp_regexp[] =
+		"^[0-9][0-9][0-9][0-9]-[01][0-9]-[0-3][0-9]"
+		" "
+		"[0-2][0-9]:[0-5][0-9]:[0-6][0-9](\\.0+)?"
+		" "
+		"[-+][0-2][0-9][0-5][0-9]\n";

Also, why are you apparently expecting micro-seconds to always be all zeroes? Maybe that's the common case, but I'd expect that somebody has non-zero microseconds on filesystems that support them..

As the comment before the quoted part of the patch says, the point of this function is not about detecting the presense of a timestamp (and parsing the time), but telling if the timestamp that represents UNIX epoch is there. The first iteration of my patch did not even use parse_date() but accepted only 1969-12-31 (west of GMT) or 1970-01-01 (east of GMT) and then checked timestamp with timezone manually. Perhaps that might be a better way to do this function? I dunno.