[PATCH] gdb: remove unused includes from dwarf2read.c

[PATCH] gdb: remove unused includes from dwarf2read.c

[Simon Marchi]

include-what-you-use says:

../../../src/binutils-gdb/gdb/dwarf2read.c should remove these lines:
- #include <ctype.h>  // lines 67-67
- #include <sys/stat.h>  // lines 59-59
- #include <sys/types.h>  // lines 83-83
- #include <cmath>  // lines 88-88
- #include <forward_list>  // lines 90-90
- #include <set>  // lines 89-89
- #include <unordered_set>  // lines 85-85
- #include "completer.h"  // lines 60-60
- #include "expression.h"  // lines 44-44
- #include "gdbsupport/byte-vector.h"  // lines 78-78
- #include "gdbsupport/filestuff.h"  // lines 71-71
- #include "gdbsupport/gdb_unlinker.h"  // lines 74-74

After a quick glance, that makes sense, so this patch removes them.

Change-Id: I13cfcb2f1d747144fddba7f66b329630b79dae90
---
 gdb/dwarf2read.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index ee9df34ed27c..5d072f569ec5 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -41,7 +41,6 @@
 #include "buildsym.h"
 #include "demangle.h"
 #include "gdb-demangle.h"
-#include "expression.h"
 #include "filenames.h"	/* for DOSish file names */
 #include "macrotab.h"
 #include "language.h"
@@ -56,38 +55,27 @@
 #include "addrmap.h"
 #include "typeprint.h"
 #include "psympriv.h"
-#include <sys/stat.h>
-#include "completer.h"
 #include "gdbsupport/vec.h"
 #include "c-lang.h"
 #include "go-lang.h"
 #include "valprint.h"
 #include "gdbcore.h" /* for gnutarget */
 #include "gdb/gdb-index.h"
-#include <ctype.h>
 #include "gdb_bfd.h"
 #include "f-lang.h"
 #include "source.h"
-#include "gdbsupport/filestuff.h"
 #include "build-id.h"
 #include "namespace.h"
-#include "gdbsupport/gdb_unlinker.h"
 #include "gdbsupport/function-view.h"
 #include "gdbsupport/gdb_optional.h"
 #include "gdbsupport/underlying.h"
-#include "gdbsupport/byte-vector.h"
 #include "gdbsupport/hash_enum.h"
 #include "filename-seen-cache.h"
 #include "producer.h"
 #include <fcntl.h>
-#include <sys/types.h>
 #include <algorithm>
-#include <unordered_set>
 #include <unordered_map>
 #include "gdbsupport/selftest.h"
-#include <cmath>
-#include <set>
-#include <forward_list>
 #include "rust-lang.h"
 #include "gdbsupport/pathstuff.h"
 
-- 
2.23.0

Re: [PATCH] gdb: remove unused includes from dwarf2read.c

[Simon Marchi]

On 2019-10-13 00:52, Simon Marchi wrote:
> include-what-you-use says:
> 
> ../../../src/binutils-gdb/gdb/dwarf2read.c should remove these lines:
> - #include <ctype.h>  // lines 67-67
> - #include <sys/stat.h>  // lines 59-59
> - #include <sys/types.h>  // lines 83-83
> - #include <cmath>  // lines 88-88
> - #include <forward_list>  // lines 90-90
> - #include <set>  // lines 89-89
> - #include <unordered_set>  // lines 85-85
> - #include "completer.h"  // lines 60-60
> - #include "expression.h"  // lines 44-44
> - #include "gdbsupport/byte-vector.h"  // lines 78-78
> - #include "gdbsupport/filestuff.h"  // lines 71-71
> - #include "gdbsupport/gdb_unlinker.h"  // lines 74-74
> 
> After a quick glance, that makes sense, so this patch removes them.
> 
> Change-Id: I13cfcb2f1d747144fddba7f66b329630b79dae90

I pushed it after Tom de Vries reviewed it on Gerrit:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/24

Simon

Re: [PATCH] gdb: remove unused includes from dwarf2read.c

[Eli Zaretskii]

> Date: Mon, 14 Oct 2019 10:21:50 -0400
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> I pushed it after Tom de Vries reviewed it on Gerrit:
> 
> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/24

Does this mean that now we need to look for patches in two places?
Can Gerrit be set up to forward the review comments to the list?

Also, if some of us decides to do the review on Gerrit, does it mean
all the others need to do that as well?

Re: [PATCH] gdb: remove unused includes from dwarf2read.c

[Simon Marchi]

On 2019-10-14 10:38, Eli Zaretskii wrote:
>> Date: Mon, 14 Oct 2019 10:21:50 -0400
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> 
>> I pushed it after Tom de Vries reviewed it on Gerrit:
>> 
>> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/24
> 
> Does this mean that now we need to look for patches in two places?
> Can Gerrit be set up to forward the review comments to the list?
> 
> Also, if some of us decides to do the review on Gerrit, does it mean
> all the others need to do that as well?

Hi Eli,

Yes, Gerrit can be set to send notifications to an arbitrary email, so 
we can set it to send them to gdb-patches.  We have talked about that 
earlier, it's just not done yet.  I'll look into it right now.

As long as we use Gerrit and mail patches in parallel, people are free 
to send patches using the system they prefer.  I think it's simpler if 
reviewers use the system that was chosen by the patch author (reply on 
Gerrit if the patch is on Gerrit, reply by email if the patch is by 
email).

In theory, it is possible to reply to some of Gerrit's email 
notifications:

https://gerrit-review.googlesource.com/Documentation/intro-user.html#reply-by-email

But our server isn't configured to receive emails for the moment, so 
that won't work.  Reading this page, I'm not sure if responding to a 
"New change" notification would work, we'd have to try it.

Simon

Re: Gerrit (was: [PATCH] gdb: remove unused includes from dwarf2read.c)

[Eli Zaretskii]

> Date: Mon, 14 Oct 2019 11:18:36 -0400
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: gdb-patches@sourceware.org
> 
> > Does this mean that now we need to look for patches in two places?
> > Can Gerrit be set up to forward the review comments to the list?
> > 
> > Also, if some of us decides to do the review on Gerrit, does it mean
> > all the others need to do that as well?
> 
> Hi Eli,
> 
> Yes, Gerrit can be set to send notifications to an arbitrary email, so 
> we can set it to send them to gdb-patches.  We have talked about that 
> earlier, it's just not done yet.  I'll look into it right now.

I see some emails from Gerrit, does it mean you already set that up?
Because those emails leave a lot to be desired, IMO.

Anyway, seeing the beginning of a patch was the only way for me to
know that a patch needs me to review the documentation parts.  Now I
wonder how I can do that when the patch is posted on Gerrit.

> As long as we use Gerrit and mail patches in parallel, people are free 
> to send patches using the system they prefer.  I think it's simpler if 
> reviewers use the system that was chosen by the patch author (reply on 
> Gerrit if the patch is on Gerrit, reply by email if the patch is by 
> email).

But I cannot reply on Gerrit without registering there, can I?

I'm also somewhat bothered by what I've read on the wiki.  I
understand that anyone can register on Gerrit, and after that push
patches for review, independently of their write access to the
sourceware repository.  Then, if gnutoolchain-gerrit.osci.io is
associated with or operated by FSF/GNU, it would mean we provide a way
for random people to push changes to GDB to a public repository
affiliated with us, without having any control on what is being pushed
ahead of the push.  Suppose someone pushes there changes that violate
the GPL, or do something else that is against the GNU policies --
wouldn't that appear as if we are "authorizing" those just by having
that code in the repository, even though it's on a branch and haven't
yet been admitted to sourceware?

Thanks.

Re: Gerrit

[Simon Marchi]

On 2019-10-14 13:12, Eli Zaretskii wrote:
> I see some emails from Gerrit, does it mean you already set that up?
> Because those emails leave a lot to be desired, IMO.

Yes, I just did.

The content of the emails is fully configurable. We can work on 
improving them if you have specific pain points or suggestions.

The page here describes which emails are sent, and what variable 
information about each change is available to include in the email:

https://gerrit-review.googlesource.com/Documentation/config-mail.html

> Anyway, seeing the beginning of a patch was the only way for me to
> know that a patch needs me to review the documentation parts.  Now I
> wonder how I can do that when the patch is posted on Gerrit.

What do you mean by "beginning of a patch", do you mean the diff stat 
that shows the changed files? If so, I believe this information is 
available in the notification sent for a new change.

This patch, for example:

   https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/29

resulted in this message being sent:

  https://sourceware.org/ml/gdb-patches/2019-10/msg00371.html

And it contains:

   M gdb/block.c
   M gdb/testsuite/gdb.dwarf2/varval.exp
   2 files changed, 55 insertions(+), 3 deletions(-)

Does that help?

>> As long as we use Gerrit and mail patches in parallel, people are free
>> to send patches using the system they prefer.  I think it's simpler if
>> reviewers use the system that was chosen by the patch author (reply on
>> Gerrit if the patch is on Gerrit, reply by email if the patch is by
>> email).
> 
> But I cannot reply on Gerrit without registering there, can I?

I can only guess, but probably not.

> I'm also somewhat bothered by what I've read on the wiki.  I
> understand that anyone can register on Gerrit, and after that push
> patches for review, independently of their write access to the
> sourceware repository.  Then, if gnutoolchain-gerrit.osci.io is
> associated with or operated by FSF/GNU, it would mean we provide a way
> for random people to push changes to GDB to a public repository
> affiliated with us, without having any control on what is being pushed
> ahead of the push.  Suppose someone pushes there changes that violate
> the GPL, or do something else that is against the GNU policies --
> wouldn't that appear as if we are "authorizing" those just by having
> that code in the repository, even though it's on a branch and haven't
> yet been admitted to sourceware?

That goes into lawyer territory, so I can't give a definitive answer.  
The way I see it is that it's not really different than that person 
posting a patch with the same content on the mailing list.  It's the 
same content, just a different format.

Simon

Re: Gerrit

[Eli Zaretskii]

> Date: Mon, 14 Oct 2019 13:31:28 -0400
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: gdb-patches@sourceware.org
> 
> On 2019-10-14 13:12, Eli Zaretskii wrote:
> > I see some emails from Gerrit, does it mean you already set that up?
> > Because those emails leave a lot to be desired, IMO.
> 
> Yes, I just did.

Thanks.

> The content of the emails is fully configurable. We can work on 
> improving them if you have specific pain points or suggestions.

It is too wordy in some parts, and in others don't say enough.  For
example, a part like this:

> Tom de Vries has posted comments on this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/28 )
> 
> Change subject: [gdb] Only force INTERP_CONSOLE ui_out for breakpoint commands in MI mode

is completely redundant (and long lines make it look ugly).

The epilogue, viz.:

> Gerrit-Project: binutils-gdb
> Gerrit-Branch: master
> Gerrit-Change-Id: Id1771e7fcc9496a7d97ec2b2ea6b1487596f1ef7
> Gerrit-Change-Number: 28
> Gerrit-PatchSet: 1
> Gerrit-Owner: Tom de Vries <tdevries@suse.de>
> Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
> Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
> Gerrit-Comment-Date: Mon, 14 Oct 2019 15:45:58 +0000
> Gerrit-HasComments: No
> Gerrit-Has-Labels: No
> Gerrit-MessageType: comment

seems also mostly useless.

OTOH, a message such as this:

> Patch Set 2:
> 
> This change is ready for review.

doesn't show the patch set at all, so it's also unhelpful.

> > Anyway, seeing the beginning of a patch was the only way for me to
> > know that a patch needs me to review the documentation parts.  Now I
> > wonder how I can do that when the patch is posted on Gerrit.
> 
> What do you mean by "beginning of a patch", do you mean the diff stat 
> that shows the changed files? If so, I believe this information is 
> available in the notification sent for a new change.

Not only the difstat part, but also the ChangeLog part.

>    M gdb/block.c
>    M gdb/testsuite/gdb.dwarf2/varval.exp
>    2 files changed, 55 insertions(+), 3 deletions(-)
> 
> Does that help?

I'd prefer to see the whole commit log message with the rationale
etc., it sometimes touches subjects where I'd like to voice an
opinion, even if that's not only about documentation.

> > I'm also somewhat bothered by what I've read on the wiki.  I
> > understand that anyone can register on Gerrit, and after that push
> > patches for review, independently of their write access to the
> > sourceware repository.  Then, if gnutoolchain-gerrit.osci.io is
> > associated with or operated by FSF/GNU, it would mean we provide a way
> > for random people to push changes to GDB to a public repository
> > affiliated with us, without having any control on what is being pushed
> > ahead of the push.  Suppose someone pushes there changes that violate
> > the GPL, or do something else that is against the GNU policies --
> > wouldn't that appear as if we are "authorizing" those just by having
> > that code in the repository, even though it's on a branch and haven't
> > yet been admitted to sourceware?
> 
> That goes into lawyer territory, so I can't give a definitive answer.  

Maybe we should ask a lwayer, then.

> The way I see it is that it's not really different than that person 
> posting a patch with the same content on the mailing list.  It's the 
> same content, just a different format.

Not exactly: having the code in a branch of our repository (again,
assuming it can be regarded as "ours") means we are redistributing it,
whereas having it in an email does not.

Re: Gerrit

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> The way I see it is that it's not really different than that person 
>> posting a patch with the same content on the mailing list.  It's the 
>> same content, just a different format.

Eli> Not exactly: having the code in a branch of our repository (again,
Eli> assuming it can be regarded as "ours") means we are redistributing it,
Eli> whereas having it in an email does not.

But the email is all archived, so I still don't see the distinction.

Tom

Re: Gerrit

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Cc: Simon Marchi <simon.marchi@polymtl.ca>,  gdb-patches@sourceware.org
> Date: Mon, 14 Oct 2019 12:03:32 -0600
> 
> >>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> The way I see it is that it's not really different than that person 
> >> posting a patch with the same content on the mailing list.  It's the 
> >> same content, just a different format.
> 
> Eli> Not exactly: having the code in a branch of our repository (again,
> Eli> assuming it can be regarded as "ours") means we are redistributing it,
> Eli> whereas having it in an email does not.
> 
> But the email is all archived, so I still don't see the distinction.

Anyone can send email to us, and we cannot be responsible for what
they send.  By contrast, having it in our repository is tantamount to
redistributing it, because repositories are nowadays treated as a
means to distribute code (and some projects, like Gnulib, have no
other means).

Re: Gerrit

[Simon Marchi]

On 2019-10-14 2:31 p.m., Eli Zaretskii wrote:
>> From: Tom Tromey <tom@tromey.com>
>> Cc: Simon Marchi <simon.marchi@polymtl.ca>,  gdb-patches@sourceware.org
>> Date: Mon, 14 Oct 2019 12:03:32 -0600
>>
>>>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> The way I see it is that it's not really different than that person 
>>>> posting a patch with the same content on the mailing list.  It's the 
>>>> same content, just a different format.
>>
>> Eli> Not exactly: having the code in a branch of our repository (again,
>> Eli> assuming it can be regarded as "ours") means we are redistributing it,
>> Eli> whereas having it in an email does not.
>>
>> But the email is all archived, so I still don't see the distinction.
> 
> Anyone can send email to us, and we cannot be responsible for what
> they send.  By contrast, having it in our repository is tantamount to
> redistributing it, because repositories are nowadays treated as a
> means to distribute code (and some projects, like Gnulib, have no
> other means).

I'm not to sure how to continue this conversation.  What can we do to
settle this?

In the mean time, I think it's fairly low risk to carry on with the
project: we are not different from the thousands of public projects
using a git-based review system, and I have never heard of this
ever being an issue.  Of course, we need to be as vigilant as today
about what we merge in the master branch, since that will be what the
GNU project releases.

Simon

Re: Gerrit

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Mon, 14 Oct 2019 21:33:14 -0400
> 
> > Anyone can send email to us, and we cannot be responsible for what
> > they send.  By contrast, having it in our repository is tantamount to
> > redistributing it, because repositories are nowadays treated as a
> > means to distribute code (and some projects, like Gnulib, have no
> > other means).
> 
> I'm not to sure how to continue this conversation.  What can we do to
> settle this?

What is the nature of the affiliation of the site where we use Gerrit
with the GDB project?  If the affiliation is lose enough, maybe this
is not a problem for us.

Re: Gerrit

[Sergio Durigan Junior]

On Tuesday, October 15 2019, Eli Zaretskii wrote:

>> Cc: gdb-patches@sourceware.org
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>> Date: Mon, 14 Oct 2019 21:33:14 -0400
>> 
>> > Anyone can send email to us, and we cannot be responsible for what
>> > they send.  By contrast, having it in our repository is tantamount to
>> > redistributing it, because repositories are nowadays treated as a
>> > means to distribute code (and some projects, like Gnulib, have no
>> > other means).
>> 
>> I'm not to sure how to continue this conversation.  What can we do to
>> settle this?
>
> What is the nature of the affiliation of the site where we use Gerrit
> with the GDB project?  If the affiliation is lose enough, maybe this
> is not a problem for us.

There is no official affiliation, I think.  The instance was set up by
Simon and I, with the help of the OSCI folks, who provides the
infrastructure.

I see the gerrit service just like I see the Buildbot service: they are
both useful for the community, but they are not backed up by the FSF/GNU
in any way.

Obligatory: IANAL.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Gerrit

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Simon Marchi <simon.marchi@polymtl.ca>,  tom@tromey.com,  gdb-patches@sourceware.org
> Date: Tue, 15 Oct 2019 11:57:54 -0400
> 
> > What is the nature of the affiliation of the site where we use Gerrit
> > with the GDB project?  If the affiliation is lose enough, maybe this
> > is not a problem for us.
> 
> There is no official affiliation, I think.  The instance was set up by
> Simon and I, with the help of the OSCI folks, who provides the
> infrastructure.

Who are the OSCI folks, and why do they provide the infrastructure you
needed for setting up this service?

Re: Gerrit

[Sergio Durigan Junior]

On Tuesday, October 15 2019, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Simon Marchi <simon.marchi@polymtl.ca>,  tom@tromey.com,  gdb-patches@sourceware.org
>> Date: Tue, 15 Oct 2019 11:57:54 -0400
>> 
>> > What is the nature of the affiliation of the site where we use Gerrit
>> > with the GDB project?  If the affiliation is lose enough, maybe this
>> > is not a problem for us.
>> 
>> There is no official affiliation, I think.  The instance was set up by
>> Simon and I, with the help of the OSCI folks, who provides the
>> infrastructure.
>
> Who are the OSCI folks, and why do they provide the infrastructure you
> needed for setting up this service?

https://osci.io/

They provide the infrastructure because I asked them if they could.

My initial plan was to host gerrit on sourceware, but after a bit of
discussion on #overseers, I realized that it would be hard to run the
latest version of gerrit on the machine.  Moreover, unfortunately gerrit
doesn't handle user registration by itself, so that meant that I'd have
to set up something for that as well (we all have users on sourceware,
but they don't have passwords associated with them).

The current plan is to run gerrit on the OSCI infrastructure for a
while, and see if the GDB community likes it.  If we do, and if we
decide we want to make gerrit our official patch review system, then we
will eventually migrate our instance to sourceware.  When that time
comes, we should probably resurrect this conversation, because then the
repository will be hosted in an "official" resource.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Gerrit

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: simon.marchi@polymtl.ca,  tom@tromey.com,  gdb-patches@sourceware.org
> Date: Tue, 15 Oct 2019 13:09:53 -0400
> 
> The current plan is to run gerrit on the OSCI infrastructure for a
> while, and see if the GDB community likes it.  If we do, and if we
> decide we want to make gerrit our official patch review system, then we
> will eventually migrate our instance to sourceware.  When that time
> comes, we should probably resurrect this conversation, because then the
> repository will be hosted in an "official" resource.

Right.  Also, if the current service will have a "master" branch, or
something that would automatically push to our official branches on
sourceware.

Thanks.

Re: Gerrit

[Sergio Durigan Junior]

On Tuesday, October 15 2019, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: simon.marchi@polymtl.ca,  tom@tromey.com,  gdb-patches@sourceware.org
>> Date: Tue, 15 Oct 2019 13:09:53 -0400
>> 
>> The current plan is to run gerrit on the OSCI infrastructure for a
>> while, and see if the GDB community likes it.  If we do, and if we
>> decide we want to make gerrit our official patch review system, then we
>> will eventually migrate our instance to sourceware.  When that time
>> comes, we should probably resurrect this conversation, because then the
>> repository will be hosted in an "official" resource.
>
> Right.  Also, if the current service will have a "master" branch, or
> something that would automatically push to our official branches on
> sourceware.

Exactly.  The current service has a read-only master branch; only our
"Sourceware to gerrit sync robot" can push to it.  We will most likely
never change this until we migrate to sourceware.

When/if we move to sourceware, we will lift this restriction, and the
official binutils-gdb.git repository will actually be managed by gerrit.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: Gerrit

[Simon Marchi]

On 2019-10-14 1:55 p.m., Eli Zaretskii wrote:
>> The content of the emails is fully configurable. We can work on 
>> improving them if you have specific pain points or suggestions.
> 
> It is too wordy in some parts, and in others don't say enough.  For
> example, a part like this:
> 
>> Tom de Vries has posted comments on this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/28 )
>>
>> Change subject: [gdb] Only force INTERP_CONSOLE ui_out for breakpoint commands in MI mode
> 
> is completely redundant (and long lines make it look ugly).

Ok, I have removed the "Change subject" line in many of the email templates.  From what I
understand, it is indeed redundant with the email subject.  I also removed some not so
important text to be more straight to the point.

I have put the URL on a separate line to avoid having this too long line.

Here's the result:

> The epilogue, viz.:
> 
>> Gerrit-Project: binutils-gdb
>> Gerrit-Branch: master
>> Gerrit-Change-Id: Id1771e7fcc9496a7d97ec2b2ea6b1487596f1ef7
>> Gerrit-Change-Number: 28
>> Gerrit-PatchSet: 1
>> Gerrit-Owner: Tom de Vries <tdevries@suse.de>
>> Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
>> Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
>> Gerrit-Comment-Date: Mon, 14 Oct 2019 15:45:58 +0000
>> Gerrit-HasComments: No
>> Gerrit-Has-Labels: No
>> Gerrit-MessageType: comment
> 
> seems also mostly useless.

I guess these can be used by people to write email filters or scripts.  But we also find almost
the same information as headers in the email:

...
X-Gerrit-Change-Id: I806ef0851c43ead90b545a11794e41f5e5178436
X-Gerrit-Change-Number: 25
X-Gerrit-ChangeURL: <https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25>
...

So I removed them.

So here's the result for a notification of type "comment" now:

https://sourceware.org/ml/gdb-patches/2019-10/msg00388.html

> OTOH, a message such as this:
> 
>> Patch Set 2:
>>
>> This change is ready for review.
> 
> doesn't show the patch set at all, so it's also unhelpful.

Indeed, this looks like (I can't find it) a notification of type "comment",
sent when somebody writes a comment about a patch set.  This type doesn't
include the full diff of the patch, and I don't think it would help to do so.

In that example, Tom has replied to the patch set 2 (i.e. version 2) of
that change.  Normally, you'd have received the diff in a previous email
in the same thread, the one where he uploaded this patch set 2.  Here, it's
possible you don't have it because we didn't have the notifications yet when
he did.

When somebody leaves a comment attached to a given line of the patch
(which you do through the web UI), the notification will include that
line above the comment.  I'd like if it was possible to include a slightly
larger context, say the 5 lines above that line too, like one would typically
do if replying by email.  But this is a bit more advanced, I don't know if
it's possible yet.

>>> Anyway, seeing the beginning of a patch was the only way for me to
>>> know that a patch needs me to review the documentation parts.  Now I
>>> wonder how I can do that when the patch is posted on Gerrit.
>>
>> What do you mean by "beginning of a patch", do you mean the diff stat 
>> that shows the changed files? If so, I believe this information is 
>> available in the notification sent for a new change.
> 
> Not only the difstat part, but also the ChangeLog part.
> 
>>    M gdb/block.c
>>    M gdb/testsuite/gdb.dwarf2/varval.exp
>>    2 files changed, 55 insertions(+), 3 deletions(-)
>>
>> Does that help?
> 
> I'd prefer to see the whole commit log message with the rationale
> etc., it sometimes touches subjects where I'd like to voice an
> opinion, even if that's not only about documentation.

The commit log is shown in the notifications sent when a user uploads a new patch
or a new version of a patch.  If they have put the ChangeLog entry in the commit log,
you should therefore see it there.  From this point of view, this is exactly like
regular email patches.

Simon

Re: [PATCH] gdb: remove unused includes from dwarf2read.c

[Luis Machado]

On 10/14/19 11:38 AM, Eli Zaretskii wrote:
>> Date: Mon, 14 Oct 2019 10:21:50 -0400
>> From: Simon Marchi <simon.marchi@polymtl.ca>
>>
>> I pushed it after Tom de Vries reviewed it on Gerrit:
>>
>> https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/24
> 
> Does this mean that now we need to look for patches in two places?
> Can Gerrit be set up to forward the review comments to the list?
> 
> Also, if some of us decides to do the review on Gerrit, does it mean
> all the others need to do that as well?
> 

The transition to some better (restrictions may apply) patch reviewing 
system is a good thing, but i agree we should think further about it. 
Personally i think we should discuss and then decide on a date by which 
we will fully transition to it.

Otherwise there is the potential for confusion since people will have to 
look into two different places for patches. People may review stuff on 
gerrit and the mailing list at the same time. This split isn't great and 
is prone to cause collision of suggestions due to reviewers not being 
aware of each other. etc.

Ideally we'd put gerrit up when it is fully configured and functional, 
being able to merge patches automatically. Then only maintainers will be 
able to +2 (approve) patches and verify contributors meet the legal 
requisites as is already the case with mailing lists?

Re: [PATCH] gdb: remove unused includes from dwarf2read.c

[Simon Marchi]

On 2019-10-14 2:02 p.m., Luis Machado wrote:
> The transition to some better (restrictions may apply) patch reviewing 
> system is a good thing, but i agree we should think further about it. 
> Personally i think we should discuss and then decide on a date by which 
> we will fully transition to it.
> 
> Otherwise there is the potential for confusion since people will have to 
> look into two different places for patches. People may review stuff on 
> gerrit and the mailing list at the same time. This split isn't great and 
> is prone to cause collision of suggestions due to reviewers not being 
> aware of each other. etc.

I am thinking that having Gerrit send notifications on gdb-patches alleviates
this problem, as people who monitor the mailing list will see the review
comments very similarly as if the review was done by email (I understand the
format of these notifications is not ideal at the moment, we got some good
feedback already).  And I expect people who use Gerrit to keep monitoring the
list at well for email patches.

Also, I expect that people will review patches using the system where the
patch was posted, so I don't think there is much risk of reviewers not being
aware of each other.

To be honest, some people jumped on Gerrit a bit faster than I expected :).
I expected to have more time to tweak it before it would be used "for real",
but I think we can tweak it as we go.

> Ideally we'd put gerrit up when it is fully configured and functional, 
> being able to merge patches automatically. Then only maintainers will be 
> able to +2 (approve) patches and verify contributors meet the legal 
> requisites as is already the case with mailing lists?

The problem with this is that we would need to involve the binutils community
as well, which we wanted to avoid initially.  We can't push to the master
branch on Gerrit while the binutils folks push to the master branch on
Sourceware: the two master branches would diverge.  So we would need to convince
them to use Gerrit too (at least, they would need to push to the Gerrit remote
and we would need to disable pushing to Sourceware).

But once we are happy with using our Gerrit instance (hopefully we'll get there)
and decide we keep it permanently, we can talk to the binutils community to see
if they would accept at least to change their push URL to our Gerrit, which would
allow us to "Submit" using the Gerrit interface.  And if they want to use Gerrit
for review too, why not.

Simon