[PATCH 1/2] Sanitize for_each_reflog_ent()

[PATCH 1/2] Sanitize for_each_reflog_ent()

[Johannes Schindelin]

It used to ignore the return value of the helper function; now, it
expects it to return 0, and stops iteration upon non-zero return
values; this value is then passed on as the return value of
for_each_reflog_ent().

Further, it makes no sense to force the parsing upon the helper
functions; for_each_reflog_ent() now calls the helper function with
old and new sha1, the email, the timestamp & timezone, and the message.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	IMHO it'd be a good idea to sanitize all for_each_*() functions
	to take a callback pointer.

 builtin-reflog.c |   17 +++++------------
 fsck-objects.c   |    4 +++-
 reachable.c      |    4 +++-
 refs.c           |   26 +++++++++++++++++++++-----
 refs.h           |    4 ++--
 revision.c       |    4 +++-
 6 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/builtin-reflog.c b/builtin-reflog.c
index a967117..ede051a 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -195,19 +195,12 @@ static int keep_entry(struct commit **it, unsigned char *sha1)
 }
 
 static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
-			     char *data, void *cb_data)
+		const char *email, unsigned long timestamp, int tz,
+		const char *message, void *cb_data)
 {
 	struct expire_reflog_cb *cb = cb_data;
-	unsigned long timestamp;
-	char *cp, *ep;
 	struct commit *old, *new;
 
-	cp = strchr(data, '>');
-	if (!cp || *++cp != ' ')
-		goto prune;
-	timestamp = strtoul(cp, &ep, 10);
-	if (*ep != ' ')
-		goto prune;
 	if (timestamp < cb->cmd->expire_total)
 		goto prune;
 
@@ -223,13 +216,13 @@ static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
 
 	if (cb->newlog)
 		fprintf(cb->newlog, "%s %s %s",
-			sha1_to_hex(osha1), sha1_to_hex(nsha1), data);
+			sha1_to_hex(osha1), sha1_to_hex(nsha1), message);
 	if (cb->cmd->verbose)
-		printf("keep %s", data);
+		printf("keep %s", message);
 	return 0;
  prune:
 	if (!cb->newlog || cb->cmd->verbose)
-		printf("%sprune %s", cb->newlog ? "" : "would ", data);
+		printf("%sprune %s", cb->newlog ? "" : "would ", message);
 	return 0;
 }
 
diff --git a/fsck-objects.c b/fsck-objects.c
index 1cc3b39..0d8a8eb 100644
--- a/fsck-objects.c
+++ b/fsck-objects.c
@@ -399,7 +399,9 @@ static void fsck_dir(int i, char *path)
 
 static int default_refs;
 
-static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1, char *datail, void *cb_data)
+static int fsck_handle_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+		const char *email, unsigned long timestamp, int tz,
+		const char *message, void *cb_data)
 {
 	struct object *obj;
 
diff --git a/reachable.c b/reachable.c
index 4dfee1d..a6a3348 100644
--- a/reachable.c
+++ b/reachable.c
@@ -104,7 +104,9 @@ static void walk_commit_list(struct rev_info *revs)
 	}
 }
 
-static int add_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1, char *datail, void *cb_data)
+static int add_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+		const char *email, unsigned long timestamp, int tz,
+		const char *message, void *cb_data)
 {
 	struct object *object;
 	struct rev_info *revs = (struct rev_info *)cb_data;
diff --git a/refs.c b/refs.c
index 5205745..ea670d4 100644
--- a/refs.c
+++ b/refs.c
@@ -1097,7 +1097,7 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *
 	return 0;
 }
 
-void for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data)
+int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data)
 {
 	const char *logfile;
 	FILE *logfp;
@@ -1106,19 +1106,35 @@ void for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data)
 	logfile = git_path("logs/%s", ref);
 	logfp = fopen(logfile, "r");
 	if (!logfp)
-		return;
+		return -1;
 	while (fgets(buf, sizeof(buf), logfp)) {
 		unsigned char osha1[20], nsha1[20];
-		int len;
+		char *email_end, *message;
+		unsigned long timestamp;
+		int len, ret, tz;
 
 		/* old SP new SP name <email> SP time TAB msg LF */
 		len = strlen(buf);
 		if (len < 83 || buf[len-1] != '\n' ||
 		    get_sha1_hex(buf, osha1) || buf[40] != ' ' ||
-		    get_sha1_hex(buf + 41, nsha1) || buf[81] != ' ')
+		    get_sha1_hex(buf + 41, nsha1) || buf[81] != ' ' ||
+		    !(email_end = strchr(buf + 82, '>')) ||
+		    email_end[1] != ' ' ||
+		    !(timestamp = strtoul(email_end + 2, &message, 10)) ||
+		    !message || message[0] != ' ' ||
+		    (message[1] != '+' && message[1] != '-') ||
+		    !isdigit(message[2]) || !isdigit(message[3]) ||
+		    !isdigit(message[4]) || !isdigit(message[5]) ||
+		    message[6] != '\t')
 			continue; /* corrupt? */
-		fn(osha1, nsha1, buf+82, cb_data);
+		email_end[1] = '\0';
+		tz = strtol(message + 1, NULL, 10);
+		message += 7;
+		ret = fn(osha1, nsha1, buf+82, timestamp, tz, message, cb_data);
+		if (ret)
+			return ret;
 	}
 	fclose(logfp);
+	return 0;
 }
 
diff --git a/refs.h b/refs.h
index de43cc7..0e877e8 100644
--- a/refs.h
+++ b/refs.h
@@ -45,8 +45,8 @@ extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, cons
 extern int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *sha1);
 
 /* iterate over reflog entries */
-typedef int each_reflog_ent_fn(unsigned char *osha1, unsigned char *nsha1, char *, void *);
-void for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data);
+typedef int each_reflog_ent_fn(unsigned char *osha1, unsigned char *nsha1, const char *, unsigned long, int, const char *, void *);
+int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data);
 
 /** Returns 0 if target has the right format for a ref. **/
 extern int check_ref_format(const char *target);
diff --git a/revision.c b/revision.c
index 6e4ec46..1e3b29a 100644
--- a/revision.c
+++ b/revision.c
@@ -505,7 +505,9 @@ static void handle_one_reflog_commit(unsigned char *sha1, void *cb_data)
 	}
 }
 
-static int handle_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1, char *detail, void *cb_data)
+static int handle_one_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+		const char *email, unsigned long timestamp, int tz,
+		const char *message, void *cb_data)
 {
 	handle_one_reflog_commit(osha1, cb_data);
 	handle_one_reflog_commit(nsha1, cb_data);
-- 
1.5.0.rc0.gcbf41-dirty

Re: [PATCH 1/2] Sanitize for_each_reflog_ent()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It used to ignore the return value of the helper function; now, it
> expects it to return 0, and stops iteration upon non-zero return
> values; this value is then passed on as the return value of
> for_each_reflog_ent().

Good idea.

> Further, it makes no sense to force the parsing upon the helper
> functions; for_each_reflog_ent() now calls the helper function with
> old and new sha1, the email, the timestamp & timezone, and the message.

Perhaps.  I did it that way deliberately because all existing
users did not have to pay overhead of parsing.

Thanks.  Will queue in 'pu' initially as I do not have much time
to look at it tonight.

Re: [PATCH 1/2] Sanitize for_each_reflog_ent()

[Johannes Schindelin]

Hi,

On Sun, 7 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Further, it makes no sense to force the parsing upon the helper
> > functions; for_each_reflog_ent() now calls the helper function with
> > old and new sha1, the email, the timestamp & timezone, and the message.
> 
> Perhaps.  I did it that way deliberately because all existing
> users did not have to pay overhead of parsing.

My reasoning is that invalid entries should rather be ignored than be 
taken into account. So, to verify that you are not walking invalid data, 
you have to parse it anyway.

Ciao,
Dscho

Re: [PATCH 1/2] Sanitize for_each_reflog_ent()

[Jakub Narebski]

Johannes Schindelin wrote:

> On Sun, 7 Jan 2007, Junio C Hamano wrote:
> 
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>>> Further, it makes no sense to force the parsing upon the helper
>>> functions; for_each_reflog_ent() now calls the helper function with
>>> old and new sha1, the email, the timestamp & timezone, and the message.
>> 
>> Perhaps.  I did it that way deliberately because all existing
>> users did not have to pay overhead of parsing.
> 
> My reasoning is that invalid entries should rather be ignored than be 
> taken into account. So, to verify that you are not walking invalid data, 
> you have to parse it anyway.

I think that Junio was talking about the fact, that if you for example
do need only refname and sha1, there is no need to parse object at all.
If you don't need to, don't parse.
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 1/2] Sanitize for_each_reflog_ent()

[Johannes Schindelin]

Hi,

On Tue, 9 Jan 2007, Jakub Narebski wrote:

> Johannes Schindelin wrote:
> 
> > On Sun, 7 Jan 2007, Junio C Hamano wrote:
> > 
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> 
> >>> Further, it makes no sense to force the parsing upon the helper
> >>> functions; for_each_reflog_ent() now calls the helper function with
> >>> old and new sha1, the email, the timestamp & timezone, and the message.
> >> 
> >> Perhaps.  I did it that way deliberately because all existing
> >> users did not have to pay overhead of parsing.
> > 
> > My reasoning is that invalid entries should rather be ignored than be 
> > taken into account. So, to verify that you are not walking invalid data, 
> > you have to parse it anyway.
> 
> I think that Junio was talking about the fact, that if you for example
> do need only refname and sha1, there is no need to parse object at all.
> If you don't need to, don't parse.

And it was exactly what _I_ was talking about, too:

if there are invalid entries, you ignore them. So for example, if there is 
no timestamp and message, you don't want the osha1 or nsha1, because it is 
an _invalid_ record.

Ciao,
Dscho

Re: [PATCH 1/2] Sanitize for_each_reflog_ent()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Tue, 9 Jan 2007, Jakub Narebski wrote:
>
>> Johannes Schindelin wrote:
>> ... 
>> > My reasoning is that invalid entries should rather be ignored than be 
>> > taken into account. So, to verify that you are not walking invalid data, 
>> > you have to parse it anyway.
>> 
>> I think that Junio was talking about the fact, that if you for example
>> do need only refname and sha1, there is no need to parse object at all.
>> If you don't need to, don't parse.
>
> And it was exactly what _I_ was talking about, too:
>
> if there are invalid entries, you ignore them. So for example, if there is 
> no timestamp and message, you don't want the osha1 or nsha1, because it is 
> an _invalid_ record.

That's fine.  I applied your patch with minimum fixups so that
it does not make the surviving records _invalid_ ones after
"reflog expire" runs ;-).

Re: [PATCH 1/2] Sanitize for_each_reflog_ent()

[Johannes Schindelin]

Hi,

On Tue, 9 Jan 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Tue, 9 Jan 2007, Jakub Narebski wrote:
> >
> >> Johannes Schindelin wrote:
> >> ... 
> >> > My reasoning is that invalid entries should rather be ignored than be 
> >> > taken into account. So, to verify that you are not walking invalid data, 
> >> > you have to parse it anyway.
> >> 
> >> I think that Junio was talking about the fact, that if you for example
> >> do need only refname and sha1, there is no need to parse object at all.
> >> If you don't need to, don't parse.
> >
> > And it was exactly what _I_ was talking about, too:
> >
> > if there are invalid entries, you ignore them. So for example, if there is 
> > no timestamp and message, you don't want the osha1 or nsha1, because it is 
> > an _invalid_ record.
> 
> That's fine.  I applied your patch with minimum fixups so that
> it does not make the surviving records _invalid_ ones after
> "reflog expire" runs ;-).

Ah! Missed that one. Thanks!

Ciao,
Dscho