Re: Setup non-pushing gerrit instance for glibc.

[Carlos O'Donell <carlos@redhat.com>]

On 10/24/19 5:41 PM, Joseph Myers wrote:
> On Thu, 24 Oct 2019, Carlos O'Donell wrote:
> 
>> * Try to setup email based code review in gerrit.
>>   - Currently email is outbound only.
> 
> Observations on teething troubles with the initial setup:
> 
> 1. What's the status of fixing the problem with insufficient diff context 
> in emails when comments relate to particular parts of the diff?  They need 
> to quote the relevant amount of context (typically at least the whole diff 
> hunk, including the hunk header showing the changed function name) rather 
> than just one line and a reference to an external URL.  It's important for 
> messages with reviews to be meaningful in themselves without depending on 
> external links.  This is a longstanding problem (it was obvious in some 
> experiments some people did with proposing GCC patches with Rietveld, 
> gerrit's predecessor, several years ago).  Someone in the GDB discussions 
> mentioned a prototype patch to add some context to the emails, so maybe we 
> could use that patch.

I don't have any status on this change.

Sergio, Were you working on a patch like this to expand the diff context?

If I understand the problem correctly it's that:
~~~
https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/303/1//COMMIT_MSG@19 
PS1, Line 19: Signed-off-by: Carlos O'Donell <carlos@redhat.com>
We do not use DCO and Signed-off-by.
~~~
Is insufficient context. It shouldn't just be line 19, it should be the
whole hunk with the comment on the line it belongs to.

> 2. Could text comments in emails from gerrit be properly wrapped?  
> Messages such as 
> <https://sourceware.org/ml/libc-alpha/2019-10/msg00715.html> are hard to 
> read in the list archives because of very long lines.  (Of course, diff 
> context / quoted source lines should not be wrapped.)

I agree that should be fixed.

I also find it hard to read.

> 3. Could we document (on the wiki, I suppose) the process for setting up 
> git remotes if you want a git repository to get local copies of all 
> versions of all changes proposed this way?  My understanding is gerrit 
> makes those available as refs named in some particular way, so adding a 
> remote with appropriate fetch config should work, but the particular 
> recipe ought to be documented.

Yes, each gerrit review is a ref you can pull and review.

For example my localedef change can be fetched like this:

git fetch "https://gnutoolchain-gerrit.osci.io/r/glibc" refs/changes/03/303/1 && git checkout FETCH_HEAD

I expect you can pull all of refs/changes/* to get all the reviews locally.

We can try to work this out.

> 4. Could we document how to get and keep up to date a complete local copy 
> of all the glibc review data in gerrit (comments etc.) using whatever APIs 
> are available?  Again, I think the relevant APIs already exist, but how to 
> use them for glibc ought to be documented (this is especially the case for 
> a service like this that's experimental, and thus might go away in 
> future).

Agreed, we can try.

> 5. It would also be useful to have documentation for how someone should 
> make a patch series appear appropriately in gerrit if they want to propose 
> a series that way.  That means the emails for a patch series should 
> include 1/N, 2/N etc. in their subjects (with a 0/N cover letter as 
> appropriate).

Agreed, that will be a next step. I'm trying to get through just one patch
review right now to see how that works.

> 6. Lower priority, but note there are certain kinds of changes involving 
> huge diffs (e.g. to generated files) that thus *would* need a message size 
> limit and pointing to a URL for the diffs in that case, for it to be 
> possible to handle such changes through gerrit.  (When sending email 
> manually for such a change one can always omit the 50 MB of diffs to 
> generated files, but as I understand it part of the point of such a patch 
> review system is that the system seems the exact change proposed to be 
> committed, complete with generated files, so enabling automated testing of 
> patch proposals if desired.)

Correct, you'd need the full commit of 50MiB, but I don't know what happens
in that case with gerrit. I could try something that is MiB's in size, like
removing all the locale data.

Thanks for your feedback and list of things you'd like to see, it will take
time to sort through them.

-- 
Cheers,
Carlos.