<div dir="auto"><br>Perhaps sending that email right before taking a few days off meaning I couldn't reply straight away wasn't the most helpful ;-)<br><div data-smartmail="gmail_signature" dir="auto"><br></div><br><div class="gmail_quote" dir="auto"><div dir="ltr">On Thu, 5 Jul 2018, 02:57 Jeremy Stanley, <<a href="mailto:fungi@yuggoth.org">fungi@yuggoth.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 2018-07-04 22:32:53 +0100 (+0100), Darragh Bailey wrote:<br>
[...]<br>
> Based on the comments at<br>
> <a href="https://git.openstack.org/cgit/openstack-infra/git-review/tree/CONTRIBUTING.rst#n5" rel="noreferrer noreferrer" target="_blank">https://git.openstack.org/cgit/openstack-infra/git-review/tree/CONTRIBUTING.rst#n5</a>,<br>
> git-review is considered feature complete, and as a consequence it<br>
> seems that reviewers have mostly moved onto other projects so it<br>
> can take quite some time to get reviews. Perfectly understandable,<br>
> everyone can only do so much and needs to pick something(s) to<br>
> prioritise. However this is such a useful tool for working with<br>
> Gerrit from the command line it seems to be the defacto git<br>
> subcommand for interfacing with Gerrit that it seems a shame to<br>
> limit it.<br>
<br>
At one time git-review had started to suffer from scope creep and<br>
was getting more random proposals for new features than actual<br>
stability improvements. Its test coverage was, for quite a long<br>
while, also sub-par so that some of the feature additions which did<br>
get accepted introduced regressions that went unnoticed sometimes<br>
for weeks or months before we'd discover they needed reverting. The<br>
original authors intended for git-review to be primarily focused on<br>
bootstrapping Gerrit connectivity from a cloned Git repository as<br>
well as simplifying the basic Git commands for retrieving changes<br>
from and pushing changes to a Gerrit. That update to the<br>
CONTRIBUTING.rst file was intended to put the brakes on future scope<br>
creep, especially in cases where an added feature would work just as<br>
well as its own separate git subcommand.<br></blockquote></div><div dir="auto"><br></div><div dir="auto">You've made one suggestion below on that applying for the list behaviour. Made me think that an etherpad listing the current outstanding or abandoned changes and discussing which would be better off outside of git-review would be a useful starting place.</div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> While I think there are a number of current reviews that would be<br>
> beneficial to git-review, as well as some pieces that don't appear<br>
> to be there currently, I'm reluctant to invest much time as it<br>
> seems unlikely enhancements would be accepted due to the current<br>
> state of feature complete. Instead of putting together various<br>
> changes to see if they might be reviewed and accepted, hoping a<br>
> chat about what paths might be available could save a bit of time.<br>
<br>
I've tried to go in and approve changes from time to time, but in<br>
all honesty the negativity I've received in the past when attempting<br>
to push back on feature additions has caused me to deprioritize<br>
reviewing more changes for it. I should probably just buck up and go<br>
in with a (very polite) machete anyway.<br></blockquote></div><div dir="auto"><br></div><div dir="auto">I think it's all due to needing to prioritise time spent and git-review has mostly done what's needed.</div><div dir="auto"><br></div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> There are a couple of things that I would like to work towards:<br>
> <br>
> * Change the tests to use a single gerrit with separate projects<br>
> instead of separate instances (faster testing)<br>
<br>
This seems reasonable if it doesn't introduce new races or odd test<br>
interdependencies from the reduced fixture isolation. I really have<br>
never been fond of the integration-testing-only model we ended up<br>
with though. I originally recommended lower-level unit testing with<br>
mocks for the Git and SSH interactions, but the one volunteer we got<br>
to implement a testsuite chose to automate Gerrit installation so it<br>
is what it is at the moment.<br></blockquote></div><div dir="auto"><br></div><div dir="auto">More mocks around ssh/http should work well, but I've found that it's not necessarily beneficial doing the same around git, as with some simple fixtures it can be tested very quickly.</div><div dir="auto"><br></div><div dir="auto">The existing tests are still useful, and changing to a single gerrit instance per runner would require some thought in the logging side (adding markers should be sufficient I think), but thetas probably the main issue.</div><div dir="auto"><br></div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> * Allow the tests to run against multiple versions of Gerrit (ensure<br>
> compatibility)<br>
<br>
This seems reasonable. We should have been bumping the Gerrit<br>
versions in the tests and/or running more jobs for different<br>
releases of it but the way version selection was implemented would<br>
need a bit of an overhaul to accommodate that.<br></blockquote></div><div dir="auto"><br></div><div dir="auto">I'm thinking also for compatibility across a few releases would be good as well.</div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> * Fix and land many of the changes making it easier to download<br>
> changes, list changes ordered with their dependencies, stashing<br>
> when downloading, etc<br>
<br>
The change listing feature really seems increasingly out of place to<br>
me, and most of the "fixes" I saw related to it were about<br>
supporting more and more of Gerrit's query language and terminal<br>
formatting. If we deprecated/removed that and recommended<br>
interacting directly with Gerrit or alternative utilities for change<br>
searches (there are a lot more options for this than there were back<br>
when git-review was first written) all of those would become<br>
unnecessary and the code would be simplified at the same time.<br></blockquote></div><div dir="auto"><br></div><div dir="auto">That's interesting, I'd consider the ability to query for what is available for review a step before downloading a change for review, and understanding that it might be bringing multiple chances down that aren't merged useful.</div><div dir="auto"><br></div><div dir="auto">If this was moved out of git-review, I suspect it might still need to know a bit about git-review and be able to use some of its configuration.</div><div dir="auto"><br></div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> * Have git-review auto configure refs/notes/review (assuming it's<br>
> available) for fetching on setup (I find it very handy and I'm<br>
> always forgetting to do this)<br>
<br>
I could see this being in scope, as it fits with the Gerrit<br>
connectivity bootstrapping mission. I too find the notes refs handy<br>
but have a global configuration in my ~/.gitconfig which seems to do<br>
the trick already so I'm curious to find out how git-review might<br>
improve on that.<br></blockquote></div><div dir="auto"><br></div><div dir="auto">I didn't think that this could be done for the 'origin' remote and would be ignored by fit for other projects where it doesn't exist? Or are you using the 'gerrit' remote?</div><div dir="auto"><br></div><div dir="auto">But to the main advantage is that it opens this up to many people that might not have been aware it exists. And also opens up to asking the user if they'd like the review notes displayed along with the logs by default for this repo by setting core.notesRef.</div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> And potentially controversially; support other workflows and<br>
> options outside of the OpenStack workflow. Although maybe not<br>
> directly, and still keeping the OpenStack one as the default.<br>
<br>
I'd love to know what about git-review is focused on OpenStack's<br>
workflow. We tried to make it as generic as possible. If there are<br>
any OpenStack-specific features still lingering in there, we should<br>
see about ripping them out as soon as is feasible. One that I'm<br>
aware of is the default topic mangling based on commit message<br>
parsing, which I've been wanting to eradicate for a while since<br>
Gerrit now makes altering topics possible without needing to push a<br>
new commit. For that matter, setting the topic based on the local<br>
branch name could also get tossed while we're at it, and just keep<br>
the -t option for directly specifying a change topic when people<br>
really want to do it at time of upload.<br></blockquote></div><div dir="auto"><br></div><div dir="auto">There may be others, I recall this coming up before around being able to set review scores for labels at the same time as uploading the change. Think it was around the 'Workflow' label. Maybe this is a case for a different command, but it seems likely to break the flow doe anyone using git-review to submit. However the labels for each project are customizable so it seems likely this would need the correct set to be worked out at setup time if included.</div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> I think there are a couple of ways that could be achieved, but I<br>
> can't see any of them working well without a decent amount of<br>
> refactoring.<br>
> <br>
> * Have git-review provide the APIs so that someone may define a<br>
> git-review-<name> that can add their workflow<br>
> <br>
> * Add support for additional behaviour to be defined with<br>
> refs/meta/config of projects<br>
> <br>
> * Allow extensions to be installed that allow additional options<br>
> to be added to the git-review CLI and config file<br>
> <br>
> That last one might require being able to specify the additional<br>
> required plugins to be listed in .gitreview, and providing the<br>
> documentation might be trickier?<br>
> <br>
> Basically make it easier to add custom behaviour without it being<br>
> builtin to git-review, and without needing to reimplement a whole<br>
> load of functionality elsewhere. But I'm pretty sure that all<br>
> requires a substantial rewrite.<br>
<br>
I'd need some concrete use case examples. From my perspective,<br>
git-review is already a plugin (by way of being a git subcommand) so<br>
adding plugins to the plugin seems like a layer violation. The<br>
examples I've seen in the past for adding new behaviors were things<br>
which made more sense to me as new git subcommands. For a<br>
counterexample, James Blair created git-restack not too long ago...<br>
it could have been implemented as a git-review option, but was<br>
sanely made to be its own distinct git subcommand instead.<br></blockquote></div><div dir="auto"><br></div><div dir="auto">Maybe the list one suggested to be outside of git-review. I suspect it'd still want to be connected to it in some way.</div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> Thoughts? Is it worth putting a plan together around some of the<br>
> initial changes? And then revisiting what would be needed to allow<br>
> extensions around other workflows?<br>
<br>
I'm all for plans to improve git-review's stability, test coverage<br>
and, most of all, simplicity. Thanks for raising the topic!<br>
-- <br>
Jeremy Stanley</blockquote></div><div dir="auto"><br></div><div dir="auto">Typing this all on a mobile so might have missed a few things and will follow up after the weekend.</div><div dir="auto"><br></div><div dir="auto"><br></div><div class="gmail_quote" dir="auto"></div><div dir="auto">--</div><div dir="auto"><span style="font-family:sans-serif">Darragh Bailey</span><br style="font-family:sans-serif"><span style="font-family:sans-serif">"Nothing is foolproof to a sufficiently talented fool" - unknown</span><br></div><div class="gmail_quote" dir="auto"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"></blockquote></div></div>