| Message ID | 20240520190131.94904-3-ericsunshine@charter.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 45db5ed3b2f9f1c4768633f3d691bbe1305cf9ca |
| Headers | show |
| Series | chainlint.pl: Extend regexp pattern for /proc/cpuinfo on Linux SPARC | expand |
On Mon, May 20, 2024 at 03:01:30PM UTC, Eric Sunshine wrote: > From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> > > On SPARC systems running Linux, individual processors are denoted with > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:". not sure if worth a reroll, but the "usual" syntax is "processor : NN" the regexp used checks for numbers before the colon to account for the syntax used on s390x which is the only one with numbers before the colon. Carlo
Hi Carlo, On Wed, 2024-05-22 at 01:32 -0700, Carlo Marcelo Arenas Belón wrote: > On Mon, May 20, 2024 at 03:01:30PM UTC, Eric Sunshine wrote: > > From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> > > > > On SPARC systems running Linux, individual processors are denoted with > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:". > > not sure if worth a reroll, but the "usual" syntax is "processor : NN" > the regexp used checks for numbers before the colon to account for the > syntax used on s390x which is the only one with numbers before the colon. Good catch. I think this can just be fixed by whoever commits the patches or is that done automatically? Adrian
On Wed, May 22, 2024 at 4:47 AM John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote: > On Wed, 2024-05-22 at 01:32 -0700, Carlo Marcelo Arenas Belón wrote: > > On Mon, May 20, 2024 at 03:01:30PM UTC, Eric Sunshine wrote: > > > From: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> > > > > > > On SPARC systems running Linux, individual processors are denoted with > > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:". > > > > not sure if worth a reroll, but the "usual" syntax is "processor : NN" > > the regexp used checks for numbers before the colon to account for the > > syntax used on s390x which is the only one with numbers before the colon. > > Good catch. I think this can just be fixed by whoever commits the patches > or is that done automatically? Inclusion of the word "usual" is such a minor flaw in the commit message that I doubt it warrants a reroll and the associated cost on reviewers and on the maintainer (Junio), especially since it does not negatively impact the intent conveyed by the commit messages nor the correctness of the actual patch. As such, I'm not worried about it. Whether Junio reads this and wants to correct it in his tree is up to him, of course.
Eric Sunshine <sunshine@sunshineco.com> writes: >> > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:". >> > >> > not sure if worth a reroll, but the "usual" syntax is "processor : NN" > ... > Inclusion of the word "usual" is such a minor flaw in the commit > message that I doubt it warrants a reroll and the associated cost on > reviewers and on the maintainer (Junio), especially since it does not > negatively impact the intent conveyed by the commit messages nor the > correctness of the actual patch. > > As such, I'm not worried about it. Whether Junio reads this and wants > to correct it in his tree is up to him, of course. I think "usual" is not what was pointed out. The order between the colon and NN is.
On Wed, May 22, 2024 at 3:00 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:". > >> > > >> > not sure if worth a reroll, but the "usual" syntax is "processor : NN" > > ... > > Inclusion of the word "usual" is such a minor flaw in the commit > > message that I doubt it warrants a reroll and the associated cost on > > reviewers and on the maintainer (Junio), especially since it does not > > negatively impact the intent conveyed by the commit messages nor the > > correctness of the actual patch. > > > > As such, I'm not worried about it. Whether Junio reads this and wants > > to correct it in his tree is up to him, of course. > > I think "usual" is not what was pointed out. The order between the > colon and NN is. Yes, I understood that, but it is the word "usual" which makes the text "processor NN:" questionable since "processor NN:" is not typical. Without the word "usual", stating "processor NN:" is not especially problematic since the existing regex (which is being changed by this patch) _does_ match "processor NN:" (among others such as "processor:"). If we want to be more accurate, better wording might be: On SPARC systems running Linux, individual processors are denoted with "CPUnn:" in /proc/cpuinfo, however, the regexp in ncores() matches only "processor:" or "processor NN:". As a result, no processors are found on SPARC. Address this shortcoming by extending the regexp to also match lines with "CPUnn:". but I doubt it is worth a reroll.
Hi, On Wed, 2024-05-22 at 15:11 -0400, Eric Sunshine wrote: > On Wed, May 22, 2024 at 3:00 PM Junio C Hamano <gitster@pobox.com> wrote: > > Eric Sunshine <sunshine@sunshineco.com> writes: > > > > > > "CPUnn:" in /proc/cpuinfo instead of the usual "processor NN:". > > > > > > > > > > not sure if worth a reroll, but the "usual" syntax is "processor : NN" > > > ... > > > Inclusion of the word "usual" is such a minor flaw in the commit > > > message that I doubt it warrants a reroll and the associated cost on > > > reviewers and on the maintainer (Junio), especially since it does not > > > negatively impact the intent conveyed by the commit messages nor the > > > correctness of the actual patch. > > > > > > As such, I'm not worried about it. Whether Junio reads this and wants > > > to correct it in his tree is up to him, of course. > > > > I think "usual" is not what was pointed out. The order between the > > colon and NN is. > > Yes, I understood that, but it is the word "usual" which makes the > text "processor NN:" questionable since "processor NN:" is not > typical. Without the word "usual", stating "processor NN:" is not > especially problematic since the existing regex (which is being > changed by this patch) _does_ match "processor NN:" (among others such > as "processor:"). > > If we want to be more accurate, better wording might be: > > On SPARC systems running Linux, individual processors are denoted > with "CPUnn:" in /proc/cpuinfo, however, the regexp in ncores() > matches only "processor:" or "processor NN:". As a result, no > processors are found on SPARC. Address this shortcoming by > extending the regexp to also match lines with "CPUnn:". > > but I doubt it is worth a reroll. So, could we get this series merged now or is there anything missing? Thanks, Adrian
On Mon, May 27, 2024 at 3:49 PM John Paul Adrian Glaubitz
<glaubitz@physik.fu-berlin.de> wrote:
> So, could we get this series merged now or is there anything missing?
This series has already migrated from the "seen" branch to the "next"
branch in Junio's tree, and according to his latest "What's Cooking"
report[*], he will be merging it to his "master" branch soon, after
which it should be incorporated into an actual release.
[*]: topic "es/chainlint-ncores-fix " in
https://lore.kernel.org/git/xmqq8qzyifnx.fsf@gitster.g/
diff --git a/t/chainlint.pl b/t/chainlint.pl index d9a2691889..d593cb95e7 100755 --- a/t/chainlint.pl +++ b/t/chainlint.pl @@ -724,7 +724,7 @@ sub ncores { if (open my $fh, '<', '/proc/cpuinfo') { my $cpuinfo = do { local $/; <$fh> }; close($fh); - my @matches = ($cpuinfo =~ /^processor[\s\d]*:/mg); + my @matches = ($cpuinfo =~ /^(processor|CPU)[\s\d]*:/mg); return @matches ? scalar(@matches) : 1; } # macOS & BSD