Copilot commented on code in PR #7931:
URL: https://github.com/apache/hbase/pull/7931#discussion_r2932334688
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -163,6 +163,23 @@ def eval_input
else
exc = nil
next
+ ensure
+ # HBASE-28660: Prevent command shadowing by incorrectly parsed
local variables
+ cmd_names = ::Shell.commands.keys.map(&:to_sym)
+ workspace_binding = @context.workspace.binding
+ shadowing_vars = workspace_binding.local_variables & cmd_names
+
+ if shadowing_vars.any?
+ shadowing_vars.each do |var|
+ puts "WARN: '#{var}' is a reserved HBase command. Local
variable assignment ignored."
Review Comment:
These warnings are printed via `puts` (stdout). In non-interactive/script
usage, stdout is often parsed/consumed as command output, so injecting `WARN:`
lines can break existing automation. Consider emitting warnings to stderr
(`warn`/`$stderr.puts`) and/or gating the message behind `@interactive` (or a
config flag) so scripted output remains stable.
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -163,6 +163,23 @@ def eval_input
else
exc = nil
next
+ ensure
+ # HBASE-28660: Prevent command shadowing by incorrectly parsed
local variables
+ cmd_names = ::Shell.commands.keys.map(&:to_sym)
+ workspace_binding = @context.workspace.binding
+ shadowing_vars = workspace_binding.local_variables & cmd_names
Review Comment:
`::Shell.commands.keys.map(&:to_sym)` allocates a new array (and many
symbols/strings conversions) on every evaluated statement. Since the command
set is effectively static after shell boot, memoize/freeze this list (or use a
`Set`) to avoid repeated allocations in the interactive loop.
##########
hbase-shell/src/main/ruby/irb/hirb.rb:
##########
@@ -163,6 +163,23 @@ def eval_input
else
exc = nil
next
+ ensure
+ # HBASE-28660: Prevent command shadowing by incorrectly parsed
local variables
+ cmd_names = ::Shell.commands.keys.map(&:to_sym)
+ workspace_binding = @context.workspace.binding
+ shadowing_vars = workspace_binding.local_variables & cmd_names
+
+ if shadowing_vars.any?
+ shadowing_vars.each do |var|
+ puts "WARN: '#{var}' is a reserved HBase command. Local
variable assignment ignored."
+ end
+
+ new_binding = @context.workspace.main.get_binding
+ (workspace_binding.local_variables - shadowing_vars).each do
|var|
+ new_binding.local_variable_set(var,
workspace_binding.local_variable_get(var))
+ end
+ @context.instance_variable_set(:@workspace,
::IRB::WorkSpace.new(new_binding))
+ end
Review Comment:
Updating the context via `@context.instance_variable_set(:@workspace, ...)`
couples this code to IRB internals and may break across IRB/JRuby upgrades.
Prefer a public API if available (e.g., `@context.workspace = ...`), or at
least wrap the mutation in a small helper with a compatibility fallback so the
dependency on `@workspace` is localized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
