Re: [PATCH 3/8] longest_ancestor_length(): use string_list_split()
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Sep 29, 2012, 05:25 UTC
- Message-ID
- <5066863E.1030005@alum.mit.edu>
- In-Reply-To
- <7vr4pnnkux.fsf@alter.siamese.dyndns.org>
On 09/28/2012 12:48 AM, Junio C Hamano wrote:
Show 13 quoted lines
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> - for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
>> - for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
>> - len = colon - ceil;
>> + string_list_split(&prefixes, prefix_list, PATH_SEP, -1);
>> +
>> + for (i = 0; i < prefixes.nr; i++) {
>> + const char *ceil = prefixes.items[i].string;
>> + int len = strlen(ceil);
>> +
>
> Much nicer than the yucky original ;-)If your winky-smiley implies irony, then I would like to object. Even though the original is not difficult to understand, it is more difficult to review than the new version because one has to think about off-by-one errors etc. The new version has a bit more boilerplate, but a quick read suffices both to understand it and to see that it is correct. Though of course I admit that the improvement is small.
But the main point of this change is to move towards using more testable parts, so it is a step forward regardless of whether it is more readable.
Show 14 quoted lines
>> if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil)) >> continue; >> - strlcpy(buf, ceil, len+1); >> + memcpy(buf, ceil, len+1); >> if (normalize_path_copy(buf, buf) < 0) >> continue; > > Why do you need this memcpy in the first place? Isn't ceil already > a NUL terminated string unlike the original code that points into a > part of the prefix_list string? IOW, why not > > normalize_path_copy(buf, ceil); > > or something?
Good point. I will fix this in v2.
> Can normalize_path_copy() overflow buf[PATH_MAX+1] here (before or > after this patch)?
normalize_path_copy() can only shrink paths, not grow them. So the length check on ceil guarantees that the result of normalize_path_copy() will fit in buf.
Michael
-- Michael Haggerty mhagger@alum.mit.edu http://softwareswirl.blogspot.com/