GNOME Bugzilla – Bug 453717
fixxref logic to determine absolute path's is flawed
Last modified: 2007-09-22 19:53:30 UTC
fixxref should check for a common prefix (between HTML_DIR and EXTRA_DIRS) and not for a leading '/' to determine wheter links need to be absolute or not.
Created attachment 91186 [details] [review] determine absolute path's
Created attachment 91201 [details] [review] determine absolute path's better patch, that also avoid rescanning.
Created attachment 91242 [details] [review] determine absolute path's now its also escaping the vars inside regexp's
Have you tested it with a HTML_DIR that doesn't end with /share/gtk-doc/html? It looks like it could possibly cause problems with an undefined or empty $path_prefix. It might call ScanIndices with the wrong arguments, though it's difficult to tell. Id prefer an explicit check for if it is empty. A few other comments: 1) + if ($dir !~ m%\Q$path_prefix\E/(.*)%) { You should probably anchor that regexp to the start of the directory, and you don't need the (.*) at the end. You should also accept the DOS separator, i.e. + if ($dir !~ m%^\Q$path_prefix\E[/\\]%) { That same expression also appears a bit later. 2) + if ($dir =~ m%\Q$vdir\E(.*)%) { This also has similar issues. This might be better: + if ($dir =~ m%^\Q$vdir\E%) { Also $vdir might not end in a directory separator so we may accidentally skip other directories with similar names. I don't know if that is worth worring about.
Created attachment 91317 [details] [review] determine absolute path's * changed the regexps to start matching from the begin * match visited dirs with '/' or completely (not trailing '/') I keept the '/' as we have had those before. I could make an extra patch that replaces them in whole fixxref with [/\\].
1) You need to use 'eq' to compare strings rather than '==': + if ($dir == $vdir || $dir =~ m%^\Q$vdir\E/%) { 2) I'd still like to handle the DOS separator. I think the existing code is OK - you only need to handle the DOS separator when matching unknown paths. It needs fixing in these 5 lines: +$HTML_DIR =~ m%(.*?)/share/gtk-doc/html%; + if ($dir !~ m%^\Q$path_prefix\E/%) { + if ($dir !~ m%^\Q$path_prefix\E/%) { + if ($dir == $vdir || $dir =~ m%^\Q$vdir\E/%) { + if ($dir !~ m%^\Q$path_prefix\E/%) { 3) I'm still worried about what happens if $path_prefix is empty/unset. i.e. if this doesn't match: +$HTML_DIR =~ m%(.*?)/share/gtk-doc/html%; +my $path_prefix=$1; what happens here: + if ($dir !~ m%^\Q$path_prefix\E/%) {
1.) oops, changed 2.) what about existing code, e.g. $dir = $dir . "/share/gtk-doc/html"; This could help: http://perldoc.perl.org/File/Spec.html $dir = $dir . File::Spec::catfile("share","gtk-doc","html"); 3.) I haven't figured where HTML_DIR actualy get set in the Makefiles. What about this change: -$HTML_DIR =~ m%(.*?)/share/gtk-doc/html%; -my $path_prefix=$1; +my $path_prefix=""; +if ($HTML_DIR =~ m%(.*?)/share/gtk-doc/html%) { + $path_prefix=$1; +# print "Path prefix: $path_prefix\n"; +} if its empty then in ($dir !~ m%^\Q$path_prefix\E/%) we get no match and we scan using an absolute path.
Created attachment 91563 [details] [review] determine absolute path's This should be fine now. It fixes the issue described in the report. Tim Janik confirmed in recent jhbuild run. I ommit the dos-path changes as this needs to be addresses separately and then not ad-hoc here. Damon, please create another ticket if you like this fixed. I am waiting for your okay to commit this.
Created attachment 91565 [details] [review] determine absolute path's its 'eq' and not'-eq' ;)
2007-07-15 Stefan Kost <ensonic@users.sf.net> * gtkdoc-fixxref.in: Improve detection of absolute/relatives paths. Skip duplicate paths. Fixes #453717.