Setup non-pushing gerrit instance for glibc.

  On 2019-10-25 10:48 a.m., Joseph Myers wrote:
> On Thu, 24 Oct 2019, Simon Marchi wrote:
> 
>> We'll maybe manage to do something a bit smarter, but I don't think we'll
>> easily be able to quote the _hunk_.  This is where the mail is generated:
> 
> I think the hunk - not just the new version of the code on its own - is 
> critical information if someone is commenting on a particular part of a 
> change, needed for such comments to be properly readable in context.

I understand that, I'm just saying I don't think I'll be able to do this change
(maybe an experienced Gerrit developer could).

>> Also, remember that you can go put a comment on an unchanged section on the
>> file (e.g. to say "you forgot to update this call"), so there would be no
>> diff hunk to show for that comment anyway.
> 
> In that case it should at least show a reasonable amount of context, with 
> some indication of what function it is in, like in the diff hunk header.

From my experience, git diff doesn't really get the function name, it just
gives you the first preceding line that starts with a non-whitespace.  It
often "breaks" when you have labels or preprocessor directives at column 0,
for example:

Since it's pretty basic, I could probably replicate that method to include a
few lines before the line of interest, as well as the first preceding line that
starts with a non-whitespace.


>> They are naturally related due to them being git commits and having a 
>> parent-child relationship.  If I check out C, I'll automatically check 
>> out its parent B and C. Gerrit doesn't see that as a series, just as 
>> individual changes that are dependent on one another.
> 
> There is a difference of *intent* between changes that depend on one 
> another and a patch series.  A patch series is saying:
> 
> * there is a common purpose motivating the patches; and

In Gerrit, you'd probably use topics to indicate that many changes have
a common purpose (whether they are interdependent or not).

https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics

> * some patches may best be understood by reference to ones later in the 
> series (if e.g. one patch adds an interface that a later one adds users 
> for), so the link to those later patches is important for review purposes.

For that second point, I believe that a preparatory patch should mention
in its commit message that it's adding something not used yet, with the
intent of using it in a later patch.  This way, somebody browsing the git
(without access to the cover letter) tree will understand why this particular
commit adds a function that's not used.  So that is clear to the reviewer
anyway.

As long as Gerrit lets you find those later patches by showing you the
relation chain, I think it's clear.

> And so a patch series should be sent out to the list for review as such.  
> There may be comments on the series as a whole, or on individual patches 
> in it.
> 
> There's also the variant of patch series that are explained in the cover 
> letter (or in other text not intended for the final commit) as being split 
> only for review purposes and intended to be squashed for commit, if the 
> pieces most convenient for review don't form bisectable commits.  I'm not 
> sure how much any review system needs to know about the distinction 
> between the two kinds of patch series if it's not pushing the commits 
> itself.

I don't think there would be a problem with that, except that the two emails
on the mailing would not be threaded together.  More thoughts about that
below.

>> Let's say I push a new version of C.  A and B are still at their v1, 
>> while C is at its v2.  Then, if I push D on top of that, D will be at 
>> its v1.
>>
>> Then, I (or even you!) could make a new change E on top of A, where E 
>> would be unrelated to B, C and D (other than sharing A in their 
>> ancestry).
>>
>> I can then decide to rebase A by itself (because master has moved on), 
>> which will create a v2 of A.  All the other changes will still be based 
>> on the v1 of A.  I will be able to rebase the other changes later on the 
>> latest version of A, if I want to.
> 
> All of this is perfectly meaningful for patch series - you can do [PATCHv2 
> 3/3] and then potentially [PATCH 4/3] for D if it's intended as part of 
> the series (or just a separate submission for D if it's not intended as 
> part of the series).  You can revise patch 1/3 with or without refreshing 
> the whole series.

I understand that we can do that with patch series.  My point is that I don't
see an easy way of translating changes from Gerrit's (current) model - i.e. git
branches - into a patch series model.

About cover letters: Sergio mentioned that a hacky way of doing a cover letter
would be to add an empty commit, whose commit message is the cover letter.  At
first I thought this commit would come before the actual commits.  But if we put
it at the end of the chain, maybe it works.  Let's say you have

  A: Preparatory patch to add function foo
  B: Preparatory patch to add function bar
  C: Big feature that uses foo and bar
  D: Cover letter that explains my series

With this relationship:

  A <- B <- C <- D

Then when you checkout out the cover letter, you check out the whole series.
If you modify C and push again, both C and D will be at v2.  So essentially,
the version of D kind of becomes the version of the series.

And when you actually push the series, you don't push D.

I understand that there is still the downside that if you are following only
from the mailing list, these mails are not threaded together.  Perhaps that
can be improved, like if I open a new change that depends on another change
that is still open (not merged yet, still up for review), then we send the
mail in reply to the mail of that other change.

Simon
  

On Fri, 25 Oct 2019, Simon Marchi wrote:

> > In that case it should at least show a reasonable amount of context, with 
> > some indication of what function it is in, like in the diff hunk header.
> 
> From my experience, git diff doesn't really get the function name, it just
> gives you the first preceding line that starts with a non-whitespace.  It
> often "breaks" when you have labels or preprocessor directives at column 0,
> for example:

Yes.  It's a reasonable heuristic that works in many common cases (and can 
be configured with a diff attribute in .gitattributes and a corresponding 
diff.<language>.xfuncname setting in .git/config, if desired).  Sometimes 
the default amount of context in a diff isn't enough either, and sometimes 
code is rearranged in a way that makes diff output unhelpful.

I'd like reasonable heuristics to be used in the gerrit messages so that 
what they show is sufficient in most cases for it to be possible to fully 
follow and contribute to the discussion via email without needing to go to 
the website.  Showing the diff hunk, with default settings (so the same 
logic as used by default by git diff to identify a function name) seems 
like a reasonable heuristic for that purpose.

> > There is a difference of *intent* between changes that depend on one 
> > another and a patch series.  A patch series is saying:
> > 
> > * there is a common purpose motivating the patches; and
> 
> In Gerrit, you'd probably use topics to indicate that many changes have
> a common purpose (whether they are interdependent or not).
> 
> https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics

If using a topic were to mark the patches as a patch series (including 
marking email subjects and threading them accordingly), that would be 
fine.  I don't think it's necessary for simply pushing multiple changes at 
once to be the way you cause something to be handled as a patch series, if 
topics are the idiomatic concept in gerrit that corresponds to a patch 
series as generated by git-format-patch.

> > There's also the variant of patch series that are explained in the cover 
> > letter (or in other text not intended for the final commit) as being split 
> > only for review purposes and intended to be squashed for commit, if the 
> > pieces most convenient for review don't form bisectable commits.  I'm not 
> > sure how much any review system needs to know about the distinction 
> > between the two kinds of patch series if it's not pushing the commits 
> > itself.
> 
> I don't think there would be a problem with that, except that the two emails
> on the mailing would not be threaded together.  More thoughts about that
> below.

Well, there would be issues if you wanted gerrit to push changes itself.  
And how does the "identify this as being the same change" system (for 
knowing when something has been committed) work if multiple separately 
reviewed patches are squashed into one commit?

(Is the valid syntax for Change-Ids documented anywhere?  In particular, 
is it OK to write a human-readable Change-Id oneself?)
  

On 2019-10-25 12:10 p.m., Joseph Myers wrote:
>>> There is a difference of *intent* between changes that depend on one 
>>> another and a patch series.  A patch series is saying:
>>>
>>> * there is a common purpose motivating the patches; and
>>
>> In Gerrit, you'd probably use topics to indicate that many changes have
>> a common purpose (whether they are interdependent or not).
>>
>> https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics
> 
> If using a topic were to mark the patches as a patch series (including 
> marking email subjects and threading them accordingly), that would be 
> fine.  I don't think it's necessary for simply pushing multiple changes at 
> once to be the way you cause something to be handled as a patch series, if 
> topics are the idiomatic concept in gerrit that corresponds to a patch 
> series as generated by git-format-patch.

I don't believe the topic is included in the email (at least not the subject),
but that is likely possible to do.  For example, including it in the prefix tag:

  [review my-topic-name]

I think that would help, but we would still be missing the information about
the order of the patches.  Any ideas for that?

>>> There's also the variant of patch series that are explained in the cover 
>>> letter (or in other text not intended for the final commit) as being split 
>>> only for review purposes and intended to be squashed for commit, if the 
>>> pieces most convenient for review don't form bisectable commits.  I'm not 
>>> sure how much any review system needs to know about the distinction 
>>> between the two kinds of patch series if it's not pushing the commits 
>>> itself.
>>
>> I don't think there would be a problem with that, except that the two emails
>> on the mailing would not be threaded together.  More thoughts about that
>> below.
> 
> Well, there would be issues if you wanted gerrit to push changes itself.  
> And how does the "identify this as being the same change" system (for 
> knowing when something has been committed) work if multiple separately 
> reviewed patches are squashed into one commit?

In that case, I would keep in the squashed commit the Change-Id of the first
commit (i.e. the commit that contains the logical change, not the generated
stuff).  When pushing that commit, it will auto-close the first change.  The
last/final version of that change would therefore be the full thing, including
the generated stuff.

I would then need to abandon the second commit.  In the reason field (when you
click Abandon) I would say that since the content has been squashed in the
parent change, which is now merged, this one is no longer needed.

> (Is the valid syntax for Change-Ids documented anywhere?  In particular, 
> is it OK to write a human-readable Change-Id oneself?)

It's not mentioned here [1] so I suppose it's not documented.  We would have
to look in the source code (or just try it) to see what's supported.

[1] https://gerrit-review.googlesource.com/Documentation/user-changeid.html

Simon