Re: [PATCH] attr: fix off-by-one directory component length calculation

[Junio C Hamano <gitster@pobox.com>]

Duy Nguyen <pclouds@gmail.com> writes:

> On Wed, Jan 16, 2013 at 9:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> On Wed, Jan 16, 2013 at 08:08:03AM +0700, Duy Nguyen wrote:
>>>> Actually I'd like to remove that function.
>>>
>>> This is what I had in mind:
>>
>> I think the replacement logic to find the basename is moderately
>> inferiour to the original.  For one thing (this may be somewhat
>> subjective), it is less readable now.
>
> Yeah, maybe it's micro optimization.

Your change is micro unoptimization (and making the result less
readable).  I wouldn't worry too much about micro-optimizing an
existing piece of code, but making an efficient code into a worse
one without a good reason is a different story.

>> Also the original only
>> scanned the string from the beginning once (instead of letting
>> strlen() to scan once and go back).
>
> But we do need to strlen() anyway in collect_all_attrs().

That is exactly my point, isn't it?

The loop to find the basename has to run to the end of the string at
least once, as it cannot not stop at the last slash---it goes from
front to back and it won't know which one is the last slash until it
sees the end of the string.  After the loop exits, you know the
length of the string without running a separate strlen() to assign
to "pathlen".

> So we scan
> the string 3 times (strlen + 2 * find_basename) in the original. Now
> we do it twice.

I already said that overall restructure of the code may be a good
idea to reduce the calls to the function.  I was only comparing the
implementations of the loop that finds the basename, so I do not
understand what you mean by that "2 *" in that comparison.  It does
not make sense to me.