Excessive file type checking when running scripts

Question

Excessive file type checking when running scripts

BinuDR opened this issue 3 years ago · 4 comments

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.

Answer 1 · 2022-02-19T06:54:26.000Z

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}$/, '')

Answer 2 · 2022-02-19T06:56:41.000Z

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

Answer 3 · 2022-02-19T07:13:47.000Z

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).

Answer 4 · 2022-02-19T07:18:41.000Z

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