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

> 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.

>
>> +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?

>
>> +     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)?
>