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

> 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;
>> +}