Thanks.
On Mon, Jun 2, 2014 at 2:21 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Ronnie Sahlberg <sahlberg@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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.
>
>> +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.
>
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html