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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 16, 2013, 05:35 UTC
Message-ID: <7vtxqhzo4m.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CACsJy8C2uEgwozpWBfowYJea3XRB72rhzjsSFuG9Ud0afuQy6w@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 13 quoted lines

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

Show 5 quoted lines

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