Fix assignment in condition.

[voxik]

I am not sure what is the purpose of this code neither I have idea if the proposed fix is actually correct, but I am quite sure that the condition does not make sense, because the assignment takes priority and therefore the branch is never accessible. So I just take a guess and submitted this PR to open the discussion ;)

Please note this was pointed out by Coverity scan of the Bundler code:

Error: DEADCODE (CWE-561):
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/source/metadata.rb:22: cond_return: Condition "nil && Bundler.rubygems().loaded_specs("bundler")", returning "nil". Now the type of "nil && Bundler.rubygems().loaded_specs("bundler")" must be null.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/source/metadata.rb:22: assignment: Assigning: "loaded_spec" = "nil && Bundler.rubygems().loaded_specs("bundler")".
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/source/metadata.rb:22: possible_types: At condition "loaded_spec = (nil && Bundler.rubygems().loaded_specs("bundler"))", the type of "loaded_spec" must be null.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/source/metadata.rb:22: implied_false: "nil" implies that the truth value of "nil" must be false.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/source/metadata.rb:22: cond_return: Condition "nil && Bundler.rubygems().loaded_specs("bundler")", returning "nil". The truth value of "nil && Bundler.rubygems().loaded_specs("bundler")" must be false.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/source/metadata.rb:22: truth: At condition "loaded_spec = (nil && Bundler.rubygems().loaded_specs("bundler"))", the truth value of "loaded_spec" must be false.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/source/metadata.rb:22: dead_error_condition: The condition "loaded_spec = (nil && Bundler.rubygems().loaded_specs("bundler"))" cannot be true.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/source/metadata.rb:22: dead_error_line: Execution cannot reach the expression "idx << loaded_spec" inside this statement: "(loaded_spec = (nil && Bund...".
#   20|               s.loaded_from = File.expand_path("..", __FILE__)
#   21|             end
#   22|->           if loaded_spec = nil && Bundler.rubygems.loaded_specs("bundler")
#   23|               idx << loaded_spec # this has to come after the fake gemspec, to override it
#   24|             elsif local_spec = Bundler.rubygems.find_name("bundler").find {|s| s.version.to_s == VERSION }

[voxik]

Ok, so the PR is wrong. So I leave it up to you 😊

[voxik]

It was just style thing, so second try and then I'll leave it to you ...

[voxik]

Hm, failed again. Maybe the branching should be removed entirely because it had not effect so far ...

[deivid-rodriguez]

@voxik I don't know what the condition wanted to do, but the branch doesn't seem accesible to me either. Can you try removing the whole branch?

[greysteil]

I took a quick look at this too and have no idea what was intended.

[deivid-rodriguez]

My guess was that it wanted to do if loaded_spec = Bundler.rubygems.loaded_specs("bundler")?

[colby-swandale]

ping @segiddins. Are you able to offer any insights?

[segiddins]

@deivid-rodriguez's guess is my guess too?

[deivid-rodriguez]

Hm, failed again. Maybe the branching should be removed entirely because it had not effect so far ...

Hei @voxik! Are you up for removing the whole branch?

[voxik]

I think the latest version might be the right fix in the end.

[voxik]

But I still can drop the condition entirely, if you prefer.

[deivid-rodriguez]

Given that the previous version of the code didn't make any sense, I think it might actually be dead code, or don't have any influence at all if it's actually run. So I'm curious about removing the branch and see if any tests fail.

[segiddins]

👍🏻

[voxik]

I don't think the test would fail if I removed the branch. I think that the branch is actually performance optimization which is not used ATM, so you can't really notice.

The code appears to add Bundler specification to the index. There is a chance that the Bunler spec is already loaded, that is what the fist branch trying to check. But if it is not loaded, then it tries to go through all available .gemspec files and load the proper specification to the memory, which might be slower.

OTOH if nobody noticed so far, then I should really just remove it ❓

[deivid-rodriguez]

You raise a good point. If we want to leave bundler running exactly as now, we should completely remove the branch. But if we apply the current patch, we get a potential speed up. So... not sure what's best... 🤷‍♂️.

[colby-swandale]

@bundlerbot r+

[deivid-rodriguez]

For what it's worth, this code was never correct, it landed to the repo as a dead branch, so it's never been run: 0f336c7#diff-51985a7d9ef96ac04e41b9878ea202c9R20.

Not saying we shouldn't do this, just sharing the history of that line of code :)

[ghost]

Build succeeded

• continuous-integration/travis-ci/push