| Message ID | 1601233948-11629-1-git-send-email-Julia.Lawall@inria.fr |
|---|---|
| Headers | show |
| Series | use semicolons rather than commas to separate statements | expand |
On Sun, 2020-09-27 at 21:12 +0200, Julia Lawall wrote: > These patches replace commas by semicolons. This was done using the > Coccinelle semantic patch (http://coccinelle.lip6.fr/) shown below. > > This semantic patch ensures that commas inside for loop headers will not be > transformed. It also doesn't touch macro definitions. Thanks. All of these appear to be correct and without effect except for __LINE__ number changes where braces are added.
On Mon, 2020-09-28 at 20:35 +0100, Mark Brown wrote: > On Sun, 27 Sep 2020 21:12:10 +0200, Julia Lawall wrote: > > These patches replace commas by semicolons. This was done using the > > Coccinelle semantic patch (http://coccinelle.lip6.fr/) shown below. > > > > This semantic patch ensures that commas inside for loop headers will not be > > transformed. It also doesn't touch macro definitions. > > > > Coccinelle ensures that braces are added as needed when a single-statement > > branch turns into a multi-statement one. > > > > [...] > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next > > Thanks! > > [1/1] regmap: debugfs: use semicolons rather than commas to separate statements > commit: 7f4a122d0b50b40c64d24a5cf7aafe26dd9487ee Hi Mark. Rather than replying to the 0/n cover letter to a patch series, can you reply to each of the specific patches in the patch series you are applying? Otherwise, it's a bit difficult to figure out which patches you are applying. thanks
On Mon, Sep 28, 2020 at 05:45:24PM -0700, Joe Perches wrote: > On Mon, 2020-09-28 at 20:35 +0100, Mark Brown wrote: > > [1/1] regmap: debugfs: use semicolons rather than commas to separate statements > > commit: 7f4a122d0b50b40c64d24a5cf7aafe26dd9487ee > Rather than replying to the 0/n cover letter to a patch > series, can you reply to each of the specific patches in > the patch series you are applying? > Otherwise, it's a bit difficult to figure out which patches > you are applying. Feel free to submit patches to b4. Ideally things like this wouldn't be being sent as serieses in the first place, there's no dependencies or interactions between the patches.
On Tue, 29 Sep 2020, Mark Brown wrote: > On Mon, Sep 28, 2020 at 05:45:24PM -0700, Joe Perches wrote: > > On Mon, 2020-09-28 at 20:35 +0100, Mark Brown wrote: > > > > [1/1] regmap: debugfs: use semicolons rather than commas to separate statements > > > commit: 7f4a122d0b50b40c64d24a5cf7aafe26dd9487ee > > > Rather than replying to the 0/n cover letter to a patch > > series, can you reply to each of the specific patches in > > the patch series you are applying? > > > Otherwise, it's a bit difficult to figure out which patches > > you are applying. > > Feel free to submit patches to b4. Ideally things like this wouldn't be > being sent as serieses in the first place, there's no dependencies or > interactions between the patches. It was suggested (a long time ago, not with respect to this patch in particular) that sending such patches in a series is useful because it allows people who are not interested in the 18 patches to skip over them more easily. So there are two conflicting needs... julia
On Sun, 27 Sep 2020 at 21:56, Julia Lawall <Julia.Lawall@inria.fr> wrote: > > These patches replace commas by semicolons. Why? > This was done using the > Coccinelle semantic patch (http://coccinelle.lip6.fr/) shown below. > > This semantic patch ensures that commas inside for loop headers will not be > transformed. It also doesn't touch macro definitions. > > Coccinelle ensures that braces are added as needed when a single-statement > branch turns into a multi-statement one. > > This semantic patch has a few false positives, for variable delcarations > such as: > > LIST_HEAD(x), *y; > > The semantic patch could be improved to avoid these, but for the moment > they have been removed manually (2 occurrences). > > // <smpl> > @initialize:ocaml@ > @@ > > let infunction p = > (* avoid macros *) > (List.hd p).current_element <> "something_else" > > let combined p1 p2 = > (List.hd p1).line_end = (List.hd p2).line || > (((List.hd p1).line_end < (List.hd p2).line) && > ((List.hd p1).col < (List.hd p2).col)) > > @bad@ > statement S; > declaration d; > position p; > @@ > > S@p > d > > // special cases where newlines are needed (hope for no more than 5) > @@ > expression e1,e2; > statement S; > position p != bad.p; > position p1; > position p2 : > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > @@ > > - e1@p1,@S@p e2@p2; > + e1; e2; > > @@ > expression e1,e2; > statement S; > position p != bad.p; > position p1; > position p2 : > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > @@ > > - e1@p1,@S@p e2@p2; > + e1; e2; > > @@ > expression e1,e2; > statement S; > position p != bad.p; > position p1; > position p2 : > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > @@ > > - e1@p1,@S@p e2@p2; > + e1; e2; > > @@ > expression e1,e2; > statement S; > position p != bad.p; > position p1; > position p2 : > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > @@ > > - e1@p1,@S@p e2@p2; > + e1; e2; > > @@ > expression e1,e2; > statement S; > position p != bad.p; > position p1; > position p2 : > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > @@ > > - e1@p1,@S@p e2@p2; > + e1; e2; > > @r@ > expression e1,e2; > statement S; > position p != bad.p; > @@ > > e1 ,@S@p e2; > > @@ > expression e1,e2; > position p1; > position p2 : > script:ocaml(p1) { infunction p1 && not(combined p1 p2) }; > statement S; > position r.p; > @@ > > e1@p1 > -,@S@p > +; > e2@p2 > ... when any > // </smpl> > > --- > > drivers/acpi/processor_idle.c | 4 +++- > drivers/ata/pata_icside.c | 21 +++++++++++++-------- > drivers/base/regmap/regmap-debugfs.c | 2 +- > drivers/bcma/driver_pci_host.c | 4 ++-- > drivers/block/drbd/drbd_receiver.c | 6 ++++-- > drivers/char/agp/amd-k7-agp.c | 2 +- > drivers/char/agp/nvidia-agp.c | 2 +- > drivers/char/agp/sworks-agp.c | 2 +- > drivers/char/hw_random/iproc-rng200.c | 8 ++++---- > drivers/char/hw_random/mxc-rnga.c | 6 +++--- > drivers/char/hw_random/stm32-rng.c | 8 ++++---- > drivers/char/ipmi/bt-bmc.c | 6 +++--- > drivers/clk/meson/meson-aoclk.c | 2 +- > drivers/clk/mvebu/ap-cpu-clk.c | 2 +- > drivers/clk/uniphier/clk-uniphier-cpugear.c | 2 +- > drivers/clk/uniphier/clk-uniphier-mux.c | 2 +- > drivers/clocksource/mps2-timer.c | 6 +++--- > drivers/clocksource/timer-armada-370-xp.c | 8 ++++---- > drivers/counter/ti-eqep.c | 2 +- > drivers/crypto/amcc/crypto4xx_alg.c | 2 +- > drivers/crypto/atmel-tdes.c | 2 +- > drivers/crypto/hifn_795x.c | 4 ++-- > drivers/crypto/talitos.c | 8 ++++---- > 23 files changed, 60 insertions(+), 51 deletions(-) > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, 29 Sep 2020, Ard Biesheuvel wrote: > On Sun, 27 Sep 2020 at 21:56, Julia Lawall <Julia.Lawall@inria.fr> wrote: > > > > These patches replace commas by semicolons. > > > Why? Among the complete 5000 lines of changes there is one probable bug where an if branch ends with a comma and thus pulls the subsequent statement under the if branch, in contradiction to what is indicated by the indentation. The use of comma often appears to be random, with a sequence of similar statements where some have commas and some don't. From a self-interested point of view, commas are not good for Coccinelle, because multiple statements are put into one. Any problems involving them are thus likely to be overlooked unless one thinks of looking for them explicitly. As an example, Christophe Jaillet noticed that one sequence of comma assignments would be better written using swap. If one looked for opportunities for using swap in the most obvious way, one wouldn't find it. julia > > > > This was done using the > > Coccinelle semantic patch (http://coccinelle.lip6.fr/) shown below. > > > > This semantic patch ensures that commas inside for loop headers will not be > > transformed. It also doesn't touch macro definitions. > > > > Coccinelle ensures that braces are added as needed when a single-statement > > branch turns into a multi-statement one. > > > > This semantic patch has a few false positives, for variable delcarations > > such as: > > > > LIST_HEAD(x), *y; > > > > The semantic patch could be improved to avoid these, but for the moment > > they have been removed manually (2 occurrences). > > > > // <smpl> > > @initialize:ocaml@ > > @@ > > > > let infunction p = > > (* avoid macros *) > > (List.hd p).current_element <> "something_else" > > > > let combined p1 p2 = > > (List.hd p1).line_end = (List.hd p2).line || > > (((List.hd p1).line_end < (List.hd p2).line) && > > ((List.hd p1).col < (List.hd p2).col)) > > > > @bad@ > > statement S; > > declaration d; > > position p; > > @@ > > > > S@p > > d > > > > // special cases where newlines are needed (hope for no more than 5) > > @@ > > expression e1,e2; > > statement S; > > position p != bad.p; > > position p1; > > position p2 : > > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > > @@ > > > > - e1@p1,@S@p e2@p2; > > + e1; e2; > > > > @@ > > expression e1,e2; > > statement S; > > position p != bad.p; > > position p1; > > position p2 : > > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > > @@ > > > > - e1@p1,@S@p e2@p2; > > + e1; e2; > > > > @@ > > expression e1,e2; > > statement S; > > position p != bad.p; > > position p1; > > position p2 : > > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > > @@ > > > > - e1@p1,@S@p e2@p2; > > + e1; e2; > > > > @@ > > expression e1,e2; > > statement S; > > position p != bad.p; > > position p1; > > position p2 : > > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > > @@ > > > > - e1@p1,@S@p e2@p2; > > + e1; e2; > > > > @@ > > expression e1,e2; > > statement S; > > position p != bad.p; > > position p1; > > position p2 : > > script:ocaml(p1) { infunction p1 && combined p1 p2 }; > > @@ > > > > - e1@p1,@S@p e2@p2; > > + e1; e2; > > > > @r@ > > expression e1,e2; > > statement S; > > position p != bad.p; > > @@ > > > > e1 ,@S@p e2; > > > > @@ > > expression e1,e2; > > position p1; > > position p2 : > > script:ocaml(p1) { infunction p1 && not(combined p1 p2) }; > > statement S; > > position r.p; > > @@ > > > > e1@p1 > > -,@S@p > > +; > > e2@p2 > > ... when any > > // </smpl> > > > > --- > > > > drivers/acpi/processor_idle.c | 4 +++- > > drivers/ata/pata_icside.c | 21 +++++++++++++-------- > > drivers/base/regmap/regmap-debugfs.c | 2 +- > > drivers/bcma/driver_pci_host.c | 4 ++-- > > drivers/block/drbd/drbd_receiver.c | 6 ++++-- > > drivers/char/agp/amd-k7-agp.c | 2 +- > > drivers/char/agp/nvidia-agp.c | 2 +- > > drivers/char/agp/sworks-agp.c | 2 +- > > drivers/char/hw_random/iproc-rng200.c | 8 ++++---- > > drivers/char/hw_random/mxc-rnga.c | 6 +++--- > > drivers/char/hw_random/stm32-rng.c | 8 ++++---- > > drivers/char/ipmi/bt-bmc.c | 6 +++--- > > drivers/clk/meson/meson-aoclk.c | 2 +- > > drivers/clk/mvebu/ap-cpu-clk.c | 2 +- > > drivers/clk/uniphier/clk-uniphier-cpugear.c | 2 +- > > drivers/clk/uniphier/clk-uniphier-mux.c | 2 +- > > drivers/clocksource/mps2-timer.c | 6 +++--- > > drivers/clocksource/timer-armada-370-xp.c | 8 ++++---- > > drivers/counter/ti-eqep.c | 2 +- > > drivers/crypto/amcc/crypto4xx_alg.c | 2 +- > > drivers/crypto/atmel-tdes.c | 2 +- > > drivers/crypto/hifn_795x.c | 4 ++-- > > drivers/crypto/talitos.c | 8 ++++---- > > 23 files changed, 60 insertions(+), 51 deletions(-) > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Tue, Sep 29, 2020 at 01:46:19PM +0200, Julia Lawall wrote: > On Tue, 29 Sep 2020, Mark Brown wrote: > > Feel free to submit patches to b4. Ideally things like this wouldn't be > > being sent as serieses in the first place, there's no dependencies or > > interactions between the patches. > It was suggested (a long time ago, not with respect to this patch in > particular) that sending such patches in a series is useful because it > allows people who are not interested in the 18 patches to skip over them > more easily. So there are two conflicting needs... I'm not convinced that there are huge numbers of people reading LKML as a list TBH, and if you are sending things as a series then the way you're doing it at the minute where you don't CC the cover letter to people makes things confusing as it's unclear if there are dependencies to worry about.
On Tue, Sep 29, 2020 at 02:20:00PM +0200, Ard Biesheuvel wrote: > On Sun, 27 Sep 2020 at 21:56, Julia Lawall <Julia.Lawall@inria.fr> wrote: > > > > These patches replace commas by semicolons. > > > Why? > In the best case, these commas are just uninitentional mess, like typing an extra space character or something. I've looked at them before and one case I see where they are introduced is when people convert a struct initializer to code. - struct foo { - .a = 1, - .b = 2, ... + foo.a = 1, + foo.b = 2, The times where commas are used deliberately to replace curly braces are just evil. Either way the code is cleaner with semi-colons. regards, dan carpenter
On Tue, 29 Sep 2020, Mark Brown wrote: > On Tue, Sep 29, 2020 at 01:46:19PM +0200, Julia Lawall wrote: > > On Tue, 29 Sep 2020, Mark Brown wrote: > > > > Feel free to submit patches to b4. Ideally things like this wouldn't be > > > being sent as serieses in the first place, there's no dependencies or > > > interactions between the patches. > > > It was suggested (a long time ago, not with respect to this patch in > > particular) that sending such patches in a series is useful because it > > allows people who are not interested in the 18 patches to skip over them > > more easily. So there are two conflicting needs... > > I'm not convinced that there are huge numbers of people reading LKML as > a list TBH, and if you are sending things as a series then the way > you're doing it at the minute where you don't CC the cover letter to > people makes things confusing as it's unclear if there are dependencies > to worry about. The cover letter goes to all of the specific mailing lists affected by the patch, or if there is no list, then to at least one developer. Sending the cover letter to everyone would lead to too many recipients for some lists. If there is a preference for the rest of these patches to be sent one by one, then that is possible. julia
On Tue, 29 Sep 2020, Dan Carpenter wrote: > On Tue, Sep 29, 2020 at 02:20:00PM +0200, Ard Biesheuvel wrote: > > On Sun, 27 Sep 2020 at 21:56, Julia Lawall <Julia.Lawall@inria.fr> wrote: > > > > > > These patches replace commas by semicolons. > > > > > > Why? > > > > In the best case, these commas are just uninitentional mess, like typing > an extra space character or something. I've looked at them before and > one case I see where they are introduced is when people convert a > struct initializer to code. > > - struct foo { > - .a = 1, > - .b = 2, > ... > + foo.a = 1, > + foo.b = 2, > > The times where commas are used deliberately to replace curly braces are > just evil. Either way the code is cleaner with semi-colons. I also found exaamples like the following to be particularly unforunate: fprintf(stderr, "page_nr %lu wrong count %Lu %Lu\n", page_nr, count, count_verify[page_nr]), exit(1); The exit is very hard to see, unless you know to look for it. julia
On Tue, 2020-09-29 at 14:47 +0200, Julia Lawall wrote: > On Tue, 29 Sep 2020, Dan Carpenter wrote: > > The times where commas are used deliberately to replace curly braces are > > just evil. Either way the code is cleaner with semi-colons. > > I also found exaamples like the following to be particularly unforunate: > > fprintf(stderr, > "page_nr %lu wrong count %Lu %Lu\n", > page_nr, count, > count_verify[page_nr]), exit(1); > > The exit is very hard to see, unless you know to look for it. I sent that patch last month. https://patchwork.kernel.org/patch/11734877/ It's still not applied.
On Tue, 29 Sep 2020, Joe Perches wrote: > On Tue, 2020-09-29 at 14:47 +0200, Julia Lawall wrote: > > On Tue, 29 Sep 2020, Dan Carpenter wrote: > > > The times where commas are used deliberately to replace curly braces are > > > just evil. Either way the code is cleaner with semi-colons. > > > > I also found exaamples like the following to be particularly unforunate: > > > > fprintf(stderr, > > "page_nr %lu wrong count %Lu %Lu\n", > > page_nr, count, > > count_verify[page_nr]), exit(1); > > > > The exit is very hard to see, unless you know to look for it. > > I sent that patch last month. > https://patchwork.kernel.org/patch/11734877/ > > It's still not applied. OK, thanks. I'll not send those then :) julia
On 9/29/20 7:34 AM, Joe Perches wrote: > On Tue, 2020-09-29 at 14:47 +0200, Julia Lawall wrote: >> On Tue, 29 Sep 2020, Dan Carpenter wrote: >>> The times where commas are used deliberately to replace curly braces are >>> just evil. Either way the code is cleaner with semi-colons. >> >> I also found exaamples like the following to be particularly unforunate: >> >> fprintf(stderr, >> "page_nr %lu wrong count %Lu %Lu\n", >> page_nr, count, >> count_verify[page_nr]), exit(1); >> >> The exit is very hard to see, unless you know to look for it. > > I sent that patch last month. > https://patchwork.kernel.org/patch/11734877/ > I see what happened. This patch touches lib, cpupower, and selftests. Guess lost in the limbo of who takes it. tools/lib/subcmd/help.c | 10 +- tools/power/cpupower/utils/cpufreq-set.c | 14 +- tools/testing/selftests/vm/gup_benchmark.c | 18 +- tools/testing/selftests/vm/userfaultfd.c | 296 +++++++++++++-------- 4 files changed, 210 insertions(+), 128 deletions(-) I can take it through one of my trees. thanks, -- Shuah
On Tue, 2020-09-29 at 12:37 +0100, Mark Brown wrote: > On Mon, Sep 28, 2020 at 05:45:24PM -0700, Joe Perches wrote: > > On Mon, 2020-09-28 at 20:35 +0100, Mark Brown wrote: > > > [1/1] regmap: debugfs: use semicolons rather than commas to separate statements > > > commit: 7f4a122d0b50b40c64d24a5cf7aafe26dd9487ee > > Rather than replying to the 0/n cover letter to a patch > > series, can you reply to each of the specific patches in > > the patch series you are applying? > > Otherwise, it's a bit difficult to figure out which patches > > you are applying. > > Feel free to submit patches to b4. Have you tried the existing option to send thank you's on a specific ranges of patches? b4 ty ~~~~~ usage: b4 ty [-h] [-g GITDIR] [-o OUTDIR] [-l] [-s SEND [SEND ...]] [-d DISCARD [DISCARD ...]] [-a] [-b BRANCH] [--since SINCE] [] -s SEND, --send SEND Generate thankyous for specific entries from -l (e.g.: 1,3-5,7-; or "all")
On Wed, Sep 30, 2020 at 12:33:39PM -0700, Joe Perches wrote: > On Tue, 2020-09-29 at 12:37 +0100, Mark Brown wrote: > > Feel free to submit patches to b4. > Have you tried the existing option to send > thank you's on a specific ranges of patches? I am relying on b4 to identify which patches that I've downloaded are in the pushed branches. Given that it explicitly lists the patches that are applied it appears to be doing an OK job here.
On 9/29/20 7:42 AM, Shuah Khan wrote: > On 9/29/20 7:34 AM, Joe Perches wrote: >> On Tue, 2020-09-29 at 14:47 +0200, Julia Lawall wrote: >>> On Tue, 29 Sep 2020, Dan Carpenter wrote: >>>> The times where commas are used deliberately to replace curly braces >>>> are >>>> just evil. Either way the code is cleaner with semi-colons. >>> >>> I also found exaamples like the following to be particularly unforunate: >>> >>> fprintf(stderr, >>> "page_nr %lu wrong count %Lu >>> %Lu\n", >>> page_nr, count, >>> count_verify[page_nr]), exit(1); >>> >>> The exit is very hard to see, unless you know to look for it. >> >> I sent that patch last month. >> https://patchwork.kernel.org/patch/11734877/ >> > > I see what happened. This patch touches lib, cpupower, and selftests. > Guess lost in the limbo of who takes it. > > tools/lib/subcmd/help.c | 10 +- > tools/power/cpupower/utils/cpufreq-set.c | 14 +- > tools/testing/selftests/vm/gup_benchmark.c | 18 +- > tools/testing/selftests/vm/userfaultfd.c | 296 +++++++++++++-------- > 4 files changed, 210 insertions(+), 128 deletions(-) > > I can take it through one of my trees. > Rafael, Andrew, This patch is now applied to https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git fixes branch. This spans pm, kselftest-mm tests and tools/lib and has been in limbo for a few weeks for that reason. I decided to take this through kselftest tree to avoid having Joe split the patches. thanks, -- Shuah
(Adding tools and Konstantin Ryabitsev) There seems to be some mismatch between b4's use of the cover letter to a patch series and what maintainers that apply a subset of the patches in the patch series. The merge description shows the entire patch series as applied, but the actual merge is only a subset of the series. Can this be improved in b4? For example, regarding: https://lore.kernel.org/linux-amlogic/160132172369.55460.9237357219623604216.b4-ty@kernel.org/ https://lore.kernel.org/lkml/b1174f9be2ce65f6b5ebefcba0b48e792926abbc.camel@perches.com/#t On Thu, 2020-10-01 at 12:01 +0100, Mark Brown wrote: > On Wed, Sep 30, 2020 at 12:33:39PM -0700, Joe Perches wrote: > > On Tue, 2020-09-29 at 12:37 +0100, Mark Brown wrote: > > > Feel free to submit patches to b4. > > Have you tried the existing option to send > > thank you's on a specific ranges of patches? > > I am relying on b4 to identify which patches that I've downloaded are in > the pushed branches. Given that it explicitly lists the patches that > are applied it appears to be doing an OK job here. I'm not so sure about that. The commit merge description in -next shows 23 files modified but the commit range shown in the merge shows only a single patch applied: From next-20201002: (I've removed some of the commit description below) $ git log --stat -1 2defc3fa18a68963a330187f5386968e50832d06 commit 2defc3fa18a68963a330187f5386968e50832d06 Merge: eb45df24fe82 7f4a122d0b50 Author: Mark Brown <broonie@kernel.org> Date: Mon Sep 28 18:28:48 2020 +0100 Merge series "use semicolons rather than commas to separate statements" from Julia Lawall <Julia.Lawall@inria.fr>: These patches replace commas by semicolons. This was done using the Coccinelle semantic patch (http://coccinelle.lip6.fr/) shown below. [some of the long description elided] --- drivers/acpi/processor_idle.c | 4 +++- drivers/ata/pata_icside.c | 21 +++++++++++++-------- drivers/base/regmap/regmap-debugfs.c | 2 +- drivers/bcma/driver_pci_host.c | 4 ++-- drivers/block/drbd/drbd_receiver.c | 6 ++++-- drivers/char/agp/amd-k7-agp.c | 2 +- drivers/char/agp/nvidia-agp.c | 2 +- drivers/char/agp/sworks-agp.c | 2 +- drivers/char/hw_random/iproc-rng200.c | 8 ++++---- drivers/char/hw_random/mxc-rnga.c | 6 +++--- drivers/char/hw_random/stm32-rng.c | 8 ++++---- drivers/char/ipmi/bt-bmc.c | 6 +++--- drivers/clk/meson/meson-aoclk.c | 2 +- drivers/clk/mvebu/ap-cpu-clk.c | 2 +- drivers/clk/uniphier/clk-uniphier-cpugear.c | 2 +- drivers/clk/uniphier/clk-uniphier-mux.c | 2 +- drivers/clocksource/mps2-timer.c | 6 +++--- drivers/clocksource/timer-armada-370-xp.c | 8 ++++---- drivers/counter/ti-eqep.c | 2 +- drivers/crypto/amcc/crypto4xx_alg.c | 2 +- drivers/crypto/atmel-tdes.c | 2 +- drivers/crypto/hifn_795x.c | 4 ++-- drivers/crypto/talitos.c | 8 ++++---- 23 files changed, 60 insertions(+), 51 deletions(-) But the commit range of the merge shows only the single commit: $ git log --stat eb45df24fe82..7f4a122d0b50 commit 7f4a122d0b50b40c64d24a5cf7aafe26dd9487ee Author: Julia Lawall <Julia.Lawall@inria.fr> Date: Sun Sep 27 21:12:24 2020 +0200 regmap: debugfs: use semicolons rather than commas to separate statements Replace commas with semicolons. What is done is essentially described by the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): // <smpl> @@ expression e1,e2; @@ e1 -, +; e2 ... when any // </smpl> Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> Link: https://lore.kernel.org/r/1601233948-11629-15-git-send-email-Julia.La> Signed-off-by: Mark Brown <broonie@kernel.org> drivers/base/regmap/regmap-debugfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
On Sat, Oct 03, 2020 at 11:40:48AM -0700, Joe Perches wrote: > (Adding tools and Konstantin Ryabitsev) > > There seems to be some mismatch between b4's use of the > cover letter to a patch series and what maintainers that > apply a subset of the patches in the patch series. > > The merge description shows the entire patch series as > applied, but the actual merge is only a subset of the > series. > > Can this be improved in b4? So, the following logic should be applied: - if the entire series was applied, reply to 0/n - if a subset only is applied, reply to each n/n of the patch that was cherry-picked out of the series Is that an accurate summary? -K
On Sat, 3 Oct 2020, Konstantin Ryabitsev wrote: > On Sat, Oct 03, 2020 at 11:40:48AM -0700, Joe Perches wrote: > > (Adding tools and Konstantin Ryabitsev) > > > > There seems to be some mismatch between b4's use of the > > cover letter to a patch series and what maintainers that > > apply a subset of the patches in the patch series. > > > > The merge description shows the entire patch series as > > applied, but the actual merge is only a subset of the > > series. > > > > Can this be improved in b4? > > So, the following logic should be applied: > > - if the entire series was applied, reply to 0/n > - if a subset only is applied, reply to each n/n of the patch that was > cherry-picked out of the series > > Is that an accurate summary? That sounds good. julia
On Sat, 2020-10-03 at 15:15 -0400, Konstantin Ryabitsev wrote: > On Sat, Oct 03, 2020 at 11:40:48AM -0700, Joe Perches wrote: > > (Adding tools and Konstantin Ryabitsev) > > > > There seems to be some mismatch between b4's use of the > > cover letter to a patch series and what maintainers that > > apply a subset of the patches in the patch series. > > > > The merge description shows the entire patch series as > > applied, but the actual merge is only a subset of the > > series. > > > > Can this be improved in b4? > > So, the following logic should be applied: > > - if the entire series was applied, reply to 0/n > - if a subset only is applied, reply to each n/n of the patch that was > cherry-picked out of the series > > Is that an accurate summary? Exactly so, thanks.
On Sat, Oct 03, 2020 at 09:18:51PM +0200, Julia Lawall wrote: > > > There seems to be some mismatch between b4's use of the > > > cover letter to a patch series and what maintainers that > > > apply a subset of the patches in the patch series. > > > > > > The merge description shows the entire patch series as > > > applied, but the actual merge is only a subset of the > > > series. > > > > > > Can this be improved in b4? > > > > So, the following logic should be applied: > > > > - if the entire series was applied, reply to 0/n > > - if a subset only is applied, reply to each n/n of the patch that was > > cherry-picked out of the series > > > > Is that an accurate summary? > > That sounds good. I'm worried that this can get unwieldy for series of 50 patches where 49 got applied. Would the following be better: ----- From: ... To: ... Subject: Re: [PATCH 00/18] use semicolons... On Sun... > These patches... > > [...] A subset of these patches was applied to https://... Thanks! [5/18] regmap: debugfs: commit: (etc) ----- In other words, we: - specifically say that it's a subset - instead of just enumerating the number of patches that were applied, as is currently the case ([1/1]) we list the exact numbers out of the posted series (e.g. [5/18]) I think this is a better solution than potentially flooding everyone with 49 emails. -K
On Sat, 2020-10-03 at 12:27 -0700, Joe Perches wrote: > On Sat, 2020-10-03 at 15:15 -0400, Konstantin Ryabitsev wrote: > > On Sat, Oct 03, 2020 at 11:40:48AM -0700, Joe Perches wrote: > > > (Adding tools and Konstantin Ryabitsev) > > > > > > There seems to be some mismatch between b4's use of the > > > cover letter to a patch series and what maintainers that > > > apply a subset of the patches in the patch series. > > > > > > The merge description shows the entire patch series as > > > applied, but the actual merge is only a subset of the > > > series. > > > > > > Can this be improved in b4? > > > > So, the following logic should be applied: > > > > - if the entire series was applied, reply to 0/n > > - if a subset only is applied, reply to each n/n of the patch that was > > cherry-picked out of the series > > > > Is that an accurate summary? > > Exactly so, thanks. And there's no need to commit the [0/n] cover letter as a part of the merge unless the entire series was committed. Or perhaps trim the cover letter to exclude the files modified by the patch series and show only the actual files committed. And I believe b4 inserts this line ahead of the 0/n series cover letter description for the merge: Merge series "<series>" from <author>: Perhaps that like could be "partial merge of" when a partial merge occurs or left as is if the entire series is applied. cheers, Joe
On Sat, 2020-10-03 at 15:31 -0400, Konstantin Ryabitsev wrote: > On Sat, Oct 03, 2020 at 09:18:51PM +0200, Julia Lawall wrote: > > > > There seems to be some mismatch between b4's use of the > > > > cover letter to a patch series and what maintainers that > > > > apply a subset of the patches in the patch series. > > > > > > > > The merge description shows the entire patch series as > > > > applied, but the actual merge is only a subset of the > > > > series. > > > > > > > > Can this be improved in b4? > > > > > > So, the following logic should be applied: > > > > > > - if the entire series was applied, reply to 0/n > > > - if a subset only is applied, reply to each n/n of the patch that was > > > cherry-picked out of the series > > > > > > Is that an accurate summary? > > > > That sounds good. > > I'm worried that this can get unwieldy for series of 50 patches where 49 > got applied. Would the following be better: > > ----- > From: ... > To: ... > Subject: Re: [PATCH 00/18] use semicolons... > > On Sun... > > These patches... > > > > [...] > > A subset of these patches was applied to > > https://... > > Thanks! > > [5/18] regmap: debugfs: > commit: > > (etc) > ----- > > In other words, we: > > - specifically say that it's a subset > - instead of just enumerating the number of patches that were applied, > as is currently the case ([1/1]) we list the exact numbers out of the > posted series (e.g. [5/18]) > > I think this is a better solution than potentially flooding everyone > with 49 emails. I think it would be better to reply individually as the likelihood that the maintainer skips just a few patches of a large series is relatively low. It's more likely for a treewide or multi-subsystem patch set for a maintainer to apply just a single one or a selected few of the patches and individual replies make it much easier to determine which ones were applied. thanks, Joe
On Sat, Oct 03, 2020 at 12:43:13PM -0700, Joe Perches wrote: > On Sat, 2020-10-03 at 15:31 -0400, Konstantin Ryabitsev wrote: > > I'm worried that this can get unwieldy for series of 50 patches where 49 > > got applied. Would the following be better: ... > > A subset of these patches was applied to > > > > https://... > > > > Thanks! > > > > [5/18] regmap: debugfs: > > commit: It's definitely an improvement but TBH I'm not sure how much it's going to help those struggling to parse the current messages. > > I think this is a better solution than potentially flooding everyone > > with 49 emails. I would tend to prefer cutting down on mail volume but I don't think there's any way to keep everyone happy with this stuff. > I think it would be better to reply individually as > the likelihood that the maintainer skips just a few > patches of a large series is relatively low. It's not at all unusual for driver updates to both add new DT bindings (either for entirely new drivers or new properties/compatibles for existing drivers) and also have DTS file updates using those bindings, these go via separate trees.
These patches replace commas by semicolons. This was done using the Coccinelle semantic patch (http://coccinelle.lip6.fr/) shown below. This semantic patch ensures that commas inside for loop headers will not be transformed. It also doesn't touch macro definitions. Coccinelle ensures that braces are added as needed when a single-statement branch turns into a multi-statement one. This semantic patch has a few false positives, for variable delcarations such as: LIST_HEAD(x), *y; The semantic patch could be improved to avoid these, but for the moment they have been removed manually (2 occurrences). // <smpl> @initialize:ocaml@ @@ let infunction p = (* avoid macros *) (List.hd p).current_element <> "something_else" let combined p1 p2 = (List.hd p1).line_end = (List.hd p2).line || (((List.hd p1).line_end < (List.hd p2).line) && ((List.hd p1).col < (List.hd p2).col)) @bad@ statement S; declaration d; position p; @@ S@p d // special cases where newlines are needed (hope for no more than 5) @@ expression e1,e2; statement S; position p != bad.p; position p1; position p2 : script:ocaml(p1) { infunction p1 && combined p1 p2 }; @@ - e1@p1,@S@p e2@p2; + e1; e2; @@ expression e1,e2; statement S; position p != bad.p; position p1; position p2 : script:ocaml(p1) { infunction p1 && combined p1 p2 }; @@ - e1@p1,@S@p e2@p2; + e1; e2; @@ expression e1,e2; statement S; position p != bad.p; position p1; position p2 : script:ocaml(p1) { infunction p1 && combined p1 p2 }; @@ - e1@p1,@S@p e2@p2; + e1; e2; @@ expression e1,e2; statement S; position p != bad.p; position p1; position p2 : script:ocaml(p1) { infunction p1 && combined p1 p2 }; @@ - e1@p1,@S@p e2@p2; + e1; e2; @@ expression e1,e2; statement S; position p != bad.p; position p1; position p2 : script:ocaml(p1) { infunction p1 && combined p1 p2 }; @@ - e1@p1,@S@p e2@p2; + e1; e2; @r@ expression e1,e2; statement S; position p != bad.p; @@ e1 ,@S@p e2; @@ expression e1,e2; position p1; position p2 : script:ocaml(p1) { infunction p1 && not(combined p1 p2) }; statement S; position r.p; @@ e1@p1 -,@S@p +; e2@p2 ... when any // </smpl> --- drivers/acpi/processor_idle.c | 4 +++- drivers/ata/pata_icside.c | 21 +++++++++++++-------- drivers/base/regmap/regmap-debugfs.c | 2 +- drivers/bcma/driver_pci_host.c | 4 ++-- drivers/block/drbd/drbd_receiver.c | 6 ++++-- drivers/char/agp/amd-k7-agp.c | 2 +- drivers/char/agp/nvidia-agp.c | 2 +- drivers/char/agp/sworks-agp.c | 2 +- drivers/char/hw_random/iproc-rng200.c | 8 ++++---- drivers/char/hw_random/mxc-rnga.c | 6 +++--- drivers/char/hw_random/stm32-rng.c | 8 ++++---- drivers/char/ipmi/bt-bmc.c | 6 +++--- drivers/clk/meson/meson-aoclk.c | 2 +- drivers/clk/mvebu/ap-cpu-clk.c | 2 +- drivers/clk/uniphier/clk-uniphier-cpugear.c | 2 +- drivers/clk/uniphier/clk-uniphier-mux.c | 2 +- drivers/clocksource/mps2-timer.c | 6 +++--- drivers/clocksource/timer-armada-370-xp.c | 8 ++++---- drivers/counter/ti-eqep.c | 2 +- drivers/crypto/amcc/crypto4xx_alg.c | 2 +- drivers/crypto/atmel-tdes.c | 2 +- drivers/crypto/hifn_795x.c | 4 ++-- drivers/crypto/talitos.c | 8 ++++---- 23 files changed, 60 insertions(+), 51 deletions(-)