Re: [PATCH 4/7] Add join_paths() to safely concatenate paths.
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:54:52
Hi, Michael G. Schwern wrote:
Otherwise you might wind up with things like...
my $path1 = undef;
my $path2 = 'foo';
my $path = $path1 . '/' . $path2;
creating '/foo'. Or this...
my $path1 = 'foo/';
my $path2 = 'bar';
my $path = $path1 . '/' . $path2;
creating 'foo//bar'.I'm still puzzled by this one, too. I don't understand the motivation. Is this to make joining paths less fragile, by preserving the property that join_paths($a, $b) names the directory you would get to by first chdir-ing into $a and then into $b? It would be easier to understand as two patches: first, one that extracts join_paths without any functional change, and then one that changes its implementation with an explanation for what positive functional effect that would have.
quoted hunk ↗ jump to hunk
--- a/git-svn.perl +++ b/git-svn.perl
[...]
quoted hunk ↗ jump to hunk
@@ -1275,7 +1276,7 @@ sub get_svnprops { $path = $cmd_dir_prefix . $path; fatal("No such file or directory: $path") unless -e $path; my $is_dir = -d $path ? 1 : 0; - $path = $gs->{path} . '/' . $path; + $path = join_paths($gs->{path}, $path); # canonicalize the path (otherwise libsvn will abort or fail to # find the file)
This can't be for the //-collapsing effect since the path is about to be canonicalized. It can't be for the initial-/ effect since that is stripped away by canonicalization, too. So no functional effect here, good or bad. [...]
quoted hunk ↗ jump to hunk
--- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm
[...]
quoted hunk ↗ jump to hunk
@@ -316,9 +320,7 @@ sub init_remote_config { } my $old_path = $self->path; $url =~ s!^\Q$min_url\E(/|$)!!; - if (length $old_path) { - $url .= "/$old_path"; - } + $url = join_paths($url, $old_path); $self->path($url);
This is probably not for the normal //-collapsing effect because $url already has its trailing / stripped off. Maybe it is for cases where $old_path has leading slashes or $min_url has multiple trailing ones? In the end it shouldn't make a difference, once a later patch teaches Git::SVN->path to canonicalize. Is the functional change in this patch for aesthetic reasons, or is there some other component (perhaps in a later patch) that relies on it? Thanks again for your help, Jonathan