Open Stack

On 2018-07-04 22:32:53 +0100 (+0100), Darragh Bailey wrote:
[...]
> Based on the comments at
> https://git.openstack.org/cgit/openstack-infra/git-review/tree/CONTRIBUTING.rst#n5,
> git-review is considered feature complete, and as a consequence it
> seems that reviewers have mostly moved onto other projects so it
> can take quite some time to get reviews. Perfectly understandable,
> everyone can only do so much and needs to pick something(s) to
> prioritise. However this is such a useful tool for working with
> Gerrit from the command line it seems to be the defacto git
> subcommand for interfacing with Gerrit that it seems a shame to
> limit it.

At one time git-review had started to suffer from scope creep and
was getting more random proposals for new features than actual
stability improvements. Its test coverage was, for quite a long
while, also sub-par so that some of the feature additions which did
get accepted introduced regressions that went unnoticed sometimes
for weeks or months before we'd discover they needed reverting. The
original authors intended for git-review to be primarily focused on
bootstrapping Gerrit connectivity from a cloned Git repository as
well as simplifying the basic Git commands for retrieving changes
from and pushing changes to a Gerrit. That update to the
CONTRIBUTING.rst file was intended to put the brakes on future scope
creep, especially in cases where an added feature would work just as
well as its own separate git subcommand.

> While I think there are a number of current reviews that would be
> beneficial to git-review, as well as some pieces that don't appear
> to be there currently, I'm reluctant to invest much time as it
> seems unlikely enhancements would be accepted due to the current
> state of feature complete. Instead of putting together various
> changes to see if they might be reviewed and accepted, hoping a
> chat about what paths might be available could save a bit of time.

I've tried to go in and approve changes from time to time, but in
all honesty the negativity I've received in the past when attempting
to push back on feature additions has caused me to deprioritize
reviewing more changes for it. I should probably just buck up and go
in with a (very polite) machete anyway.

> There are a couple of things that I would like to work towards:
> 
> * Change the tests to use a single gerrit with separate projects
> instead of separate instances (faster testing)

This seems reasonable if it doesn't introduce new races or odd test
interdependencies from the reduced fixture isolation. I really have
never been fond of the integration-testing-only model we ended up
with though. I originally recommended lower-level unit testing with
mocks for the Git and SSH interactions, but the one volunteer we got
to implement a testsuite chose to automate Gerrit installation so it
is what it is at the moment.

> * Allow the tests to run against multiple versions of Gerrit (ensure
> compatibility)

This seems reasonable. We should have been bumping the Gerrit
versions in the tests and/or running more jobs for different
releases of it but the way version selection was implemented would
need a bit of an overhaul to accommodate that.

> * Fix and land many of the changes making it easier to download
> changes, list changes ordered with their dependencies, stashing
> when downloading, etc

The change listing feature really seems increasingly out of place to
me, and most of the "fixes" I saw related to it were about
supporting more and more of Gerrit's query language and terminal
formatting. If we deprecated/removed that and recommended
interacting directly with Gerrit or alternative utilities for change
searches (there are a lot more options for this than there were back
when git-review was first written) all of those would become
unnecessary and the code would be simplified at the same time.

> * Have git-review auto configure refs/notes/review (assuming it's
> available) for fetching on setup (I find it very handy and I'm
> always forgetting to do this)

I could see this being in scope, as it fits with the Gerrit
connectivity bootstrapping mission. I too find the notes refs handy
but have a global configuration in my ~/.gitconfig which seems to do
the trick already so I'm curious to find out how git-review might
improve on that.

> And potentially controversially; support other workflows and
> options outside of the OpenStack workflow. Although maybe not
> directly, and still keeping the OpenStack one as the default.

I'd love to know what about git-review is focused on OpenStack's
workflow. We tried to make it as generic as possible. If there are
any OpenStack-specific features still lingering in there, we should
see about ripping them out as soon as is feasible. One that I'm
aware of is the default topic mangling based on commit message
parsing, which I've been wanting to eradicate for a while since
Gerrit now makes altering topics possible without needing to push a
new commit. For that matter, setting the topic based on the local
branch name could also get tossed while we're at it, and just keep
the -t option for directly specifying a change topic when people
really want to do it at time of upload.

> I think there are a couple of ways that could be achieved, but I
> can't see any of them working well without a decent amount of
> refactoring.
> 
> * Have git-review provide the APIs so that someone may define a
> git-review-<name> that can add their workflow
> 
> * Add support for additional behaviour to be defined with
> refs/meta/config of projects
> 
> * Allow extensions to be installed that allow additional options
> to be added to the git-review CLI and config file
> 
> That last one might require being able to specify the additional
> required plugins to be listed in .gitreview, and providing the
> documentation might be trickier?
> 
> Basically make it easier to add custom behaviour without it being
> builtin to git-review, and without needing to reimplement a whole
> load of functionality elsewhere. But I'm pretty sure that all
> requires a substantial rewrite.

I'd need some concrete use case examples. From my perspective,
git-review is already a plugin (by way of being a git subcommand) so
adding plugins to the plugin seems like a layer violation. The
examples I've seen in the past for adding new behaviors were things
which made more sense to me as new git subcommands. For a
counterexample, James Blair created git-restack not too long ago...
it could have been implemented as a git-review option, but was
sanely made to be its own distinct git subcommand instead.

> Thoughts? Is it worth putting a plan together around some of the
> initial changes? And then revisiting what would be needed to allow
> extensions around other workflows?

I'm all for plans to improve git-review's stability, test coverage
and, most of all, simplicity. Thanks for raising the topic!
-- 
Jeremy Stanley
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 963 bytes
Desc: not available
URL: <http://lists.openstack.org/pipermail/openstack-infra/attachments/20180705/217c8dc2/attachment.sig>