Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 22 Dec 2015, Joe Perches wrote: > On Wed, 2015-12-23 at 13:03 +1100, Finn Thain wrote: > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > > > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > > > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > > > > This patch is just the result of two substitutions. The first > > > > > > removes any tabs and spaces at the end of the line. The second > > > > > > replaces runs of tabs and spaces at the beginning of comment lines > > > > > > with a single space. > > > > > > > > > > I think the second of these isn't done well. > > > > > > > > The aim of this patch is not to fix code style, but to make it > > > > possible to compare these two files so that the fork can be repaired. > > > > Regexp is very helpful in creating uniformity (and a minimal diff). > > > > > > > > If this was a coding style issue, we would be discussing the use of > > > > kernel-doc format for the affected comments, not whitespace. > > > > > > > > > Many of these comments post reformatting are much worse to read > > > > > because of lost alignment. > > > > > > > > You exaggerate a very trivial point. > > > > > > > > > > > > I prefer that all patches be improvements. > > > > > > > Agreed. But the example you cited is an improvement, in that it creates > > consistency. > > I think "consistency" isn't a useful argument. > The kernel code doesn't care about any other > external code bases. I prefer that the drivers I maintain be self-consistent. > > > Like you, I prefer to see formal parameters aligned when wrapped. But this > > isn't a formal parameter list, it is a comment, and no comment should > > duplicate code. > > > > Can you suggest a better regexp? Since this is patch 68 in the series, > > there is a good chance that it will need to be regenerated. > > I suggest you do 2 patches here. One that removes > unnecessary trailing spaces Those are resolved by this patch. > and converts multiple leading spaces to tabs where appropriate As I said, trivial cleanups are better done after the fork is resolved, to avoid churn. To assist with resolving the fork, this patch addresses inconsistencies. > and a > second patch that fixes whatever odd indentation > that does exist after comment leading *. Those are resolved by this patch. > I think > there aren't many instances of those and I think > those should be done by hand rather than regex. I don't know why a regexp wouldn't work and I don't know what you have in mind when you say "[fix] odd indentation". Is there some kind of style guide applicable here, which this patch violates? Upon re-reading this patch, I did find a table where I think the regexp is detrimental. @@ -2096,7 +2096,7 @@ static void NCR5380_information_transfer * Byte * 0EXTENDED_MESSAGE == 1 * 1length (includes one byte for code, doesn't -* include first two bytes) +* include first two bytes) * 2code * 3..length+1 arguments * This table is interesting. Even though the author took the trouble to duplicate a portion of the SCSI spec in the source, they were still able to stuff it up. See patch 44/77 in this series, "ncr5380: Fix off-by-one bug in extended_msg[] bounds check". So how about I remove this table in patch 67, along with the other dud comments, and then regenerate this patch. That way, perhaps we can all agree that the regexp is not actually detrimental? --
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 22 Dec 2015, James Bottomley wrote:
> I don't think it is trivial. I can't actually find a single instance in
> this patch where collapsing the space at the start of the comment looks
> justified; most of the time it eliminates intended formatting.
The present formatting is broken. It differs between the two core driver
forks. One uses spaces, the other tabs. For example, line 3.
$ grep -c "^ [*] *\t" drivers/scsi/{atari_,}NCR5380.c
drivers/scsi/atari_NCR5380.c:14
drivers/scsi/NCR5380.c:23
This patch resolves the issue by deliberately adopting an easy and
foolproof formatting convention.
But clearly there are different views as to what convention should be used
here. It would be great if you would indicate an acceptable convention so
we don't have to bikeshed the use of whitespace in comments.
To set an example, would you be kind enough to reformat, say, the comment
block at the top of the two files? Or some other comment where kernel-doc
is not appropriate, and the comment isn't merely duplicating actual code?
Thanks.
> Even if there's an odd one I've missed where space at the beginning of a
> comment is a problem, I think not doing that part of the regexp and just
> correcting the odd missed case by hand later will be much better.
>
> James
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Wed, 2015-12-23 at 13:03 +1100, Finn Thain wrote: > On Tue, 22 Dec 2015, Joe Perches wrote: > > > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > > > This patch is just the result of two substitutions. The first > > > > > removes any tabs and spaces at the end of the line. The second > > > > > replaces runs of tabs and spaces at the beginning of comment lines > > > > > with a single space. > > > > > > > > I think the second of these isn't done well. > > > > > > The aim of this patch is not to fix code style, but to make it > > > possible to compare these two files so that the fork can be repaired. > > > Regexp is very helpful in creating uniformity (and a minimal diff). > > > > > > If this was a coding style issue, we would be discussing the use of > > > kernel-doc format for the affected comments, not whitespace. > > > > > > > Many of these comments post reformatting are much worse to read > > > > because of lost alignment. > > > > > > You exaggerate a very trivial point. > > > > > > > > I prefer that all patches be improvements. > > > > Agreed. But the example you cited is an improvement, in that it creates > consistency. I think "consistency" isn't a useful argument. The kernel code doesn't care about any other external code bases. > Like you, I prefer to see formal parameters aligned when wrapped. But this > isn't a formal parameter list, it is a comment, and no comment should > duplicate code. > > Can you suggest a better regexp? Since this is patch 68 in the series, > there is a good chance that it will need to be regenerated. I suggest you do 2 patches here. One that removes unnecessary trailing spaces and converts multiple leading spaces to tabs where appropriate and a second patch that fixes whatever odd indentation that does exist after comment leading *. I think there aren't many instances of those and I think those should be done by hand rather than regex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 22 Dec 2015, Joe Perches wrote: > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > > This patch is just the result of two substitutions. The first > > > > removes any tabs and spaces at the end of the line. The second > > > > replaces runs of tabs and spaces at the beginning of comment lines > > > > with a single space. > > > > > > I think the second of these isn't done well. > > > > The aim of this patch is not to fix code style, but to make it > > possible to compare these two files so that the fork can be repaired. > > Regexp is very helpful in creating uniformity (and a minimal diff). > > > > If this was a coding style issue, we would be discussing the use of > > kernel-doc format for the affected comments, not whitespace. > > > > > Many of these comments post reformatting are much worse to read > > > because of lost alignment. > > > > You exaggerate a very trivial point. > > > > I prefer that all patches be improvements. > Agreed. But the example you cited is an improvement, in that it creates consistency. Like you, I prefer to see formal parameters aligned when wrapped. But this isn't a formal parameter list, it is a comment, and no comment should duplicate code. Can you suggest a better regexp? Since this is patch 68 in the series, there is a good chance that it will need to be regenerated. > > I admit that a small proportion of comments are slightly less > > readable. But it is the diff that needs to be readable in order to > > resolve the fork. > > diff -w works well. > Yes, it works well at times. For this I prefer to use meld. It does have text filters that allow the user to elide differences that match a given regexp. But I don't want every meld user to have to write those regexps. --
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > On Tue, 22 Dec 2015, Joe Perches wrote: > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > This patch is just the result of two substitutions. The first > removes > > > any tabs and spaces at the end of the line. The second replaces > runs > > > of tabs and spaces at the beginning of comment lines with a > single > > > space. > > > > I think the second of these isn't done well. > > The aim of this patch is not to fix code style, but to make it > possible to compare these two files so that the fork can be repaired. > Regexp is very helpful in creating uniformity (and a minimal diff). > > If this was a coding style issue, we would be discussing the use of > kernel-doc format for the affected comments, not whitespace. > > > Many of these comments post reformatting are > > much worse to read because of lost alignment. > > You exaggerate a very trivial point. > > I admit that a small proportion of comments are slightly less > readable. But it is the diff that needs to be readable in order to > resolve the fork. > > As I said in patch 0, I am aware that this patch series can be > faulted on trivial grounds but in order to avoid churn I don't wish > to address those issues until the fork has been resolved. I don't think it is trivial. I can't actually find a single instance in this patch where collapsing the space at the start of the comment looks justified; most of the time it eliminates intended formatting. Even if there's an odd one I've missed where space at the beginning of a comment is a problem, I think not doing that part of the regexp and just correcting the odd missed case by hand later will be much better. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > On Tue, 22 Dec 2015, Joe Perches wrote: > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > This patch is just the result of two substitutions. The first removes > > > any tabs and spaces at the end of the line. The second replaces runs > > > of tabs and spaces at the beginning of comment lines with a single > > > space. > > > > I think the second of these isn't done well. > > The aim of this patch is not to fix code style, but to make it possible to > compare these two files so that the fork can be repaired. Regexp is very > helpful in creating uniformity (and a minimal diff). > > If this was a coding style issue, we would be discussing the use of > kernel-doc format for the affected comments, not whitespace. > > > Many of these comments post reformatting are > > much worse to read because of lost alignment. > > You exaggerate a very trivial point. I prefer that all patches be improvements. > I admit that a small proportion of comments are slightly less readable. > But it is the diff that needs to be readable in order to resolve the fork. diff -w works well. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 22 Dec 2015, Joe Perches wrote: > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > This patch is just the result of two substitutions. The first removes > > any tabs and spaces at the end of the line. The second replaces runs > > of tabs and spaces at the beginning of comment lines with a single > > space. > > I think the second of these isn't done well. The aim of this patch is not to fix code style, but to make it possible to compare these two files so that the fork can be repaired. Regexp is very helpful in creating uniformity (and a minimal diff). If this was a coding style issue, we would be discussing the use of kernel-doc format for the affected comments, not whitespace. > Many of these comments post reformatting are > much worse to read because of lost alignment. You exaggerate a very trivial point. I admit that a small proportion of comments are slightly less readable. But it is the diff that needs to be readable in order to resolve the fork. As I said in patch 0, I am aware that this patch series can be faulted on trivial grounds but in order to avoid churn I don't wish to address those issues until the fork has been resolved. Thanks for your review. > > For instance: > > > +/* > > * Function : int NCR5380_select(struct Scsi_Host *instance, > > - * struct scsi_cmnd *cmd) > > + * struct scsi_cmnd *cmd) > > * > > * Purpose : establishes I_T_L or I_T_L_Q nexus for new or existing > > command, > > - * including ARBITRATION, SELECTION, and initial message out for > > - * IDENTIFY and queue messages. > > + * including ARBITRATION, SELECTION, and initial message out for > > + * IDENTIFY and queue messages. > --
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > This patch is just the result of two substitutions. The first removes any > tabs and spaces at the end of the line. The second replaces runs of > tabs and spaces at the beginning of comment lines with a single space. I think the second of these isn't done well. Many of these comments post reformatting are much worse to read because of lost alignment. For instance: > +/* > * Function : int NCR5380_select(struct Scsi_Host *instance, > - * struct scsi_cmnd *cmd) > + * struct scsi_cmnd *cmd) > * > * Purpose : establishes I_T_L or I_T_L_Q nexus for new or existing command, > - * including ARBITRATION, SELECTION, and initial message out for > - * IDENTIFY and queue messages. > + * including ARBITRATION, SELECTION, and initial message out for > + * IDENTIFY and queue messages. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On 12/22/2015 02:18 AM, Finn Thain wrote:
This patch is just the result of two substitutions. The first removes any
tabs and spaces at the end of the line. The second replaces runs of
tabs and spaces at the beginning of comment lines with a single space.
perl -i -pe 's,[\t ]+$,,; s,^(\t*[/ ]\*)[ \t]+,$1 ,'
drivers/scsi/{atari_,}NCR5380.c
This removes some unimportant discrepancies between the two core driver
forks so that 'diff' can be used to reveal the important ones, to
facilitate reunification.
Signed-off-by: Finn Thain
---
drivers/scsi/NCR5380.c | 550
+--
drivers/scsi/atari_NCR5380.c | 110
2 files changed, 330 insertions(+), 330 deletions(-)
Reviewed-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Wed, 2015-12-23 at 13:03 +1100, Finn Thain wrote: > On Tue, 22 Dec 2015, Joe Perches wrote: > > > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > > > This patch is just the result of two substitutions. The first > > > > > removes any tabs and spaces at the end of the line. The second > > > > > replaces runs of tabs and spaces at the beginning of comment lines > > > > > with a single space. > > > > > > > > I think the second of these isn't done well. > > > > > > The aim of this patch is not to fix code style, but to make it > > > possible to compare these two files so that the fork can be repaired. > > > Regexp is very helpful in creating uniformity (and a minimal diff). > > > > > > If this was a coding style issue, we would be discussing the use of > > > kernel-doc format for the affected comments, not whitespace. > > > > > > > Many of these comments post reformatting are much worse to read > > > > because of lost alignment. > > > > > > You exaggerate a very trivial point. > > > > > > > > I prefer that all patches be improvements. > > > > Agreed. But the example you cited is an improvement, in that it creates > consistency. I think "consistency" isn't a useful argument. The kernel code doesn't care about any other external code bases. > Like you, I prefer to see formal parameters aligned when wrapped. But this > isn't a formal parameter list, it is a comment, and no comment should > duplicate code. > > Can you suggest a better regexp? Since this is patch 68 in the series, > there is a good chance that it will need to be regenerated. I suggest you do 2 patches here. One that removes unnecessary trailing spaces and converts multiple leading spaces to tabs where appropriate and a second patch that fixes whatever odd indentation that does exist after comment leading *. I think there aren't many instances of those and I think those should be done by hand rather than regex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 22 Dec 2015, James Bottomley wrote:
> I don't think it is trivial. I can't actually find a single instance in
> this patch where collapsing the space at the start of the comment looks
> justified; most of the time it eliminates intended formatting.
The present formatting is broken. It differs between the two core driver
forks. One uses spaces, the other tabs. For example, line 3.
$ grep -c "^ [*] *\t" drivers/scsi/{atari_,}NCR5380.c
drivers/scsi/atari_NCR5380.c:14
drivers/scsi/NCR5380.c:23
This patch resolves the issue by deliberately adopting an easy and
foolproof formatting convention.
But clearly there are different views as to what convention should be used
here. It would be great if you would indicate an acceptable convention so
we don't have to bikeshed the use of whitespace in comments.
To set an example, would you be kind enough to reformat, say, the comment
block at the top of the two files? Or some other comment where kernel-doc
is not appropriate, and the comment isn't merely duplicating actual code?
Thanks.
> Even if there's an odd one I've missed where space at the beginning of a
> comment is a problem, I think not doing that part of the regexp and just
> correcting the odd missed case by hand later will be much better.
>
> James
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > On Tue, 22 Dec 2015, Joe Perches wrote: > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > This patch is just the result of two substitutions. The first > removes > > > any tabs and spaces at the end of the line. The second replaces > runs > > > of tabs and spaces at the beginning of comment lines with a > single > > > space. > > > > I think the second of these isn't done well. > > The aim of this patch is not to fix code style, but to make it > possible to compare these two files so that the fork can be repaired. > Regexp is very helpful in creating uniformity (and a minimal diff). > > If this was a coding style issue, we would be discussing the use of > kernel-doc format for the affected comments, not whitespace. > > > Many of these comments post reformatting are > > much worse to read because of lost alignment. > > You exaggerate a very trivial point. > > I admit that a small proportion of comments are slightly less > readable. But it is the diff that needs to be readable in order to > resolve the fork. > > As I said in patch 0, I am aware that this patch series can be > faulted on trivial grounds but in order to avoid churn I don't wish > to address those issues until the fork has been resolved. I don't think it is trivial. I can't actually find a single instance in this patch where collapsing the space at the start of the comment looks justified; most of the time it eliminates intended formatting. Even if there's an odd one I've missed where space at the beginning of a comment is a problem, I think not doing that part of the regexp and just correcting the odd missed case by hand later will be much better. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 22 Dec 2015, Joe Perches wrote: > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > This patch is just the result of two substitutions. The first removes > > any tabs and spaces at the end of the line. The second replaces runs > > of tabs and spaces at the beginning of comment lines with a single > > space. > > I think the second of these isn't done well. The aim of this patch is not to fix code style, but to make it possible to compare these two files so that the fork can be repaired. Regexp is very helpful in creating uniformity (and a minimal diff). If this was a coding style issue, we would be discussing the use of kernel-doc format for the affected comments, not whitespace. > Many of these comments post reformatting are > much worse to read because of lost alignment. You exaggerate a very trivial point. I admit that a small proportion of comments are slightly less readable. But it is the diff that needs to be readable in order to resolve the fork. As I said in patch 0, I am aware that this patch series can be faulted on trivial grounds but in order to avoid churn I don't wish to address those issues until the fork has been resolved. Thanks for your review. > > For instance: > > > +/* > > * Function : int NCR5380_select(struct Scsi_Host *instance, > > - * struct scsi_cmnd *cmd) > > + * struct scsi_cmnd *cmd) > > * > > * Purpose : establishes I_T_L or I_T_L_Q nexus for new or existing > > command, > > - * including ARBITRATION, SELECTION, and initial message out for > > - * IDENTIFY and queue messages. > > + * including ARBITRATION, SELECTION, and initial message out for > > + * IDENTIFY and queue messages. > --
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > On Tue, 22 Dec 2015, Joe Perches wrote: > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > This patch is just the result of two substitutions. The first removes > > > any tabs and spaces at the end of the line. The second replaces runs > > > of tabs and spaces at the beginning of comment lines with a single > > > space. > > > > I think the second of these isn't done well. > > The aim of this patch is not to fix code style, but to make it possible to > compare these two files so that the fork can be repaired. Regexp is very > helpful in creating uniformity (and a minimal diff). > > If this was a coding style issue, we would be discussing the use of > kernel-doc format for the affected comments, not whitespace. > > > Many of these comments post reformatting are > > much worse to read because of lost alignment. > > You exaggerate a very trivial point. I prefer that all patches be improvements. > I admit that a small proportion of comments are slightly less readable. > But it is the diff that needs to be readable in order to resolve the fork. diff -w works well. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 22 Dec 2015, Joe Perches wrote: > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > > This patch is just the result of two substitutions. The first > > > > removes any tabs and spaces at the end of the line. The second > > > > replaces runs of tabs and spaces at the beginning of comment lines > > > > with a single space. > > > > > > I think the second of these isn't done well. > > > > The aim of this patch is not to fix code style, but to make it > > possible to compare these two files so that the fork can be repaired. > > Regexp is very helpful in creating uniformity (and a minimal diff). > > > > If this was a coding style issue, we would be discussing the use of > > kernel-doc format for the affected comments, not whitespace. > > > > > Many of these comments post reformatting are much worse to read > > > because of lost alignment. > > > > You exaggerate a very trivial point. > > > > I prefer that all patches be improvements. > Agreed. But the example you cited is an improvement, in that it creates consistency. Like you, I prefer to see formal parameters aligned when wrapped. But this isn't a formal parameter list, it is a comment, and no comment should duplicate code. Can you suggest a better regexp? Since this is patch 68 in the series, there is a good chance that it will need to be regenerated. > > I admit that a small proportion of comments are slightly less > > readable. But it is the diff that needs to be readable in order to > > resolve the fork. > > diff -w works well. > Yes, it works well at times. For this I prefer to use meld. It does have text filters that allow the user to elide differences that match a given regexp. But I don't want every meld user to have to write those regexps. --
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 22 Dec 2015, Joe Perches wrote: > On Wed, 2015-12-23 at 13:03 +1100, Finn Thain wrote: > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > On Wed, 2015-12-23 at 11:56 +1100, Finn Thain wrote: > > > > On Tue, 22 Dec 2015, Joe Perches wrote: > > > > > > > > > On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > > > > > > This patch is just the result of two substitutions. The first > > > > > > removes any tabs and spaces at the end of the line. The second > > > > > > replaces runs of tabs and spaces at the beginning of comment lines > > > > > > with a single space. > > > > > > > > > > I think the second of these isn't done well. > > > > > > > > The aim of this patch is not to fix code style, but to make it > > > > possible to compare these two files so that the fork can be repaired. > > > > Regexp is very helpful in creating uniformity (and a minimal diff). > > > > > > > > If this was a coding style issue, we would be discussing the use of > > > > kernel-doc format for the affected comments, not whitespace. > > > > > > > > > Many of these comments post reformatting are much worse to read > > > > > because of lost alignment. > > > > > > > > You exaggerate a very trivial point. > > > > > > > > > > > > I prefer that all patches be improvements. > > > > > > > Agreed. But the example you cited is an improvement, in that it creates > > consistency. > > I think "consistency" isn't a useful argument. > The kernel code doesn't care about any other > external code bases. I prefer that the drivers I maintain be self-consistent. > > > Like you, I prefer to see formal parameters aligned when wrapped. But this > > isn't a formal parameter list, it is a comment, and no comment should > > duplicate code. > > > > Can you suggest a better regexp? Since this is patch 68 in the series, > > there is a good chance that it will need to be regenerated. > > I suggest you do 2 patches here. One that removes > unnecessary trailing spaces Those are resolved by this patch. > and converts multiple leading spaces to tabs where appropriate As I said, trivial cleanups are better done after the fork is resolved, to avoid churn. To assist with resolving the fork, this patch addresses inconsistencies. > and a > second patch that fixes whatever odd indentation > that does exist after comment leading *. Those are resolved by this patch. > I think > there aren't many instances of those and I think > those should be done by hand rather than regex. I don't know why a regexp wouldn't work and I don't know what you have in mind when you say "[fix] odd indentation". Is there some kind of style guide applicable here, which this patch violates? Upon re-reading this patch, I did find a table where I think the regexp is detrimental. @@ -2096,7 +2096,7 @@ static void NCR5380_information_transfer * Byte * 0EXTENDED_MESSAGE == 1 * 1length (includes one byte for code, doesn't -* include first two bytes) +* include first two bytes) * 2code * 3..length+1 arguments * This table is interesting. Even though the author took the trouble to duplicate a portion of the SCSI spec in the source, they were still able to stuff it up. See patch 44/77 in this series, "ncr5380: Fix off-by-one bug in extended_msg[] bounds check". So how about I remove this table in patch 67, along with the other dud comments, and then regenerate this patch. That way, perhaps we can all agree that the regexp is not actually detrimental? --
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On 12/22/2015 02:18 AM, Finn Thain wrote:
This patch is just the result of two substitutions. The first removes any
tabs and spaces at the end of the line. The second replaces runs of
tabs and spaces at the beginning of comment lines with a single space.
perl -i -pe 's,[\t ]+$,,; s,^(\t*[/ ]\*)[ \t]+,$1 ,'
drivers/scsi/{atari_,}NCR5380.c
This removes some unimportant discrepancies between the two core driver
forks so that 'diff' can be used to reveal the important ones, to
facilitate reunification.
Signed-off-by: Finn Thain
---
drivers/scsi/NCR5380.c | 550
+--
drivers/scsi/atari_NCR5380.c | 110
2 files changed, 330 insertions(+), 330 deletions(-)
Reviewed-by: Hannes Reinecke
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
h...@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
On Tue, 2015-12-22 at 12:18 +1100, Finn Thain wrote: > This patch is just the result of two substitutions. The first removes any > tabs and spaces at the end of the line. The second replaces runs of > tabs and spaces at the beginning of comment lines with a single space. I think the second of these isn't done well. Many of these comments post reformatting are much worse to read because of lost alignment. For instance: > +/* > * Function : int NCR5380_select(struct Scsi_Host *instance, > - * struct scsi_cmnd *cmd) > + * struct scsi_cmnd *cmd) > * > * Purpose : establishes I_T_L or I_T_L_Q nexus for new or existing command, > - * including ARBITRATION, SELECTION, and initial message out for > - * IDENTIFY and queue messages. > + * including ARBITRATION, SELECTION, and initial message out for > + * IDENTIFY and queue messages. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
This patch is just the result of two substitutions. The first removes any
tabs and spaces at the end of the line. The second replaces runs of
tabs and spaces at the beginning of comment lines with a single space.
perl -i -pe 's,[\t ]+$,,; s,^(\t*[/ ]\*)[ \t]+,$1 ,'
drivers/scsi/{atari_,}NCR5380.c
This removes some unimportant discrepancies between the two core driver
forks so that 'diff' can be used to reveal the important ones, to
facilitate reunification.
Signed-off-by: Finn Thain
---
drivers/scsi/NCR5380.c | 550 +--
drivers/scsi/atari_NCR5380.c | 110
2 files changed, 330 insertions(+), 330 deletions(-)
Index: linux/drivers/scsi/NCR5380.c
===
--- linux.orig/drivers/scsi/NCR5380.c 2015-12-22 12:17:18.0 +1100
+++ linux/drivers/scsi/NCR5380.c2015-12-22 12:17:19.0 +1100
@@ -1,17 +1,17 @@
-/*
+/*
* NCR 5380 generic driver routines. These should make it *trivial*
- * to implement 5380 SCSI drivers under Linux with a non-trantor
- * architecture.
+ * to implement 5380 SCSI drivers under Linux with a non-trantor
+ * architecture.
*
- * Note that these routines also work with NR53c400 family chips.
+ * Note that these routines also work with NR53c400 family chips.
*
* Copyright 1993, Drew Eckhardt
- * Visionary Computing
- * (Unix and Linux consulting and custom programming)
- * d...@colorado.edu
- * +1 (303) 666-5836
+ * Visionary Computing
+ * (Unix and Linux consulting and custom programming)
+ * d...@colorado.edu
+ * +1 (303) 666-5836
*
- * For more information, please consult
+ * For more information, please consult
*
* NCR 5380 Family
* SCSI Protocol Controller
@@ -30,17 +30,17 @@
*/
/*
- * Further development / testing that should be done :
+ * Further development / testing that should be done :
* 1. Cleanup the NCR5380_transfer_dma function and DMA operation complete
- * code so that everything does the same thing that's done at the
- * end of a pseudo-DMA read operation.
+ * code so that everything does the same thing that's done at the
+ * end of a pseudo-DMA read operation.
*
* 2. Fix REAL_DMA (interrupt driven, polled works fine) -
- * basically, transfer size needs to be reduced by one
- * and the last byte read as is done with PSEUDO_DMA.
- *
- * 4. Test SCSI-II tagged queueing (I have no devices which support
- * tagged queueing)
+ * basically, transfer size needs to be reduced by one
+ * and the last byte read as is done with PSEUDO_DMA.
+ *
+ * 4. Test SCSI-II tagged queueing (I have no devices which support
+ * tagged queueing)
*/
#ifndef notyet
@@ -56,27 +56,27 @@
/*
* Design
*
- * This is a generic 5380 driver. To use it on a different platform,
+ * This is a generic 5380 driver. To use it on a different platform,
* one simply writes appropriate system specific macros (ie, data
- * transfer - some PC's will use the I/O bus, 68K's must use
+ * transfer - some PC's will use the I/O bus, 68K's must use
* memory mapped) and drops this file in their 'C' wrapper.
*
- * As far as command queueing, two queues are maintained for
+ * As far as command queueing, two queues are maintained for
* each 5380 in the system - commands that haven't been issued yet,
- * and commands that are currently executing. This means that an
- * unlimited number of commands may be queued, letting
- * more commands propagate from the higher driver levels giving higher
- * throughput. Note that both I_T_L and I_T_L_Q nexuses are supported,
- * allowing multiple commands to propagate all the way to a SCSI-II device
+ * and commands that are currently executing. This means that an
+ * unlimited number of commands may be queued, letting
+ * more commands propagate from the higher driver levels giving higher
+ * throughput. Note that both I_T_L and I_T_L_Q nexuses are supported,
+ * allowing multiple commands to propagate all the way to a SCSI-II device
* while a command is already executing.
*
*
- * Issues specific to the NCR5380 :
+ * Issues specific to the NCR5380 :
*
- * When used in a PIO or pseudo-dma mode, the NCR5380 is a braindead
- * piece of hardware that requires you to sit in a loop polling for
- * the REQ signal as long as you are connected. Some devices are
- * brain dead (ie, many TEXEL CD ROM drives) and won't disconnect
+ * When used in a PIO or pseudo-dma mode, the NCR5380 is a braindead
+ * piece of hardware that requires you to sit in a loop polling for
+ * the REQ signal as long as you are connected. Some devices are
+ * brain dead (ie, many TEXEL CD ROM drives) and won't disconnect
* while doing long seek operations. [...] These
* broken devices are the exception rather than the rule and I'd rather
* spend my time optimizing for the normal case.
@@ -87,23 +87,23 @@
* which is started from a workqueue
[PATCH v3 68/77] ncr5380: Fix whitespace issues using regexp
This patch is just the result of two substitutions. The first removes any
tabs and spaces at the end of the line. The second replaces runs of
tabs and spaces at the beginning of comment lines with a single space.
perl -i -pe 's,[\t ]+$,,; s,^(\t*[/ ]\*)[ \t]+,$1 ,'
drivers/scsi/{atari_,}NCR5380.c
This removes some unimportant discrepancies between the two core driver
forks so that 'diff' can be used to reveal the important ones, to
facilitate reunification.
Signed-off-by: Finn Thain
---
drivers/scsi/NCR5380.c | 550 +--
drivers/scsi/atari_NCR5380.c | 110
2 files changed, 330 insertions(+), 330 deletions(-)
Index: linux/drivers/scsi/NCR5380.c
===
--- linux.orig/drivers/scsi/NCR5380.c 2015-12-22 12:17:18.0 +1100
+++ linux/drivers/scsi/NCR5380.c2015-12-22 12:17:19.0 +1100
@@ -1,17 +1,17 @@
-/*
+/*
* NCR 5380 generic driver routines. These should make it *trivial*
- * to implement 5380 SCSI drivers under Linux with a non-trantor
- * architecture.
+ * to implement 5380 SCSI drivers under Linux with a non-trantor
+ * architecture.
*
- * Note that these routines also work with NR53c400 family chips.
+ * Note that these routines also work with NR53c400 family chips.
*
* Copyright 1993, Drew Eckhardt
- * Visionary Computing
- * (Unix and Linux consulting and custom programming)
- * d...@colorado.edu
- * +1 (303) 666-5836
+ * Visionary Computing
+ * (Unix and Linux consulting and custom programming)
+ * d...@colorado.edu
+ * +1 (303) 666-5836
*
- * For more information, please consult
+ * For more information, please consult
*
* NCR 5380 Family
* SCSI Protocol Controller
@@ -30,17 +30,17 @@
*/
/*
- * Further development / testing that should be done :
+ * Further development / testing that should be done :
* 1. Cleanup the NCR5380_transfer_dma function and DMA operation complete
- * code so that everything does the same thing that's done at the
- * end of a pseudo-DMA read operation.
+ * code so that everything does the same thing that's done at the
+ * end of a pseudo-DMA read operation.
*
* 2. Fix REAL_DMA (interrupt driven, polled works fine) -
- * basically, transfer size needs to be reduced by one
- * and the last byte read as is done with PSEUDO_DMA.
- *
- * 4. Test SCSI-II tagged queueing (I have no devices which support
- * tagged queueing)
+ * basically, transfer size needs to be reduced by one
+ * and the last byte read as is done with PSEUDO_DMA.
+ *
+ * 4. Test SCSI-II tagged queueing (I have no devices which support
+ * tagged queueing)
*/
#ifndef notyet
@@ -56,27 +56,27 @@
/*
* Design
*
- * This is a generic 5380 driver. To use it on a different platform,
+ * This is a generic 5380 driver. To use it on a different platform,
* one simply writes appropriate system specific macros (ie, data
- * transfer - some PC's will use the I/O bus, 68K's must use
+ * transfer - some PC's will use the I/O bus, 68K's must use
* memory mapped) and drops this file in their 'C' wrapper.
*
- * As far as command queueing, two queues are maintained for
+ * As far as command queueing, two queues are maintained for
* each 5380 in the system - commands that haven't been issued yet,
- * and commands that are currently executing. This means that an
- * unlimited number of commands may be queued, letting
- * more commands propagate from the higher driver levels giving higher
- * throughput. Note that both I_T_L and I_T_L_Q nexuses are supported,
- * allowing multiple commands to propagate all the way to a SCSI-II device
+ * and commands that are currently executing. This means that an
+ * unlimited number of commands may be queued, letting
+ * more commands propagate from the higher driver levels giving higher
+ * throughput. Note that both I_T_L and I_T_L_Q nexuses are supported,
+ * allowing multiple commands to propagate all the way to a SCSI-II device
* while a command is already executing.
*
*
- * Issues specific to the NCR5380 :
+ * Issues specific to the NCR5380 :
*
- * When used in a PIO or pseudo-dma mode, the NCR5380 is a braindead
- * piece of hardware that requires you to sit in a loop polling for
- * the REQ signal as long as you are connected. Some devices are
- * brain dead (ie, many TEXEL CD ROM drives) and won't disconnect
+ * When used in a PIO or pseudo-dma mode, the NCR5380 is a braindead
+ * piece of hardware that requires you to sit in a loop polling for
+ * the REQ signal as long as you are connected. Some devices are
+ * brain dead (ie, many TEXEL CD ROM drives) and won't disconnect
* while doing long seek operations. [...] These
* broken devices are the exception rather than the rule and I'd rather
* spend my time optimizing for the normal case.
@@ -87,23 +87,23 @@
* which is
