Re: [PATCH] fix crash in path.c on Windows

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Johannes Sixt schrieb:
> René Scharfe schrieb:
>> 	set a=	getenv("a")
>> 	======= ===========
>> 	c	c
>> 	/c	c:/
>> 	c/	c/
>> 	/c/	c:/
>> 	c:c	c:c
>> 	/c:c	c:c
>> 	c:/c	c:/c
>> 	/c:/c	c:/c
>> 	c/:/c	c\;c:\
>> 	/c:c/	c:c/
>> 	/c/:c	/c/:c
>> 	/c/:/c	c:\;c:\
>> 	/c:/c/	c:/c/
>> 	/c/:/c/	c:\;c:\
> 
> Bash translates leading single-letter path components to drive prefix
> notation if it invokes a non-MSYS program; and it also translates ':' to
> ';' if the value looks like a path list. Sometimes there is an ambiguity
> and bash guesses wrong.

Sure, but what rules or heuristics does it follow?  Do we need to
post-process the results or can we simply change the test case in t1504?

>> @@ -387,7 +387,7 @@ int normalize_absolute_path(char *buf, const char *path)
>>  	assert(path);
>>  
>>  	while (*comp_start) {
>> -		assert(*comp_start == '/');
>> +		assert(is_absolute_path(comp_start));
>>  		while (*++comp_end && *comp_end != '/')
>>  			; /* nothing */
>>  		comp_len = comp_end - comp_start;
> 
> Junio has pointed out your thinko here. Furthermore, *all* uses of '/' in
> this loop must be replaced by is_dir_sep().

It's not a thinko because I didn't think there. :)  It's more a case of
mechanical refactoring resulting in confusing code.  If we inline
is_absolute_path() manually there, we get the following:

	assert(*comp_start == '/' || has_dos_drive_prefix(comp_start));

The loop works because DOS drive prefixes never contain slashes.

Is is_absolute_path() too forgiving on Windows, i.e. should it stop
classifying paths starting with a slash as absolute on that platform?

> But I would really appreciate if you could unify normalize_absolute_path()
> with setup.c's sanitary_path_copy() because they do almost the same thing
> (and the latter already works on Windows).

Good idea.

>> @@ -438,11 +438,20 @@ int longest_ancestor_length(const char *path, const char *prefix_list)
>>  		return -1;
>>  
>>  	for (colon = ceil = prefix_list; *colon; ceil = colon+1) {
>> -		for (colon = ceil; *colon && *colon != ':'; colon++);
>> +		for (colon = ceil; *colon && *colon != PATH_SEP; colon++);
>>  		len = colon - ceil;
>>  		if (len == 0 || len > PATH_MAX || !is_absolute_path(ceil))
>>  			continue;
>>  		strlcpy(buf, ceil, len+1);
>> +
>> +		if (has_dos_drive_prefix(buf)) {
>> +			char *p;
>> +			for (p = buf; *p; p++) {
>> +				if (*p == '\\')
>> +					*p = '/';
>> +			}
> 
> IMNSHO this is a kind of normalization and, therefore, must be done by
> normalize_absolute_path() (sanitary_path_copy() already does this).

Makes sense.

René