Re: git log - crash and core dump
On Tue, Apr 16, 2013 at 10:26:40PM -0700, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: René Scharfe rene.scha...@lsrfire.ath.cx writes: How about making split_ident_line() a bit friendlier be letting it provide the epoch as default time stamp instead of NULL? Two knee-jerk concerns I have without going back to the callers: * Would that 0 ever be given to the approxidate parser, which rejects ancient dates in numbers-since-epoch format without @ prefix? * Does any existing caller use the NULL as a sign to see the input was without date and act on that information? I looked at all the callers (there aren't that many), and none of them did Do this on a person-only ident, and do that on an ident with timestamp. So for the callers that ignore timestamp, your patch will be a no-op, and for others that assume there is a timestamp, it will turn a crash/segv into output with funny timestamp. What about sane_ident_split in builtin/commit.c? It explicitly rejects a NULL date. The logic in determine_author_info is a little hard to follow (it assembles the ident line with fmt_ident and then reparses it), but I believe it should be catching a bogus line from commit -c, or from GIT_AUTHOR_DATE in the environment. As a side note, when determine_author_info sees a bogus ident, it appears to just silently ignore it, which is probably a bad thing. Shouldn't we by complaining? Or am I mis-reading the code? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
On Wed, Apr 17, 2013 at 09:22:01AM +0400, Ivan Lyapunov wrote: I checked René Scharfe's patch and it works - no more git log crash. Also I checked a broken commits by git show and the most of them created by me. I can confirm I never used anyting else except console git commit or netbeans gui to commit, which is just git gui wrapper tool. Doesn't NetBeans use JGit[1]? That makes it a bit more than a wrapper for the Git command line tools. [1] http://eclipse.org/jgit/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
netbeans use some integrated git wrapper and I don't know about JGit source base or not. In Eclipse we use Egit. Also all broken commits limited to november 2012, but we still continue to use the same development environments without any problems Ivan 2013/4/17 John Keeping j...@keeping.me.uk: On Wed, Apr 17, 2013 at 09:22:01AM +0400, Ivan Lyapunov wrote: I checked René Scharfe's patch and it works - no more git log crash. Also I checked a broken commits by git show and the most of them created by me. I can confirm I never used anyting else except console git commit or netbeans gui to commit, which is just git gui wrapper tool. Doesn't NetBeans use JGit[1]? That makes it a bit more than a wrapper for the Git command line tools. [1] http://eclipse.org/jgit/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
On Wed, 17 Apr 2013 13:14:48 +0400 Ivan Lyapunov dron...@gmail.com wrote: netbeans use some integrated git wrapper and I don't know about JGit source base or not. In Eclipse we use Egit. Also all broken commits limited to november 2012, but we still continue to use the same development environments without any problems Does the same also mean of the same version? I mean that if, say, you managed to update netbeans after November, 2012 that would explain a lot as the update might just silently pull a fix for the Git-interfacing code. The same might apply to Git itself as well. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
missed a list sorry for dup I ment the same because git changes the version too. Also netbeans/eclipse upgrade are not synced, because of different products. So the same ment only names, not versions. But in deep search another repos I found the broken commits in Eclipse repo dated by 13 march 2013. Can them produced because of previous broken commits? And probably you can be right about java git wrappers can produce this issues, I'll try to make a broken repo from scratch later. Ivan 2013/4/17 Ivan Lyapunov dron...@gmail.com: I ment the same because git changes the version too. Also netbeans/eclipse upgrade are not synced, because of different products. So the same ment only names, not versions. But in deep search another repos I found the broken commits in Eclipse repo dated by 13 march 2013. Can them produced because of previous broken commits? And probably you can be right about java git wrappers can produce this issues, I'll try to make a broken repo from scratch later. Ivan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
Jeff King p...@peff.net writes: What about sane_ident_split in builtin/commit.c? It explicitly rejects a NULL date. The logic in determine_author_info is a little hard to follow (it assembles the ident line with fmt_ident and then reparses it), but I believe it should be catching a bogus line from commit -c, or from GIT_AUTHOR_DATE in the environment. Yeah, you are of course right. I noticed that fmt_ident then parse sequence a bit funny, too. It seems to manually parse to prepare what it feeds fmt_ident. As a side note, when determine_author_info sees a bogus ident, it appears to just silently ignore it, which is probably a bad thing. True, too. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
Am 17.04.2013 08:39, schrieb Jeff King: On Tue, Apr 16, 2013 at 10:26:40PM -0700, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: René Scharfe rene.scha...@lsrfire.ath.cx writes: How about making split_ident_line() a bit friendlier be letting it provide the epoch as default time stamp instead of NULL? Two knee-jerk concerns I have without going back to the callers: * Would that 0 ever be given to the approxidate parser, which rejects ancient dates in numbers-since-epoch format without @ prefix? * Does any existing caller use the NULL as a sign to see the input was without date and act on that information? I looked at all the callers (there aren't that many), and none of them did Do this on a person-only ident, and do that on an ident with timestamp. So for the callers that ignore timestamp, your patch will be a no-op, and for others that assume there is a timestamp, it will turn a crash/segv into output with funny timestamp. What about sane_ident_split in builtin/commit.c? It explicitly rejects a NULL date. The logic in determine_author_info is a little hard to follow (it assembles the ident line with fmt_ident and then reparses it), but I believe it should be catching a bogus line from commit -c, or from GIT_AUTHOR_DATE in the environment. Right, so let's keep the NULLs and fix the individual cases. A quick git grep -W -e date_begin -e date_end -e tz_begin -e tz_end reveals that there are only the ones we talked about: blame, pretty, commit and -- of course -- ident. And only the first two need fixing. As a side note, when determine_author_info sees a bogus ident, it appears to just silently ignore it, which is probably a bad thing. Shouldn't we by complaining? Or am I mis-reading the code? The code looks complicated, but I just tried it: fmt_ident() dies if you give it an invalid date. René -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
On Wed, Apr 17, 2013 at 07:59:28PM +0200, René Scharfe wrote: What about sane_ident_split in builtin/commit.c? It explicitly rejects a NULL date. The logic in determine_author_info is a little hard to follow (it assembles the ident line with fmt_ident and then reparses it), but I believe it should be catching a bogus line from commit -c, or from GIT_AUTHOR_DATE in the environment. Right, so let's keep the NULLs and fix the individual cases. A quick git grep -W -e date_begin -e date_end -e tz_begin -e tz_end reveals that there are only the ones we talked about: blame, pretty, commit and -- of course -- ident. And only the first two need fixing. Yes, that matches the list I came up with in February. I think we also need to do something about git cat-file -p, which does not use the split_ident_line parser (but has its own problems with the home-grown parser). As a side note, when determine_author_info sees a bogus ident, it appears to just silently ignore it, which is probably a bad thing. Shouldn't we by complaining? Or am I mis-reading the code? The code looks complicated, but I just tried it: fmt_ident() dies if you give it an invalid date. It does seem like determine_author_info can be greatly simplified, but I haven't looked all that closely. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
Am 17.04.2013 20:02, schrieb Jeff King: I think we also need to do something about git cat-file -p, which does not use the split_ident_line parser (but has its own problems with the home-grown parser). Ah, while it prints commit object contents verbatim, it formats the date of tags. And it does it without help from tag.c (or ident.c), which in turn does its own parsing as well. So it looks like we have two more candidates for conversion to split_ident_line() here. René -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
Am 16.04.2013 18:55, schrieb Ivan Lyapunov:
git version 1.8.2.1 crashes on my ArchLinux x86_64 on git log command
gdb bt is attached
git log | less
does not crash in same repo
I cannot share a repo for a debug purposes since it's private repo of
my employer
but I can perform any suitable tests on repo to help this bug to be fixed
#0 0x7722b3e6 in strtoull_l_internal () from /usr/lib/libc.so.6
#1 0x004b31d4 in pp_user_info (pp=pp@entry=0x7fffd310,
what=what@entry=0x521379 Author, sb=sb@entry=0x7fffd290,
line=line@entry=0x7b3a45 Ivan Lyapunov ilyapu...@trueconf.ru-
1354083115 +0400\ncommitter Ivan Lyapunov ilyapu...@trueconf.ru
So this is the author information, correct?
Ivan Lyapunov ilyapu...@trueconf.ru- 1354083115 +0400
|author name| |---author email| ^^^ |--time--| |tz-|
How did you manage to add the - after the email address?
What does git log in version 1.8.1 or earlier show for this commit?
1354083115 +0400\n\n- small merge fixes,
encoding=encoding@entry=0x505400 UTF-8) at pretty.c:441
#2 0x004b533a in pp_header (sb=0x7fffd290,
msg_p=0x7fffd228, commit=0x7c1e10, encoding=0x505400 UTF-8,
pp=0x7fffd310) at pretty.c:1415
#3 pretty_print_commit (pp=pp@entry=0x7fffd310,
commit=commit@entry=0x7c1e10, sb=sb@entry=0x7fffd290) at
pretty.c:1545
#4 0x004a0b45 in show_log (opt=opt@entry=0x7fffd4d0) at
log-tree.c:683
#5 0x004a1616 in log_tree_commit
(opt=opt@entry=0x7fffd4d0, commit=commit@entry=0x7c1e10) at
log-tree.c:859
#6 0x00438b03 in cmd_log_walk (rev=rev@entry=0x7fffd4d0)
at builtin/log.c:310
#7 0x004395dd in cmd_log (argc=1, argv=0x7fffdd30,
prefix=0x0) at builtin/log.c:582
#8 0x0040562d in run_builtin (argv=0x7fffdd30, argc=1,
p=0x754d18 commands.21404+1080) at git.c:282
#9 handle_internal_command (argc=1, argv=0x7fffdd30) at git.c:444
#10 0x00404a6f in run_argv (argv=0x7fffdbd0,
argcp=0x7fffdbdc) at git.c:490
#11 main (argc=1, argv=0x7fffdd30) at git.c:565
Does this patch help?
pretty.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
const char *mailbuf, *namebuf;
size_t namelen, maillen;
int max_length = 78; /* per rfc2822 */
- unsigned long time;
- int tz;
+ unsigned long time = 0;
+ int tz = 0;
if (pp-fmt == CMIT_FMT_ONELINE)
return;
@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(name, namebuf, namelen);
namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
- time = strtoul(ident.date_begin, date, 10);
- tz = strtol(date, NULL, 10);
+ if (ident.date_begin) {
+ time = strtoul(ident.date_begin, date, 10);
+ tz = strtol(date, NULL, 10);
+ }
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
René Scharfe rene.scha...@lsrfire.ath.cx writes:
Does this patch help?
pretty.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
const char *mailbuf, *namebuf;
size_t namelen, maillen;
int max_length = 78; /* per rfc2822 */
- unsigned long time;
- int tz;
+ unsigned long time = 0;
+ int tz = 0;
if (pp-fmt == CMIT_FMT_ONELINE)
return;
@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(name, namebuf, namelen);
namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
- time = strtoul(ident.date_begin, date, 10);
- tz = strtol(date, NULL, 10);
+ if (ident.date_begin) {
+ time = strtoul(ident.date_begin, date, 10);
+ tz = strtol(date, NULL, 10);
+ }
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
Looks like a sensible change. split_ident_line() decided that the
given input was mangled and decided there is no valid date (the
input had where the timestamp string was required), so the
updated code leaves the time/tz unspecified.
It still is curious how a malformed line was created in the first
place. I wouldn't worry if a private tool used hash-object to
create such a commit, but if it is something that is commonly used
(e.g. git commit), others may suffer from the same and the tool
needs to be tightened a bit.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
First, lest I forget again: Thank you, Ivan, for the very useful
bug report!
Am 16.04.2013 21:45, schrieb Junio C Hamano:
René Scharfe rene.scha...@lsrfire.ath.cx writes:
Does this patch help?
pretty.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
const char *mailbuf, *namebuf;
size_t namelen, maillen;
int max_length = 78; /* per rfc2822 */
-unsigned long time;
-int tz;
+unsigned long time = 0;
+int tz = 0;
if (pp-fmt == CMIT_FMT_ONELINE)
return;
@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context *pp,
strbuf_add(name, namebuf, namelen);
namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
-time = strtoul(ident.date_begin, date, 10);
-tz = strtol(date, NULL, 10);
+if (ident.date_begin) {
+time = strtoul(ident.date_begin, date, 10);
+tz = strtol(date, NULL, 10);
+}
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
Looks like a sensible change. split_ident_line() decided that the
given input was mangled and decided there is no valid date (the
input had where the timestamp string was required), so the
updated code leaves the time/tz unspecified.
We'd need update pretty.c::format_person_part() and
builtin/blame.c::get_ac_line() as well, though.
How about making split_ident_line() a bit friendlier be letting it
provide the epoch as default time stamp instead of NULL? We shouldn't
do that if we'd like to be able to tell a missing/broken time stamp
apart from a commit that was actually made back in 1970 (e.g. an
imported one). Or if we'd like to not show a time stamp in git log
output at all in that case.
-- 8 --
Subject: ident: let split_ident_line() provide a default time stamp
If a commit has a broken time stamp, split_ident_line() sets
date_begin, date_end, tz_begin and tz_end to NULL. Not all callers
are prepared to handle that case and segfault.
Instead of fixing them and having to be careful while implementing
the next caller, provide a string consisting of the number zero as
default value, representing the UNIX epoch. That's the value that
git log showed before it was converted to use split_ident_line().
Reported-by: Ivan Lyapunov dron...@gmail.com
Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx
---
ident.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/ident.c b/ident.c
index 1c123e6..ee840f4 100644
--- a/ident.c
+++ b/ident.c
@@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb,
const char *src)
sb-buf[sb-len] = '\0';
}
+static const char zero_string[] = 0;
+
/*
* Reverse of fmt_ident(); given an ident line, split the fields
* to allow the caller to parse it.
@@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const
char *line, int len)
return 0;
person_only:
- split-date_begin = NULL;
- split-date_end = NULL;
- split-tz_begin = NULL;
- split-tz_end = NULL;
+ split-date_begin = zero_string;
+ split-date_end = zero_string + strlen(zero_string);
+ split-tz_begin = zero_string;
+ split-tz_end = zero_string + strlen(zero_string);
return 0;
}
--
1.8.2.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
On Tue, Apr 16, 2013 at 9:45 PM, Junio C Hamano gits...@pobox.com wrote: It still is curious how a malformed line was created in the first place. I wouldn't worry if a private tool used hash-object to create such a commit, but if it is something that is commonly used (e.g. git commit), others may suffer from the same and the tool needs to be tightened a bit. I already happened to see one like that, and it was clearly imported through remote-hg. I've not been able to reproduce though, and the parser in git-fast-import seemed already robust enough to me to not allow this kind of messed-up line. I will see if I can find some time to reproduce/investigate this deeper. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
On Tue, Apr 16, 2013 at 12:45:03PM -0700, Junio C Hamano wrote:
René Scharfe rene.scha...@lsrfire.ath.cx writes:
Does this patch help?
pretty.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/pretty.c b/pretty.c
index d3a82d2..713eefc 100644
--- a/pretty.c
+++ b/pretty.c
@@ -405,8 +405,8 @@ void pp_user_info(const struct pretty_print_context *pp,
const char *mailbuf, *namebuf;
size_t namelen, maillen;
int max_length = 78; /* per rfc2822 */
- unsigned long time;
- int tz;
+ unsigned long time = 0;
+ int tz = 0;
if (pp-fmt == CMIT_FMT_ONELINE)
return;
@@ -438,8 +438,10 @@ void pp_user_info(const struct pretty_print_context
*pp,
strbuf_add(name, namebuf, namelen);
namelen = name.len + mail.len + 3; /* ' ' + '' + '' */
- time = strtoul(ident.date_begin, date, 10);
- tz = strtol(date, NULL, 10);
+ if (ident.date_begin) {
+ time = strtoul(ident.date_begin, date, 10);
+ tz = strtol(date, NULL, 10);
+ }
if (pp-fmt == CMIT_FMT_EMAIL) {
strbuf_addstr(sb, From: );
Looks like a sensible change. split_ident_line() decided that the
given input was mangled and decided there is no valid date (the
input had where the timestamp string was required), so the
updated code leaves the time/tz unspecified.
Hmm. This seemed oddly familiar to me, and indeed:
http://thread.gmane.org/gmane.comp.version-control.git/216870/focus=217077
We need to fix blame, too, and there is a question of how cat-file -p
behaves.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
René Scharfe rene.scha...@lsrfire.ath.cx writes: How about making split_ident_line() a bit friendlier be letting it provide the epoch as default time stamp instead of NULL? Two knee-jerk concerns I have without going back to the callers: * Would that 0 ever be given to the approxidate parser, which rejects ancient dates in numbers-since-epoch format without @ prefix? * Does any existing caller use the NULL as a sign to see the input was without date and act on that information? -- 8 -- Subject: ident: let split_ident_line() provide a default time stamp If a commit has a broken time stamp, split_ident_line() sets date_begin, date_end, tz_begin and tz_end to NULL. Not all callers are prepared to handle that case and segfault. Instead of fixing them and having to be careful while implementing the next caller, provide a string consisting of the number zero as default value, representing the UNIX epoch. That's the value that git log showed before it was converted to use split_ident_line(). Reported-by: Ivan Lyapunov dron...@gmail.com Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx --- ident.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ident.c b/ident.c index 1c123e6..ee840f4 100644 --- a/ident.c +++ b/ident.c @@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src) sb-buf[sb-len] = '\0'; } +static const char zero_string[] = 0; + /* * Reverse of fmt_ident(); given an ident line, split the fields * to allow the caller to parse it. @@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const char *line, int len) return 0; person_only: - split-date_begin = NULL; - split-date_end = NULL; - split-tz_begin = NULL; - split-tz_end = NULL; + split-date_begin = zero_string; + split-date_end = zero_string + strlen(zero_string); + split-tz_begin = zero_string; + split-tz_end = zero_string + strlen(zero_string); return 0; } -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git log - crash and core dump
Looks like I missed a lot of fun I'm sleeping ;) The current repo is a product of our core crossplatform team, working on Linux/MacOSX/Win32 environment. The Linux environment also separates on Ubuntu/ArchLinux distros, and probably most of repo bugs coming from ArchLinux, since other environments are stable in time. I can't say exact git verison the bugs was introduced, but we have a commits, dated by 0 secs since the Epoch and invalid Author fields also. Also git fsck --full got the following stats on repo Checking object directories: 100% (256/256), done. error in commit 7eeb541987af8a589c6ebee53346c48a13142233: invalid author/committer line - missing space before date error in commit c23b0a487143e5d5d96cdc5354975e95114241ee: invalid author/committer line - missing space before date error in commit c7fa421863e073996b5d1ba6beb6001b9d146cba: invalid author/committer line - missing space before date error in commit 131155bd75c588bcd251b719c483d1d5bcb78504: invalid author/committer line - missing space before date error in commit 0888e7ffe6ae0aaf1b6d1ba67d05715487f88a52: invalid author/committer line - missing space before date error in commit 3cdeddd15c251a13fb3e79844ed3ea0e02cb611a: invalid author/committer line - missing space before date error in commit 21f9fe1565d89da845fc7080495c922103bacf24: invalid author/committer line - missing space before date error in commit f557d3427ba1bb33a1c1fd2c7936efa7e7c70281: invalid author/committer line - missing space before date error in commit c625943779c72b41b08b41730e56126b89cbb7b4: invalid author/committer line - missing space before date error in commit a83fc863991aae2bdad148a5897ed4315792dd82: invalid author/committer line - missing space before date error in commit 207321f773e695b2ae4c34620bc663383f90: invalid author/committer line - missing space before date error in commit 67368e9eda9892acd6c6ebf03dd6f22b6de2db8a: invalid author/committer line - missing space before date error in commit 525a5d508a7f466a1339752e921517f4db8c4af6: invalid author/committer line - missing space before date error in commit 38215e27f74caa342e3353c4cd548fcf8c1df3dc: invalid author/committer line - missing space before date error in commit 1ee0167194eb34caca2c20ce5c74d062fc898718: invalid author/committer line - missing space before date error in commit 3274c469b981285f9a4d0b0a62afbb8f4d3e93ae: invalid author/committer line - missing space before date error in commit f37ab83f71ca93f42256e05efdd4244eb321efaf: invalid author/committer line - missing space before date error in commit 9bb6f7d63bb5a37b8afc3ae090bd6f34deb68633: invalid author/committer line - missing space before date Checking objects: 100% (9576/9576), done. And I haven't find a way to fix without loosing a commits history, so we left them as it is. I will check the approved patches and writeback a little bit later. Thanks a lot for looking this Ivan 2013/4/17 Junio C Hamano gits...@pobox.com: René Scharfe rene.scha...@lsrfire.ath.cx writes: How about making split_ident_line() a bit friendlier be letting it provide the epoch as default time stamp instead of NULL? Two knee-jerk concerns I have without going back to the callers: * Would that 0 ever be given to the approxidate parser, which rejects ancient dates in numbers-since-epoch format without @ prefix? * Does any existing caller use the NULL as a sign to see the input was without date and act on that information? -- 8 -- Subject: ident: let split_ident_line() provide a default time stamp If a commit has a broken time stamp, split_ident_line() sets date_begin, date_end, tz_begin and tz_end to NULL. Not all callers are prepared to handle that case and segfault. Instead of fixing them and having to be careful while implementing the next caller, provide a string consisting of the number zero as default value, representing the UNIX epoch. That's the value that git log showed before it was converted to use split_ident_line(). Reported-by: Ivan Lyapunov dron...@gmail.com Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx --- ident.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ident.c b/ident.c index 1c123e6..ee840f4 100644 --- a/ident.c +++ b/ident.c @@ -191,6 +191,8 @@ static void strbuf_addstr_without_crud(struct strbuf *sb, const char *src) sb-buf[sb-len] = '\0'; } +static const char zero_string[] = 0; + /* * Reverse of fmt_ident(); given an ident line, split the fields * to allow the caller to parse it. @@ -254,10 +256,10 @@ int split_ident_line(struct ident_split *split, const char *line, int len) return 0; person_only: - split-date_begin = NULL; - split-date_end = NULL; - split-tz_begin = NULL; - split-tz_end = NULL; + split-date_begin = zero_string; + split-date_end = zero_string + strlen(zero_string); + split-tz_begin = zero_string; + split-tz_end = zero_string + strlen(zero_string);
Re: git log - crash and core dump
I checked René Scharfe's patch and it works - no more git log crash. Also I checked a broken commits by git show and the most of them created by me. I can confirm I never used anyting else except console git commit or netbeans gui to commit, which is just git gui wrapper tool. Several commits is a just branch merge commits with no changes at all. The modification time across broken commits is around from near end of Oct to end of Nov 2012 and there are good commits between broken maded on the same machine. I hope it helps to reproduce the problem. Also it would be good if there are some way to fix this errors in repo, since we found the same bugs in our Eclipse-based Android project repo Ivan 2013/4/17 Ivan Lyapunov dron...@gmail.com: Looks like I missed a lot of fun I'm sleeping ;) The current repo is a product of our core crossplatform team, working on Linux/MacOSX/Win32 environment. The Linux environment also separates on Ubuntu/ArchLinux distros, and probably most of repo bugs coming from ArchLinux, since other environments are stable in time. I can't say exact git verison the bugs was introduced, but we have a commits, dated by 0 secs since the Epoch and invalid Author fields also. Also git fsck --full got the following stats on repo Checking object directories: 100% (256/256), done. error in commit 7eeb541987af8a589c6ebee53346c48a13142233: invalid author/committer line - missing space before date error in commit c23b0a487143e5d5d96cdc5354975e95114241ee: invalid author/committer line - missing space before date error in commit c7fa421863e073996b5d1ba6beb6001b9d146cba: invalid author/committer line - missing space before date error in commit 131155bd75c588bcd251b719c483d1d5bcb78504: invalid author/committer line - missing space before date error in commit 0888e7ffe6ae0aaf1b6d1ba67d05715487f88a52: invalid author/committer line - missing space before date error in commit 3cdeddd15c251a13fb3e79844ed3ea0e02cb611a: invalid author/committer line - missing space before date error in commit 21f9fe1565d89da845fc7080495c922103bacf24: invalid author/committer line - missing space before date error in commit f557d3427ba1bb33a1c1fd2c7936efa7e7c70281: invalid author/committer line - missing space before date error in commit c625943779c72b41b08b41730e56126b89cbb7b4: invalid author/committer line - missing space before date error in commit a83fc863991aae2bdad148a5897ed4315792dd82: invalid author/committer line - missing space before date error in commit 207321f773e695b2ae4c34620bc663383f90: invalid author/committer line - missing space before date error in commit 67368e9eda9892acd6c6ebf03dd6f22b6de2db8a: invalid author/committer line - missing space before date error in commit 525a5d508a7f466a1339752e921517f4db8c4af6: invalid author/committer line - missing space before date error in commit 38215e27f74caa342e3353c4cd548fcf8c1df3dc: invalid author/committer line - missing space before date error in commit 1ee0167194eb34caca2c20ce5c74d062fc898718: invalid author/committer line - missing space before date error in commit 3274c469b981285f9a4d0b0a62afbb8f4d3e93ae: invalid author/committer line - missing space before date error in commit f37ab83f71ca93f42256e05efdd4244eb321efaf: invalid author/committer line - missing space before date error in commit 9bb6f7d63bb5a37b8afc3ae090bd6f34deb68633: invalid author/committer line - missing space before date Checking objects: 100% (9576/9576), done. And I haven't find a way to fix without loosing a commits history, so we left them as it is. I will check the approved patches and writeback a little bit later. Thanks a lot for looking this Ivan 2013/4/17 Junio C Hamano gits...@pobox.com: René Scharfe rene.scha...@lsrfire.ath.cx writes: How about making split_ident_line() a bit friendlier be letting it provide the epoch as default time stamp instead of NULL? Two knee-jerk concerns I have without going back to the callers: * Would that 0 ever be given to the approxidate parser, which rejects ancient dates in numbers-since-epoch format without @ prefix? * Does any existing caller use the NULL as a sign to see the input was without date and act on that information? -- 8 -- Subject: ident: let split_ident_line() provide a default time stamp If a commit has a broken time stamp, split_ident_line() sets date_begin, date_end, tz_begin and tz_end to NULL. Not all callers are prepared to handle that case and segfault. Instead of fixing them and having to be careful while implementing the next caller, provide a string consisting of the number zero as default value, representing the UNIX epoch. That's the value that git log showed before it was converted to use split_ident_line(). Reported-by: Ivan Lyapunov dron...@gmail.com Signed-off-by: Rene Scharfe rene.scha...@lsrfire.ath.cx --- ident.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/ident.c b/ident.c
Re: git log - crash and core dump
Junio C Hamano gits...@pobox.com writes: René Scharfe rene.scha...@lsrfire.ath.cx writes: How about making split_ident_line() a bit friendlier be letting it provide the epoch as default time stamp instead of NULL? Two knee-jerk concerns I have without going back to the callers: * Would that 0 ever be given to the approxidate parser, which rejects ancient dates in numbers-since-epoch format without @ prefix? * Does any existing caller use the NULL as a sign to see the input was without date and act on that information? I looked at all the callers (there aren't that many), and none of them did Do this on a person-only ident, and do that on an ident with timestamp. So for the callers that ignore timestamp, your patch will be a no-op, and for others that assume there is a timestamp, it will turn a crash/segv into output with funny timestamp. So I think the patch is a right thing to do (we would need in-code comment to warn new callers about the semantics, though). Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
