{"thread":{"id":"36795","subject":"[PATCH] refs.c: change read_ref_at to use the reflog iterators","startedAt":"2014-05-30T19:51:02Z","lastAt":"2014-06-02T18:12:20Z","messageCount":5,"participants":["Ronnie Sahlberg","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243042","messageId":"1401479462-2329-1-git-send-email-sahlberg@google.com","threadId":"36795","inReplyTo":null,"subject":"[PATCH] refs.c: change read_ref_at to use the reflog iterators","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-30T19:51:02Z","receivedAt":"2014-05-30T19:51:02Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"read_ref_at has its own parsing of the reflog file for no really good reason\nso lets change this to use the existing reflog iterators. This removes one\ninstance where we manually unmarshall the reflog file format.\n\nLog messages for errors are changed slightly. We no longer print the file\nname for the reflog, instead we refer to it as 'Log for ref <refname>'.\nThis might be a minor useability regression, but I don't really think so, since\nexperienced users would know where the log is anyway and inexperienced users\nwould not know what to do about/how to repair 'Log ... has gap ...' anyway.\n\nAdapt the t1400 test to handle the cahnge in log messages.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n refs.c                | 217 ++++++++++++++++++++++++++++----------------------\n t/t1400-update-ref.sh |   4 +-\n 2 files changed, 122 insertions(+), 99 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6898263..99d4832 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2936,109 +2936,132 @@ static char *ref_msg(const char *line, const char *endp)\n \treturn xmemdupz(line, ep - line);\n }\n \n+struct read_ref_at_cb {\n+\tconst char *refname;\n+\tunsigned long at_time;\n+\tint cnt;\n+\tint reccnt;\n+\tunsigned char *sha1;\n+\tint found_it;\n+\n+\tchar osha1[20];\n+\tchar nsha1[20];\n+\tint tz;\n+\tunsigned long date;\n+\tchar **msg;\n+\tunsigned long *cutoff_time;\n+\tint *cutoff_tz;\n+\tint *cutoff_cnt;\n+};\n+\n+static int read_ref_at_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 read_ref_at_cb *cb = cb_data;\n+\n+\tcb->reccnt++;\n+\tcb->tz = tz;\n+\tcb->date = timestamp;\n+\n+\tif (timestamp <= cb->at_time || cb->cnt == 0) {\n+\t\tif (cb->msg)\n+\t\t\t*cb->msg = xstrdup(message);\n+\t\tif (cb->cutoff_time)\n+\t\t\t*cb->cutoff_time = timestamp;\n+\t\tif (cb->cutoff_tz)\n+\t\t\t*cb->cutoff_tz = tz;\n+\t\tif (cb->cutoff_cnt)\n+\t\t\t*cb->cutoff_cnt = cb->reccnt - 1;\n+\n+\t\t/*\n+\t\t * we have not yet updated cb->[n|o]sha1 so they still\n+\t\t * hold the values for the previous record.\n+\t\t */\n+\t\tif (!is_null_sha1(cb->osha1)) {\n+\t\t\thashcpy(cb->sha1, nsha1);\n+\t\t\tif (hashcmp(cb->osha1, nsha1))\n+\t\t\t\twarning(\"Log for ref %s has gap after %s.\",\n+\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_RFC2822));\n+\t\t}\n+\t\telse if (cb->date == cb->at_time)\n+\t\t\thashcpy(cb->sha1, nsha1);\n+\t\telse\n+\t\t\tif (hashcmp(nsha1, cb->sha1))\n+\t\t\t\twarning(\"Log for ref %s unexpectedly ended on %s.\",\n+\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz,\n+\t\t\t\t\t\t\t   DATE_RFC2822));\n+\n+\t\t/* \n+\t\t * return 1. Not to signal an error but to break the loop\n+\t\t * since we have found the entry we want.\n+\t\t */\n+\t\thashcpy(cb->osha1, osha1);\n+\t\thashcpy(cb->nsha1, nsha1);\n+\t\tcb->found_it = 1;\n+\t\treturn 1;\n+\t}\n+\n+\thashcpy(cb->osha1, osha1);\n+\thashcpy(cb->nsha1, nsha1);\n+\tif (cb->cnt > 0)\n+\t\tcb->cnt--;\n+\n+\treturn 0;\n+}\n+\n+static int read_ref_at_ent_oldest(unsigned char *osha1, unsigned char *nsha1,\n+\t\t\t\t  const char *email, unsigned long timestamp,\n+\t\t\t\t  int tz, const char *message, void *cb_data)\n+{\n+\tstruct read_ref_at_cb *cb = cb_data;\n+\n+\tif (cb->msg)\n+\t\t*cb->msg = xstrdup(message);\n+\tif (cb->cutoff_time)\n+\t\t*cb->cutoff_time = timestamp;\n+\tif (cb->cutoff_tz)\n+\t\t*cb->cutoff_tz = tz;\n+\tif (cb->cutoff_cnt)\n+\t\t*cb->cutoff_cnt = cb->reccnt - 1;\n+\n+\thashcpy(cb->sha1, osha1);\n+\tif (is_null_sha1(cb->sha1))\n+\t\thashcpy(cb->sha1, nsha1);\n+\n+\t/* \n+\t * return 1. Not to signal an error but to break the loop\n+\t * since we have found the entry we want.\n+\t */\n+\treturn 1;\n+}\n+\n int read_ref_at(const char *refname, unsigned long at_time, int cnt,\n \t\tunsigned char *sha1, char **msg,\n \t\tunsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt)\n {\n-\tconst char *logfile, *logdata, *logend, *rec, *lastgt, *lastrec;\n-\tchar *tz_c;\n-\tint logfd, tz, reccnt = 0;\n-\tstruct stat st;\n-\tunsigned long date;\n-\tunsigned char logged_sha1[20];\n-\tvoid *log_mapped;\n-\tsize_t mapsz;\n+\tstruct read_ref_at_cb cb;\n \n-\tlogfile = git_path(\"logs/%s\", refname);\n-\tlogfd = open(logfile, O_RDONLY, 0);\n-\tif (logfd < 0)\n-\t\tdie_errno(\"Unable to read log '%s'\", logfile);\n-\tfstat(logfd, &st);\n-\tif (!st.st_size)\n-\t\tdie(\"Log %s is empty.\", logfile);\n-\tmapsz = xsize_t(st.st_size);\n-\tlog_mapped = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, logfd, 0);\n-\tlogdata = log_mapped;\n-\tclose(logfd);\n+\tmemset(&cb, 0, sizeof(cb));\n+\tcb.refname = refname;\n+\tcb.at_time = at_time;\n+\tcb.cnt = cnt;\n+\tcb.msg = msg;\n+\tcb.cutoff_time = cutoff_time;\n+\tcb.cutoff_tz = cutoff_tz;\n+\tcb.cutoff_cnt = cutoff_cnt;\n+\tcb.sha1 = sha1;\n+\n+\tfor_each_reflog_ent_reverse(refname, read_ref_at_ent, &cb);\n+\n+\tif (!cb.reccnt)\n+\t\tdie(\"Log for %s is empty.\", refname);\n+\n+\tif (cb.found_it)\n+\t\treturn 0;\n+\n+\tfor_each_reflog_ent(refname, read_ref_at_ent_oldest, &cb);\n \n-\tlastrec = NULL;\n-\trec = logend = logdata + st.st_size;\n-\twhile (logdata < rec) {\n-\t\treccnt++;\n-\t\tif (logdata < rec && *(rec-1) == '\\n')\n-\t\t\trec--;\n-\t\tlastgt = NULL;\n-\t\twhile (logdata < rec && *(rec-1) != '\\n') {\n-\t\t\trec--;\n-\t\t\tif (*rec == '>')\n-\t\t\t\tlastgt = rec;\n-\t\t}\n-\t\tif (!lastgt)\n-\t\t\tdie(\"Log %s is corrupt.\", logfile);\n-\t\tdate = strtoul(lastgt + 1, &tz_c, 10);\n-\t\tif (date <= at_time || cnt == 0) {\n-\t\t\ttz = strtoul(tz_c, NULL, 10);\n-\t\t\tif (msg)\n-\t\t\t\t*msg = ref_msg(rec, logend);\n-\t\t\tif (cutoff_time)\n-\t\t\t\t*cutoff_time = date;\n-\t\t\tif (cutoff_tz)\n-\t\t\t\t*cutoff_tz = tz;\n-\t\t\tif (cutoff_cnt)\n-\t\t\t\t*cutoff_cnt = reccnt - 1;\n-\t\t\tif (lastrec) {\n-\t\t\t\tif (get_sha1_hex(lastrec, logged_sha1))\n-\t\t\t\t\tdie(\"Log %s is corrupt.\", logfile);\n-\t\t\t\tif (get_sha1_hex(rec + 41, sha1))\n-\t\t\t\t\tdie(\"Log %s is corrupt.\", logfile);\n-\t\t\t\tif (hashcmp(logged_sha1, sha1)) {\n-\t\t\t\t\twarning(\"Log %s has gap after %s.\",\n-\t\t\t\t\t\tlogfile, show_date(date, tz, DATE_RFC2822));\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\telse if (date == at_time) {\n-\t\t\t\tif (get_sha1_hex(rec + 41, sha1))\n-\t\t\t\t\tdie(\"Log %s is corrupt.\", logfile);\n-\t\t\t}\n-\t\t\telse {\n-\t\t\t\tif (get_sha1_hex(rec + 41, logged_sha1))\n-\t\t\t\t\tdie(\"Log %s is corrupt.\", logfile);\n-\t\t\t\tif (hashcmp(logged_sha1, sha1)) {\n-\t\t\t\t\twarning(\"Log %s unexpectedly ended on %s.\",\n-\t\t\t\t\t\tlogfile, show_date(date, tz, DATE_RFC2822));\n-\t\t\t\t}\n-\t\t\t}\n-\t\t\tmunmap(log_mapped, mapsz);\n-\t\t\treturn 0;\n-\t\t}\n-\t\tlastrec = rec;\n-\t\tif (cnt > 0)\n-\t\t\tcnt--;\n-\t}\n-\n-\trec = logdata;\n-\twhile (rec < logend && *rec != '>' && *rec != '\\n')\n-\t\trec++;\n-\tif (rec == logend || *rec == '\\n')\n-\t\tdie(\"Log %s is corrupt.\", logfile);\n-\tdate = strtoul(rec + 1, &tz_c, 10);\n-\ttz = strtoul(tz_c, NULL, 10);\n-\tif (get_sha1_hex(logdata, sha1))\n-\t\tdie(\"Log %s is corrupt.\", logfile);\n-\tif (is_null_sha1(sha1)) {\n-\t\tif (get_sha1_hex(logdata + 41, sha1))\n-\t\t\tdie(\"Log %s is corrupt.\", logfile);\n-\t}\n-\tif (msg)\n-\t\t*msg = ref_msg(logdata, logend);\n-\tmunmap(log_mapped, mapsz);\n-\n-\tif (cutoff_time)\n-\t\t*cutoff_time = date;\n-\tif (cutoff_tz)\n-\t\t*cutoff_tz = tz;\n-\tif (cutoff_cnt)\n-\t\t*cutoff_cnt = reccnt;\n \treturn 1;\n }\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 4e2459a..0218e96 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -235,7 +235,7 @@ test_expect_success \\\n \t'rm -f o e &&\n \t git rev-parse --verify \"master@{2005-05-26 23:33:01}\" >o 2>e &&\n \t test '\"$B\"' = $(cat o) &&\n-\t test \"warning: Log .git/logs/'\"$m has gap after $gd\"'.\" = \"$(cat e)\"'\n+\t test \"warning: Log for ref '\"$m has gap after $gd\"'.\" = \"$(cat e)\"'\n test_expect_success \\\n \t'Query \"master@{2005-05-26 23:38:00}\" (middle of history)' \\\n \t'rm -f o e &&\n@@ -253,7 +253,7 @@ test_expect_success \\\n \t'rm -f o e &&\n \t git rev-parse --verify \"master@{2005-05-28}\" >o 2>e &&\n \t test '\"$D\"' = $(cat o) &&\n-\t test \"warning: Log .git/logs/'\"$m unexpectedly ended on $ld\"'.\" = \"$(cat e)\"'\n+\t test \"warning: Log for ref '\"$m unexpectedly ended on $ld\"'.\" = \"$(cat e)\"'\n \n \n rm -f .git/$m .git/logs/$m expect\n-- \n2.0.0.527.g539d2ad\n"},{"id":"243063","messageId":"xmqqfvjr6i7u.fsf@gitster.dls.corp.google.com","threadId":"36795","inReplyTo":"1401479462-2329-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH] refs.c: change read_ref_at to use the reflog iterators","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T21:59:33Z","receivedAt":"2014-05-30T21:59:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ronnie Sahlberg <sahlberg@google.com> writes:\n\n> read_ref_at has its own parsing of the reflog file for no really good reason\n> so lets change this to use the existing reflog iterators. This removes one\n> instance where we manually unmarshall the reflog file format.\n>\n> Log messages for errors are changed slightly. We no longer print the file\n> name for the reflog, instead we refer to it as 'Log for ref <refname>'.\n> This might be a minor useability regression, but I don't really think so, since\n> experienced users would know where the log is anyway and inexperienced users\n> would not know what to do about/how to repair 'Log ... has gap ...' anyway.\n>\n> Adapt the t1400 test to handle the cahnge in log messages.\n>\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  refs.c                | 217 ++++++++++++++++++++++++++++----------------------\n>  t/t1400-update-ref.sh |   4 +-\n>  2 files changed, 122 insertions(+), 99 deletions(-)\n\nAfter reading the log message \"we are removing one redundant\nimplementation\", I would have expected that such a change would\nremove a lot more lines than it would add.  Seeing the diffstat, I\nhave to wonder if the fact that we need more lines to reuse the API\nis an indication that the reflog iterator API is overly cumbersome\nto use, perhaps requiring too much boilerplate or something.\n\nThe update in the error message may be a side-effect, but I think it\nis a change in the good direction.\n\nThanks.\n\n> diff --git a/refs.c b/refs.c\n> index 6898263..99d4832 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2936,109 +2936,132 @@ static char *ref_msg(const char *line, const char *endp)\n>  \treturn xmemdupz(line, ep - line);\n>  }\n>  \n> +struct read_ref_at_cb {\n> +\tconst char *refname;\n> +\tunsigned long at_time;\n> +\tint cnt;\n> +\tint reccnt;\n> +\tunsigned char *sha1;\n> +\tint found_it;\n> +\n> +\tchar osha1[20];\n> +\tchar nsha1[20];\n> +\tint tz;\n> +\tunsigned long date;\n> +\tchar **msg;\n> +\tunsigned long *cutoff_time;\n> +\tint *cutoff_tz;\n> +\tint *cutoff_cnt;\n> +};\n> +\n> +static int read_ref_at_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 read_ref_at_cb *cb = cb_data;\n> +\n> +\tcb->reccnt++;\n> +\tcb->tz = tz;\n> +\tcb->date = timestamp;\n> +\n> +\tif (timestamp <= cb->at_time || cb->cnt == 0) {\n> +\t\tif (cb->msg)\n> +\t\t\t*cb->msg = xstrdup(message);\n> +\t\tif (cb->cutoff_time)\n> +\t\t\t*cb->cutoff_time = timestamp;\n> +\t\tif (cb->cutoff_tz)\n> +\t\t\t*cb->cutoff_tz = tz;\n> +\t\tif (cb->cutoff_cnt)\n> +\t\t\t*cb->cutoff_cnt = cb->reccnt - 1;\n> +\n> +\t\t/*\n> +\t\t * we have not yet updated cb->[n|o]sha1 so they still\n> +\t\t * hold the values for the previous record.\n> +\t\t */\n> +\t\tif (!is_null_sha1(cb->osha1)) {\n> +\t\t\thashcpy(cb->sha1, nsha1);\n> +\t\t\tif (hashcmp(cb->osha1, nsha1))\n> +\t\t\t\twarning(\"Log for ref %s has gap after %s.\",\n> +\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_RFC2822));\n> +\t\t}\n> +\t\telse if (cb->date == cb->at_time)\n> +\t\t\thashcpy(cb->sha1, nsha1);\n> +\t\telse\n> +\t\t\tif (hashcmp(nsha1, cb->sha1))\n> +\t\t\t\twarning(\"Log for ref %s unexpectedly ended on %s.\",\n> +\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz,\n> +\t\t\t\t\t\t\t   DATE_RFC2822));\n> +\n> +\t\t/* \n> +\t\t * return 1. Not to signal an error but to break the loop\n> +\t\t * since we have found the entry we want.\n> +\t\t */\n> +\t\thashcpy(cb->osha1, osha1);\n> +\t\thashcpy(cb->nsha1, nsha1);\n> +\t\tcb->found_it = 1;\n> +\t\treturn 1;\n> +\t}\n> +\n> +\thashcpy(cb->osha1, osha1);\n> +\thashcpy(cb->nsha1, nsha1);\n> +\tif (cb->cnt > 0)\n> +\t\tcb->cnt--;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int read_ref_at_ent_oldest(unsigned char *osha1, unsigned char *nsha1,\n> +\t\t\t\t  const char *email, unsigned long timestamp,\n> +\t\t\t\t  int tz, const char *message, void *cb_data)\n> +{\n> +\tstruct read_ref_at_cb *cb = cb_data;\n> +\n> +\tif (cb->msg)\n> +\t\t*cb->msg = xstrdup(message);\n> +\tif (cb->cutoff_time)\n> +\t\t*cb->cutoff_time = timestamp;\n> +\tif (cb->cutoff_tz)\n> +\t\t*cb->cutoff_tz = tz;\n> +\tif (cb->cutoff_cnt)\n> +\t\t*cb->cutoff_cnt = cb->reccnt - 1;\n> +\n> +\thashcpy(cb->sha1, osha1);\n> +\tif (is_null_sha1(cb->sha1))\n> +\t\thashcpy(cb->sha1, nsha1);\n> +\n> +\t/* \n> +\t * return 1. Not to signal an error but to break the loop\n> +\t * since we have found the entry we want.\n> +\t */\n> +\treturn 1;\n> +}\n> +\n>  int read_ref_at(const char *refname, unsigned long at_time, int cnt,\n>  \t\tunsigned char *sha1, char **msg,\n>  \t\tunsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt)\n>  {\n> -\tconst char *logfile, *logdata, *logend, *rec, *lastgt, *lastrec;\n> -\tchar *tz_c;\n> -\tint logfd, tz, reccnt = 0;\n> -\tstruct stat st;\n> -\tunsigned long date;\n> -\tunsigned char logged_sha1[20];\n> -\tvoid *log_mapped;\n> -\tsize_t mapsz;\n> +\tstruct read_ref_at_cb cb;\n>  \n> -\tlogfile = git_path(\"logs/%s\", refname);\n> -\tlogfd = open(logfile, O_RDONLY, 0);\n> -\tif (logfd < 0)\n> -\t\tdie_errno(\"Unable to read log '%s'\", logfile);\n> -\tfstat(logfd, &st);\n> -\tif (!st.st_size)\n> -\t\tdie(\"Log %s is empty.\", logfile);\n> -\tmapsz = xsize_t(st.st_size);\n> -\tlog_mapped = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, logfd, 0);\n> -\tlogdata = log_mapped;\n> -\tclose(logfd);\n> +\tmemset(&cb, 0, sizeof(cb));\n> +\tcb.refname = refname;\n> +\tcb.at_time = at_time;\n> +\tcb.cnt = cnt;\n> +\tcb.msg = msg;\n> +\tcb.cutoff_time = cutoff_time;\n> +\tcb.cutoff_tz = cutoff_tz;\n> +\tcb.cutoff_cnt = cutoff_cnt;\n> +\tcb.sha1 = sha1;\n> +\n> +\tfor_each_reflog_ent_reverse(refname, read_ref_at_ent, &cb);\n> +\n> +\tif (!cb.reccnt)\n> +\t\tdie(\"Log for %s is empty.\", refname);\n> +\n> +\tif (cb.found_it)\n> +\t\treturn 0;\n> +\n> +\tfor_each_reflog_ent(refname, read_ref_at_ent_oldest, &cb);\n>  \n> -\tlastrec = NULL;\n> -\trec = logend = logdata + st.st_size;\n> -\twhile (logdata < rec) {\n> -\t\treccnt++;\n> -\t\tif (logdata < rec && *(rec-1) == '\\n')\n> -\t\t\trec--;\n> -\t\tlastgt = NULL;\n> -\t\twhile (logdata < rec && *(rec-1) != '\\n') {\n> -\t\t\trec--;\n> -\t\t\tif (*rec == '>')\n> -\t\t\t\tlastgt = rec;\n> -\t\t}\n> -\t\tif (!lastgt)\n> -\t\t\tdie(\"Log %s is corrupt.\", logfile);\n> -\t\tdate = strtoul(lastgt + 1, &tz_c, 10);\n> -\t\tif (date <= at_time || cnt == 0) {\n> -\t\t\ttz = strtoul(tz_c, NULL, 10);\n> -\t\t\tif (msg)\n> -\t\t\t\t*msg = ref_msg(rec, logend);\n> -\t\t\tif (cutoff_time)\n> -\t\t\t\t*cutoff_time = date;\n> -\t\t\tif (cutoff_tz)\n> -\t\t\t\t*cutoff_tz = tz;\n> -\t\t\tif (cutoff_cnt)\n> -\t\t\t\t*cutoff_cnt = reccnt - 1;\n> -\t\t\tif (lastrec) {\n> -\t\t\t\tif (get_sha1_hex(lastrec, logged_sha1))\n> -\t\t\t\t\tdie(\"Log %s is corrupt.\", logfile);\n> -\t\t\t\tif (get_sha1_hex(rec + 41, sha1))\n> -\t\t\t\t\tdie(\"Log %s is corrupt.\", logfile);\n> -\t\t\t\tif (hashcmp(logged_sha1, sha1)) {\n> -\t\t\t\t\twarning(\"Log %s has gap after %s.\",\n> -\t\t\t\t\t\tlogfile, show_date(date, tz, DATE_RFC2822));\n> -\t\t\t\t}\n> -\t\t\t}\n> -\t\t\telse if (date == at_time) {\n> -\t\t\t\tif (get_sha1_hex(rec + 41, sha1))\n> -\t\t\t\t\tdie(\"Log %s is corrupt.\", logfile);\n> -\t\t\t}\n> -\t\t\telse {\n> -\t\t\t\tif (get_sha1_hex(rec + 41, logged_sha1))\n> -\t\t\t\t\tdie(\"Log %s is corrupt.\", logfile);\n> -\t\t\t\tif (hashcmp(logged_sha1, sha1)) {\n> -\t\t\t\t\twarning(\"Log %s unexpectedly ended on %s.\",\n> -\t\t\t\t\t\tlogfile, show_date(date, tz, DATE_RFC2822));\n> -\t\t\t\t}\n> -\t\t\t}\n> -\t\t\tmunmap(log_mapped, mapsz);\n> -\t\t\treturn 0;\n> -\t\t}\n> -\t\tlastrec = rec;\n> -\t\tif (cnt > 0)\n> -\t\t\tcnt--;\n> -\t}\n> -\n> -\trec = logdata;\n> -\twhile (rec < logend && *rec != '>' && *rec != '\\n')\n> -\t\trec++;\n> -\tif (rec == logend || *rec == '\\n')\n> -\t\tdie(\"Log %s is corrupt.\", logfile);\n> -\tdate = strtoul(rec + 1, &tz_c, 10);\n> -\ttz = strtoul(tz_c, NULL, 10);\n> -\tif (get_sha1_hex(logdata, sha1))\n> -\t\tdie(\"Log %s is corrupt.\", logfile);\n> -\tif (is_null_sha1(sha1)) {\n> -\t\tif (get_sha1_hex(logdata + 41, sha1))\n> -\t\t\tdie(\"Log %s is corrupt.\", logfile);\n> -\t}\n> -\tif (msg)\n> -\t\t*msg = ref_msg(logdata, logend);\n> -\tmunmap(log_mapped, mapsz);\n> -\n> -\tif (cutoff_time)\n> -\t\t*cutoff_time = date;\n> -\tif (cutoff_tz)\n> -\t\t*cutoff_tz = tz;\n> -\tif (cutoff_cnt)\n> -\t\t*cutoff_cnt = reccnt;\n>  \treturn 1;\n>  }\n>  \n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 4e2459a..0218e96 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -235,7 +235,7 @@ test_expect_success \\\n>  \t'rm -f o e &&\n>  \t git rev-parse --verify \"master@{2005-05-26 23:33:01}\" >o 2>e &&\n>  \t test '\"$B\"' = $(cat o) &&\n> -\t test \"warning: Log .git/logs/'\"$m has gap after $gd\"'.\" = \"$(cat e)\"'\n> +\t test \"warning: Log for ref '\"$m has gap after $gd\"'.\" = \"$(cat e)\"'\n>  test_expect_success \\\n>  \t'Query \"master@{2005-05-26 23:38:00}\" (middle of history)' \\\n>  \t'rm -f o e &&\n> @@ -253,7 +253,7 @@ test_expect_success \\\n>  \t'rm -f o e &&\n>  \t git rev-parse --verify \"master@{2005-05-28}\" >o 2>e &&\n>  \t test '\"$D\"' = $(cat o) &&\n> -\t test \"warning: Log .git/logs/'\"$m unexpectedly ended on $ld\"'.\" = \"$(cat e)\"'\n> +\t test \"warning: Log for ref '\"$m unexpectedly ended on $ld\"'.\" = \"$(cat e)\"'\n>  \n>  \n>  rm -f .git/$m .git/logs/$m expect\n"},{"id":"243064","messageId":"CAPig+cTi5qzS0xOvZcu05SK9vihufPDOxYYEEyo9AqG6wsKxoQ@mail.gmail.com","threadId":"36795","inReplyTo":"1401479462-2329-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH] refs.c: change read_ref_at to use the reflog iterators","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-05-30T22:51:06Z","receivedAt":"2014-05-30T22:51:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 30, 2014 at 3:51 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:\n> read_ref_at has its own parsing of the reflog file for no really good reason\n> so lets change this to use the existing reflog iterators. This removes one\n> instance where we manually unmarshall the reflog file format.\n>\n> Log messages for errors are changed slightly. We no longer print the file\n> name for the reflog, instead we refer to it as 'Log for ref <refname>'.\n> This might be a minor useability regression, but I don't really think so, since\n> experienced users would know where the log is anyway and inexperienced users\n> would not know what to do about/how to repair 'Log ... has gap ...' anyway.\n>\n> Adapt the t1400 test to handle the cahnge in log messages.\n\ns/cahnge/change/\n\nMore below.\n\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n> diff --git a/refs.c b/refs.c\n> index 6898263..99d4832 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2936,109 +2936,132 @@ static char *ref_msg(const char *line, const char *endp)\n>         return xmemdupz(line, ep - line);\n>  }\n>\n> +static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,\n> +               const char *email, unsigned long timestamp, int tz,\n> +               const char *message, void *cb_data)\n> +{\n> +       struct read_ref_at_cb *cb = cb_data;\n> +\n> +       cb->reccnt++;\n> +       cb->tz = tz;\n> +       cb->date = timestamp;\n> +\n> +       if (timestamp <= cb->at_time || cb->cnt == 0) {\n> +               if (cb->msg)\n> +                       *cb->msg = xstrdup(message);\n> +               if (cb->cutoff_time)\n> +                       *cb->cutoff_time = timestamp;\n> +               if (cb->cutoff_tz)\n> +                       *cb->cutoff_tz = tz;\n> +               if (cb->cutoff_cnt)\n> +                       *cb->cutoff_cnt = cb->reccnt - 1;\n> +\n> +               /*\n> +                * we have not yet updated cb->[n|o]sha1 so they still\n> +                * hold the values for the previous record.\n> +                */\n> +               if (!is_null_sha1(cb->osha1)) {\n> +                       hashcpy(cb->sha1, nsha1);\n> +                       if (hashcmp(cb->osha1, nsha1))\n> +                               warning(\"Log for ref %s has gap after %s.\",\n> +                                       cb->refname, show_date(cb->date, cb->tz, DATE_RFC2822));\n> +               }\n> +               else if (cb->date == cb->at_time)\n> +                       hashcpy(cb->sha1, nsha1);\n> +               else\n> +                       if (hashcmp(nsha1, cb->sha1))\n\nThis could be an 'else if', allowing you to drop one level of indentation.\n\n> +                               warning(\"Log for ref %s unexpectedly ended on %s.\",\n> +                                       cb->refname, show_date(cb->date, cb->tz,\n> +                                                          DATE_RFC2822));\n> +\n> +               /*\n> +                * return 1. Not to signal an error but to break the loop\n> +                * since we have found the entry we want.\n> +                */\n> +               hashcpy(cb->osha1, osha1);\n> +               hashcpy(cb->nsha1, nsha1);\n> +               cb->found_it = 1;\n> +               return 1;\n> +       }\n> +\n> +       hashcpy(cb->osha1, osha1);\n> +       hashcpy(cb->nsha1, nsha1);\n> +       if (cb->cnt > 0)\n> +               cb->cnt--;\n> +\n> +       return 0;\n> +}\n"},{"id":"243138","messageId":"CAL=YDWmc40RdtMfOra0y4fnAfFAnQMsxmpF7oMBNNvjF5t54ig@mail.gmail.com","threadId":"36795","inReplyTo":"CAPig+cTi5qzS0xOvZcu05SK9vihufPDOxYYEEyo9AqG6wsKxoQ@mail.gmail.com","subject":"Re: [PATCH] refs.c: change read_ref_at to use the reflog iterators","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-02T17:59:05Z","receivedAt":"2014-06-02T17:59:05Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Fixed. Thanks.\n\nOn Fri, May 30, 2014 at 3:51 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, May 30, 2014 at 3:51 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:\n>> read_ref_at has its own parsing of the reflog file for no really good reason\n>> so lets change this to use the existing reflog iterators. This removes one\n>> instance where we manually unmarshall the reflog file format.\n>>\n>> Log messages for errors are changed slightly. We no longer print the file\n>> name for the reflog, instead we refer to it as 'Log for ref <refname>'.\n>> This might be a minor useability regression, but I don't really think so, since\n>> experienced users would know where the log is anyway and inexperienced users\n>> would not know what to do about/how to repair 'Log ... has gap ...' anyway.\n>>\n>> Adapt the t1400 test to handle the cahnge in log messages.\n>\n> s/cahnge/change/\n>\n> More below.\n>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>> diff --git a/refs.c b/refs.c\n>> index 6898263..99d4832 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2936,109 +2936,132 @@ static char *ref_msg(const char *line, const char *endp)\n>>         return xmemdupz(line, ep - line);\n>>  }\n>>\n>> +static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,\n>> +               const char *email, unsigned long timestamp, int tz,\n>> +               const char *message, void *cb_data)\n>> +{\n>> +       struct read_ref_at_cb *cb = cb_data;\n>> +\n>> +       cb->reccnt++;\n>> +       cb->tz = tz;\n>> +       cb->date = timestamp;\n>> +\n>> +       if (timestamp <= cb->at_time || cb->cnt == 0) {\n>> +               if (cb->msg)\n>> +                       *cb->msg = xstrdup(message);\n>> +               if (cb->cutoff_time)\n>> +                       *cb->cutoff_time = timestamp;\n>> +               if (cb->cutoff_tz)\n>> +                       *cb->cutoff_tz = tz;\n>> +               if (cb->cutoff_cnt)\n>> +                       *cb->cutoff_cnt = cb->reccnt - 1;\n>> +\n>> +               /*\n>> +                * we have not yet updated cb->[n|o]sha1 so they still\n>> +                * hold the values for the previous record.\n>> +                */\n>> +               if (!is_null_sha1(cb->osha1)) {\n>> +                       hashcpy(cb->sha1, nsha1);\n>> +                       if (hashcmp(cb->osha1, nsha1))\n>> +                               warning(\"Log for ref %s has gap after %s.\",\n>> +                                       cb->refname, show_date(cb->date, cb->tz, DATE_RFC2822));\n>> +               }\n>> +               else if (cb->date == cb->at_time)\n>> +                       hashcpy(cb->sha1, nsha1);\n>> +               else\n>> +                       if (hashcmp(nsha1, cb->sha1))\n>\n> This could be an 'else if', allowing you to drop one level of indentation.\n>\n>> +                               warning(\"Log for ref %s unexpectedly ended on %s.\",\n>> +                                       cb->refname, show_date(cb->date, cb->tz,\n>> +                                                          DATE_RFC2822));\n>> +\n>> +               /*\n>> +                * return 1. Not to signal an error but to break the loop\n>> +                * since we have found the entry we want.\n>> +                */\n>> +               hashcpy(cb->osha1, osha1);\n>> +               hashcpy(cb->nsha1, nsha1);\n>> +               cb->found_it = 1;\n>> +               return 1;\n>> +       }\n>> +\n>> +       hashcpy(cb->osha1, osha1);\n>> +       hashcpy(cb->nsha1, nsha1);\n>> +       if (cb->cnt > 0)\n>> +               cb->cnt--;\n>> +\n>> +       return 0;\n>> +}\n"},{"id":"243141","messageId":"CAL=YDWm-7f3sgx-y6QnwwJK9k6fPr1WosBvrQLxh6O79kMbE-w@mail.gmail.com","threadId":"36795","inReplyTo":"xmqqfvjr6i7u.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] refs.c: change read_ref_at to use the reflog iterators","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-06-02T18:12:20Z","receivedAt":"2014-06-02T18:12:20Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Fri, May 30, 2014 at 2:59 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ronnie Sahlberg <sahlberg@google.com> writes:\n>\n>> read_ref_at has its own parsing of the reflog file for no really good reason\n>> so lets change this to use the existing reflog iterators. This removes one\n>> instance where we manually unmarshall the reflog file format.\n>>\n>> Log messages for errors are changed slightly. We no longer print the file\n>> name for the reflog, instead we refer to it as 'Log for ref <refname>'.\n>> This might be a minor useability regression, but I don't really think so, since\n>> experienced users would know where the log is anyway and inexperienced users\n>> would not know what to do about/how to repair 'Log ... has gap ...' anyway.\n>>\n>> Adapt the t1400 test to handle the cahnge in log messages.\n>>\n>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n>> ---\n>>  refs.c                | 217 ++++++++++++++++++++++++++++----------------------\n>>  t/t1400-update-ref.sh |   4 +-\n>>  2 files changed, 122 insertions(+), 99 deletions(-)\n>\n> After reading the log message \"we are removing one redundant\n> implementation\", I would have expected that such a change would\n> remove a lot more lines than it would add.  Seeing the diffstat, I\n> have to wonder if the fact that we need more lines to reuse the API\n> is an indication that the reflog iterator API is overly cumbersome\n> to use, perhaps requiring too much boilerplate or something.\n\nYeah. We simplify the code and make it reuse existing unmarshallers\nand make it easier to read,  and the line count goes up :-(\n\nMost of the extra code is the due to the structure to hold all the\ndata we need in the callbacks and is a bit less compact.\nThat said, I think the new code is a little easier to read.\n\nThe biggest value is that we reduce the number of reflog unmarshallers\nby one which will save work when we start storing reflogs in a\ndifferent type of backend.\n\n>\n> The update in the error message may be a side-effect, but I think it\n> is a change in the good direction.\n>\n\nThe update in error message is also to prepare for the possibility\nthat we might get a different type of ref and reflog store in the\nfuture.\nSo that we only generate log messages that refer to filenames in those\nplaces where we are actually accessing files directly.\n\nThanks. I will resend the patch later with Eric's suggestions.\n\n\nronnie sahlberg\n"}]}