search

shanesmith / gerrit-cli

Gerrit in your command lines.
MIT License
46 stars 19 forks source link

Use default gerrit user if remoteUrl does not contain a username. #26

Closed cmccandless closed 7 years ago

cmccandless commented 7 years ago

Travis CI build failed because some tests are contradictory to the changes contained in this pull request. For example: an extra config field was added, so all tests that compare the output of gerrit config will fail.

shanesmith commented 7 years ago

thank you very much for your pull request! i think the ideas contained here do have merit but i have some thoughts:

  1. i don't think the remote name should be set at the gerrit config level, it's not something that would be widely used in my mind (ie: that a repo cloned from a certain gerrit config should have a certain remote name other than origin)

  2. I do like the idea of adding the ability to define a default remote other than origin for gerrit commands, seems to me like this would be a local git config (ie: for each repo) but there's currently no interface in gerrit-cli for such local configs... we could simply add a hidden config of gerrit.defaultRemote item that it would know to read, but i would like to make it more discoverable somehow...

  3. in which case does it happen where the remote url would not have a user but the gerrit config would? the remote url is built from the gerrit config... unless the remote url was specific manually, in which case could you not just add the missing user to the remote url?

cmccandless commented 7 years ago

@shanesmith

  1. I think you're probably right.

  2. I like the idea of gerrit.defaultRemote. As for discover-ability, perhaps gerrit-cli could set (but not display) this to "origin" upon initial configuration? That way, git config -l would reveal the setting, but the gerrit config user experience would remain unchanged.

  3. I will be unable to access my workstation for a couple weeks, and I cannot recall my configuration (or the steps I took to create it). I will take a look at it when I can and try to determine how remoteUrl was set on my system, but I do not recall setting it manually, and the value I had for it did not contain a username.

cmccandless commented 7 years ago

@shanesmith

I will take a look at it when I can and try to determine how remoteUrl was set on my system, but I do not recall setting it manually, and the value I had for it did not contain a username.

My particular setup involves a VM that was already configured to use Gerrit. My guess is that the creator of the VM set the remote URL without a username (for reasons unknown). You are right that setting the username manually fixes the issue in gerrit-cli, and this does not appear to have other side-effects on my system, so this change can be discarded.

I like the idea of gerrit.defaultRemote. As for discover-ability, perhaps gerrit-cli could set (but not display) this to "origin" upon initial configuration? That way, git config -l would reveal the setting, but the gerrit config user experience would remain unchanged.

What are your thoughts on this solution? If we can come to an agreement regarding a solution for a default remote, I propose the following:

In my fork:

  1. Discard both commits that are unique to my fork
  2. Apply agreed solution
  3. Either resubmit pull request, or simply leave this one for you to merge (or not) at your discretion.
shanesmith commented 7 years ago

I think that would be a great solution, and a new PR would be appreciated. =)