| Message ID | 71b06567-1ea9-204d-d61b-2af4426d9cd1@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 12, 2016 at 8:44 PM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sun, 11 Sep 2016 13:37:34 +0200 > > Add a space character before a single jump label in this function > according to the current Linux coding style convention. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/block/rbd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > index f4212e1..d61a066 100644 > --- a/drivers/block/rbd.c > +++ b/drivers/block/rbd.c > @@ -1064,7 +1064,7 @@ static int rbd_header_from_disk(struct rbd_device *rbd_dev, > header->snap_sizes = snap_sizes; > > return 0; > -out_2big: > + out_2big: > ret = -EIO; > kfree(snap_sizes); > free_names: > -- > 2.10.0 > Can you point where this current convention is documented? Certainly not in CodingStyle, AFAICT... I know some people prefer a single space in there because it makes "diff -p" work better, but nowadays with "git diff" this argument is pretty moot. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> @@ -1064,7 +1064,7 @@ static int rbd_header_from_disk(struct rbd_device *rbd_dev, >> header->snap_sizes = snap_sizes; >> >> return 0; >> -out_2big: >> + out_2big: >> ret = -EIO; >> kfree(snap_sizes); >> free_names: … > Can you point where this current convention is documented? Yes. https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51 Do you find the software update "CodingStyle: Clarify and complete chapter 7" interesting? > Certainly not in CodingStyle, AFAICT... I suggest to look at the current version once more. > I know some people prefer a single space in there because it makes > "diff -p" work better, but nowadays with "git diff" this argument is > pretty moot. Would you like to discuss the corresponding software evolution a bit more? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 13, 2016 at 10:12 AM, SF Markus Elfring <elfring@users.sourceforge.net> wrote: >>> @@ -1064,7 +1064,7 @@ static int rbd_header_from_disk(struct rbd_device *rbd_dev, >>> header->snap_sizes = snap_sizes; >>> >>> return 0; >>> -out_2big: >>> + out_2big: >>> ret = -EIO; >>> kfree(snap_sizes); >>> free_names: > … >> Can you point where this current convention is documented? > > Yes. > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51 Huh. That patch is not in Linus' tree. > > Do you find the software update "CodingStyle: Clarify and complete chapter 7" interesting? > > >> Certainly not in CodingStyle, AFAICT... > > I suggest to look at the current version once more. > > >> I know some people prefer a single space in there because it makes >> "diff -p" work better, but nowadays with "git diff" this argument is >> pretty moot. > > Would you like to discuss the corresponding software evolution a bit more? Jon, could you please yank 865a1caa4b6b ("CodingStyle: Clarify and complete chapter 7") from your linux-next branch or at least change "It is advised to indent labels" to something less stronger? It hasn't even hit mainline yet and we are already getting spammed. Looks like 9 out of 10 labels are not indented $ git grep '^[a-z0-9]\+:' -- *.c | wc -l 27945 $ git grep '^ [a-z0-9]\+:' -- *.c | wc -l 2925 so I'd say that's a bad advise as far as consistency goes, and the "diff -p" argument is pretty moot nowadays. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ilya, Thanks for adding me. On Tue, 13 Sep 2016 11:16:13 +0200, Ilya Dryomov wrote: > On Tue, Sep 13, 2016 at 10:12 AM, SF Markus Elfring > <elfring@users.sourceforge.net> wrote: > >>> @@ -1064,7 +1064,7 @@ static int rbd_header_from_disk(struct rbd_device *rbd_dev, > >>> header->snap_sizes = snap_sizes; > >>> > >>> return 0; > >>> -out_2big: > >>> + out_2big: > >>> ret = -EIO; > >>> kfree(snap_sizes); > >>> free_names: > > … > >> Can you point where this current convention is documented? > > > > Yes. > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51 > > Huh. That patch is not in Linus' tree. > > > > > Do you find the software update "CodingStyle: Clarify and complete chapter 7" interesting? > > > > > >> Certainly not in CodingStyle, AFAICT... > > > > I suggest to look at the current version once more. > > > > > >> I know some people prefer a single space in there because it makes > >> "diff -p" work better, but nowadays with "git diff" this argument is > >> pretty moot. > > > > Would you like to discuss the corresponding software evolution a bit more? > > Jon, could you please yank 865a1caa4b6b ("CodingStyle: Clarify and > complete chapter 7") from your linux-next branch or at least change "It > is advised to indent labels" to something less stronger? It hasn't > even hit mainline yet and we are already getting spammed. The problem isn't the documentation update nor whether you or me like a space before labels or not. The problem is Markus Elfring. The guy just spend his time flooding maintainers with unneeded changes they never asked for. Ignore him and you'll be much better. If he was not flooding you with this, he would find something else :-( When I wrote "It is advised to indent labels with one space", I never meant that all the existing code should be converted that way. I expressed a preference, and provided a rationale for this preference. After that, an advice is just that: an advice. > Looks like 9 out of 10 labels are not indented > > $ git grep '^[a-z0-9]\+:' -- *.c | wc -l > 27945 > $ git grep '^ [a-z0-9]\+:' -- *.c | wc -l > 2925 Your regexps are wrong ;-) but the ratio is correct. > so I'd say that's a bad advise as far as consistency goes, and the > "diff -p" argument is pretty moot nowadays. It wasn't moot when I sent the documentation update patch. Or why would you think it was? "git diff", by default, behaves exactly the same as "diff -p" with regards to unindented labels (i.e. it doesn't handle them properly.) However, since then the issue was discussed somewhere else: https://lkml.org/lkml/2016/9/5/214 As you can see, alternatives to indenting labels with one space were found. Therefore you will soon be correct saying "the diff -p argument is pretty moot." As soon as my patch hits mainline, actually. Which shouldn't take too long as Andrew Morton picked it 4 days ago. Once this happens, I'm fine with CodingStyle being updated again to reflect the current situation. Hope it clarifies,
On Tue, Sep 13, 2016 at 4:36 PM, Jean Delvare <jdelvare@suse.de> wrote: > Hi Ilya, > > Thanks for adding me. > > On Tue, 13 Sep 2016 11:16:13 +0200, Ilya Dryomov wrote: >> On Tue, Sep 13, 2016 at 10:12 AM, SF Markus Elfring >> <elfring@users.sourceforge.net> wrote: >> >>> @@ -1064,7 +1064,7 @@ static int rbd_header_from_disk(struct rbd_device *rbd_dev, >> >>> header->snap_sizes = snap_sizes; >> >>> >> >>> return 0; >> >>> -out_2big: >> >>> + out_2big: >> >>> ret = -EIO; >> >>> kfree(snap_sizes); >> >>> free_names: >> > … >> >> Can you point where this current convention is documented? >> > >> > Yes. >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51 >> >> Huh. That patch is not in Linus' tree. >> >> > >> > Do you find the software update "CodingStyle: Clarify and complete chapter 7" interesting? >> > >> > >> >> Certainly not in CodingStyle, AFAICT... >> > >> > I suggest to look at the current version once more. >> > >> > >> >> I know some people prefer a single space in there because it makes >> >> "diff -p" work better, but nowadays with "git diff" this argument is >> >> pretty moot. >> > >> > Would you like to discuss the corresponding software evolution a bit more? >> >> Jon, could you please yank 865a1caa4b6b ("CodingStyle: Clarify and >> complete chapter 7") from your linux-next branch or at least change "It >> is advised to indent labels" to something less stronger? It hasn't >> even hit mainline yet and we are already getting spammed. > > The problem isn't the documentation update nor whether you or me like a > space before labels or not. The problem is Markus Elfring. The guy just > spend his time flooding maintainers with unneeded changes they never > asked for. Ignore him and you'll be much better. If he was not flooding > you with this, he would find something else :-( > > When I wrote "It is advised to indent labels with one space", I never > meant that all the existing code should be converted that way. I Hi Jean, That much is clear, however ... > expressed a preference, and provided a rationale for this preference. > After that, an advice is just that: an advice. > >> Looks like 9 out of 10 labels are not indented >> >> $ git grep '^[a-z0-9]\+:' -- *.c | wc -l >> 27945 >> $ git grep '^ [a-z0-9]\+:' -- *.c | wc -l >> 2925 > > Your regexps are wrong ;-) but the ratio is correct. ... one of the main points of any coding style is consistency. When someone new wanting to submit say a new driver opens CodingStyle and sees "It is advised to indent labels ...", they might start indenting labels in their code and advise others to do the same. Given the 9/10 existing ratio, that advice is wrong. If I wanted to clarify the situation, I'd have gone with "one space indented labels are also acceptable" or so. The example you've re-indented dates back to 2.6.4 times... > >> so I'd say that's a bad advise as far as consistency goes, and the >> "diff -p" argument is pretty moot nowadays. > > It wasn't moot when I sent the documentation update patch. Or why would > you think it was? "git diff", by default, behaves exactly the same as > "diff -p" with regards to unindented labels (i.e. it doesn't handle > them properly.) The git diff xfuncname incantation is a few years old now. git diff also works on regular files, BTW. > > However, since then the issue was discussed somewhere else: > https://lkml.org/lkml/2016/9/5/214 > > As you can see, alternatives to indenting labels with one space were > found. Therefore you will soon be correct saying "the diff -p argument > is pretty moot." As soon as my patch hits mainline, actually. Which > shouldn't take too long as Andrew Morton picked it 4 days ago. > > Once this happens, I'm fine with CodingStyle being updated again to > reflect the current situation. I'm not sure which patch you are talking about - the message you linked is not a patch and it's impossible to follow large threads on lkml.org. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 13 Sep 2016 17:30:33 +0200, Ilya Dryomov wrote: > On Tue, Sep 13, 2016 at 4:36 PM, Jean Delvare <jdelvare@suse.de> wrote: > > On Tue, 13 Sep 2016 11:16:13 +0200, Ilya Dryomov wrote: > >> Jon, could you please yank 865a1caa4b6b ("CodingStyle: Clarify and > >> complete chapter 7") from your linux-next branch or at least change "It > >> is advised to indent labels" to something less stronger? It hasn't > >> even hit mainline yet and we are already getting spammed. > > > > The problem isn't the documentation update nor whether you or me like a > > space before labels or not. The problem is Markus Elfring. The guy just > > spend his time flooding maintainers with unneeded changes they never > > asked for. Ignore him and you'll be much better. If he was not flooding > > you with this, he would find something else :-( > > > > When I wrote "It is advised to indent labels with one space", I never > > meant that all the existing code should be converted that way. I > > Hi Jean, > > That much is clear, however ... > > > expressed a preference, and provided a rationale for this preference. > > After that, an advice is just that: an advice. > > > >> Looks like 9 out of 10 labels are not indented > >> > >> $ git grep '^[a-z0-9]\+:' -- *.c | wc -l > >> 27945 > >> $ git grep '^ [a-z0-9]\+:' -- *.c | wc -l > >> 2925 > > > > Your regexps are wrong ;-) but the ratio is correct. > > ... one of the main points of any coding style is consistency. When > someone new wanting to submit say a new driver opens CodingStyle and > sees "It is advised to indent labels ...", they might start indenting > labels in their code and advise others to do the same. Given the 9/10 > existing ratio, that advice is wrong. Or you could read the thread I pointed you to, where I explain why this reasoning is wrong. > If I wanted to clarify the > situation, I'd have gone with "one space indented labels are also > acceptable" or so. The example you've re-indented dates back to 2.6.4 > times... I can't see how this is relevant. > >> so I'd say that's a bad advise as far as consistency goes, and the > >> "diff -p" argument is pretty moot nowadays. > > > > It wasn't moot when I sent the documentation update patch. Or why would > > you think it was? "git diff", by default, behaves exactly the same as > > "diff -p" with regards to unindented labels (i.e. it doesn't handle > > them properly.) > > The git diff xfuncname incantation is a few years old now. It doesn't help if a majority of developers don't know about it (as was my case until last week), and the project itself doesn't carry the required configuration bits to make it work out of the box (which hopefully will be the case soon, see below.) > git diff also works on regular files, BTW. I have no idea what you mean here, sorry. > > However, since then the issue was discussed somewhere else: > > https://lkml.org/lkml/2016/9/5/214 > > > > As you can see, alternatives to indenting labels with one space were > > found. Therefore you will soon be correct saying "the diff -p argument > > is pretty moot." As soon as my patch hits mainline, actually. Which > > shouldn't take too long as Andrew Morton picked it 4 days ago. > > > > Once this happens, I'm fine with CodingStyle being updated again to > > reflect the current situation. > > I'm not sure which patch you are talking about - the message you linked > is not a patch and it's impossible to follow large threads on lkml.org. The solution was in this thread, which I expected you to read. The patch proper was sent separately: http://marc.info/?l=linux-kernel&m=147325166209844&w=2 It uses the git diff xfuncname feature you mentioned above. To be honest I'm surprised it isn't the git default, it seems odd to have so many diff drivers included in git and not enable them on obvious file extensions. Oh well.
On Tue, Sep 13, 2016 at 6:50 PM, Jean Delvare <jdelvare@suse.de> wrote: > On Tue, 13 Sep 2016 17:30:33 +0200, Ilya Dryomov wrote: >> On Tue, Sep 13, 2016 at 4:36 PM, Jean Delvare <jdelvare@suse.de> wrote: >> > On Tue, 13 Sep 2016 11:16:13 +0200, Ilya Dryomov wrote: >> >> Jon, could you please yank 865a1caa4b6b ("CodingStyle: Clarify and >> >> complete chapter 7") from your linux-next branch or at least change "It >> >> is advised to indent labels" to something less stronger? It hasn't >> >> even hit mainline yet and we are already getting spammed. >> > >> > The problem isn't the documentation update nor whether you or me like a >> > space before labels or not. The problem is Markus Elfring. The guy just >> > spend his time flooding maintainers with unneeded changes they never >> > asked for. Ignore him and you'll be much better. If he was not flooding >> > you with this, he would find something else :-( >> > >> > When I wrote "It is advised to indent labels with one space", I never >> > meant that all the existing code should be converted that way. I >> >> Hi Jean, >> >> That much is clear, however ... >> >> > expressed a preference, and provided a rationale for this preference. >> > After that, an advice is just that: an advice. >> > >> >> Looks like 9 out of 10 labels are not indented >> >> >> >> $ git grep '^[a-z0-9]\+:' -- *.c | wc -l >> >> 27945 >> >> $ git grep '^ [a-z0-9]\+:' -- *.c | wc -l >> >> 2925 >> > >> > Your regexps are wrong ;-) but the ratio is correct. >> >> ... one of the main points of any coding style is consistency. When >> someone new wanting to submit say a new driver opens CodingStyle and >> sees "It is advised to indent labels ...", they might start indenting >> labels in their code and advise others to do the same. Given the 9/10 >> existing ratio, that advice is wrong. > > Or you could read the thread I pointed you to, where I explain why this > reasoning is wrong. Sorry, navigating lkml.org archive is a pain, and I was expecting to see patch. Your points "The acceptance of an optional single space before labels dates back to at least June 2007, as supported by the very first incarnation of checkpatch.pl. So nothing really new here, except for a preference (my preference, admittedly, but I'm know I'm not alone) being expressed in the coding style document." "Recommendations are not meant to document what people are currently doing but what we think they should be doing." are valid, but note that there is a world of difference between an acceptance and a preference. The *only* point of whitespace guidelines is to keep the code base consistent. You don't go changing whitespace preferences in such a huge project, not unless you have a *very* good rationale and existing code base is swayed (which it isn't, given the 9/10 ratio). > >> If I wanted to clarify the >> situation, I'd have gone with "one space indented labels are also >> acceptable" or so. The example you've re-indented dates back to 2.6.4 >> times... > > I can't see how this is relevant. That was a 12 year old example, codifying an existing style used in ~90% cases, serving as a guideline for new contributors. > >> >> so I'd say that's a bad advise as far as consistency goes, and the >> >> "diff -p" argument is pretty moot nowadays. >> > >> > It wasn't moot when I sent the documentation update patch. Or why would >> > you think it was? "git diff", by default, behaves exactly the same as >> > "diff -p" with regards to unindented labels (i.e. it doesn't handle >> > them properly.) >> >> The git diff xfuncname incantation is a few years old now. > > It doesn't help if a majority of developers don't know about it (as was > my case until last week), and the project itself doesn't carry the > required configuration bits to make it work out of the box (which > hopefully will be the case soon, see below.) > >> git diff also works on regular files, BTW. > > I have no idea what you mean here, sorry. Oh, just that it works outside of git repos too, so you aren't stuck with diffutils if you want to diff two random .c files. > >> > However, since then the issue was discussed somewhere else: >> > https://lkml.org/lkml/2016/9/5/214 >> > >> > As you can see, alternatives to indenting labels with one space were >> > found. Therefore you will soon be correct saying "the diff -p argument >> > is pretty moot." As soon as my patch hits mainline, actually. Which >> > shouldn't take too long as Andrew Morton picked it 4 days ago. >> > >> > Once this happens, I'm fine with CodingStyle being updated again to >> > reflect the current situation. >> >> I'm not sure which patch you are talking about - the message you linked >> is not a patch and it's impossible to follow large threads on lkml.org. > > The solution was in this thread, which I expected you to read. The > patch proper was sent separately: > > http://marc.info/?l=linux-kernel&m=147325166209844&w=2 > > It uses the git diff xfuncname feature you mentioned above. To be > honest I'm surprised it isn't the git default, it seems odd to have so > many diff drivers included in git and not enable them on obvious file > extensions. Oh well. This came up before: http://www.spinics.net/lists/git/msg164216.html, Linus didn't like it. I suggest you add him to the CC on this patch to see if he changed his mind. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ilya, Sorry for the late answer. On Tue, 13 Sep 2016 20:31:57 +0200, Ilya Dryomov wrote: > Sorry, navigating lkml.org archive is a pain, and I was expecting to > see patch. Your points > > "The acceptance of an optional single space before labels dates back to > at least June 2007, as supported by the very first incarnation of > checkpatch.pl. So nothing really new here, except for a preference > (my preference, admittedly, but I'm know I'm not alone) being expressed > in the coding style document." > > "Recommendations are not meant to document what people are currently > doing but what we think they should be doing." > > are valid, but note that there is a world of difference between an > acceptance and a preference. The *only* point of whitespace guidelines > is to keep the code base consistent. Consistency is half of the reason, the other half is readability. This is why the CodingStyle document has a number of rationales explained. This is also why we put whitespace in the first place, while the C language doesn't require any ;-) The sense of my proposal was to address a readability (or usability) issue. > You don't go changing whitespace > preferences in such a huge project, not unless you have a *very* good > rationale and existing code base is swayed (which it isn't, given the > 9/10 ratio). I did consider the reason to be good enough to warrant a "change", actually. Or more exactly from "one space is allowed" to "one space is recommended." Which is quite different from changing all the code actively. I can understand how you don't like it, but again, this "inconsistency" has been accepted for almost a decade now, so I find it strange to see so much resistance when someone finally tries to sort it out. > >> If I wanted to clarify the > >> situation, I'd have gone with "one space indented labels are also > >> acceptable" or so. The example you've re-indented dates back to 2.6.4 > >> times... > > > > I can't see how this is relevant. > > That was a 12 year old example, codifying an existing style used in > ~90% cases, serving as a guideline for new contributors. OK, I get your point now. But the CodingStyle document isn't carved into stone. I see 43 changes to that file in recent history (since April 2005), some of which are actual changes or clarifications of our coding style. This very section of the document was updated in December 2014, so not so long ago. In the end I suppose it boils down to how problematic you consider the current situation to be. Apparently you and several other maintainers think it's just fine, while me (and a few others apparently) think it is not. > >> git diff also works on regular files, BTW. > > > > I have no idea what you mean here, sorry. > > Oh, just that it works outside of git repos too, so you aren't stuck > with diffutils if you want to diff two random .c files. Oh, I had never thought of that. Thanks for the hint :-) > > (...) > > http://marc.info/?l=linux-kernel&m=147325166209844&w=2 > > > > It uses the git diff xfuncname feature you mentioned above. To be > > honest I'm surprised it isn't the git default, it seems odd to have so > > many diff drivers included in git and not enable them on obvious file > > extensions. Oh well. > > This came up before: http://www.spinics.net/lists/git/msg164216.html, > Linus didn't like it. I suggest you add him to the CC on this patch to > see if he changed his mind. Thanks for the pointer. It is interesting to see many people had been bothered by the same problem for many years and even proposed solution for it. But also sad to see that nothing happened :-( Well Linus suggested to improve the default, he was not opposed to the change per se I think. But it was 5 years ago and nothing happened since then, so I'd rather go with what is available today. Which means either one space before labels, or drivers in .gitattributes. Choose your poison ;-) Thanks,
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index f4212e1..d61a066 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -1064,7 +1064,7 @@ static int rbd_header_from_disk(struct rbd_device *rbd_dev, header->snap_sizes = snap_sizes; return 0; -out_2big: + out_2big: ret = -EIO; kfree(snap_sizes); free_names: