Wow! Your change works but the logic is twisted because of the way the Lib and Base classes work (?) together. Not your fault. I think the division of labor between Git::Base and Git::Lib is not right. i.e. the abstraction is wrong.

One question I have is what is the purpose of the return result of Git::Lib#clone? It seems to be to return the opts needed to pass to Git::Base.new. If that is the case, maybe it would be best to add the logger there AND make it more explicit what the return value is intended for.

Here is my untested version of this change. I have elided the main body of Git::Lib.clone which would remain unchanged.

What do you think?

module Git
  class Base
    def self.clone(repository, name, opts = {})
      self.new(Git::Lib.new(nil, opts[:log]).clone(repository, name, opts))
    end
  end

  class Lib
    def clone(...)
      ...
      return_base_opts_from_clone(clone_dir, opts)
    end

    private:

    # Build the opts hash needed for Git::Base.new based on the clone command
    #
    def return_base_opts_from_clone(clone_dir, opts)
      base_opts = {}
      base_opts[:repository] = clone_dir if (opts[:bare] || opts[:mirror])
      base_opts[:working_directory] = clone_dir unless (opts[:bare] || opts[:mirror])
      base_opts[:log] = opts[:log] if opts[:log]
      base_opts
    end
  end
end

This makes it clear that there is an unhealthy circular dependency between Git::Base and Git::Lib. I think making it more explicit is a good step in the right direction.

One last thing. There should really be a test that shows that calling Git.clone with :log in options results in an object that has its logger set appropriately. I am happy to help with the test if needed.

@gigorok I have submitted PR #501 with the changes I suggested. Please give a +1 there if you like it.

Hey @jcouball
Your suggestion #501 will be better. I like it.
Thanks for your help!

Thanks @gigorok. Closing this PR in favor of PR #501.