Re: [PATCH 2/5] sha1_name.c: don't waste cycles in the @-parsing loop
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 2, 2013, 00:04 UTC
- Message-ID
- <7vmwse2qmt.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CAMP44s1j6GdQBjgCdBrG3xvXr_XifCTRZWJAQUrzts-MzFg--g@mail.gmail.com>
Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 47 quoted lines
> On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
> <artagnon@gmail.com> wrote:
>> The @-parsing loop unnecessarily checks for the sequence "@{" from
>> len - 2 unnecessarily. We can safely check from len - 4: write out a
>> comment justifying this.
>>
>> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>> ---
>> sha1_name.c | 18 +++++++++++++++++-
>> 1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/sha1_name.c b/sha1_name.c
>> index 3820f28..be1d12c 100644
>> --- a/sha1_name.c
>> +++ b/sha1_name.c
>> @@ -445,7 +445,23 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>> /* basic@{time or number or -number} format to query ref-log */
>> reflog_len = at = 0;
>> if (len && str[len-1] == '}') {
>> - for (at = len-2; at >= 0; at--) {
>> + /* str = @}
>> + * ^
>> + * len - 2; expression is senseless
>> + *
>> + * str = @{}
>> + * ^
>> + * len - 3; expression is still senseless
>> + *
>> + * str = @{.}
>> + * ^
>> + * len - 4 where . is any character; expression
>> + * is worth investigating
>> + *
>> + * Therefore, if str ends with }, search three
>> + * characters earlier for @{
>> + */
>
> I think this comment is overkill.
>
>> + for (at = len - 4; at >= 0; at--) {
>
> The change seems OK to me, but there's no need to explain where you
> are starting, and if there's a need:
>
> /* start from where reflogs can start: @{.} */
>
> Does the trick nicely.As the fact that nobody noticed nor bothered with the two-byte optimization opportunity shows that this is trickier than trivial, I agree with both of you that this change deserves an in-code comment.
Start checking at len - 4, because there has to be at least one
byte inside "@{.}" for it to be worth checking.would be sufficient. The 16-line comment is way overkill.
Not that I think this change really matters, though.
>> if (str[at] == '@' && str[at+1] == '{') {
>> if (!upstream_mark(str + at, len - at)) {
>> reflog_len = (len-1) - (at+2);
>> --