Skip to content

Commit

Permalink
avoid possible loss of last directory
Browse files Browse the repository at this point in the history
  • Loading branch information
@xsawyerx
xsawyerx committed Feb 20, 2011
1 parent 23a53e0 commit d8e79e0
Showing 1 changed file with 7 additions and 0 deletions.
7 changes: 7 additions & 0 deletions lib/Dancer.pm
View file
Expand Up @@ -312,6 +312,13 @@ sub _init {

my ($script_vol, $script_dirs, $script_name) =
File::Spec->splitpath(File::Spec->rel2abs($script));

# normalize
if ( -d ( my $fulldir = File::Spec->catdir( $script_dirs, $script_name ) ) ) {
$script_dirs = $fulldir;
$script_name = '';
}

my @script_dirs = File::Spec->splitdir($script_dirs);
my $script_path = Dancer::FileUtils::d_catdir($script_vol, $script_dirs);

Expand Down

8 comments on commit d8e79e0

@rowanthorpe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When adding the previous patch which this patch augments, I didn't bother adding a normalize check like that because the splitpath is splitting the location of the $script which is pulled in by shift. By definition the script will always be a file, so $script_name will always be a file, so as far as I can see this check is a bit unnecessary, and just adds clock cycles - or am I missing something...?

@xsawyerx
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately I got bit pretty good by it. I don't remember when, but I can explain why.

splitdir() did not check whether it's a directory or file, and continued to shift the last node. It was pretty fucked up and I saw the issue live.

I think splitdir() checks for stuff that end with a trailing slash as an indication of a directory, much like realpath().

Anyway, it was put in place to prevent a nasty bug which occurred. :)

@rowanthorpe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know what you mean about the ambiguity of splitdir(), that is why I rejigged path_no_verify() in Dancer::FileUtils to use splitpath() instead. But with respect to this particular instance of the sanity check, the line exactly before it creates $script_name by splitting $script, which is, of course, the name of the currently executing script. By definition it will always be a file (the script can't be a directory!?), so having the check on $script_name here seems redundant. Anyway, if you retain it just as a mental prompter for caution (because There Be Dragons) I can see the validity, but I just mention that here a speedup can be acheived by removing that particular instance of the check because it will always be positive. Maybe it could be replaced with a comment urging to reinstate the check if anything more funky is done with $script_name in the preceding lines...?

@rowanthorpe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If of course there is ever the chance that the @_ which is passed into _init_script_dir() is anything other than the actual script name (?!) then disregard my comments here entirely, although if that is the case it seems a little confusingly non-intuitive to be calling it $script...

@xsawyerx
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rowan, I think you misunderstood me.

This isn't a mental reminder, this fixed a bug. :)

@rowanthorpe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-) I understood that you fixed the error when $script has been set to a directory and was wrongly treated as a file, and that led me to my next point... This function starts by taking the name of the $script as input, trimming the file-part, and setting the parent directory as the $script_path. If anything is handing this function a directory-name as $script (?!) then that is the actual bug. Logic dictates that this function should never receive a directory as the script-name. I think the fix you add in this patch obviously fixes some error, but the error it fixes is in whatever function commits the error, and should fix it there, rather than concealing it after it reaches this function, because then it might also conceal any other as-yet-unknown errors being handed in by other functions. Also, on reflection, the "handing a directory to _init_script_dir()" error might be connected to a buggy behaviour I've already noticed a few times - where a run of some tests from the test-suite end up leaving "logs" dirs parallel to the app_dir rather than inside it (setting the grandparent-dir-of-the-script, rather than the parent-dir)...

@xsawyerx
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see what you're saying, and I agree with you.

I don't intend to do this at the moment, but I do believe the entire path issue needs a clean rewrite.

@rowanthorpe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. At least the "heads up" is registered :-) I'll post the "$script receiving dir-names" bug as an issue so it serves as a TODO of sorts.

Please sign in to comment.