From: Eric Sunshine <sunshine@sunshineco.com>
Date: Fri, 30 May 2014 22:51:06 GMT
Subject: Re: [PATCH] refs.c: change read_ref_at to use the reflog iterators
Message-ID: <CAPig+cTi5qzS0xOvZcu05SK9vihufPDOxYYEEyo9AqG6wsKxoQ@mail.gmail.com>
In-Reply-To: <1401479462-2329-1-git-send-email-sahlberg@google.com>

On Fri, May 30, 2014 at 3:51 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> 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 cahnge in log messages.

s/cahnge/change/

More below.

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
> diff --git a/refs.c b/refs.c
> index 6898263..99d4832 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2936,109 +2936,132 @@ static char *ref_msg(const char *line, const char *endp)
>         return xmemdupz(line, ep - line);
>  }
>
> +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))

This could be an 'else if', allowing you to drop one level of indentation.

> +                               warning("Log for ref %s unexpectedly ended on %s.",
> +                                       cb->refname, show_date(cb->date, cb->tz,
> +                                                          DATE_RFC2822));
> +
> +               /*
> +                * return 1. Not to signal an error but to break the loop
> +                * since we have found the entry we want.
> +                */
> +               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;
> +}