Re: [PATCH 04/23] prefix_ref_iterator: don't trim too much

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: May 18, 2017, 04:50 UTC
Message-ID: <e9f987c6-3a02-4121-bd21-e7b78f28f7db@alum.mit.edu>
In-Reply-To: <xmqq1srmvlt8.fsf@gitster.mtv.corp.google.com>

On 05/18/2017 06:19 AM, Junio C Hamano wrote:

Show 20 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> The `trim` parameter can be set independently of `prefix`. So if some
>> caller were to set `trim` to be greater than `strlen(prefix)`, we
>> could end up pointing the `refname` field of the iterator past the NUL
>> of the actual reference name string.
>>
>> That can't happen currently, because `trim` is always set either to
>> zero or to `strlen(prefix)`. But even the latter could lead to
>> confusion, if a refname is exactly equal to the prefix, because then
>> we would set the outgoing `refname` to the empty string.
>>
>> And we're about to decouple the `prefix` and `trim` arguments even
>> more, so let's be cautious here. Skip over any references whose names
>> are not longer than `trim`.
> 
> Should we be silently continuing, or should we use die("BUG") here
> instead, if the motivation is to be cautions?  Personally, I do not
> find this memchr() implementation too bad, especially when our
> objective is to play cautious, but strlen() based one is fine, too.

True; it would not make much sense to trim off more characters than a prefix that you have selected for. This also implies that callers should only check-and-trim a prefix that ends with an explicit "/". Otherwise, say for example if a caller tries to check-and-trim the prefix "refs/bisect" and the user has a reference named "refs/bisect", the result could be the empty string.

I'll change this to die("BUG") and document the above.

> It's not like refname field would point at a run of non-NUL bytes at
> the end of the last-mapped page and taking strlen() would segfault,
> right?

No, refname should be a legit string.

Michael