{"thread":{"id":"6273","subject":"[PATCH 1/2] Sanitize for_each_reflog_ent()","startedAt":"2007-01-08T00:59:54Z","lastAt":"2007-01-09T23:40:02Z","messageCount":7,"participants":["Johannes Schindelin","Junio C Hamano","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"31093","messageId":"Pine.LNX.4.63.0701080158500.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6273","inReplyTo":null,"subject":"[PATCH 1/2] Sanitize for_each_reflog_ent()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-08T00:59:54Z","receivedAt":"2007-01-08T00:59:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nIt used to ignore the return value of the helper function; now, it\nexpects it to return 0, and stops iteration upon non-zero return\nvalues; this value is then passed on as the return value of\nfor_each_reflog_ent().\n\nFurther, it makes no sense to force the parsing upon the helper\nfunctions; for_each_reflog_ent() now calls the helper function with\nold and new sha1, the email, the timestamp & timezone, and the message.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\n\tIMHO it'd be a good idea to sanitize all for_each_*() functions\n\tto take a callback pointer.\n\n builtin-reflog.c |   17 +++++------------\n fsck-objects.c   |    4 +++-\n reachable.c      |    4 +++-\n refs.c           |   26 +++++++++++++++++++++-----\n refs.h           |    4 ++--\n revision.c       |    4 +++-\n 6 files changed, 37 insertions(+), 22 deletions(-)\n\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex a967117..ede051a 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -195,19 +195,12 @@ static int keep_entry(struct commit **it, unsigned char *sha1)\n }\n \n static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n-\t\t\t     char *data, void *cb_data)\n+\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *message, void *cb_data)\n {\n \tstruct expire_reflog_cb *cb = cb_data;\n-\tunsigned long timestamp;\n-\tchar *cp, *ep;\n \tstruct commit *old, *new;\n \n-\tcp = strchr(data, '>');\n-\tif (!cp || *++cp != ' ')\n-\t\tgoto prune;\n-\ttimestamp = strtoul(cp, &ep, 10);\n-\tif (*ep != ' ')\n-\t\tgoto prune;\n \tif (timestamp < cb->cmd->expire_total)\n \t\tgoto prune;\n \n@@ -223,13 +216,13 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n \n \tif (cb->newlog)\n \t\tfprintf(cb->newlog, \"%s %s %s\",\n-\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1), data);\n+\t\t\tsha1_to_hex(osha1), sha1_to_hex(nsha1), message);\n \tif (cb->cmd->verbose)\n-\t\tprintf(\"keep %s\", data);\n+\t\tprintf(\"keep %s\", message);\n \treturn 0;\n  prune:\n \tif (!cb->newlog || cb->cmd->verbose)\n-\t\tprintf(\"%sprune %s\", cb->newlog ? \"\" : \"would \", data);\n+\t\tprintf(\"%sprune %s\", cb->newlog ? \"\" : \"would \", message);\n \treturn 0;\n }\n \ndiff --git a/fsck-objects.c b/fsck-objects.c\nindex 1cc3b39..0d8a8eb 100644\n--- a/fsck-objects.c\n+++ b/fsck-objects.c\n@@ -399,7 +399,9 @@ static void fsck_dir(int i, char *path)\n \n static int default_refs;\n \n-static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1, char *datail, void *cb_data)\n+static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *message, void *cb_data)\n {\n \tstruct object *obj;\n \ndiff --git a/reachable.c b/reachable.c\nindex 4dfee1d..a6a3348 100644\n--- a/reachable.c\n+++ b/reachable.c\n@@ -104,7 +104,9 @@ static void walk_commit_list(struct rev_info *revs)\n \t}\n }\n \n-static int add_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1, char *datail, void *cb_data)\n+static int add_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *message, void *cb_data)\n {\n \tstruct object *object;\n \tstruct rev_info *revs = (struct rev_info *)cb_data;\ndiff --git a/refs.c b/refs.c\nindex 5205745..ea670d4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1097,7 +1097,7 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *\n \treturn 0;\n }\n \n-void for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data)\n+int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data)\n {\n \tconst char *logfile;\n \tFILE *logfp;\n@@ -1106,19 +1106,35 @@ void for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data)\n \tlogfile = git_path(\"logs/%s\", ref);\n \tlogfp = fopen(logfile, \"r\");\n \tif (!logfp)\n-\t\treturn;\n+\t\treturn -1;\n \twhile (fgets(buf, sizeof(buf), logfp)) {\n \t\tunsigned char osha1[20], nsha1[20];\n-\t\tint len;\n+\t\tchar *email_end, *message;\n+\t\tunsigned long timestamp;\n+\t\tint len, ret, tz;\n \n \t\t/* old SP new SP name <email> SP time TAB msg LF */\n \t\tlen = strlen(buf);\n \t\tif (len < 83 || buf[len-1] != '\\n' ||\n \t\t    get_sha1_hex(buf, osha1) || buf[40] != ' ' ||\n-\t\t    get_sha1_hex(buf + 41, nsha1) || buf[81] != ' ')\n+\t\t    get_sha1_hex(buf + 41, nsha1) || buf[81] != ' ' ||\n+\t\t    !(email_end = strchr(buf + 82, '>')) ||\n+\t\t    email_end[1] != ' ' ||\n+\t\t    !(timestamp = strtoul(email_end + 2, &message, 10)) ||\n+\t\t    !message || message[0] != ' ' ||\n+\t\t    (message[1] != '+' && message[1] != '-') ||\n+\t\t    !isdigit(message[2]) || !isdigit(message[3]) ||\n+\t\t    !isdigit(message[4]) || !isdigit(message[5]) ||\n+\t\t    message[6] != '\\t')\n \t\t\tcontinue; /* corrupt? */\n-\t\tfn(osha1, nsha1, buf+82, cb_data);\n+\t\temail_end[1] = '\\0';\n+\t\ttz = strtol(message + 1, NULL, 10);\n+\t\tmessage += 7;\n+\t\tret = fn(osha1, nsha1, buf+82, timestamp, tz, message, cb_data);\n+\t\tif (ret)\n+\t\t\treturn ret;\n \t}\n \tfclose(logfp);\n+\treturn 0;\n }\n \ndiff --git a/refs.h b/refs.h\nindex de43cc7..0e877e8 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -45,8 +45,8 @@ extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, cons\n extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *sha1);\n \n /* iterate over reflog entries */\n-typedef int each_reflog_ent_fn(unsigned char *osha1, unsigned char *nsha1, char *, void *);\n-void for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data);\n+typedef int each_reflog_ent_fn(unsigned char *osha1, unsigned char *nsha1, const char *, unsigned long, int, const char *, void *);\n+int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data);\n \n /** Returns 0 if target has the right format for a ref. **/\n extern int check_ref_format(const char *target);\ndiff --git a/revision.c b/revision.c\nindex 6e4ec46..1e3b29a 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -505,7 +505,9 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)\n \t}\n }\n \n-static int handle_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1, char *detail, void *cb_data)\n+static int handle_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1,\n+\t\tconst char *email, unsigned long timestamp, int tz,\n+\t\tconst char *message, void *cb_data)\n {\n \thandle_one_reflog_commit(osha1, cb_data);\n \thandle_one_reflog_commit(nsha1, cb_data);\n-- \n1.5.0.rc0.gcbf41-dirty\n"},{"id":"31099","messageId":"7vr6u6z3x8.fsf@assigned-by-dhcp.cox.net","threadId":"6273","inReplyTo":"Pine.LNX.4.63.0701080158500.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 1/2] Sanitize for_each_reflog_ent()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-08T01:46:11Z","receivedAt":"2007-01-08T01:46:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> It used to ignore the return value of the helper function; now, it\n> expects it to return 0, and stops iteration upon non-zero return\n> values; this value is then passed on as the return value of\n> for_each_reflog_ent().\n\nGood idea.\n\n> Further, it makes no sense to force the parsing upon the helper\n> functions; for_each_reflog_ent() now calls the helper function with\n> old and new sha1, the email, the timestamp & timezone, and the message.\n\nPerhaps.  I did it that way deliberately because all existing\nusers did not have to pay overhead of parsing.\n\nThanks.  Will queue in 'pu' initially as I do not have much time\nto look at it tonight.\n"},{"id":"31129","messageId":"Pine.LNX.4.63.0701081316110.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6273","inReplyTo":"7vr6u6z3x8.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/2] Sanitize for_each_reflog_ent()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-08T12:18:24Z","receivedAt":"2007-01-08T12:18:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 7 Jan 2007, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > Further, it makes no sense to force the parsing upon the helper\n> > functions; for_each_reflog_ent() now calls the helper function with\n> > old and new sha1, the email, the timestamp & timezone, and the message.\n> \n> Perhaps.  I did it that way deliberately because all existing\n> users did not have to pay overhead of parsing.\n\nMy reasoning is that invalid entries should rather be ignored than be \ntaken into account. So, to verify that you are not walking invalid data, \nyou have to parse it anyway.\n\nCiao,\nDscho\n"},{"id":"31169","messageId":"enumq8$oa$1@sea.gmane.org","threadId":"6273","inReplyTo":"Pine.LNX.4.63.0701081316110.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 1/2] Sanitize for_each_reflog_ent()","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-01-09T00:15:15Z","receivedAt":"2007-01-09T00:15:15Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Johannes Schindelin wrote:\n\n> On Sun, 7 Jan 2007, Junio C Hamano wrote:\n> \n>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>> \n>>> Further, it makes no sense to force the parsing upon the helper\n>>> functions; for_each_reflog_ent() now calls the helper function with\n>>> old and new sha1, the email, the timestamp & timezone, and the message.\n>> \n>> Perhaps.  I did it that way deliberately because all existing\n>> users did not have to pay overhead of parsing.\n> \n> My reasoning is that invalid entries should rather be ignored than be \n> taken into account. So, to verify that you are not walking invalid data, \n> you have to parse it anyway.\n\nI think that Junio was talking about the fact, that if you for example\ndo need only refname and sha1, there is no need to parse object at all.\nIf you don't need to, don't parse.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"31218","messageId":"Pine.LNX.4.63.0701091056520.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6273","inReplyTo":"enumq8$oa$1@sea.gmane.org","subject":"Re: [PATCH 1/2] Sanitize for_each_reflog_ent()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-09T09:57:58Z","receivedAt":"2007-01-09T09:57:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 9 Jan 2007, Jakub Narebski wrote:\n\n> Johannes Schindelin wrote:\n> \n> > On Sun, 7 Jan 2007, Junio C Hamano wrote:\n> > \n> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> >> \n> >>> Further, it makes no sense to force the parsing upon the helper\n> >>> functions; for_each_reflog_ent() now calls the helper function with\n> >>> old and new sha1, the email, the timestamp & timezone, and the message.\n> >> \n> >> Perhaps.  I did it that way deliberately because all existing\n> >> users did not have to pay overhead of parsing.\n> > \n> > My reasoning is that invalid entries should rather be ignored than be \n> > taken into account. So, to verify that you are not walking invalid data, \n> > you have to parse it anyway.\n> \n> I think that Junio was talking about the fact, that if you for example\n> do need only refname and sha1, there is no need to parse object at all.\n> If you don't need to, don't parse.\n\nAnd it was exactly what _I_ was talking about, too:\n\nif there are invalid entries, you ignore them. So for example, if there is \nno timestamp and message, you don't want the osha1 or nsha1, because it is \nan _invalid_ record.\n\nCiao,\nDscho\n"},{"id":"31275","messageId":"7vmz4sj6hb.fsf@assigned-by-dhcp.cox.net","threadId":"6273","inReplyTo":"Pine.LNX.4.63.0701091056520.22628@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 1/2] Sanitize for_each_reflog_ent()","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-09T20:22:08Z","receivedAt":"2007-01-09T20:22:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Tue, 9 Jan 2007, Jakub Narebski wrote:\n>\n>> Johannes Schindelin wrote:\n>> ... \n>> > My reasoning is that invalid entries should rather be ignored than be \n>> > taken into account. So, to verify that you are not walking invalid data, \n>> > you have to parse it anyway.\n>> \n>> I think that Junio was talking about the fact, that if you for example\n>> do need only refname and sha1, there is no need to parse object at all.\n>> If you don't need to, don't parse.\n>\n> And it was exactly what _I_ was talking about, too:\n>\n> if there are invalid entries, you ignore them. So for example, if there is \n> no timestamp and message, you don't want the osha1 or nsha1, because it is \n> an _invalid_ record.\n\nThat's fine.  I applied your patch with minimum fixups so that\nit does not make the surviving records _invalid_ ones after\n\"reflog expire\" runs ;-).\n"},{"id":"31307","messageId":"Pine.LNX.4.63.0701100039480.22628@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"6273","inReplyTo":"7vmz4sj6hb.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/2] Sanitize for_each_reflog_ent()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-01-09T23:40:02Z","receivedAt":"2007-01-09T23:40:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 9 Jan 2007, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > On Tue, 9 Jan 2007, Jakub Narebski wrote:\n> >\n> >> Johannes Schindelin wrote:\n> >> ... \n> >> > My reasoning is that invalid entries should rather be ignored than be \n> >> > taken into account. So, to verify that you are not walking invalid data, \n> >> > you have to parse it anyway.\n> >> \n> >> I think that Junio was talking about the fact, that if you for example\n> >> do need only refname and sha1, there is no need to parse object at all.\n> >> If you don't need to, don't parse.\n> >\n> > And it was exactly what _I_ was talking about, too:\n> >\n> > if there are invalid entries, you ignore them. So for example, if there is \n> > no timestamp and message, you don't want the osha1 or nsha1, because it is \n> > an _invalid_ record.\n> \n> That's fine.  I applied your patch with minimum fixups so that\n> it does not make the surviving records _invalid_ ones after\n> \"reflog expire\" runs ;-).\n\nAh! Missed that one. Thanks!\n\nCiao,\nDscho\n"}]}