All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Avoid Wunused-but-set warning
@ 2011-07-09 16:24 Raghavendra D Prabhu
  2011-07-10 16:16 ` Américo Wang
  0 siblings, 1 reply; 27+ messages in thread
From: Raghavendra D Prabhu @ 2011-07-09 16:24 UTC (permalink / raw)
  To: linux-kbuild; +Cc: Nir Tzachar, linux-kernel

Hi,
     I am seeing Wunused-but-set warning while make nconfig.  Looks like
     active_menu is not used. Removing it fixes the warning.

Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
---
  scripts/kconfig/nconf.c |    2 --
  1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
index 488dd74..5dad8ae 100644
--- a/scripts/kconfig/nconf.c
+++ b/scripts/kconfig/nconf.c
@@ -1067,7 +1067,6 @@ static void conf(struct menu *menu)
  	struct menu *submenu = 0;
  	const char *prompt = menu_get_prompt(menu);
  	struct symbol *sym;
-	struct menu *active_menu = NULL;
  	int res;
  	int current_index = 0;
  	int last_top_row = 0;
@@ -1152,7 +1151,6 @@ static void conf(struct menu *menu)
  			continue;
  
  		submenu = (struct menu *) item_data();
-		active_menu = (struct menu *)item_data();
  		if (!submenu || !menu_is_visible(submenu))
  			continue;
  		if (submenu)
-- 
1.7.6

--------------------------
Raghavendra Prabhu
GPG Id : 0xD72BE977
Fingerprint: B93F EBCB 8E05 7039 CD3C A4B8 A616 DCA1 D72B E977
www: wnohang.net

^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-09 16:24 [PATCH] Avoid Wunused-but-set warning Raghavendra D Prabhu
@ 2011-07-10 16:16 ` Américo Wang
  2011-07-10 16:24   ` Arnaud Lacombe
                     ` (2 more replies)
  0 siblings, 3 replies; 27+ messages in thread
From: Américo Wang @ 2011-07-10 16:16 UTC (permalink / raw)
  To: Raghavendra D Prabhu; +Cc: linux-kbuild, Nir Tzachar, linux-kernel

On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
<rprabhu@wnohang.net> wrote:
> Hi,
>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
>    active_menu is not used. Removing it fixes the warning.
>
> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>

Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

Thanks.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:16 ` Américo Wang
@ 2011-07-10 16:24   ` Arnaud Lacombe
  2011-07-10 16:28     ` Pekka Enberg
  2011-07-10 16:49     ` Américo Wang
  2011-07-13 11:39   ` Michal Marek
  2011-07-18 19:00   ` Arnaud Lacombe
  2 siblings, 2 replies; 27+ messages in thread
From: Arnaud Lacombe @ 2011-07-10 16:24 UTC (permalink / raw)
  To: Américo Wang
  Cc: Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

Hi,

On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> <rprabhu@wnohang.net> wrote:
>> Hi,
>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
>>    active_menu is not used. Removing it fixes the warning.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>
> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
>
Out of curiosity, what is your status to ACK such patch ? You are
neither nconf author (Nir Tzachar), nor kbuild/kconfig maintainer
(Michal Marek), neither have you made any change in scripts/kconfig,
and finally your are not in the MAINTAINERS file (may have had to do
with some fancy tree-wide permission on trivial patch).

Thanks,
 - Arnaud

> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:24   ` Arnaud Lacombe
@ 2011-07-10 16:28     ` Pekka Enberg
  2011-07-10 16:49       ` Arnaud Lacombe
  2011-07-10 16:49     ` Américo Wang
  1 sibling, 1 reply; 27+ messages in thread
From: Pekka Enberg @ 2011-07-10 16:28 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Américo Wang, Raghavendra D Prabhu, linux-kbuild,
	Nir Tzachar, linux-kernel

On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Hi,
>
> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
>> <rprabhu@wnohang.net> wrote:
>>> Hi,
>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
>>>    active_menu is not used. Removing it fixes the warning.
>>>
>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
>>
> Out of curiosity, what is your status to ACK such patch ?

What kind of status do you need to ACK such a simple patch?

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:28     ` Pekka Enberg
@ 2011-07-10 16:49       ` Arnaud Lacombe
  2011-07-10 16:51         ` Américo Wang
                           ` (2 more replies)
  0 siblings, 3 replies; 27+ messages in thread
From: Arnaud Lacombe @ 2011-07-10 16:49 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Américo Wang, Raghavendra D Prabhu, linux-kbuild,
	Nir Tzachar, linux-kernel

Hi,

On Sun, Jul 10, 2011 at 12:28 PM, Pekka Enberg <penberg@kernel.org> wrote:
> On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>> Hi,
>>
>> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
>>> <rprabhu@wnohang.net> wrote:
>>>> Hi,
>>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
>>>>    active_menu is not used. Removing it fixes the warning.
>>>>
>>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>>
>>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
>>>
>> Out of curiosity, what is your status to ACK such patch ?
>
> What kind of status do you need to ACK such a simple patch?
>
As per Documentation/SubmittingPatches:

<<
13) When to use Acked-by: and Cc:
The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path.

If a person was not directly involved in the preparation or handling of a
patch but wishes to signify and record their approval of it then they can
arrange to have an Acked-by: line added to the patch's changelog.

Acked-by: is often used by the maintainer of the affected code when that
maintainer neither contributed to nor forwarded the patch.
>>

That said, it is not a strong requirement... unfortunately. So, let's
have some fun and go ACK thousand of trivial patch just to generate
traffic on the LKML and give myself self-importance :-)

 - Arnaud

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:24   ` Arnaud Lacombe
  2011-07-10 16:28     ` Pekka Enberg
@ 2011-07-10 16:49     ` Américo Wang
  1 sibling, 0 replies; 27+ messages in thread
From: Américo Wang @ 2011-07-10 16:49 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Mon, Jul 11, 2011 at 12:24 AM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> Hi,
>
> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
>> <rprabhu@wnohang.net> wrote:
>>> Hi,
>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
>>>    active_menu is not used. Removing it fixes the warning.
>>>
>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>>
>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
>>
> Out of curiosity, what is your status to ACK such patch ? You are
> neither nconf author (Nir Tzachar), nor kbuild/kconfig maintainer
> (Michal Marek), neither have you made any change in scripts/kconfig,
> and finally your are not in the MAINTAINERS file (may have had to do
> with some fancy tree-wide permission on trivial patch).

I believe I worked on either Linux kernel or kbuild longer than you. :)

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:49       ` Arnaud Lacombe
@ 2011-07-10 16:51         ` Américo Wang
  2011-07-10 17:22           ` Arnaud Lacombe
  2011-07-10 17:24         ` Pekka Enberg
  2011-07-10 18:02         ` Randy Dunlap
  2 siblings, 1 reply; 27+ messages in thread
From: Américo Wang @ 2011-07-10 16:51 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Pekka Enberg, Raghavendra D Prabhu, linux-kbuild, Nir Tzachar,
	linux-kernel

On Mon, Jul 11, 2011 at 12:49 AM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>
> That said, it is not a strong requirement... unfortunately. So, let's
> have some fun and go ACK thousand of trivial patch just to generate
> traffic on the LKML and give myself self-importance :-)

That is how LKML works, get used to it.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:51         ` Américo Wang
@ 2011-07-10 17:22           ` Arnaud Lacombe
  0 siblings, 0 replies; 27+ messages in thread
From: Arnaud Lacombe @ 2011-07-10 17:22 UTC (permalink / raw)
  To: Américo Wang
  Cc: Pekka Enberg, Raghavendra D Prabhu, linux-kbuild, Nir Tzachar,
	linux-kernel

Hi,

On Sun, Jul 10, 2011 at 12:51 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Jul 11, 2011 at 12:49 AM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>>
>> That said, it is not a strong requirement... unfortunately. So, let's
>> have some fun and go ACK thousand of trivial patch just to generate
>> traffic on the LKML and give myself self-importance :-)
>
> That is how LKML works, get used to it.
>
That's why I generally avoid to go there, SNR is too low (and, this
message clearly does not make it better).

 - Arnaud

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:49       ` Arnaud Lacombe
  2011-07-10 16:51         ` Américo Wang
@ 2011-07-10 17:24         ` Pekka Enberg
  2011-07-10 18:02         ` Randy Dunlap
  2 siblings, 0 replies; 27+ messages in thread
From: Pekka Enberg @ 2011-07-10 17:24 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Américo Wang, Raghavendra D Prabhu, linux-kbuild,
	Nir Tzachar, linux-kernel

Hi Arnaud!

On Sun, Jul 10, 2011 at 7:49 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
>> What kind of status do you need to ACK such a simple patch?
>>
> As per Documentation/SubmittingPatches:

[snip]

> That said, it is not a strong requirement... unfortunately. So, let's
> have some fun and go ACK thousand of trivial patch just to generate
> traffic on the LKML and give myself self-importance :-)

Hey, thank you so much for quoting Documentation/SubmittingPatches for me!

I've read it a long time ago but I now see I've completely
misunderstood how things work around here. How silly of me to assume
patches posted on LKML were fair game for anyone to review and that it
was appreciated given that there's chronic lack of reviewers for
pretty much everything. Thank you for setting me straight on the
subject!

I see you've been rather busy with important Kconfg work recently but
I can only hope you'll find the time to enforce "Acked-by" status on
LKML in the future as well!

P.S. There are other tags such as Tested-by and Reviewed-by which you
might also want to police a bit as well - I hear people without proper
testing or reviewing status are doing those things all the time!

                               Pekka

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:49       ` Arnaud Lacombe
  2011-07-10 16:51         ` Américo Wang
  2011-07-10 17:24         ` Pekka Enberg
@ 2011-07-10 18:02         ` Randy Dunlap
  2011-07-10 22:56           ` Jesper Juhl
  2 siblings, 1 reply; 27+ messages in thread
From: Randy Dunlap @ 2011-07-10 18:02 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Pekka Enberg, Américo Wang, Raghavendra D Prabhu,
	linux-kbuild, Nir Tzachar, linux-kernel

On Sun, 10 Jul 2011 12:49:20 -0400 Arnaud Lacombe wrote:

> Hi,
> 
> On Sun, Jul 10, 2011 at 12:28 PM, Pekka Enberg <penberg@kernel.org> wrote:
> > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> >> Hi,
> >>
> >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> >>> <rprabhu@wnohang.net> wrote:
> >>>> Hi,
> >>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
> >>>>    active_menu is not used. Removing it fixes the warning.
> >>>>
> >>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> >>>
> >>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
> >>>
> >> Out of curiosity, what is your status to ACK such patch ?
> >
> > What kind of status do you need to ACK such a simple patch?
> >
> As per Documentation/SubmittingPatches:
> 
> <<
> 13) When to use Acked-by: and Cc:
> The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch's delivery path.
> 
> If a person was not directly involved in the preparation or handling of a
> patch but wishes to signify and record their approval of it then they can
> arrange to have an Acked-by: line added to the patch's changelog.
> 
> Acked-by: is often used by the maintainer of the affected code when that
> maintainer neither contributed to nor forwarded the patch.
> >>
> 
> That said, it is not a strong requirement... unfortunately. So, let's
> have some fun and go ACK thousand of trivial patch just to generate
> traffic on the LKML and give myself self-importance :-)

Acked-by: is mostly used as a weak version of Reviewed-by:
and the "definition" in SubmittingPatches is not accurate IMO.
I.e., it can be used by anyone.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 18:02         ` Randy Dunlap
@ 2011-07-10 22:56           ` Jesper Juhl
  2011-07-10 23:07             ` Randy Dunlap
  0 siblings, 1 reply; 27+ messages in thread
From: Jesper Juhl @ 2011-07-10 22:56 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Arnaud Lacombe, Pekka Enberg, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2595 bytes --]

On Sun, 10 Jul 2011, Randy Dunlap wrote:

> On Sun, 10 Jul 2011 12:49:20 -0400 Arnaud Lacombe wrote:
> 
> > Hi,
> > 
> > On Sun, Jul 10, 2011 at 12:28 PM, Pekka Enberg <penberg@kernel.org> wrote:
> > > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> > >> Hi,
> > >>
> > >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> > >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> > >>> <rprabhu@wnohang.net> wrote:
> > >>>> Hi,
> > >>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
> > >>>>    active_menu is not used. Removing it fixes the warning.
> > >>>>
> > >>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> > >>>
> > >>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
> > >>>
> > >> Out of curiosity, what is your status to ACK such patch ?
> > >
> > > What kind of status do you need to ACK such a simple patch?
> > >
> > As per Documentation/SubmittingPatches:
> > 
> > <<
> > 13) When to use Acked-by: and Cc:
> > The Signed-off-by: tag indicates that the signer was involved in the
> > development of the patch, or that he/she was in the patch's delivery path.
> > 
> > If a person was not directly involved in the preparation or handling of a
> > patch but wishes to signify and record their approval of it then they can
> > arrange to have an Acked-by: line added to the patch's changelog.
> > 
> > Acked-by: is often used by the maintainer of the affected code when that
> > maintainer neither contributed to nor forwarded the patch.
> > >>
> > 
> > That said, it is not a strong requirement... unfortunately. So, let's
> > have some fun and go ACK thousand of trivial patch just to generate
> > traffic on the LKML and give myself self-importance :-)
> 
> Acked-by: is mostly used as a weak version of Reviewed-by:
> and the "definition" in SubmittingPatches is not accurate IMO.
> I.e., it can be used by anyone.
> 

Interesting. I was under the impression that Reviewed-by: was a weaker 
thing than Acked-by: - I certainly have been using it as such.

I've always interpreted Acked-by: as being something you could apply if 
you were the author, maintainer or other person with similar strong 
background knowledge of the code. Where Reviewed-by: could be used by 
anyone, as long as they had taken the time to read the patch and try and 
understand what was going on and the result/conclusion looked good.

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 22:56           ` Jesper Juhl
@ 2011-07-10 23:07             ` Randy Dunlap
  2011-07-10 23:21               ` Jesper Juhl
  0 siblings, 1 reply; 27+ messages in thread
From: Randy Dunlap @ 2011-07-10 23:07 UTC (permalink / raw)
  To: Jesper Juhl
  Cc: Arnaud Lacombe, Pekka Enberg, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Mon, 11 Jul 2011 00:56:57 +0200 (CEST) Jesper Juhl wrote:

> On Sun, 10 Jul 2011, Randy Dunlap wrote:
> 
> > On Sun, 10 Jul 2011 12:49:20 -0400 Arnaud Lacombe wrote:
> > 
> > > Hi,
> > > 
> > > On Sun, Jul 10, 2011 at 12:28 PM, Pekka Enberg <penberg@kernel.org> wrote:
> > > > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> > > >> Hi,
> > > >>
> > > >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> > > >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> > > >>> <rprabhu@wnohang.net> wrote:
> > > >>>> Hi,
> > > >>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
> > > >>>>    active_menu is not used. Removing it fixes the warning.
> > > >>>>
> > > >>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> > > >>>
> > > >>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
> > > >>>
> > > >> Out of curiosity, what is your status to ACK such patch ?
> > > >
> > > > What kind of status do you need to ACK such a simple patch?
> > > >
> > > As per Documentation/SubmittingPatches:
> > > 
> > > <<
> > > 13) When to use Acked-by: and Cc:
> > > The Signed-off-by: tag indicates that the signer was involved in the
> > > development of the patch, or that he/she was in the patch's delivery path.
> > > 
> > > If a person was not directly involved in the preparation or handling of a
> > > patch but wishes to signify and record their approval of it then they can
> > > arrange to have an Acked-by: line added to the patch's changelog.
> > > 
> > > Acked-by: is often used by the maintainer of the affected code when that
> > > maintainer neither contributed to nor forwarded the patch.
> > > >>
> > > 
> > > That said, it is not a strong requirement... unfortunately. So, let's
> > > have some fun and go ACK thousand of trivial patch just to generate
> > > traffic on the LKML and give myself self-importance :-)
> > 
> > Acked-by: is mostly used as a weak version of Reviewed-by:
> > and the "definition" in SubmittingPatches is not accurate IMO.
> > I.e., it can be used by anyone.
> > 
> 
> Interesting. I was under the impression that Reviewed-by: was a weaker 
> thing than Acked-by: - I certainly have been using it as such.
> 
> I've always interpreted Acked-by: as being something you could apply if 
> you were the author, maintainer or other person with similar strong 
> background knowledge of the code. Where Reviewed-by: could be used by 
> anyone, as long as they had taken the time to read the patch and try and 
> understand what was going on and the result/conclusion looked good.

I don't see it in SubmittingPatches, but there was some discussion at the
time (IIRC!!) that Reviewed-by: indicates that you are willing to support/fix
the patch if the patch author(s) disappear.  I.e., you are willing to take
some ownership responsibility of the patch.

or I could be dreaming...

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 23:07             ` Randy Dunlap
@ 2011-07-10 23:21               ` Jesper Juhl
  2011-07-10 23:53                 ` Randy Dunlap
  2011-07-11  5:55                 ` Pekka Enberg
  0 siblings, 2 replies; 27+ messages in thread
From: Jesper Juhl @ 2011-07-10 23:21 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Arnaud Lacombe, Pekka Enberg, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4141 bytes --]

On Sun, 10 Jul 2011, Randy Dunlap wrote:

> On Mon, 11 Jul 2011 00:56:57 +0200 (CEST) Jesper Juhl wrote:
> 
> > On Sun, 10 Jul 2011, Randy Dunlap wrote:
> > 
> > > On Sun, 10 Jul 2011 12:49:20 -0400 Arnaud Lacombe wrote:
> > > 
> > > > Hi,
> > > > 
> > > > On Sun, Jul 10, 2011 at 12:28 PM, Pekka Enberg <penberg@kernel.org> wrote:
> > > > > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> > > > >> Hi,
> > > > >>
> > > > >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> > > > >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> > > > >>> <rprabhu@wnohang.net> wrote:
> > > > >>>> Hi,
> > > > >>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
> > > > >>>>    active_menu is not used. Removing it fixes the warning.
> > > > >>>>
> > > > >>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> > > > >>>
> > > > >>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
> > > > >>>
> > > > >> Out of curiosity, what is your status to ACK such patch ?
> > > > >
> > > > > What kind of status do you need to ACK such a simple patch?
> > > > >
> > > > As per Documentation/SubmittingPatches:
> > > > 
> > > > <<
> > > > 13) When to use Acked-by: and Cc:
> > > > The Signed-off-by: tag indicates that the signer was involved in the
> > > > development of the patch, or that he/she was in the patch's delivery path.
> > > > 
> > > > If a person was not directly involved in the preparation or handling of a
> > > > patch but wishes to signify and record their approval of it then they can
> > > > arrange to have an Acked-by: line added to the patch's changelog.
> > > > 
> > > > Acked-by: is often used by the maintainer of the affected code when that
> > > > maintainer neither contributed to nor forwarded the patch.
> > > > >>
> > > > 
> > > > That said, it is not a strong requirement... unfortunately. So, let's
> > > > have some fun and go ACK thousand of trivial patch just to generate
> > > > traffic on the LKML and give myself self-importance :-)
> > > 
> > > Acked-by: is mostly used as a weak version of Reviewed-by:
> > > and the "definition" in SubmittingPatches is not accurate IMO.
> > > I.e., it can be used by anyone.
> > > 
> > 
> > Interesting. I was under the impression that Reviewed-by: was a weaker 
> > thing than Acked-by: - I certainly have been using it as such.
> > 
> > I've always interpreted Acked-by: as being something you could apply if 
> > you were the author, maintainer or other person with similar strong 
> > background knowledge of the code. Where Reviewed-by: could be used by 
> > anyone, as long as they had taken the time to read the patch and try and 
> > understand what was going on and the result/conclusion looked good.
> 
> I don't see it in SubmittingPatches, but there was some discussion at the
> time (IIRC!!) that Reviewed-by: indicates that you are willing to support/fix
> the patch if the patch author(s) disappear.  I.e., you are willing to take
> some ownership responsibility of the patch.
> 
> or I could be dreaming...
> 
I'm not going to claim that I recall all the discussion that went on at 
the time, there was quite a bit IIRC (and I'm too lazy to read up on all 
of it). But to me it seems to make sense that if you have strong knowledge 
of/involvement with the code being patched then you can offer your 
Acked-by: after reviewing the patch. If you don't have such 
knowledge/involvement but have nevertheless reviewed the code and found it 
to be OK, then you can signal that with a Reviewed-by:.

In any case, you can't expect people to base their Acked-by/Reviewed-by 
replies on some conclusion in some email thread that happened years ago 
but was never written down in some document in the repository. 
It is only reasonable to expect people to behave according to the rules 
laid out in SubmittingPatches and similar documents, and those rules 
currently seem to support my interpretation, not yours.

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 23:21               ` Jesper Juhl
@ 2011-07-10 23:53                 ` Randy Dunlap
  2011-07-11  5:09                   ` Mark Brown
  2011-07-11 20:32                   ` Jesper Juhl
  2011-07-11  5:55                 ` Pekka Enberg
  1 sibling, 2 replies; 27+ messages in thread
From: Randy Dunlap @ 2011-07-10 23:53 UTC (permalink / raw)
  To: Jesper Juhl
  Cc: Arnaud Lacombe, Pekka Enberg, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Mon, 11 Jul 2011 01:21:36 +0200 (CEST) Jesper Juhl wrote:

> On Sun, 10 Jul 2011, Randy Dunlap wrote:
> 
> > On Mon, 11 Jul 2011 00:56:57 +0200 (CEST) Jesper Juhl wrote:
> > 
> > > On Sun, 10 Jul 2011, Randy Dunlap wrote:
> > > 
> > > > On Sun, 10 Jul 2011 12:49:20 -0400 Arnaud Lacombe wrote:
> > > > 
> > > > > Hi,
> > > > > 
> > > > > On Sun, Jul 10, 2011 at 12:28 PM, Pekka Enberg <penberg@kernel.org> wrote:
> > > > > > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> > > > > >> Hi,
> > > > > >>
> > > > > >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> > > > > >>> <rprabhu@wnohang.net> wrote:
> > > > > >>>> Hi,
> > > > > >>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
> > > > > >>>>    active_menu is not used. Removing it fixes the warning.
> > > > > >>>>
> > > > > >>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> > > > > >>>
> > > > > >>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
> > > > > >>>
> > > > > >> Out of curiosity, what is your status to ACK such patch ?
> > > > > >
> > > > > > What kind of status do you need to ACK such a simple patch?
> > > > > >
> > > > > As per Documentation/SubmittingPatches:
> > > > > 
> > > > > <<
> > > > > 13) When to use Acked-by: and Cc:
> > > > > The Signed-off-by: tag indicates that the signer was involved in the
> > > > > development of the patch, or that he/she was in the patch's delivery path.
> > > > > 
> > > > > If a person was not directly involved in the preparation or handling of a
> > > > > patch but wishes to signify and record their approval of it then they can
> > > > > arrange to have an Acked-by: line added to the patch's changelog.
> > > > > 
> > > > > Acked-by: is often used by the maintainer of the affected code when that
> > > > > maintainer neither contributed to nor forwarded the patch.
> > > > > >>
> > > > > 
> > > > > That said, it is not a strong requirement... unfortunately. So, let's
> > > > > have some fun and go ACK thousand of trivial patch just to generate
> > > > > traffic on the LKML and give myself self-importance :-)
> > > > 
> > > > Acked-by: is mostly used as a weak version of Reviewed-by:
> > > > and the "definition" in SubmittingPatches is not accurate IMO.
> > > > I.e., it can be used by anyone.
> > > > 
> > > 
> > > Interesting. I was under the impression that Reviewed-by: was a weaker 
> > > thing than Acked-by: - I certainly have been using it as such.
> > > 
> > > I've always interpreted Acked-by: as being something you could apply if 
> > > you were the author, maintainer or other person with similar strong 
> > > background knowledge of the code. Where Reviewed-by: could be used by 
> > > anyone, as long as they had taken the time to read the patch and try and 
> > > understand what was going on and the result/conclusion looked good.
> > 
> > I don't see it in SubmittingPatches, but there was some discussion at the
> > time (IIRC!!) that Reviewed-by: indicates that you are willing to support/fix
> > the patch if the patch author(s) disappear.  I.e., you are willing to take
> > some ownership responsibility of the patch.
> > 
> > or I could be dreaming...
> > 
> I'm not going to claim that I recall all the discussion that went on at 
> the time, there was quite a bit IIRC (and I'm too lazy to read up on all 
> of it). But to me it seems to make sense that if you have strong knowledge 
> of/involvement with the code being patched then you can offer your 
> Acked-by: after reviewing the patch. If you don't have such 
> knowledge/involvement but have nevertheless reviewed the code and found it 
> to be OK, then you can signal that with a Reviewed-by:.
> 
> In any case, you can't expect people to base their Acked-by/Reviewed-by 
> replies on some conclusion in some email thread that happened years ago 

ack that.

> but was never written down in some document in the repository. 
> It is only reasonable to expect people to behave according to the rules 
> laid out in SubmittingPatches and similar documents, and those rules 
> currently seem to support my interpretation, not yours.

In Documentation/SubmittingPatches, Reviewed-by: contains a "Reviewer's
statment of oversight."  That alone is more formal than Acked-by: is.

Plus this paragraph acknowledges that "Review" can be a serious and
time-consuming task, not a simple "looks OK to me":

"A Reviewed-by tag is a statement of opinion that the patch is an
appropriate modification of the kernel without any remaining serious
technical issues.  Any interested reviewer (who has done the work) can
offer a Reviewed-by tag for a patch.  This tag serves to give credit to
reviewers and to inform maintainers of the degree of review which has been
done on the patch.  Reviewed-by: tags, when supplied by reviewers known to
understand the subject area and to perform thorough reviews, will normally
increase the likelihood of your patch getting into the kernel."

I can see little about Acked-by: that is formal when it comes to patch review.
E.g.:
'Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
has at least reviewed the patch and has indicated acceptance.  Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by:.'

But do as you like.  Which parts of SubmittingPatches do you think
support your interpretation?

and should we have this line:
	Acked-by: is not as formal as Signed-off-by:.
changed to:
	Acked-by: is not as formal as Signed-off-by: or Reviewed-by:.
e.g.?

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 23:53                 ` Randy Dunlap
@ 2011-07-11  5:09                   ` Mark Brown
  2011-07-11 17:11                     ` Randy Dunlap
  2011-07-11 20:32                   ` Jesper Juhl
  1 sibling, 1 reply; 27+ messages in thread
From: Mark Brown @ 2011-07-11  5:09 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Jesper Juhl, Arnaud Lacombe, Pekka Enberg, Am?rico Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Sun, Jul 10, 2011 at 04:53:33PM -0700, Randy Dunlap wrote:

> But do as you like.  Which parts of SubmittingPatches do you think
> support your interpretation?

> and should we have this line:
> 	Acked-by: is not as formal as Signed-off-by:.
> changed to:
> 	Acked-by: is not as formal as Signed-off-by: or Reviewed-by:.
> e.g.?

Current practice seems to be that Acked-by is used instead of
Reviewed-by - the latter is comparatively rare.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 23:21               ` Jesper Juhl
  2011-07-10 23:53                 ` Randy Dunlap
@ 2011-07-11  5:55                 ` Pekka Enberg
  2011-07-11 20:37                   ` Jesper Juhl
  2011-07-13  0:35                   ` Valdis.Kletnieks
  1 sibling, 2 replies; 27+ messages in thread
From: Pekka Enberg @ 2011-07-11  5:55 UTC (permalink / raw)
  To: Jesper Juhl
  Cc: Randy Dunlap, Arnaud Lacombe, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

Hi Jesper,

On Mon, Jul 11, 2011 at 2:21 AM, Jesper Juhl <jj@chaosbits.net> wrote:
> In any case, you can't expect people to base their Acked-by/Reviewed-by
> replies on some conclusion in some email thread that happened years ago
> but was never written down in some document in the repository.
> It is only reasonable to expect people to behave according to the rules
> laid out in SubmittingPatches and similar documents, and those rules
> currently seem to support my interpretation, not yours.

The definitions in SubmittingPatches are not hard rules and are, in
fact, out of date. See Documentation/development-process/5.Posting for
alternative definitions:

 - Acked-by: indicates an agreement by another developer (often a
   maintainer of the relevant code) that the patch is appropriate for
   inclusion into the kernel.

and

 - Reviewed-by: the named developer has reviewed the patch for correctness;
   see the reviewer's statement in Documentation/SubmittingPatches for more
   detail.

and then compare the dates for these definitions:

0f44cd23 (Andrew Morton      2007-06-08 13:46:45 -0700 401) Acked-by:
is often used by the maintainer of the affected code when that
0f44cd23 (Andrew Morton      2007-06-08 13:46:45 -0700 402) maintainer
neither contributed to nor forwarded the patch.

75b02146 (Jonathan Corbet 2008-09-30 15:15:56 -0600 205)  - Acked-by:
indicates an agreement by another developer (often a
75b02146 (Jonathan Corbet 2008-09-30 15:15:56 -0600 206)    maintainer
of the relevant code) that the patch is appropriate for
75b02146 (Jonathan Corbet 2008-09-30 15:15:56 -0600 207)    inclusion
into the kernel.

So it is NOT reasonable 'to expect people to behave according to the
rules laid out in SubmittingPatches and similar documents' because
such documents have never had any hard rules! The documents are
guidelines that attempt to document how things work here, not lay down
the law.

                                 Pekka

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-11  5:09                   ` Mark Brown
@ 2011-07-11 17:11                     ` Randy Dunlap
  2011-07-11 22:42                       ` Mark Brown
  2011-07-13  1:15                       ` Américo Wang
  0 siblings, 2 replies; 27+ messages in thread
From: Randy Dunlap @ 2011-07-11 17:11 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jesper Juhl, Arnaud Lacombe, Pekka Enberg, Am?rico Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Mon, 11 Jul 2011 06:09:52 +0100 Mark Brown wrote:

> On Sun, Jul 10, 2011 at 04:53:33PM -0700, Randy Dunlap wrote:
> 
> > But do as you like.  Which parts of SubmittingPatches do you think
> > support your interpretation?
> 
> > and should we have this line:
> > 	Acked-by: is not as formal as Signed-off-by:.
> > changed to:
> > 	Acked-by: is not as formal as Signed-off-by: or Reviewed-by:.
> > e.g.?
> 
> Current practice seems to be that Acked-by is used instead of
> Reviewed-by - the latter is comparatively rare.


ISTM that more education and encouragement are needed about Reviewed-by:.
(Patch Review is a possible kernel summit topic.)

and that SubmittingPatches should be updated since we generally refer people
to that file and not to Documentation/development-process/


Samples from my partial mailing list archives:

linux-pci mailing list:		Acked-by: 93	Reviewed-by: 81
linux-mm mailing list:		Acked-by: 2104	Reviewed-by: 1344
netdev mailing list:		Acked-by: 1366	Reviewed-by: 659

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 23:53                 ` Randy Dunlap
  2011-07-11  5:09                   ` Mark Brown
@ 2011-07-11 20:32                   ` Jesper Juhl
  1 sibling, 0 replies; 27+ messages in thread
From: Jesper Juhl @ 2011-07-11 20:32 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Arnaud Lacombe, Pekka Enberg, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6628 bytes --]

On Sun, 10 Jul 2011, Randy Dunlap wrote:

> On Mon, 11 Jul 2011 01:21:36 +0200 (CEST) Jesper Juhl wrote:
> 
> > On Sun, 10 Jul 2011, Randy Dunlap wrote:
> > 
> > > On Mon, 11 Jul 2011 00:56:57 +0200 (CEST) Jesper Juhl wrote:
> > > 
> > > > On Sun, 10 Jul 2011, Randy Dunlap wrote:
> > > > 
> > > > > On Sun, 10 Jul 2011 12:49:20 -0400 Arnaud Lacombe wrote:
> > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > On Sun, Jul 10, 2011 at 12:28 PM, Pekka Enberg <penberg@kernel.org> wrote:
> > > > > > > On Sun, Jul 10, 2011 at 7:24 PM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> > > > > > >> Hi,
> > > > > > >>
> > > > > > >> On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > >>> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> > > > > > >>> <rprabhu@wnohang.net> wrote:
> > > > > > >>>> Hi,
> > > > > > >>>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
> > > > > > >>>>    active_menu is not used. Removing it fixes the warning.
> > > > > > >>>>
> > > > > > >>>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> > > > > > >>>
> > > > > > >>> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
> > > > > > >>>
> > > > > > >> Out of curiosity, what is your status to ACK such patch ?
> > > > > > >
> > > > > > > What kind of status do you need to ACK such a simple patch?
> > > > > > >
> > > > > > As per Documentation/SubmittingPatches:
> > > > > > 
> > > > > > <<
> > > > > > 13) When to use Acked-by: and Cc:
> > > > > > The Signed-off-by: tag indicates that the signer was involved in the
> > > > > > development of the patch, or that he/she was in the patch's delivery path.
> > > > > > 
> > > > > > If a person was not directly involved in the preparation or handling of a
> > > > > > patch but wishes to signify and record their approval of it then they can
> > > > > > arrange to have an Acked-by: line added to the patch's changelog.
> > > > > > 
> > > > > > Acked-by: is often used by the maintainer of the affected code when that
> > > > > > maintainer neither contributed to nor forwarded the patch.
> > > > > > >>
> > > > > > 
> > > > > > That said, it is not a strong requirement... unfortunately. So, let's
> > > > > > have some fun and go ACK thousand of trivial patch just to generate
> > > > > > traffic on the LKML and give myself self-importance :-)
> > > > > 
> > > > > Acked-by: is mostly used as a weak version of Reviewed-by:
> > > > > and the "definition" in SubmittingPatches is not accurate IMO.
> > > > > I.e., it can be used by anyone.
> > > > > 
> > > > 
> > > > Interesting. I was under the impression that Reviewed-by: was a weaker 
> > > > thing than Acked-by: - I certainly have been using it as such.
> > > > 
> > > > I've always interpreted Acked-by: as being something you could apply if 
> > > > you were the author, maintainer or other person with similar strong 
> > > > background knowledge of the code. Where Reviewed-by: could be used by 
> > > > anyone, as long as they had taken the time to read the patch and try and 
> > > > understand what was going on and the result/conclusion looked good.
> > > 
> > > I don't see it in SubmittingPatches, but there was some discussion at the
> > > time (IIRC!!) that Reviewed-by: indicates that you are willing to support/fix
> > > the patch if the patch author(s) disappear.  I.e., you are willing to take
> > > some ownership responsibility of the patch.
> > > 
> > > or I could be dreaming...
> > > 
> > I'm not going to claim that I recall all the discussion that went on at 
> > the time, there was quite a bit IIRC (and I'm too lazy to read up on all 
> > of it). But to me it seems to make sense that if you have strong knowledge 
> > of/involvement with the code being patched then you can offer your 
> > Acked-by: after reviewing the patch. If you don't have such 
> > knowledge/involvement but have nevertheless reviewed the code and found it 
> > to be OK, then you can signal that with a Reviewed-by:.
> > 
> > In any case, you can't expect people to base their Acked-by/Reviewed-by 
> > replies on some conclusion in some email thread that happened years ago 
> 
> ack that.
> 
> > but was never written down in some document in the repository. 
> > It is only reasonable to expect people to behave according to the rules 
> > laid out in SubmittingPatches and similar documents, and those rules 
> > currently seem to support my interpretation, not yours.
> 
> In Documentation/SubmittingPatches, Reviewed-by: contains a "Reviewer's
> statment of oversight."  That alone is more formal than Acked-by: is.
> 
> Plus this paragraph acknowledges that "Review" can be a serious and
> time-consuming task, not a simple "looks OK to me":
> 
> "A Reviewed-by tag is a statement of opinion that the patch is an
> appropriate modification of the kernel without any remaining serious
> technical issues.  Any interested reviewer (who has done the work) can
> offer a Reviewed-by tag for a patch.  This tag serves to give credit to
> reviewers and to inform maintainers of the degree of review which has been
> done on the patch.  Reviewed-by: tags, when supplied by reviewers known to
> understand the subject area and to perform thorough reviews, will normally
> increase the likelihood of your patch getting into the kernel."
> 
> I can see little about Acked-by: that is formal when it comes to patch review.
> E.g.:
> 'Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
> has at least reviewed the patch and has indicated acceptance.  Hence patch
> mergers will sometimes manually convert an acker's "yep, looks good to me"
> into an Acked-by:.'
> 

I'll concede to those points. My original understanding/reading was 
different but you make some good points.
 

> But do as you like.  Which parts of SubmittingPatches do you think
> support your interpretation?
> 

Originally, text such as this:

"Acked-by: is often used by the maintainer of the affected code..."

"Reviewed-by:, instead, indicates that the patch has been reviewed and 
found acceptable ... I do not (unless explicitly stated elsewhere) make 
any warranties or guarantees that it will achieve its stated purpose ..."

But I take that back. Your interpretation now seems like the more correct 
one to me.

> and should we have this line:
> 	Acked-by: is not as formal as Signed-off-by:.
> changed to:
> 	Acked-by: is not as formal as Signed-off-by: or Reviewed-by:.
> e.g.?
> 

Might make sense.

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-11  5:55                 ` Pekka Enberg
@ 2011-07-11 20:37                   ` Jesper Juhl
  2011-07-13  0:35                   ` Valdis.Kletnieks
  1 sibling, 0 replies; 27+ messages in thread
From: Jesper Juhl @ 2011-07-11 20:37 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Randy Dunlap, Arnaud Lacombe, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Mon, 11 Jul 2011, Pekka Enberg wrote:

> Hi Jesper,
> 
> On Mon, Jul 11, 2011 at 2:21 AM, Jesper Juhl <jj@chaosbits.net> wrote:
> > In any case, you can't expect people to base their Acked-by/Reviewed-by
> > replies on some conclusion in some email thread that happened years ago
> > but was never written down in some document in the repository.
> > It is only reasonable to expect people to behave according to the rules
> > laid out in SubmittingPatches and similar documents, and those rules
> > currently seem to support my interpretation, not yours.
> 
> The definitions in SubmittingPatches are not hard rules and are, in
> fact, out of date. See Documentation/development-process/5.Posting for
> alternative definitions:
> 
Thank you for pointing me at that document. Was not aware of it (or I'd 
forgotten if I ever did know of it).

>  - Acked-by: indicates an agreement by another developer (often a
>    maintainer of the relevant code) that the patch is appropriate for
>    inclusion into the kernel.
> 
> and
> 
>  - Reviewed-by: the named developer has reviewed the patch for correctness;
>    see the reviewer's statement in Documentation/SubmittingPatches for more
>    detail.
> 
> and then compare the dates for these definitions:
> 
> 0f44cd23 (Andrew Morton      2007-06-08 13:46:45 -0700 401) Acked-by:
> is often used by the maintainer of the affected code when that
> 0f44cd23 (Andrew Morton      2007-06-08 13:46:45 -0700 402) maintainer
> neither contributed to nor forwarded the patch.
> 
> 75b02146 (Jonathan Corbet 2008-09-30 15:15:56 -0600 205)  - Acked-by:
> indicates an agreement by another developer (often a
> 75b02146 (Jonathan Corbet 2008-09-30 15:15:56 -0600 206)    maintainer
> of the relevant code) that the patch is appropriate for
> 75b02146 (Jonathan Corbet 2008-09-30 15:15:56 -0600 207)    inclusion
> into the kernel.
> 
> So it is NOT reasonable 'to expect people to behave according to the
> rules laid out in SubmittingPatches and similar documents' because
> such documents have never had any hard rules! The documents are
> guidelines that attempt to document how things work here, not lay down
> the law.
> 

I know that. I was never trying to say that they were hard rules/law. 
I was just trying to express my personal interpretation of those 
guidelines and how I have been using them. If it came across as me saying 
that they were set-in-stone rules then I guess I need to improve my 
communication skills.


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-11 17:11                     ` Randy Dunlap
@ 2011-07-11 22:42                       ` Mark Brown
  2011-07-13  1:15                       ` Américo Wang
  1 sibling, 0 replies; 27+ messages in thread
From: Mark Brown @ 2011-07-11 22:42 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Jesper Juhl, Arnaud Lacombe, Pekka Enberg, Am?rico Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Mon, Jul 11, 2011 at 10:11:34AM -0700, Randy Dunlap wrote:
> On Mon, 11 Jul 2011 06:09:52 +0100 Mark Brown wrote:

> > Current practice seems to be that Acked-by is used instead of
> > Reviewed-by - the latter is comparatively rare.

> ISTM that more education and encouragement are needed about Reviewed-by:.
> (Patch Review is a possible kernel summit topic.)

> and that SubmittingPatches should be updated since we generally refer people
> to that file and not to Documentation/development-process/

I'm not sure I see any value in that - they're both pretty much
interchangible as far as I can tell.  The important thing is that people
are doing review, not the form filling part.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-11  5:55                 ` Pekka Enberg
  2011-07-11 20:37                   ` Jesper Juhl
@ 2011-07-13  0:35                   ` Valdis.Kletnieks
  2011-07-13  0:55                     ` Randy Dunlap
  1 sibling, 1 reply; 27+ messages in thread
From: Valdis.Kletnieks @ 2011-07-13  0:35 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Jesper Juhl, Randy Dunlap, Arnaud Lacombe, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On Mon, 11 Jul 2011 08:55:19 +0300, Pekka Enberg said:

> The definitions in SubmittingPatches are not hard rules and are, in
> fact, out of date. See Documentation/development-process/5.Posting for
> alternative definitions:
> 
>  - Acked-by: indicates an agreement by another developer (often a
>    maintainer of the relevant code) that the patch is appropriate for
>    inclusion into the kernel.
> 
> and
> 
>  - Reviewed-by: the named developer has reviewed the patch for correctness;
>    see the reviewer's statement in Documentation/SubmittingPatches for more
>    detail.

Unfortunately, SubmittingPatches says:

        By offering my Reviewed-by: tag, I state that:

         (a) I have carried out a technical review of this patch to
             evaluate its appropriateness and readiness for inclusion into
             the mainline kernel.

         (b) Any problems, concerns, or questions relating to the patch
             have been communicated back to the submitter.  I am satisfied
             with the submitter's response to my comments.

         (c) While there may be things that could be improved with this
             submission, I believe that it is, at this time, (1) a
             worthwhile modification to the kernel, and (2) free of known
             issues which would argue against its inclusion.

         (d) While I have reviewed the patch and believe it to be sound, I
             do not (unless explicitly stated elsewhere) make any
             warranties or guarantees that it will achieve its stated
             purpose or function properly in any given situation.

and often, I'm only comfortable stating (b) - often, I'd like to *disavow* both
(a) and (c)(1) - I usually *don't* do a tech review, and may have no opinion as
to whether it's "cooked" enough to be included.  Also, usually, the only "known
issue" from (c)(2) is the one thing I commented on for part (b)...

Comments-Addressed-Acked: anybody? :)



[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-13  0:35                   ` Valdis.Kletnieks
@ 2011-07-13  0:55                     ` Randy Dunlap
  0 siblings, 0 replies; 27+ messages in thread
From: Randy Dunlap @ 2011-07-13  0:55 UTC (permalink / raw)
  To: Valdis.Kletnieks
  Cc: Pekka Enberg, Jesper Juhl, Arnaud Lacombe, Américo Wang,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Tue, 12 Jul 2011 20:35:12 -0400 Valdis.Kletnieks@vt.edu wrote:

> On Mon, 11 Jul 2011 08:55:19 +0300, Pekka Enberg said:
> 
> > The definitions in SubmittingPatches are not hard rules and are, in
> > fact, out of date. See Documentation/development-process/5.Posting for
> > alternative definitions:
> > 
> >  - Acked-by: indicates an agreement by another developer (often a
> >    maintainer of the relevant code) that the patch is appropriate for
> >    inclusion into the kernel.
> > 
> > and
> > 
> >  - Reviewed-by: the named developer has reviewed the patch for correctness;
> >    see the reviewer's statement in Documentation/SubmittingPatches for more
> >    detail.
> 
> Unfortunately, SubmittingPatches says:
> 
>         By offering my Reviewed-by: tag, I state that:
> 
>          (a) I have carried out a technical review of this patch to
>              evaluate its appropriateness and readiness for inclusion into
>              the mainline kernel.
> 
>          (b) Any problems, concerns, or questions relating to the patch
>              have been communicated back to the submitter.  I am satisfied
>              with the submitter's response to my comments.
> 
>          (c) While there may be things that could be improved with this
>              submission, I believe that it is, at this time, (1) a
>              worthwhile modification to the kernel, and (2) free of known
>              issues which would argue against its inclusion.
> 
>          (d) While I have reviewed the patch and believe it to be sound, I
>              do not (unless explicitly stated elsewhere) make any
>              warranties or guarantees that it will achieve its stated
>              purpose or function properly in any given situation.
> 
> and often, I'm only comfortable stating (b) - often, I'd like to *disavow* both
> (a) and (c)(1) - I usually *don't* do a tech review, and may have no opinion as
> to whether it's "cooked" enough to be included.  Also, usually, the only "known
> issue" from (c)(2) is the one thing I commented on for part (b)...
> 
> Comments-Addressed-Acked: anybody? :)

then you just want to use Acked-by instead of Reviewed-by.  I think.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-11 17:11                     ` Randy Dunlap
  2011-07-11 22:42                       ` Mark Brown
@ 2011-07-13  1:15                       ` Américo Wang
  2011-07-13  2:21                         ` Arnaud Lacombe
  1 sibling, 1 reply; 27+ messages in thread
From: Américo Wang @ 2011-07-13  1:15 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Mark Brown, Jesper Juhl, Arnaud Lacombe, Pekka Enberg,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Tue, Jul 12, 2011 at 1:11 AM, Randy Dunlap <rdunlap@xenotime.net> wrote:
> On Mon, 11 Jul 2011 06:09:52 +0100 Mark Brown wrote:
>
>> On Sun, Jul 10, 2011 at 04:53:33PM -0700, Randy Dunlap wrote:
>>
>> > But do as you like.  Which parts of SubmittingPatches do you think
>> > support your interpretation?
>>
>> > and should we have this line:
>> >     Acked-by: is not as formal as Signed-off-by:.
>> > changed to:
>> >     Acked-by: is not as formal as Signed-off-by: or Reviewed-by:.
>> > e.g.?
>>
>> Current practice seems to be that Acked-by is used instead of
>> Reviewed-by - the latter is comparatively rare.
>
>
> ISTM that more education and encouragement are needed about Reviewed-by:.
> (Patch Review is a possible kernel summit topic.)
>
> and that SubmittingPatches should be updated since we generally refer people
> to that file and not to Documentation/development-process/

Agreed, mind to send a patch? ;-)

>
> Samples from my partial mailing list archives:
>
> linux-pci mailing list:         Acked-by: 93    Reviewed-by: 81
> linux-mm mailing list:          Acked-by: 2104  Reviewed-by: 1344
> netdev mailing list:            Acked-by: 1366  Reviewed-by: 659
>

Yup, take netdev as an example, Davem is the only maintainer (not to
say things like wireless)
but definitely people like Eirc or Herbert is qualified to give Acked-by too.

Thanks.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-13  1:15                       ` Américo Wang
@ 2011-07-13  2:21                         ` Arnaud Lacombe
  2011-07-13 12:16                           ` Pekka Enberg
  0 siblings, 1 reply; 27+ messages in thread
From: Arnaud Lacombe @ 2011-07-13  2:21 UTC (permalink / raw)
  To: Américo Wang
  Cc: Randy Dunlap, Mark Brown, Jesper Juhl, Pekka Enberg,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

Hi,

On Tue, Jul 12, 2011 at 9:15 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Jul 12, 2011 at 1:11 AM, Randy Dunlap <rdunlap@xenotime.net> wrote:
>> On Mon, 11 Jul 2011 06:09:52 +0100 Mark Brown wrote:
>>
>>> On Sun, Jul 10, 2011 at 04:53:33PM -0700, Randy Dunlap wrote:
>>>
>>> > But do as you like.  Which parts of SubmittingPatches do you think
>>> > support your interpretation?
>>>
>>> > and should we have this line:
>>> >     Acked-by: is not as formal as Signed-off-by:.
>>> > changed to:
>>> >     Acked-by: is not as formal as Signed-off-by: or Reviewed-by:.
>>> > e.g.?
>>>
>>> Current practice seems to be that Acked-by is used instead of
>>> Reviewed-by - the latter is comparatively rare.
>>
>>
>> ISTM that more education and encouragement are needed about Reviewed-by:.
>> (Patch Review is a possible kernel summit topic.)
>>
>> and that SubmittingPatches should be updated since we generally refer people
>> to that file and not to Documentation/development-process/
>
> Agreed, mind to send a patch? ;-)
>
>>
>> Samples from my partial mailing list archives:
>>
>> linux-pci mailing list:         Acked-by: 93    Reviewed-by: 81
>> linux-mm mailing list:          Acked-by: 2104  Reviewed-by: 1344
>> netdev mailing list:            Acked-by: 1366  Reviewed-by: 659
>>
>
> Yup, take netdev as an example, Davem is the only maintainer (not to
> say things like wireless)
> but definitely people like Eirc or Herbert is qualified to give Acked-by too.
>
I have the feeling from this thread that "Acked-by:" does not need any
particular qualification, whereas Reviewed-by: "kinda" does. But I may
have understood that all wrong. Btw, I say "kinda" as I see nothing in
the Reviewed-by: or Acked-by: definition that require any
qualification on the involved subsystem to give an Acked-by: or a
Reviewed-by:. Maybe we [not?] need such some formal requirement.

Just to highlight my point, you have never had any involvement[0] in
scripts/kconfig/, but still gave an Acked-by:, how trivial the
original patch might have been[1].

 - Arnaud

[0]: git's history can back me on this affirmation, no matter what you
affirm, even using the linux-glx-history.git tree, as long as kconfig
has been held in scripts/kconfig; .gitignore fixes does not count.

[1]: please do not see any kind attack here, that's not the point.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:16 ` Américo Wang
  2011-07-10 16:24   ` Arnaud Lacombe
@ 2011-07-13 11:39   ` Michal Marek
  2011-07-18 19:00   ` Arnaud Lacombe
  2 siblings, 0 replies; 27+ messages in thread
From: Michal Marek @ 2011-07-13 11:39 UTC (permalink / raw)
  To: Américo Wang
  Cc: Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Mon, Jul 11, 2011 at 12:16:11AM +0800, Américo Wang wrote:
> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> <rprabhu@wnohang.net> wrote:
> > Hi,
> >    I am seeing Wunused-but-set warning while make nconfig.  Looks like
> >    active_menu is not used. Removing it fixes the warning.
> >
> > Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
> 
> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>

Applied as "nconfig: Avoid Wunused-but-set warning" to
kbuild-2.6.git#kconfig. Raghavendra, please use the "subsystem: Fix this
and that" format of the subject line next time, thanks.

Now continue your fruitful discussion.

Michal

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-13  2:21                         ` Arnaud Lacombe
@ 2011-07-13 12:16                           ` Pekka Enberg
  0 siblings, 0 replies; 27+ messages in thread
From: Pekka Enberg @ 2011-07-13 12:16 UTC (permalink / raw)
  To: Arnaud Lacombe
  Cc: Américo Wang, Randy Dunlap, Mark Brown, Jesper Juhl,
	Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel

On Wed, Jul 13, 2011 at 5:21 AM, Arnaud Lacombe <lacombar@gmail.com> wrote:
> I have the feeling from this thread that "Acked-by:" does not need any
> particular qualification, whereas Reviewed-by: "kinda" does. But I may
> have understood that all wrong. Btw, I say "kinda" as I see nothing in
> the Reviewed-by: or Acked-by: definition that require any
> qualification on the involved subsystem to give an Acked-by: or a
> Reviewed-by:. Maybe we [not?] need such some formal requirement.

The formal requirement for 'Reviewed-by' is that you're OK with Linus
and the gang handing your ass to you on a plate when a patch you've
blessed with your tag breaks the world. 'Acked-by' is similar in
nature but the repercussions are less severe.

                       Pekka

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] Avoid Wunused-but-set warning
  2011-07-10 16:16 ` Américo Wang
  2011-07-10 16:24   ` Arnaud Lacombe
  2011-07-13 11:39   ` Michal Marek
@ 2011-07-18 19:00   ` Arnaud Lacombe
  2 siblings, 0 replies; 27+ messages in thread
From: Arnaud Lacombe @ 2011-07-18 19:00 UTC (permalink / raw)
  To: Michal Marek
  Cc: Raghavendra D Prabhu, linux-kbuild, Nir Tzachar, linux-kernel,
	Américo Wang

Hi,

On Sun, Jul 10, 2011 at 12:16 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Sun, Jul 10, 2011 at 12:24 AM, Raghavendra D Prabhu
> <rprabhu@wnohang.net> wrote:
>> Hi,
>>    I am seeing Wunused-but-set warning while make nconfig.  Looks like
>>    active_menu is not used. Removing it fixes the warning.
>>
>> Signed-off-by: Raghavendra D Prabhu <rprabhu@wnohang.net>
>
> Acked-by: WANG Cong <xiyou.wangcong@gmail.com>
>
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Michal, ping ?

 - Arnaud

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2011-07-18 19:00 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-09 16:24 [PATCH] Avoid Wunused-but-set warning Raghavendra D Prabhu
2011-07-10 16:16 ` Américo Wang
2011-07-10 16:24   ` Arnaud Lacombe
2011-07-10 16:28     ` Pekka Enberg
2011-07-10 16:49       ` Arnaud Lacombe
2011-07-10 16:51         ` Américo Wang
2011-07-10 17:22           ` Arnaud Lacombe
2011-07-10 17:24         ` Pekka Enberg
2011-07-10 18:02         ` Randy Dunlap
2011-07-10 22:56           ` Jesper Juhl
2011-07-10 23:07             ` Randy Dunlap
2011-07-10 23:21               ` Jesper Juhl
2011-07-10 23:53                 ` Randy Dunlap
2011-07-11  5:09                   ` Mark Brown
2011-07-11 17:11                     ` Randy Dunlap
2011-07-11 22:42                       ` Mark Brown
2011-07-13  1:15                       ` Américo Wang
2011-07-13  2:21                         ` Arnaud Lacombe
2011-07-13 12:16                           ` Pekka Enberg
2011-07-11 20:32                   ` Jesper Juhl
2011-07-11  5:55                 ` Pekka Enberg
2011-07-11 20:37                   ` Jesper Juhl
2011-07-13  0:35                   ` Valdis.Kletnieks
2011-07-13  0:55                     ` Randy Dunlap
2011-07-10 16:49     ` Américo Wang
2011-07-13 11:39   ` Michal Marek
2011-07-18 19:00   ` Arnaud Lacombe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.