[patch] git-review doesn't parse scp-style git urls properly

[Alexander Jones]

We use git urls of the form "gerrit:projectName" which is equivalent to "ssh://gerrit/projectName" and works fine in git but not in git-review with e.g. -s.

Fix to follow

[Alexander Jones (alex-weej)]

Review open here: https://review.openstack.org/#/c/72751/

[Jeremy Stanley (fungi)]

Just to confirm, because it's still unclear to me the specific steps I should follow to reproduce this bug, do I need to have a preexisting remote named "gerrit" prior to running git-review on a project for the first time? Is this only an issue for projects which lack a .gitreview file containing the more explicit remote URL?

[Jeremy Stanley (fungi)]

Presumably I also need to add a host-specific ~/.ssh/config entry to point to the Gerrit API port number so git doesn't try to use the default SSH port?

[Jeremy Stanley (fungi)]

This does seem to be mitigated by having a .gitreview file, unless I'm missing something...

fungi@workstation:~/scratch$ cat ~/.ssh/config
Host gerrit
  HostName review.openstack.org
  Port 29418
fungi@workstation:~/scratch$ git clone gerrit:openstack-infra/zuul
Cloning into 'zuul'...
remote: Counting objects: 2610, done
remote: Finding sources: 100% (2610/2610)
remote: Total 2610 (delta 1232), reused 2047 (delta 1232)
Receiving objects: 100% (2610/2610), 2.07 MiB | 2.32 MiB/s, done.
Resolving deltas: 100% (1232/1232), done.
Checking connectivity... done.
fungi@workstation:~/scratch$ cd zuul/
fungi@workstation:~/scratch/zuul$ ls -l .git/hooks/commit-msg
ls: cannot access .git/hooks/commit-msg: No such file or directory
fungi@workstation:~/scratch/zuul$ cat .gitreview
[gerrit]
host=review.openstack.org
port=29418
project=openstack-infra/zuul.git
fungi@workstation:~/scratch/zuul$ git review -s
Creating a git remote called "gerrit" that maps to:
        ssh://<email address hidden>:29418/openstack-infra/zuul.git
fungi@workstation:~/zuul$ ls -l .git/hooks/commit-msg
-rwxr-xr-x 1 fungi fungi 4262 Mar 4 02:52 .git/hooks/commit-msg

[Alexander Jones (alex-weej)]

Our gerrit instance listens on port 22 precisely for ease of use. It's not uncommon for git servers to listen for SSH on port 22 in general, particularly on hosts with multiple IP addresses at their disposal.

The .gitreview file is duplicating the information in the ssh config, in your case, which I'm sure you'll agree is not the right thing to do. SCP will use the ssh config in the exact same way git does - git-review does not need to get in the way of this.

In any event, git-review has to deal more gracefully with this situation, and as I said in the review, the code to simply Do The Right Thing is almost as trivial.

[Yuriy Taraday (yorik-sar)]

The lase message seems to be related to bug https://bugs.launchpad.net/git-review/+bug/1075751 that covers default port selection for SSH.

[Alexander Jones (alex-weej)]

Yes, that patch looks like it had addressed this issue and added support for the port=None case I had proposed as a separate fix.

[Jeremy Stanley (fungi)]

Well, for us (the OpenStack community) the .gitreview file is not duplicating any sort of information. Most of our users don't ever clone directly from the Gerrit service itself since it's a Java-based resource hog and already under plenty of load from other activities. We have an entirely separate farm of replicated Git servers for that purpose along with actively synchronized mirrors from other popular third-party Git services. We also configure Gerrit to replicate a local mirror on its own filesystem and serve that from the same Apache which we use to front-end the Gerrit WebUI. Thus for our (OpenStack developer) users, the Gerrit SSH-based Git service is only ever a secondary remote which gets automatically added for them the first time they run git-review, based on the .gitreview file in the repository they already cloned from one of those many other convenient locations.

I'm still unconvinced that 22/tcp is the appropriate fallback assumption for git-review in most cases, since it's primarily a Gerrit client and Gerrit's default SSH-based API/Git service listening port is not 22/tcp. Administrators configuring Gerrit to listen on a non-standard (for Gerrit) port should provide some means of overriding the default to something appropriate to their environment if they intend for their developer community to use git-review to interact with their Gerrit (for example by specifying the proper URL in a .gitreview file in each project).

I don't think the proposed change is likely to cause any problems directly, but it does further reinforce workflows which git-review was not designed to support (using the internal Gerrit Git server as your origin, running Gerrit on a nonstandard port number without using a .gitreview file in your projects, et cetera) which could lead to further confusion in the user base, so I'd like to get more input from other git-review users on this. For the moment I'm marking the bug "opinion" and I'll ask around before we press forward with the patch.

[Alexander Jones (alex-weej)]

gerrit isn't a part of openstack, but git-review is the de facto git extension for gerrit, and I dare say that a significant, if not majority proportion of git-review users are not openstack users, so are unfazed by its conventions and deployment limitations.

I am not in a position to engage in a political war over this patch, so if the openstack community truly wishes to focus git-review entirely on openstack, then we will just have to maintain a fork instead, which is incredibly frustrating.

[Jeremy Stanley (fungi)]

I wasn't attempting to make any sort of political statement. I was merely pointing out that git-review already has features intended to make your use case work, but you are for some reason insistent on not using them. I also was not claiming that OpenStack's use of Gerrit was in any way typical, merely that we choose to deploy ours listening on its default-configured port and I expect *most* other Gerrit deployers do similarly. It's a default. In Gerrit. Making git-review, which is a Gerrit client, expect a fallback to an SSH Git service port which is *not* the same as Gerrit's default for that service seems (to me) incorrect. And git-review will even gladly check the .gitreview file in your project (if you add one) to find out what other port it should use instead if you use something which isn't Gerrit's default configuration. That's why I wished to solicit input from other communities who also use the tool heavily and usually keep an eye on our bug reports and code reviews (particularly Wikimedia Foundation, but there are others as well).

I don't mean for this to be frustrating, I'm just attempting to be fair and not overcomplicate a relatively simple tool which a lot of people already rely on in a variety of software development communities, without garnering at least some input from those communities. There is some evolving consensus among the OpenStack infrastructure team that trying to support too many divergent workflows will lead to confusion among users who are new to Git and Gerrit (and for that matter often Unix command-line tools in general). We've seen evidence of this sort of confusion already in the past. But I don't want to let our community alone make the decision that this particular workflow is not something the tool should support, if there are many others who feel strongly to the contrary.

[Jeremy Stanley (fungi)]

After some further discussion, I think I'm in favor of this mainly on the grounds that right now git-review does assume ssh://somehost/project gerrit remote URLs are likely preexisting entries it may not have added, and defaults to port 22 when there's no explicit port number.

I also agree with the comments in the review and the other related bug that forcing unspecified port numbers to 22 for SSH URLs breaks the ability to have ports specified for host entries in your SSH configuration and we should just not pass -P to the scp call at all in those cases, but am fine with that being a separate patch (and will likely write it myself if nobody beats me to it).

[Alexander Jones (alex-weej)]

Merged. Thanks all.

[Alexander Jones (alex-weej)]

For the record, the default port scenario was addressed by this patch.