This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: 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:

diff --git a/src/plugins/ctf/common/metadata/decoder.c b/src/plugins/ctf/common/metadata/decoder.c
index a62beebfcd99..5e4facb4ff27 100644
--- a/src/plugins/ctf/common/metadata/decoder.c
+++ b/src/plugins/ctf/common/metadata/decoder.c
@@ -351,6 +351,7 @@ end:
 				"mdec-addr=%p", mdec);
 		}
 	}
+	hello

 	free(buf);

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]