Bug 137982 - prepare-changelog does not read paths containing spaces properly
Summary: prepare-changelog does not read paths containing spaces properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on:
Blocks: 204236
  Show dependency treegraph
 
Reported: 2014-10-22 14:45 PDT by Remy Demarest
Modified: 2019-11-15 11:17 PST (History)
4 users (show)

See Also:


Attachments
Proposed Fix (1.81 KB, patch)
2016-01-14 15:01 PST, BJ Burg
joepeck: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Remy Demarest 2014-10-22 14:45:52 PDT
I run prepare-changelog on a repository that contains changes inside folders whose name contains spaces and I get multiple errors like this:
fatal: Path 'Safari/AppKit' does not exist in 'origin/master'
We have two folder named "AppKit Extra Classes" and "AppKit Extra Methods" for which this could apply.
Comment 1 BJ Burg 2016-01-14 13:53:49 PST
Ugh, just ran into this too.
Comment 2 BJ Burg 2016-01-14 14:19:38 PST
Relevant trace from using `perl -d:Trace prepare-Changelog`:

>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:335:                 my @deleted_function_ranges = get_function_line_ranges(\*SOURCE, $file);
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:688:     my ($file_handle, $file_name) = @_;
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:692:     return get_function_line_ranges_for_cpp($file_handle, $file_name) if $file_name =~ /\.(c|cpp|m|mm|h)$/;
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:693:     return get_function_line_ranges_for_java($file_handle, $file_name) if $file_name =~ /\.java$/;
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:694:     return get_function_line_ranges_for_javascript($file_handle, $file_name) if $file_name =~ /\.js$/;
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:695:     return get_selector_line_ranges_for_css($file_handle, $file_name) if $file_name =~ /\.css$/;
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:696:     return get_function_line_ranges_for_perl($file_handle, $file_name) if $file_name =~ /\.p[lm]$/;
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:697:     return get_function_line_ranges_for_python($file_handle, $file_name) if $file_name =~ /\.py$/ or $file_name =~ /master\.cfg$/;
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:698:     return get_function_line_ranges_for_swift($file_handle, $file_name) if $file_name =~ /\.swift$/;
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:702:     my $first_line = <$file_handle>;
fatal: Path 'Safari.xcworkspace/xcshareddata/xcschemes/Only' does not exist in 'HEAD'
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:703:     seek($file_handle, 0, 0);
>> /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog:705:     return () unless $first_line =~ m|^#!(?:/usr/bin/env\s+)?(\S+)|;
Use of uninitialized value $first_line in pattern match (m//) at /Users/bburg/repos/webkit-tot/OpenSource/Tools/Scripts/prepare-ChangeLog line 705.
Comment 3 BJ Burg 2016-01-14 15:01:32 PST
Created attachment 269012 [details]
Proposed Fix
Comment 4 Joseph Pecoraro 2016-01-14 15:03:21 PST
Comment on attachment 269012 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=269012&action=review

r=me

> Tools/Scripts/prepare-ChangeLog:319
> +        $file =~ s/ /\\ /;

I think this needs the trailing /g in order to change all occurrences.
Comment 5 BJ Burg 2016-01-14 15:07:58 PST
Committed r195080: <http://trac.webkit.org/changeset/195080>
Comment 6 Daniel Bates 2019-11-15 11:14:51 PST
Comment on attachment 269012 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=269012&action=review

>> Tools/Scripts/prepare-ChangeLog:319
>> +        $file =~ s/ /\\ /;
> 
> I think this needs the trailing /g in order to change all occurrences.

This change, including the chomp(), is not necessary. The actual fix for this bug was below with using the open() call that takes a mode. In fact, this change breaks function list generation for file paths that have a space in them because the code above ^^^ built up a dictionaries of line ranges keyed off the **unescaped** filename. However, after this substitution the code below will query these dictionaries for the **escaped** filename and hence **never** find a match.

> Tools/Scripts/prepare-ChangeLog:323
> +            open(SOURCE, "<", $file) or next;

This is the actual fix!