Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Excessive file type checking when running scripts #67

Open
BinuDR opened this issue Feb 19, 2022 · 4 comments
Open

Excessive file type checking when running scripts #67

BinuDR opened this issue Feb 19, 2022 · 4 comments

Comments

@BinuDR
Copy link
Contributor

BinuDR commented Feb 19, 2022

When looking for scripts in the ./scripts and ./scripts/custom folder, lich is looking for the following extensions:
.lic, .rb, .cmd, .wiz, .lic.gz, .rb.gz, .cmd.gz, .wiz.gz, .lic.z, .rb.z, .cmd.z, and .wiz.z

This seems excessive. The following block of code for matching the file name is also very convoluted. It seems like it could be cut in half.

if file_name = (file_list.find { |val| val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/ || val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/i } || file_list.find { |val| val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}[^.]+\.(?i:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/ } || file_list.find { |val| val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}[^.]+\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/i })
         script_name = file_name.sub(/\..{1,3}$/, '')
      end

file_list in the above block of code is:
file_list = Dir.children(File.join(SCRIPT_DIR, "custom")).map{ |s| s.prepend("/custom/") } + Dir.children(SCRIPT_DIR)

There are 3 sections to it. The first find block is checking if any of the files match either of these 2 patterns:
/^(?:\/custom\/)?#{Regexp.escape(script_name)}\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/
/^(?:\/custom\/)?#{Regexp.escape(script_name)}\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/i

I am unsure why these two are both needed. It seems like the second with the insensitive tag would match anything that the first one would match.

Then the next find block is checking if the file name matches this pattern:
/^(?:\/custom\/)?#{Regexp.escape(script_name)}[^.]+\.(?i:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/

And then the last find block checks for this pattern:
/^(?:\/custom\/)?#{Regexp.escape(script_name)}[^.]+\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/i

The only differences between these two are that in the first one the extension (lic, rb, cmd, and wiz) are a case insensitive check, and in the second one the whole pattern is case insensitive.

These last two blocks allow for matching partial script names so you can run ;repos and it'll find repository.lic

In trying to consolidate these, I came up with this. I was trying to make it so that you could run a script with the extension ;repository.lic. Currently if you do that, it will say script not found. In doing so, I forgot about running scripts with partial filenames. Here is what I had come up with for that:

if file_name = ( file_list.find { |val| val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}(?:\.lic|\.rb|\.cmd|\.wiz)?(?:\.gz|\.Z)?$/i } )

This does allow you to run any script with or without its extension, but it won't allow you to run a partial script name.

I think this captures all possible cases:
if file_name = ( file_list.find { |val| val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}(?:\.lic|\.rb|\.cmd|\.wiz)?(?:\.gz|\.Z)?$/i || val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}[^.]+\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/i } )

It could use some more eyes on it though.

@BinuDR
Copy link
Contributor Author

BinuDR commented Feb 19, 2022

Also, later on in the file there's that big block of File.exists?(..) statements that Robert mentioned in my last PR, asking if it could be reduced to an array loop of some sort.

Original code:

   @@elevated_exists = proc { |script_name|
      if script_name =~ /\\|\//
         nil
      elsif script_name =~ /\.(?:lic|lich|rb|cmd|wiz)(?:\.gz)?$/i
         File.exists?("#{SCRIPT_DIR}/#{script_name}") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}")
      else
         File.exists?("#{SCRIPT_DIR}/#{script_name}.lic") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.lic") ||
         File.exists?("#{SCRIPT_DIR}/#{script_name}.lich") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.lich") ||
         File.exists?("#{SCRIPT_DIR}/#{script_name}.rb") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.rb") ||
         File.exists?("#{SCRIPT_DIR}/#{script_name}.cmd") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.cmd") ||
         File.exists?("#{SCRIPT_DIR}/#{script_name}.wiz") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.wiz") ||
         File.exists?("#{SCRIPT_DIR}/#{script_name}.lic.gz") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.lic.gz") ||
         File.exists?("#{SCRIPT_DIR}/#{script_name}.rb.gz") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.rb.gz") ||
         File.exists?("#{SCRIPT_DIR}/#{script_name}.cmd.gz") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.cmd.gz") ||
         File.exists?("#{SCRIPT_DIR}/#{script_name}.wiz.gz") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}.wiz.gz")
      end

Possible solution:

   @@elevated_exists = proc { |script_name|
      if script_name =~ /\\|\//
         nil
      elsif script_name =~ /\.(?:lic|lich|rb|cmd|wiz)(?:\.gz)?$/i
         # _respond "<pushBold/>Testing #{script_name} in the elsif section:<popBold/>"
         File.exists?("#{SCRIPT_DIR}/#{script_name}") || File.exists?("#{SCRIPT_DIR}/custom/#{script_name}")
      else
         # _respond "<pushBold/>Testing script #{script_name} in the else section:<popBold/>"
         ["#{SCRIPT_DIR}/#{script_name}", "#{SCRIPT_DIR}/custom/#{script_name}"].each{ |path|
            ['lic', 'lich', 'rb', 'cmd', 'wiz', 'lic.gz', 'lich.gz', 'rb.gz', 'cmd.gz', 'wiz.gz'].each{ |ext|
               File.exists?("#{path}.#{ext}")
            }
         }
      end
   }

The commented out _respond statements were for my testing. The are not staying.

I'm not sure why it's checking for .lich as a file type, the other portion of the code won't deal with extensions longer than 3 characters. In fact, it makes me wonder why it's also trying to accomodate .gz extensions because the substitution done after it finds the file in the directory only works for up to 3 characters after the ..

script_name = file_name.sub(/\..{1,3}$/, '')

@BinuDR
Copy link
Contributor Author

BinuDR commented Feb 19, 2022

Oh, and apparently File.exists?(..) is deprecated and it's suggested that we use File.exist?(..) now.

https://ruby-doc.org/core-2.5.0/File.html#method-c-exists-3F

@BinuDR
Copy link
Contributor Author

BinuDR commented Feb 19, 2022

Ok, this causes a really weird error. I had like 50+ copies of bootstrap and common-healing2 running and lich was grindingly slow.

if file_name = ( file_list.find { |val| val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}(?:\.lic|\.rb|\.cmd|\.wiz)?(?:\.gz|\.Z)?$/i || val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}[^.]+\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/i } )

Separating the 2 sections into their own .find blocks fixes that error. I'm not sure why though.

if file_name = ( file_list.find { |val| val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}(?:\.lic|\.rb|\.cmd|\.wiz)?(?:\.gz|\.Z)?$/i } || file_list.find{ |val| val =~ /^(?:\/custom\/)?#{Regexp.escape(script_name)}[^.]+\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/i } )

This latest change now allows me to run scripts with a partial name again (;repos list), and also with the extension (;repository.lic list).

@BinuDR
Copy link
Contributor Author

BinuDR commented Feb 19, 2022

Now that I think about it, I think the purpose of having both of these sections, instead of just the 1 with the case insensitive flag, is so that lich can differentiate between say ;repository and ;REPOSITORY

/^(?:\/custom\/)?#{Regexp.escape(script_name)}\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/
/^(?:\/custom\/)?#{Regexp.escape(script_name)}\.(?:lic|rb|cmd|wiz)(?:\.gz|\.Z)?$/i

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant