Re: [PATCH] Fixed most indent issues in tty_io.c

Re: [PATCH] Fixed most indent issues in tty_io.c

[Sandro Volery]

>>> On 7 Sep 2019, at 19:29, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote:
>> Dear Greg,
>> I am pretty sure the issue was, that I did too many things at once. However, all the things I did are related to spaces / tabs, maybe that still works?
> 
> <snip>
> 
> For some reason you sent this only to me, which is a bit rude to
> everyone else on the mailing list.  I'll be glad to respond if you
> resend it to everyone.

I'm sorry, newbie here. I thought it'd be better to not annoy everyone with responses but learning things everyday I guess :)

I am pretty sure the issue with my patch was that there was too many changes, however all of them were spaces and tabs related, so I think this could be fine?

Sincerely,
Sandro V

Re: [PATCH] Fixed most indent issues in tty_io.c

[Greg KH]

On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote:
> 
> 
> >>> On 7 Sep 2019, at 19:29, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote:
> >> Dear Greg,
> >> I am pretty sure the issue was, that I did too many things at once. However, all the things I did are related to spaces / tabs, maybe that still works?
> > 
> > <snip>
> > 
> > For some reason you sent this only to me, which is a bit rude to
> > everyone else on the mailing list.  I'll be glad to respond if you
> > resend it to everyone.
> 
> I'm sorry, newbie here. I thought it'd be better to not annoy everyone with responses but learning things everyday I guess :)

No problem, but you should also line-wrap your emails :)

> I am pretty sure the issue with my patch was that there was too many changes, however all of them were spaces and tabs related, so I think this could be fine?

As the bot said, break it out into "one patch per logical change", and
"fix all whitespace issues" is not "one logical change".

thanks,

greg k-h

Re: [PATCH] Fixed most indent issues in tty_io.c

[Sandro Volery]

> On 7 Sep 2019, at 19:42, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote:
>> 
>> 
>>>>>> On 7 Sep 2019, at 19:29, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote:
>>>>> Dear Greg,
>>>>> I am pretty sure the issue was, that I did too many things at once. However, all the things I did are related to spaces / tabs, maybe that still works?
>>> 
>>> <snip>
>>> 
>>> For some reason you sent this only to me, which is a bit rude to
>>> everyone else on the mailing list.  I'll be glad to respond if you
>>> resend it to everyone.
>> 
>> I'm sorry, newbie here. I thought it'd be better to not annoy everyone with responses but learning things everyday I guess :)
> 
> No problem, but you should also line-wrap your emails :)

I've just been told that before, I'll try to change 
those settings asap.

> 
>> I am pretty sure the issue with my patch was that there was too many changes, however all of them were spaces and tabs related, so I think this could be fine?
> 
> As the bot said, break it out into "one patch per logical change", and
> "fix all whitespace issues" is not "one logical change".

So a logical change would be if I make one patch
to replace all spaces with tabs and a separate
patch to add a space before the : ?

Thanks,
Sandro V.

Re: [PATCH] Fixed most indent issues in tty_io.c

[Greg KH]

On Sat, Sep 07, 2019 at 07:49:43PM +0200, Sandro Volery wrote:
> 
> 
> 
> 
> > On 7 Sep 2019, at 19:42, Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote:
> >> 
> >> 
> >>>>>> On 7 Sep 2019, at 19:29, Greg KH <gregkh@linuxfoundation.org> wrote:
> >>>>> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote:
> >>>>> Dear Greg,
> >>>>> I am pretty sure the issue was, that I did too many things at once. However, all the things I did are related to spaces / tabs, maybe that still works?
> >>> 
> >>> <snip>
> >>> 
> >>> For some reason you sent this only to me, which is a bit rude to
> >>> everyone else on the mailing list.  I'll be glad to respond if you
> >>> resend it to everyone.
> >> 
> >> I'm sorry, newbie here. I thought it'd be better to not annoy everyone with responses but learning things everyday I guess :)
> > 
> > No problem, but you should also line-wrap your emails :)
> 
> I've just been told that before, I'll try to change 
> those settings asap.
> 
> > 
> >> I am pretty sure the issue with my patch was that there was too many changes, however all of them were spaces and tabs related, so I think this could be fine?
> > 
> > As the bot said, break it out into "one patch per logical change", and
> > "fix all whitespace issues" is not "one logical change".
> 
> So a logical change would be if I make one patch
> to replace all spaces with tabs and a separate
> patch to add a space before the : ?

Yes, that would be good.  Make it a patch series please.

thanks,

greg k-h

Re: [PATCH] Fixed most indent issues in tty_io.c

[Sandro Volery LKML]

Sandro V

> On 7 Sep 2019, at 20:03, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Sat, Sep 07, 2019 at 07:49:43PM +0200, Sandro Volery wrote:
>> 
>> 
>> 
>> 
>>>> On 7 Sep 2019, at 19:42, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> 
>>> On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote:
>>>> 
>>>> 
>>>>>>>> On 7 Sep 2019, at 19:29, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>>>> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote:
>>>>>>> Dear Greg,
>>>>>>> I am pretty sure the issue was, that I did too many things at once. However, all the things I did are related to spaces / tabs, maybe that still works?
>>>>> 
>>>>> <snip>
>>>>> 
>>>>> For some reason you sent this only to me, which is a bit rude to
>>>>> everyone else on the mailing list.  I'll be glad to respond if you
>>>>> resend it to everyone.
>>>> 
>>>> I'm sorry, newbie here. I thought it'd be better to not annoy everyone with responses but learning things everyday I guess :)
>>> 
>>> No problem, but you should also line-wrap your emails :)
>> 
>> I've just been told that before, I'll try to change 
>> those settings asap.
>> 
>>> 
>>>> I am pretty sure the issue with my patch was that there was too many changes, however all of them were spaces and tabs related, so I think this could be fine?
>>> 
>>> As the bot said, break it out into "one patch per logical change", and
>>> "fix all whitespace issues" is not "one logical change".
>> 
>> So a logical change would be if I make one patch
>> to replace all spaces with tabs and a separate
>> patch to add a space before the : ?
> 
> Yes, that would be good.  Make it a patch series please.

Thanks!

Any suggestion on how I should do the line wrapping
when I want to send emails from my phone?
Since I don't always have my laptop handy.

Sandro V.

Re: [PATCH] Fixed most indent issues in tty_io.c

[Joe Perches]

On Sat, 2019-09-07 at 18:42 +0100, Greg KH wrote:
> On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote:
> > 
> > > > > On 7 Sep 2019, at 19:29, Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote:
> > > > Dear Greg,
> > > > I am pretty sure the issue was, that I did too many things at once. However, all the things I did are related to spaces / tabs, maybe that still works?
> > > 
> > > <snip>
> > > 
> > > For some reason you sent this only to me, which is a bit rude to
> > > everyone else on the mailing list.  I'll be glad to respond if you
> > > resend it to everyone.
> > 
> > I'm sorry, newbie here. I thought it'd be better to not annoy everyone with responses but learning things everyday I guess :)
> 
> No problem, but you should also line-wrap your emails :)
> 
> > I am pretty sure the issue with my patch was that there was too many changes, however all of them were spaces and tabs related, so I think this could be fine?
> 
> As the bot said, break it out into "one patch per logical change", and
> "fix all whitespace issues" is not "one logical change".

As long as git diff -w shows no difference and a compiled
object comparison before and after the change shows no
difference, I think it's fine.

In fact, it avoid multiple commits where the only changes
are whitespace.

Re: [PATCH] Fixed most indent issues in tty_io.c

[Sandro Volery]

> On 7 Sep 2019, at 21:27, Joe Perches <joe@perches.com> wrote:
> 
> On Sat, 2019-09-07 at 18:42 +0100, Greg KH wrote:
>>> On Sat, Sep 07, 2019 at 07:35:42PM +0200, Sandro Volery wrote:
>>> 
>>>>>> On 7 Sep 2019, at 19:29, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>> On Sat, Sep 07, 2019 at 07:23:59PM +0200, Sandro Volery wrote:
>>>>> Dear Greg,
>>>>> I am pretty sure the issue was, that I did too many things at once. However, all the things I did are related to spaces / tabs, maybe that still works?
>>>> 
>>>> <snip>
>>>> 
>>>> For some reason you sent this only to me, which is a bit rude to
>>>> everyone else on the mailing list.  I'll be glad to respond if you
>>>> resend it to everyone.
>>> 
>>> I'm sorry, newbie here. I thought it'd be better to not annoy everyone with responses but learning things everyday I guess :)
>> 
>> No problem, but you should also line-wrap your emails :)
>> 
>>> I am pretty sure the issue with my patch was that there was too many changes, however all of them were spaces and tabs related, so I think this could be fine?
>> 
>> As the bot said, break it out into "one patch per logical change", and
>> "fix all whitespace issues" is not "one logical change".
> 
> As long as git diff -w shows no difference and a compiled
> object comparison before and after the change shows no
> difference, I think it's fine.

My thoughts, too. I didn't feel comfortable arguing as a newbie tho
so I'll see what I can do once I get home.

> 
> In fact, it avoid multiple commits where the only changes
> are whitespace.
> 
> 
> 

Re: [PATCH] Fixed most indent issues in tty_io.c

[Joe Perches]

On Sat, 2019-09-07 at 21:51 +0200, Sandro Volery wrote:
> > On 7 Sep 2019, at 21:27, Joe Perches <joe@perches.com> wrote:
[]
> > As long as git diff -w shows no difference and a compiled
> > object comparison before and after the change shows no
> > difference, I think it's fine.
> 
> My thoughts, too. I didn't feel comfortable arguing as a newbie tho
> so I'll see what I can do once I get home.

If you do that, it's important to mention both elements in
the commit message:

	1: git diff -w shows no difference
	2: pre and post compilation objects are identical

It is also good to verify that allyesconfig and defconfig
objects with the minimal CONFIG_ required for compilation
are also identical.

Whitespace only changes should only change horizontal
spacing and should not have vertical line changes.

Re: [PATCH] Fixed most indent issues in tty_io.c

[Sandro Volery]

> On 8 Sep 2019, at 22:59, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> 

Hi Ted,

Thank you, for elaborating all this to me! I will try to limit my patches to the staging tree, until I feel confident enough to tackle a real coding issue in the kernel. 

Thanks,
Sandro V

[PATCH] Fixed most indent issues in tty_io.c

[volery]

There were a lot of styling problems using space then tab or spaces
instead of tabs in that file. Especially the entire function at line
2677.
Also added a space before the : on line 2221.

Signed-off-by: Sandro Volery <sandro@volery.com>
---
 drivers/tty/tty_io.c | 60 ++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 566728fbaf3c..38c36ff7221b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -33,7 +33,7 @@
  *	-- Nick Holloway <alfie@dcs.warwick.ac.uk>, 27th May 1993.
  *
  * Rewrote canonical mode and added more termios flags.
- * 	-- julian@uhunix.uhcc.hawaii.edu (J. Cowley), 13Jan94
+ *	-- julian@uhunix.uhcc.hawaii.edu (J. Cowley), 13Jan94
  *
  * Reorganized FASYNC support so mouse code can share it.
  *	-- ctm@ardi.com, 9Sep95
@@ -1026,13 +1026,13 @@ static ssize_t tty_write(struct file *file, const char __user *buf,
 						size_t count, loff_t *ppos)
 {
 	struct tty_struct *tty = file_tty(file);
- 	struct tty_ldisc *ld;
+	struct tty_ldisc *ld;
 	ssize_t ret;
 
 	if (tty_paranoia_check(tty, file_inode(file), "tty_write"))
 		return -EIO;
 	if (!tty || !tty->ops->write ||	tty_io_error(tty))
-			return -EIO;
+		return -EIO;
 	/* Short term debug to catch buggy drivers */
 	if (tty->ops->write_room == NULL)
 		tty_err(tty, "missing write_room method\n");
@@ -1247,8 +1247,8 @@ static void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *
 }
 
 /*
- * 	tty_reopen()	- fast re-open of an open tty
- * 	@tty	- the tty to open
+ *	tty_reopen()	- fast re-open of an open tty
+ *	@tty	- the tty to open
  *
  *	Return 0 on success, -errno on error.
  *	Re-opens on master ptys are not allowed and return -EIO.
@@ -1829,8 +1829,8 @@ static struct tty_struct *tty_open_current_tty(dev_t device, struct file *filp)
  *	@index: index for the device in the @return driver
  *	@return: driver for this inode (with increased refcount)
  *
- * 	If @return is not erroneous, the caller is responsible to decrement the
- * 	refcount by tty_driver_kref_put.
+ *	If @return is not erroneous, the caller is responsible to decrement the
+ *	refcount by tty_driver_kref_put.
  *
  *	Locking: tty_mutex protects get_tty_driver
  */
@@ -2218,7 +2218,7 @@ static int tiocgwinsz(struct tty_struct *tty, struct winsize __user *arg)
 	err = copy_to_user(arg, &tty->winsize, sizeof(*arg));
 	mutex_unlock(&tty->winsize_mutex);
 
-	return err ? -EFAULT: 0;
+	return err ? -EFAULT : 0;
 }
 
 /**
@@ -2674,25 +2674,25 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 #ifdef CONFIG_COMPAT
 
 struct serial_struct32 {
-        compat_int_t    type;
-        compat_int_t    line;
-        compat_uint_t   port;
-        compat_int_t    irq;
-        compat_int_t    flags;
-        compat_int_t    xmit_fifo_size;
-        compat_int_t    custom_divisor;
-        compat_int_t    baud_base;
-        unsigned short  close_delay;
-        char    io_type;
-        char    reserved_char[1];
-        compat_int_t    hub6;
-        unsigned short  closing_wait; /* time to wait before closing */
-        unsigned short  closing_wait2; /* no longer used... */
-        compat_uint_t   iomem_base;
-        unsigned short  iomem_reg_shift;
-        unsigned int    port_high;
+	compat_int_t    type;
+	compat_int_t    line;
+	compat_uint_t   port;
+	compat_int_t    irq;
+	compat_int_t    flags;
+	compat_int_t    xmit_fifo_size;
+	compat_int_t    custom_divisor;
+	compat_int_t    baud_base;
+	unsigned short  close_delay;
+	char    io_type;
+	char    reserved_char[1];
+	compat_int_t    hub6;
+	unsigned short  closing_wait; /* time to wait before closing */
+	unsigned short  closing_wait2; /* no longer used... */
+	compat_uint_t   iomem_base;
+	unsigned short  iomem_reg_shift;
+	unsigned int    port_high;
      /* compat_ulong_t  iomap_base FIXME */
-        compat_int_t    reserved[1];
+	compat_int_t    reserved[1];
 };
 
 static int compat_tty_tiocsserial(struct tty_struct *tty,
@@ -3176,11 +3176,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 EXPORT_SYMBOL_GPL(tty_register_device_attr);
 
 /**
- * 	tty_unregister_device - unregister a tty device
- * 	@driver: the tty driver that describes the tty device
- * 	@index: the index in the tty driver for this tty device
+ *	tty_unregister_device - unregister a tty device
+ *	@driver: the tty driver that describes the tty device
+ *	@index: the index in the tty driver for this tty device
  *
- * 	If a tty device is registered with a call to tty_register_device() then
+ *	If a tty device is registered with a call to tty_register_device() then
  *	this function must be called when the tty device is gone.
  *
  *	Locking: ??
-- 
2.23.0

Re: [PATCH] Fixed most indent issues in tty_io.c

[Greg KH]

On Sat, Sep 07, 2019 at 11:09:48AM +0200, volery wrote:
> There were a lot of styling problems using space then tab or spaces
> instead of tabs in that file. Especially the entire function at line
> 2677.
> Also added a space before the : on line 2221.
> 
> Signed-off-by: Sandro Volery <sandro@volery.com>
> ---
>  drivers/tty/tty_io.c | 60 ++++++++++++++++++++++----------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch breaks the build.

- Your patch did many different things all at once, making it difficult
  to review.  All Linux kernel patches need to only do one thing at a
  time.  If you need to do multiple things (such as clean up all coding
  style issues in a file/driver), do it in a sequence of patches, each
  one doing only one thing.  This will make it easier to review the
  patches to ensure that they are correct, and to help alleviate any
  merge issues that larger patches can cause.

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what is needed in order to
  properly describe the change.

- You did not write a descriptive Subject: for the patch, allowing Greg,
  and everyone else, to know what this patch is all about.  Please read
  the section entitled "The canonical patch format" in the kernel file,
  Documentation/SubmittingPatches for what a proper Subject: line should
  look like.

- It looks like you did not use your "real" name for the patch on either
  the Signed-off-by: line, or the From: line (both of which have to
  match).  Please read the kernel file, Documentation/SubmittingPatches
  for how to do this correctly.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH] Fixed most indent issues in tty_io.c

[Joe Perches]

On Sat, 2019-09-07 at 11:09 +0200, volery wrote:
> There were a lot of styling problems using space then tab or spaces
> instead of tabs in that file. Especially the entire function at line
> 2677.
> Also added a space before the : on line 2221.

You do not have an appropriate subject line.

This subject should probably be something like:

[PATCH] tty: tty_io: Use normal indentation

Please make changes only in drivers/staging until you are
really familiar with the linux kernel patch process.

Re: [PATCH] Fixed most indent issues in tty_io.c

[Sandro Volery]

Sandro V

>> On 7 Sep 2019, at 22:04, Joe Perches <joe@perches.com> wrote:
>> 
>> On Sat, 2019-09-07 at 11:09 +0200, volery wrote:
>> There were a lot of styling problems using space then tab or spaces
>> instead of tabs in that file. Especially the entire function at line
>> 2677.
>> Also added a space before the : on line 2221.
> 
> You do not have an appropriate subject line.
> 
> This subject should probably be something like:
> 
> [PATCH] tty: tty_io: Use normal indentation
> 
> Please make changes only in drivers/staging until you are
> really familiar with the linux kernel patch process.
Yep, noticed that I had the subject line wrong on this one too 
after Dan told me about that today.
I'll take that drivers/staging thing into account :) 

Thanks again
Sandro

Re: [PATCH] Fixed most indent issues in tty_io.c

[Theodore Y. Ts'o]

Hi Sandro,

It's not mentioned in the process documentation (but maybe we should
add this), is that it's up to individual maintainers about whether or
not whitespace cleanups are accepted outside of the staging tree.

That's because whitespace cleanups are a great "training wheel" for
newbies who are learning the ropes, but they do have some costs.  For
example, for actively developed portions of the kernel whitespace
cleans can often break other pending changes.  Also, trivial cleanups
(e.g., spelling and whitespace cleanups) makes it more likely that
future bug fixes in that portion of the kernel will fail to be
automatically backported to the stable kernel, thus requiring a manual
backport effort.  

As a result, some maintainers will reject trivial cleanups unless they
are part of a patch series that is making some kind of substantive
improvement to the kernel (beyond trivial cleanups).

There are some good aspects of fixing whitespace issues, of course,
which is why they are encouraged in the staging tree, but there is not
consensus amongst maintainers about whether it is a net benefit to do
clean up patches just for the sake of doing cleanup patches.

(And of course, sometimes the checkpatch rules change over time --- at
one point, checkpatch would warn if *any* line was longer than 80
characters, and so there were tons and tons of trivial cleanups to
"fix" this, including breaking up strings.  When enough people
complained that this actually made it harder to find kernel messages
that got split, checkpatch changed to complain when strings were split
across lines, and more trivial patches got sent out undoing previous
trivial patches.  And this caused all of the same downsides of
breaking automated stable backports, *twice*.  As such, newbies are
strongly encouraged to restrict their checkpatch cleanups to the
staging tree, since when such cleanup patches are considered welcome
very much depends on the kernel subsystem and the maintainers
involved.)

Cheers,

						- Ted

Re: [PATCH] Fixed most indent issues in tty_io.c

[Sandro Volery]

> strongly encouraged to restrict their checkpatch cleanups to the
> staging tree, since when such cleanup patches are considered welcome
> very much depends on the kernel subsystem and the maintainers
> involved.)

Should I still send the ones I tried to submit in this thread again
as separate patches? Since Greg said I should do it that way
so I'm not sure if I should do that quick or just stick to staging.

Thanks,
Sandro V.