threads / patch / 36817

patchrefs.c: change read_ref_at to use the reflog iterators

Subject: [PATCH] refs.c: change read_ref_at to use the reflog iterators

## tl;dr

3 messages between Jun 2, 2014 and Jun 3, 2014. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Ronnie Sahlberg· Jun 2, 2014, 18:15 UTC · lore

read_ref_at has its own parsing of the reflog file for no really good reason so lets change this to use the existing reflog iterators. This removes one instance where we manually unmarshall the reflog file format.

Log messages for errors are changed slightly. We no longer print the file name for the reflog, instead we refer to it as 'Log for ref <refname>'. This might be a minor useability regression, but I don't really think so, since experienced users would know where the log is anyway and inexperienced users would not know what to do about/how to repair 'Log ... has gap ...' anyway.

Adapt the t1400 test to handle the change in log messages.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 refs.c                | 202 ++++++++++++++++++++++++++------------------------
 t/t1400-update-ref.sh |   4 +-
 2 files changed, 107 insertions(+), 99 deletions(-)
Show changes to 2 files +107 −99

refs.c, t/t1400-update-ref.sh

diff --git a/refs.c b/refs.c
index 6898263..b45bb2f 100644
--- a/refs.c
+++ b/refs.c
@@ -2936,109 +2936,117 @@ static char *ref_msg(const char *line, const char *endp)
 	return xmemdupz(line, ep - line);
 }
 
+struct read_ref_at_cb {
+	const char *refname;
+	unsigned long at_time;
+	int cnt;
+	int reccnt;
+	unsigned char *sha1;
+	int found_it;
+
+	char osha1[20];
+	char nsha1[20];
+	int tz;
+	unsigned long date;
+	char **msg;
+	unsigned long *cutoff_time;
+	int *cutoff_tz;
+	int *cutoff_cnt;
+};
+
+static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,
+		const char *email, unsigned long timestamp, int tz,
+		const char *message, void *cb_data)
+{
+	struct read_ref_at_cb *cb = cb_data;
+
+	cb->reccnt++;
+	cb->tz = tz;
+	cb->date = timestamp;
+
+	if (timestamp <= cb->at_time || cb->cnt == 0) {
+		if (cb->msg)
+			*cb->msg = xstrdup(message);
+		if (cb->cutoff_time)
+			*cb->cutoff_time = timestamp;
+		if (cb->cutoff_tz)
+			*cb->cutoff_tz = tz;
+		if (cb->cutoff_cnt)
+			*cb->cutoff_cnt = cb->reccnt - 1;
+		/*
+		 * we have not yet updated cb->[n|o]sha1 so they still
+		 * hold the values for the previous record.
+		 */
+		if (!is_null_sha1(cb->osha1)) {
+			hashcpy(cb->sha1, nsha1);
+			if (hashcmp(cb->osha1, nsha1))
+				warning("Log for ref %s has gap after %s.",
+					cb->refname, show_date(cb->date, cb->tz, DATE_RFC2822));
+		}
+		else if (cb->date == cb->at_time)
+			hashcpy(cb->sha1, nsha1);
+		else if (hashcmp(nsha1, cb->sha1))
+			warning("Log for ref %s unexpectedly ended on %s.",
+				cb->refname, show_date(cb->date, cb->tz,
+						   DATE_RFC2822));
+		hashcpy(cb->osha1, osha1);
+		hashcpy(cb->nsha1, nsha1);
+		cb->found_it = 1;
+		return 1;
+	}
+	hashcpy(cb->osha1, osha1);
+	hashcpy(cb->nsha1, nsha1);
+	if (cb->cnt > 0)
+		cb->cnt--;
+	return 0;
+}
+
+static int read_ref_at_ent_oldest(unsigned char *osha1, unsigned char *nsha1,
+				  const char *email, unsigned long timestamp,
+				  int tz, const char *message, void *cb_data)
+{
+	struct read_ref_at_cb *cb = cb_data;
+
+	if (cb->msg)
+		*cb->msg = xstrdup(message);
+	if (cb->cutoff_time)
+		*cb->cutoff_time = timestamp;
+	if (cb->cutoff_tz)
+		*cb->cutoff_tz = tz;
+	if (cb->cutoff_cnt)
+		*cb->cutoff_cnt = cb->reccnt;
+	hashcpy(cb->sha1, osha1);
+	if (is_null_sha1(cb->sha1))
+		hashcpy(cb->sha1, nsha1);
+	/* We just want the first entry */
+	return 1;
+}
+
 int read_ref_at(const char *refname, unsigned long at_time, int cnt,
 		unsigned char *sha1, char **msg,
 		unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt)
 {
-	const char *logfile, *logdata, *logend, *rec, *lastgt, *lastrec;
-	char *tz_c;
-	int logfd, tz, reccnt = 0;
-	struct stat st;
-	unsigned long date;
-	unsigned char logged_sha1[20];
-	void *log_mapped;
-	size_t mapsz;
+	struct read_ref_at_cb cb;
 
-	logfile = git_path("logs/%s", refname);
-	logfd = open(logfile, O_RDONLY, 0);
-	if (logfd < 0)
-		die_errno("Unable to read log '%s'", logfile);
-	fstat(logfd, &st);
-	if (!st.st_size)
-		die("Log %s is empty.", logfile);
-	mapsz = xsize_t(st.st_size);
-	log_mapped = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, logfd, 0);
-	logdata = log_mapped;
-	close(logfd);
+	memset(&cb, 0, sizeof(cb));
+	cb.refname = refname;
+	cb.at_time = at_time;
+	cb.cnt = cnt;
+	cb.msg = msg;
+	cb.cutoff_time = cutoff_time;
+	cb.cutoff_tz = cutoff_tz;
+	cb.cutoff_cnt = cutoff_cnt;
+	cb.sha1 = sha1;
+
+	for_each_reflog_ent_reverse(refname, read_ref_at_ent, &cb);
+
+	if (!cb.reccnt)
+		die("Log for %s is empty.", refname);
+	if (cb.found_it)
+		return 0;
+
+	for_each_reflog_ent(refname, read_ref_at_ent_oldest, &cb);
 
-	lastrec = NULL;
-	rec = logend = logdata + st.st_size;
-	while (logdata < rec) {
-		reccnt++;
-		if (logdata < rec && *(rec-1) == '\n')
-			rec--;
-		lastgt = NULL;
-		while (logdata < rec && *(rec-1) != '\n') {
-			rec--;
-			if (*rec == '>')
-				lastgt = rec;
-		}
-		if (!lastgt)
-			die("Log %s is corrupt.", logfile);
-		date = strtoul(lastgt + 1, &tz_c, 10);
-		if (date <= at_time || cnt == 0) {
-			tz = strtoul(tz_c, NULL, 10);
-			if (msg)
-				*msg = ref_msg(rec, logend);
-			if (cutoff_time)
-				*cutoff_time = date;
-			if (cutoff_tz)
-				*cutoff_tz = tz;
-			if (cutoff_cnt)
-				*cutoff_cnt = reccnt - 1;
-			if (lastrec) {
-				if (get_sha1_hex(lastrec, logged_sha1))
-					die("Log %s is corrupt.", logfile);
-				if (get_sha1_hex(rec + 41, sha1))
-					die("Log %s is corrupt.", logfile);
-				if (hashcmp(logged_sha1, sha1)) {
-					warning("Log %s has gap after %s.",
-						logfile, show_date(date, tz, DATE_RFC2822));
-				}
-			}
-			else if (date == at_time) {
-				if (get_sha1_hex(rec + 41, sha1))
-					die("Log %s is corrupt.", logfile);
-			}
-			else {
-				if (get_sha1_hex(rec + 41, logged_sha1))
-					die("Log %s is corrupt.", logfile);
-				if (hashcmp(logged_sha1, sha1)) {
-					warning("Log %s unexpectedly ended on %s.",
-						logfile, show_date(date, tz, DATE_RFC2822));
-				}
-			}
-			munmap(log_mapped, mapsz);
-			return 0;
-		}
-		lastrec = rec;
-		if (cnt > 0)
-			cnt--;
-	}
-
-	rec = logdata;
-	while (rec < logend && *rec != '>' && *rec != '\n')
-		rec++;
-	if (rec == logend || *rec == '\n')
-		die("Log %s is corrupt.", logfile);
-	date = strtoul(rec + 1, &tz_c, 10);
-	tz = strtoul(tz_c, NULL, 10);
-	if (get_sha1_hex(logdata, sha1))
-		die("Log %s is corrupt.", logfile);
-	if (is_null_sha1(sha1)) {
-		if (get_sha1_hex(logdata + 41, sha1))
-			die("Log %s is corrupt.", logfile);
-	}
-	if (msg)
-		*msg = ref_msg(logdata, logend);
-	munmap(log_mapped, mapsz);
-
-	if (cutoff_time)
-		*cutoff_time = date;
-	if (cutoff_tz)
-		*cutoff_tz = tz;
-	if (cutoff_cnt)
-		*cutoff_cnt = reccnt;
 	return 1;
 }
 
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 4e2459a..0218e96 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -235,7 +235,7 @@ test_expect_success \
 	'rm -f o e &&
 	 git rev-parse --verify "master@{2005-05-26 23:33:01}" >o 2>e &&
 	 test '"$B"' = $(cat o) &&
-	 test "warning: Log .git/logs/'"$m has gap after $gd"'." = "$(cat e)"'
+	 test "warning: Log for ref '"$m has gap after $gd"'." = "$(cat e)"'
 test_expect_success \
 	'Query "master@{2005-05-26 23:38:00}" (middle of history)' \
 	'rm -f o e &&
@@ -253,7 +253,7 @@ test_expect_success \
 	'rm -f o e &&
 	 git rev-parse --verify "master@{2005-05-28}" >o 2>e &&
 	 test '"$D"' = $(cat o) &&
-	 test "warning: Log .git/logs/'"$m unexpectedly ended on $ld"'." = "$(cat e)"'
+	 test "warning: Log for ref '"$m unexpectedly ended on $ld"'." = "$(cat e)"'
 
 
 rm -f .git/$m .git/logs/$m expect
-- 
2.0.0.527.g539d2ad
Junio C Hamano· Jun 2, 2014, 21:21 UTC · re: Ronnie Sahlberg · lore

Re: [PATCH] refs.c: change read_ref_at to use the reflog iterators

Ronnie Sahlberg <sahlberg@google.com> writes:
Show 25 quoted lines
> read_ref_at has its own parsing of the reflog file for no really good reason
> so lets change this to use the existing reflog iterators. This removes one
> instance where we manually unmarshall the reflog file format.
>
> Log messages for errors are changed slightly. We no longer print the file
> name for the reflog, instead we refer to it as 'Log for ref <refname>'.
> This might be a minor useability regression, but I don't really think so, since
> experienced users would know where the log is anyway and inexperienced users
> would not know what to do about/how to repair 'Log ... has gap ...' anyway.
>
> Adapt the t1400 test to handle the change in log messages.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  refs.c                | 202 ++++++++++++++++++++++++++------------------------
>  t/t1400-update-ref.sh |   4 +-
>  2 files changed, 107 insertions(+), 99 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 6898263..b45bb2f 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2936,109 +2936,117 @@ static char *ref_msg(const char *line, const char *endp)
>  	return xmemdupz(line, ep - line);
>  }

If I am not mistaken, this function will become unused with this rewrite. Let's drop it and justify it in the log message.

Show 10 quoted lines
> +struct read_ref_at_cb {
> +	const char *refname;
> +	unsigned long at_time;
> +	int cnt;
> +	int reccnt;
> +	unsigned char *sha1;
> +	int found_it;
> +
> +	char osha1[20];
> +	char nsha1[20];
These should be unsigned char, shouldn't they?
Show 8 quoted lines
> +	for_each_reflog_ent_reverse(refname, read_ref_at_ent, &cb);
> +
> +	if (!cb.reccnt)
> +		die("Log for %s is empty.", refname);
> +	if (cb.found_it)
> +		return 0;
> +
> +	for_each_reflog_ent(refname, read_ref_at_ent_oldest, &cb);
Hmph.

We have already scanned the same reflog in the backward direction in full. Do we need to make another call just to pick up the entry at the beginning? Is this because the callback is not told that it is fed the last entry (in other words, if there is some clue that this is the last call from for-each-reflog-ent-reverse to the callback, the callback could record the value it received in its cb-data)?

Ronnie Sahlberg· Jun 3, 2014, 16:08 UTC · re: Junio C Hamano · lore

Re: [PATCH] refs.c: change read_ref_at to use the reflog iterators

Thanks.
On Mon, Jun 2, 2014 at 2:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 30 quoted lines
> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> read_ref_at has its own parsing of the reflog file for no really good reason
>> so lets change this to use the existing reflog iterators. This removes one
>> instance where we manually unmarshall the reflog file format.
>>
>> Log messages for errors are changed slightly. We no longer print the file
>> name for the reflog, instead we refer to it as 'Log for ref <refname>'.
>> This might be a minor useability regression, but I don't really think so, since
>> experienced users would know where the log is anyway and inexperienced users
>> would not know what to do about/how to repair 'Log ... has gap ...' anyway.
>>
>> Adapt the t1400 test to handle the change in log messages.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  refs.c                | 202 ++++++++++++++++++++++++++------------------------
>>  t/t1400-update-ref.sh |   4 +-
>>  2 files changed, 107 insertions(+), 99 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 6898263..b45bb2f 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2936,109 +2936,117 @@ static char *ref_msg(const char *line, const char *endp)
>>       return xmemdupz(line, ep - line);
>>  }
>
> If I am not mistaken, this function will become unused with this
> rewrite.  Let's drop it and justify it in the log message.
Done.
Show 13 quoted lines
>
>> +struct read_ref_at_cb {
>> +     const char *refname;
>> +     unsigned long at_time;
>> +     int cnt;
>> +     int reccnt;
>> +     unsigned char *sha1;
>> +     int found_it;
>> +
>> +     char osha1[20];
>> +     char nsha1[20];
>
> These should be unsigned char, shouldn't they?
Done.
Show 19 quoted lines
>
>> +     for_each_reflog_ent_reverse(refname, read_ref_at_ent, &cb);
>> +
>> +     if (!cb.reccnt)
>> +             die("Log for %s is empty.", refname);
>> +     if (cb.found_it)
>> +             return 0;
>> +
>> +     for_each_reflog_ent(refname, read_ref_at_ent_oldest, &cb);
>
> Hmph.
>
> We have already scanned the same reflog in the backward direction in
> full.  Do we need to make another call just to pick up the entry at
> the beginning?  Is this because the callback is not told that it is
> fed the last entry (in other words, if there is some clue that this
> is the last call from for-each-reflog-ent-reverse to the callback,
> the callback could record the value it received in its cb-data)?
>

It is mainly because the callback does not provide the information "this is the last entry". We could add a flag for that to the callback arguments but I don't know if it is worth it for this special case since read_ref_at() for a timestamp that is outside the reflog horizon should be uncommon.

regards ronnie sahlberg

← back to recent threads