Re: maint: import tests/init.sh from Gnulib during bootstrap

[Jim Meyering]

On Thu, Jul 4, 2024 at 1:36 AM Pádraig Brady  wrote:
>
> On 04/07/2024 02:08, Collin Funk wrote:
> > Hi,
> >
> > Recently there was a bug fix in Gnulib's tests/init.sh. I then noticed
> > many packages copy this manually from Gnulib. Bruno and I discussed the
> > proper way to import the file from Gnulib [1].
> >
> > What do you think of the attached patch? Basically remove the file from
> > version control. Then add 'gnulib-tool --copy-file tests/init.sh' to
> > bootstrap_post_import_hook. That way every time ./bootstrap is invoked
> > the proper version is imported.

Thanks. I've done the same for gzip, sed, diffutils, grep and cppi.
It needed a tiny adaptation for sed's use of "testsuite" as the
directory name rather than the more common "tests".
Also, there were two new warnings exposed in gzip (because I built
with bleeding edge GCC).
Along the way, I updated each to latest gnulib+bootstrap, too.

Re: [PATCH] cat,cp,mv,install,split: Set the minimum IO block size used to 256KiB

[Jim Meyering]

On Wed, Feb 28, 2024 at 9:09 AM Pádraig Brady  wrote:
>  >> On 11/30/23 12:11, Pádraig Brady wrote:
>  >>> Though that will generally give 128K, which is good when processing all
>  >>> of a file,
>  >>> but perhaps overkill when processing just the last part of a file.
>  >>
>  >> The 128 KiB number was computed as being better for apps like 'sed' that
>  >> typically read all or most of the file. 'tail' sometimes behaves that
>  >> way (e.g., 'tail -c +10') and so 'tail' should use 128 KiB in those
>  >> cases. The simplest way to do that is for 'tail' to use 128 KiB all the
>  >> time - that would cost little for uses like plain 'tail' and it could be
>  >> a significant win for uses like 'tail -c +10'.
>  >
>  > Yes I agree we should use io_blksize() in other routines in tail
>  > where we may dump lots of a file. However in this (most common) case
>  > the routine is dealing with the end of a regular file,
>  > so it's probably best to somewhat minimize the amount of data read,
>  > and more directly check the page_size which is issue at hand.
>  > I've pushed the fix at 
> https://github.com/coreutils/coreutils/commit/73d119f4f
>  > where the adjustment (which also corresponds to what we do in wc) is:
>  >
>  > if (sb->st_size % page_size == 0)
>  >   bufsize = MAX (BUFSIZ, page_size);
>  >
>  > I'll follow up with another patch to address the performance aspect,
>  > which uses io_blksize() where appropriate.
>
> More testing shows that 256KiB does indeed give a 10-20% bump on modern 
> systems.
> Proposed change is attached.

Thanks. Probably worth setting an alarm to check once a year :-)
I think you'll want to change this comment, too:
> /* As of May 2014, 128KiB is determined to be the minimum

Re: fix thousands grouping issues

[Jim Meyering]

On Wed, Dec 27, 2023 at 4:14 PM Pádraig Brady  wrote:
> On 27/12/2023 20:52, Jim Meyering wrote:
> > Thanks. Nice improvement.
> > I compared --help and --version output, which showed no difference
> > other than the intended ones.
>
> Thanks for the review. Pushed.
>
> >
> > make check passed, modulo this unrelated failure on Fedora 38:
> >
> > numfmt.pl: test lcl-dbl-to-human-1: stdout mismatch, comparing
> > lcl-dbl-to-human-1.1 (expected) and lcl-dbl-to-human-1.O (actual)
> > *** lcl-dbl-to-human-1.1Wed Dec 27 12:47:48 2023
> > --- lcl-dbl-to-human-1.OWed Dec 27 12:47:48 2023
> > ***
> > *** 1 
> > ! 1,1K
> > --- 1 
> > ! 1,1k
>
> Oh interesting.
> I suspect that's due to glibc-all-langpacks not being installed on your 
> system.
> That would mean a regular space was used as the thousands grouping char,
> and thus the test was run for you, and not for me.
>
> I see a similar skipping issue in tests/sort/sort-h-thousands-sep.sh,
> and in fact that was hiding a sort bug introduced in v9.1 !
>
> Hopefully the 3 patches attached addresses these issues.
> Paul are you Ok with the change to sort in the last patch?

Thanks. That looks right to me.
The other two patches look impeccable, too.

Re: [PATCH] chgrp: --from parameter similar to chown

[Jim Meyering]

Thanks. Nice improvement.
I compared --help and --version output, which showed no difference
other than the intended ones.

make check passed, modulo this unrelated failure on Fedora 38:

numfmt.pl: test lcl-dbl-to-human-1: stdout mismatch, comparing
lcl-dbl-to-human-1.1 (expected) and lcl-dbl-to-human-1.O (actual)
*** lcl-dbl-to-human-1.1Wed Dec 27 12:47:48 2023
--- lcl-dbl-to-human-1.OWed Dec 27 12:47:48 2023
***
*** 1 
! 1,1K
--- 1 
! 1,1k

Re: [PATCH] chgrp: --from parameter similar to chown

[Jim Meyering]

On Wed, Dec 27, 2023 at 8:20 AM Pádraig Brady  wrote:
> On 17/12/2023 14:17, Pádraig Brady wrote:
> > On 16/12/2023 22:01, Bernhard Voelker wrote:
> >> On 12/15/23 14:22, Pádraig Brady wrote:
> >>> On 31/10/2023 19:49, Pádraig Brady wrote:
>  On 31/10/2023 17:51, Ed Neville wrote:
> > Hi there,
> >
> > Please could one of the maintainers take a look at this patch? Is it
> > ready for merge? It should be a straight forward change that brings the
> > same functionality from chown.
> >
> > Best regards,
> > Ed
> 
>  Yes I'll get to this one soon.
> >>>
> >>> I'll merge the attached later.
> >>> It's a complete patch adding docs, tests, some refactoring.
> >>
> >> The patch looks good ... yet didn't
> >>  chown --from=:SOMEGROUP :OTHERGROUP ...
> >> suffice?
> >
> > Heh I was thinking that myself.
> > But then what's the point of chgrp at all :)
> > We could (and perhaps should) treat chown/chgrp like we do for ls/dir.
>
> I'll push the attached later to merge chown/chgrp sources,
> which will avoid divergence in future.

Hi Pádraig! Happy holidays :-)
An important new file, chown.h, is not included in that diff.

Re: [PATCH] maint: use C99 int size specifiers rather than PRI.MAX defines

[Jim Meyering]

Nice. Thanks.

Re: fix intermittent test failure triggered by split /tmp usage

[Jim Meyering]

On Tue, Jul 18, 2023 at 11:14 AM Pádraig Brady  wrote:
> The new test where `split -n 1/2 /dev/zero` exhausts /tmp
> will trigger other false positive failures in the test suite
> which happens often when running tests with RUN_VERY_EXPENSIVE_TESTS set.
>
> The attached patches tackle this by consolidating temp file handling
> for tac and split, so that have a configurable $TMPDIR.
>
> We then use $TMPDIR in an adjusted test to point to
> a mounted file system to provide better isolation.

Nice work. That all looks fine. Thanks!
The only nit I saw was the use of $(pwd), when $PWD works fine.
Sure, existing uses of $(pwd) outnumber $PWD 25 to 10, but those
25 should probably change, too.

Re: [PATCH] maint: add a syntax check to prevent use of NULL

[Jim Meyering]

On Tue, Jul 18, 2023 at 3:14 PM Pádraig Brady  wrote:
>
> * cfg.mk (sc_prohibit_NULL): Direct to use nullptr instead.

Good one. Now I want to add that in each of at least diffutils, grep, sed, gzip.

Re: [PATCH] cksum: improve problematic_chars function

[Jim Meyering]

On Fri, Jul 14, 2023 at 3:27 AM Pádraig Brady  wrote:
> On 14/07/2023 07:54, Jim Meyering wrote:
> > Small suggested improvement:
>
> Looks good thanks.
> Testing with the attached shows a 23% improvement on average
> with using strcspn() over strchr()x3.

Thanks for checking. Pushed.

[PATCH] cksum: improve problematic_chars function

[Jim Meyering]

Small suggested improvement:

>From c0e7e4a1d41049bbf38cf902d13746b1ab5b1e38 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 13 Jul 2023 22:29:52 -0700
Subject: [PATCH] cksum: improve problematic_chars function

* src/digest.c (problematic_chars): Implement using strcspn,
and traversing S only once, rather than once per escaped byte.
---
 src/digest.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/digest.c b/src/digest.c
index e80eb3406..60ba82e5f 100644
--- a/src/digest.c
+++ b/src/digest.c
@@ -568,7 +568,8 @@ ATTRIBUTE_PURE
 static bool
 problematic_chars (char const *s)
 {
-  return strchr (s, '\\') || strchr (s, '\n') || strchr (s, '\r');
+  size_t length = strcspn (s, "\\\n\r");
+  return s[length] != '\0';
 }

 #define ISWHITE(c) ((c) == ' ' || (c) == '\t')
-- 
2.41.0.327.gaa9166bcc0

Re: RFC: change cksum -b to allow full emulation of older utils

[Jim Meyering]

On Sat, Jul 8, 2023 at 1:58 PM Pádraig Brady  wrote:
> On 08/07/2023 19:52, Jim Meyering wrote:
> > On Sat, Jul 8, 2023 at 7:28 AM Pádraig Brady  wrote:
> >> It would be nice to be able to support users who want to replace all
> >> the *sum utilities with a script that did:
> >>
> >> exec cksum -a  --untagged "$@"
> >>
> >> To that end, it would be good to support all the options of the older 
> >> utils.
> >> This is a bit awkward with --base64 added in the last few months, as that
> >> also added the short -b option.  However it's only a few months released,
> >> so it's probably not too onerous to change.
> >>
> >> The attached makes the change, which I'll augment with NEWS and tests/ 
> >> upon agreement.
> >
> > Good idea. I agree that the added value outweighs the cost of
> > divergence with OpenBSD cksum's -b (https://man.openbsd.org/cksum.1)
>
> Oh right. Though all OpenBSD's cksum options are not supported
> so we don't have to worry too much about direct compat here.
>
> > One nit: the in-comment reference to -b needs to say --base64, too (in
> > tests/cksum/cksum-base64.pl):
> >
> >  # Ensure that each of the above works with -b:
> >  (map {my ($h,$v)= @$_; my $o=fmt $h,$v;
> > - [$h, "-ba $h", {IN=>{f=>''}}, {OUT=>"$o\n"}]} @pairs),
> > + [$h, "--base64 -a $h", {IN=>{f=>''}}, {OUT=>"$o\n"}]} @pairs),
>
> Latest patch attached.

Nice. I like seeing so many ifdefs being deleted.
One nit is in NEWS. I'd change this:
+  'cksum -b' will no longer print base64-encoded checksums.  Rather that

to this:
+  'cksum -b' no longer prints base64-encoded checksums.  Rather that

oh, and also simplify this:
+printf -- '%s' "$pmode" | grep '' > /dev/null && pmode="$tmode"

to this:
+case $pmode in *'') pmode=$tmode;; esac

If you were to use e.g., MODE as the marker instead of , you
could drop the single quotes, too.

Thanks!

Re: RFC: change cksum -b to allow full emulation of older utils

[Jim Meyering]

On Sat, Jul 8, 2023 at 7:28 AM Pádraig Brady  wrote:
> It would be nice to be able to support users who want to replace all
> the *sum utilities with a script that did:
>
>exec cksum -a  --untagged "$@"
>
> To that end, it would be good to support all the options of the older utils.
> This is a bit awkward with --base64 added in the last few months, as that
> also added the short -b option.  However it's only a few months released,
> so it's probably not too onerous to change.
>
> The attached makes the change, which I'll augment with NEWS and tests/ upon 
> agreement.

Good idea. I agree that the added value outweighs the cost of
divergence with OpenBSD cksum's -b (https://man.openbsd.org/cksum.1)

One nit: the in-comment reference to -b needs to say --base64, too (in
tests/cksum/cksum-base64.pl):

# Ensure that each of the above works with -b:
(map {my ($h,$v)= @$_; my $o=fmt $h,$v;
- [$h, "-ba $h", {IN=>{f=>''}}, {OUT=>"$o\n"}]} @pairs),
+ [$h, "--base64 -a $h", {IN=>{f=>''}}, {OUT=>"$o\n"}]} @pairs),

Re: Bug#1037275: dd: regression - posix expression syntax no longer supported

[Jim Meyering]

On Sat, Jun 10, 2023 at 6:50 AM Pádraig Brady  wrote:
> On 10/06/2023 02:45, Marc Lehmann wrote:
> > Package: coreutils
> > Version: 9.1-1
> > Severity: normal
> >
> > Dear Maintainer,
> >
> > I have a script that was used for some decades on multiple
> > unices. Beginning with bookworm, it stopped working because dd no longer
> > understands POSIX expression syntax for bs=:
> >
> > $ dd if=... bs=1024x1024x32
> > dd: invalid number: ‘1024x1024x32’
> >
> > This should be valid syntax according to POSIX, and was understood on
> > older versions of Debian GNU/Linux:
> >
> > Two or more positive decimal numbers (with or without k or b) separated 
> > by x, specifying the product of the indicated values
>
> Yes this was a regression in coreutils 9.1
> The patch attached is the proposed upstream fix.

Thanks. The patch looks fine.
My only suggestion would be a stylistic one, to change the ">"
comparison to be the equivalent "<" one, i.e., change this:

+  && *suffix == 'B' && (suffix > str && suffix[-1] != 'B'))

to this (also dropping the unnecessary parentheses):

+  && *suffix == 'B' && str < suffix && suffix[-1] != 'B')

Why? Because of the increasing-to-right number-line argument, where
smaller things are visually on the left of larger ones.

Currently, in src/, the uses of space-delimited < and <= outnumber
uses of > and >= by four to one, 1678 to 428.

Re: error: Support the compiler's control flow analysis better (was: copy-file: Silence gcc warnings)

[Jim Meyering]

On Wed, May 31, 2023 at 9:12 AM Pádraig Brady  wrote:
> On 30/05/2023 22:29, Paul Eggert wrote:
> > On 5/28/23 06:07, Pádraig Brady wrote:
> >> There still is a gotcha (hit in dd.c in coreutils)
> >> where if you define an error macro yourself
> >> you get a macro redefinition error,
> >
> > I see you fixed that by adding a quick "#define _GL_NO_INLINE_ERROR" to
> > coreutils/src/dd.c. It's a bit cleaner to fix the underlying naming
> > problem instead, so that dd.c need not define the Gnulib internals macro
> > (or its own quirky error macro), so I installed the attached to
> > coreutils to do that.
>
> I was debating that option but decided against it
> as we'd then lose some of the syntax checks on error(args).
> But we can augment the syntax checks to cater for this class of function,
> which I'm doing as follows.
>
> cheers,
> Pádraig
>
> diff --git a/cfg.mk b/cfg.mk
> index 263bc0cfd..64db2bec4 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -189,12 +189,15 @@ sc_prohibit_quotes_notation:
> exit 1; }  \
>|| :
>
> +error_fns = (error|die|diagnose)
> +
>   # Files in src/ should quote all strings in error() output, so that
>   # unexpected input chars like \r etc. don't corrupt the error.
>   # In edge cases this can be avoided by putting the format string
>   # on a separate line to the arguments, or the arguments in parenthesis.
>   sc_error_quotes:
> -   @cd $(srcdir)/src && GIT_PAGER= git grep -n 'error *(.*%s.*, 
> [^(]*);$$'\
> +   @cd $(srcdir)/src \
> + && GIT_PAGER= git grep -E -n '${error_fns} *\(.*%s.*, [^(]*\);$$' \
>*.c | grep -v ', q' \
>&& { echo '$(ME): '"Use quote() for error string arguments" 1>&2; \
> exit 1; }  \
> @@ -206,7 +209,7 @@ sc_error_quotes:
>   sc_error_shell_quotes:
>  @cd $(srcdir)/src && \
>{ GIT_PAGER= git grep -E \
> -   'error \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \
> +   '${error_fns} \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \

Thanks!
Was there a reason to prefer curly braces there, rather than the more
conventional parentheses?

   '$(error_fns) \(.*%s[:"], .*(name|file)[^"]*\);$$' *.c; \

Re: consider using latest autoconf for upcoming release?

[Jim Meyering]

On Mon, Mar 27, 2023 at 5:00 AM Pádraig Brady  wrote:
> On 27/03/2023 02:27, Jim Meyering wrote:
> > Hi Pádraig,
> >
> > I recently made the grep-3.10 release using the very latest autoconf
> > (built from master). I'm preparing to make an autoconf snapshot, and
> > hopefully soon after, a "stable" 2.73 release. So far, no one has
> > reported any issue with grep-3.10, but it has significantly less
> > autoconf coverage than coreutils.
> >
> > What do you think about using either autoconf's master or the imminent
> > snapshot when preparing coreutils-9.3?
>
> My thinking for the last few releases was to always
> use the latest versions from the latest version of Fedora.
> If 2.73 is being cut though, it makes sense to align with that.
> We're due a 9.3 release very soon anyway, so it does make sense to align 
> these.

autoconf-2.72c snapshot announcement here:
https://lists.gnu.org/r/autoconf/2023-03/msg00020.html

Re: consider using latest autoconf for upcoming release?

[Jim Meyering]

On Sun, Mar 26, 2023 at 6:27 PM Jim Meyering  wrote:
> Hi Pádraig,
>
> I recently made the grep-3.10 release using the very latest autoconf
> (built from master). I'm preparing to make an autoconf snapshot, and
> hopefully soon after, a "stable" 2.73 release. So far, no one has
> reported any issue with grep-3.10, but it has significantly less
> autoconf coverage than coreutils.
>
> What do you think about using either autoconf's master or the imminent
> snapshot when preparing coreutils-9.3?

FTR, I've verified that when bootstrapped with the latest of both
automake and autoconf, coreutils v9.2-6-g6272817de passes "make
distcheck" on fedora 37 x86_64.

consider using latest autoconf for upcoming release?

[Jim Meyering]

Hi Pádraig,

I recently made the grep-3.10 release using the very latest autoconf
(built from master). I'm preparing to make an autoconf snapshot, and
hopefully soon after, a "stable" 2.73 release. So far, no one has
reported any issue with grep-3.10, but it has significantly less
autoconf coverage than coreutils.

What do you think about using either autoconf's master or the imminent
snapshot when preparing coreutils-9.3?

Thanks,
Jim

Re: Fwd: Bug#1015273: coreutils: rm -d doesn't try to remove unreadable directories, lies in error message, with *fails to prompt* with -i

[Jim Meyering]

On Tue, Feb 21, 2023 at 4:45 AM Pádraig Brady  wrote:
> On 21/02/2023 02:59, Jim Meyering wrote:
...
> > Thank you for the bug report.
> > I've attached a patch that fixes those bugs.
>
> lgtm
> modulo the commit summary line should start with rm: not rm ...
> Do you have git hooks disabled?

Thanks for the speedy review. I did indeed. Fixed that, inserted a
colon, and pushed.

BTW, when I pulled latest, I see (but haven't investigated) a new
build failure using this recent gcc:
gcc version 13.0.1 20230219 (experimental) (GCC)

In function 'lseek_copy',
inlined from 'copy_reg' at src/copy.c:1501:16,
inlined from 'copy_internal' at src/copy.c:2961:13:
src/copy.c:485:23: error: 'scan_inference.ext_start' may be used
uninitialized [-Werror=maybe-uninitialized]
  485 |   off_t ext_end = lseek (src_fd, ext_start, SEEK_HOLE);
  |   ^~~~
src/copy.c: In function 'copy_internal':
src/copy.c:1165:24: note: 'scan_inference.ext_start' was declared here
 1165 |   union scan_inference scan_inference;
  |^~

Fwd: Bug#1015273: coreutils: rm -d doesn't try to remove unreadable directories, lies in error message, with *fails to prompt* with -i

[Jim Meyering]

-- Forwarded message -
From: Jim Meyering 
Date: Mon, Feb 20, 2023 at 6:27 PM
Subject: Re: Bug#1015273: coreutils: rm -d doesn't try to remove
unreadable directories, lies in error message, with *fails to prompt*
with -i
To: наб , <1015...@bugs.debian.org>
Cc: Debian Bug Tracking System 

On Mon, Jul 18, 2022 at 12:21 PM наб  wrote:
> Package: coreutils
> Version: 8.32-4.1
> Severity: normal
>
> Dear Maintainer,
>
> Fun one for ya: the baseline:
> -- >8 --
> $ mkdir -p /tmp/psko
> $ rm -vid /tmp/psko
> rm: remove directory '/tmp/psko'? y
> removed directory '/tmp/psko'
> -- >8 --
>
> Bug a:
> -- >8 --
> $ mkdir -p /tmp/psko
> $ chmod -r /tmp/psko
> $ rm -vid /tmp/psko; echo $?
> rm: cannot remove '/tmp/psko': Directory not empty
> 1
> -- >8 --
>
> Absolutely 0 prompt, despite -i!
> That's very fun (and a POSIX violation)!
>
> Bug b:
> -- >8 --
> $ strace rm -vid /tmp/psko 2>&1 | grep -v locale
> execve("/bin/rm", ["rm", "-vid", "/tmp/psko"], 0xff8fbc48 /* 24 vars */) = 0
> /* ... */
> arch_prctl(ARCH_SET_FS, 0xf7f9e240) = 0
> mprotect(0xf7f8b000, 8192, PROT_READ)   = 0
> mprotect(0x40f000, 4096, PROT_READ) = 0
> mprotect(0xf7fcd000, 8192, PROT_READ)   = 0
> munmap(0xf7f96000, 26859)   = 0
> brk(NULL)   = 0xaa6000
> brk(0xac7000)   = 0xac7000
> brk(0xac8000)   = 0xac8000
> newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=3041504, ...}, 
> AT_EMPTY_PATH) = 0
> mmap(NULL, 2097152, PROT_READ, MAP_PRIVATE, 3, 0) = 0xf7a0
> mmap(NULL, 2596864, PROT_READ, MAP_PRIVATE, 3, 0x6d000) = 0xf7786000
> close(3)= 0
> ioctl(0, TCGETS, {B38400 opost isig icanon echo ...}) = 0
> newfstatat(AT_FDCWD, "/tmp/psko", {st_mode=S_IFDIR|0311, st_size=40, ...}, 
> AT_SYMLINK_NOFOLLOW) = 0
> openat(AT_FDCWD, "/tmp/psko", 
> O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_DIRECTORY) = -1 EACCES (Permission 
> denied)
> newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=2996, ...}, AT_EMPTY_PATH) = > 0
> read(3, "# Locale name alias data base.\n#"..., 4096) = 2996
> read(3, "", 4096)   = 0
> close(3)= 0
> write(2, "rm: ", 4rm: ) = 4
> write(2, "cannot remove '/tmp/psko'", 25cannot remove '/tmp/psko') = 25
> newfstatat(3, "", {st_mode=S_IFREG|0644, st_size=1433, ...}, AT_EMPTY_PATH) = > 0
> mmap(NULL, 1433, PROT_READ, MAP_PRIVATE, 3, 0) = 0xf7f9c000
> close(3)= 0
> write(2, ": Directory not empty", 21: Directory not empty)   = 21
> write(2, "\n", 1
> )   = 1
> lseek(0, 0, SEEK_CUR)   = -1 ESPIPE (Illegal seek)
> close(0)= 0
> close(1)= 0
> close(2)= 0
> exit_group(1)   = ?
> +++ exited with 1 +++
> -- >8 --
>
> Can you spot a rmdir(2) equivalent? I can't! So why does it lie and say
> that it tried to remove it?
>
> Also, the directory /isn't nonempty/! Bug c:
> -- >8 --
> $ rmdir -v /tmp/psko/; echo $?
> rmdir: removing directory, '/tmp/psko/'
> 0
> -- >8 --
> And it's not there! Because, shockingly, there's nothing stopping you
> from removing it! So why on /earth/ is rm failing to prompt, failing to
> try to remove it, then lying that it had and failed with an obviously
> false errno?
>
> The same applies to just plain rm -d /tmp/psko (no -i)
> (except for bug a).

Thank you for the bug report.
I've attached a patch that fixes those bugs.


rm--dir.diff
Description: Binary data

Re: [PATCH] cksum: add --raw option to output a binary digest

[Jim Meyering]

On Sat, Feb 4, 2023 at 10:01 AM Pádraig Brady  wrote:
>
> On 04/02/2023 16:58, Jim Meyering wrote:
> > On Sat, Feb 4, 2023 at 5:52 AM Pádraig Brady  wrote:
> >>
> >> --raw output is the most composable format, and also is a
> >> robust way to discard the file name without parsing (escaped) output.
> >
> > Very nice. I applied it and ran all tests, and distcheck. All passed.
> > TIny suggestions:
> >
> >> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> >> index 4d7d9439d..536301d63 100644
> >> --- a/doc/coreutils.texi
> >> +++ b/doc/coreutils.texi
> >> @@ -4059,6 +4059,14 @@ input digest string as what is output.  I.e., 
> >> removing or adding any
> >>   @opindex --debug
> >>   Output extra information to stderr, like the checksum implementation 
> >> being used.
> >>
> >> +@item --raw
> >> +@opindex --raw
> >> +@cindex raw binary checksum
> >> +Print an unencoded raw binary digest, not hexadecimal.
> >> +No file name or other information is output in this mode.
> >> +Bytes are output in network byte order (big endian).
> >> +This option is ignored with @option{--check}.
> >
> > Maybe insert "only" and use active voice, and do mention the "only one
> > input" limit:
> >
> >Print only the unencoded raw binary digest for a single file.
> >Do not output the file name or anything else.
> >Use network byte order (big endian) where applicable: for bsd, crc and 
> > sysv.
> >This option is ignored with @option{--check} and works only with a
> > single input.
>
> Much better, thanks.
>
> > Is it worth saying explicitly that unlike other output formats, cksum
> > provides no way to check a --raw checksum?
>
> Yes. I'll change your last line to:
>
>This option works only with a single input.
>Unlike other output formats, cksum provides no way to --check a --raw 
> checksum.

I noticed a sporadic test failure when the bsd checksum happened to
have one or more leading zeros.
Compare these:

  $ src/cksum --raw --algorithm=bsd <<< 2|od --endian=big -An -w1024
-tu2|tr -d ' '
  35
  $ src/cksum --untagged --algorithm=bsd <<< 2
  00035 1

At first I thought I'd strip the excess zeros before comparing,
but thought it slightly better to zero-pad any too-narrow value.

The attached seems to do the job:


cksum-raw-zero-padding.diff
Description: Binary data

Re: [PATCH] cksum: add --raw option to output a binary digest

[Jim Meyering]

On Sat, Feb 4, 2023 at 5:52 AM Pádraig Brady  wrote:
>
> --raw output is the most composable format, and also is a
> robust way to discard the file name without parsing (escaped) output.

Very nice. I applied it and ran all tests, and distcheck. All passed.
TIny suggestions:

> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 4d7d9439d..536301d63 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -4059,6 +4059,14 @@ input digest string as what is output.  I.e., removing 
> or adding any
>  @opindex --debug
>  Output extra information to stderr, like the checksum implementation being 
> used.
>
> +@item --raw
> +@opindex --raw
> +@cindex raw binary checksum
> +Print an unencoded raw binary digest, not hexadecimal.
> +No file name or other information is output in this mode.
> +Bytes are output in network byte order (big endian).
> +This option is ignored with @option{--check}.

Maybe insert "only" and use active voice, and do mention the "only one
input" limit:

  Print only the unencoded raw binary digest for a single file.
  Do not output the file name or anything else.
  Use network byte order (big endian) where applicable: for bsd, crc and sysv.
  This option is ignored with @option{--check} and works only with a
single input.

Is it worth saying explicitly that unlike other output formats, cksum
provides no way to check a --raw checksum?

Re: RFC: cksum --base64/-b support

[Jim Meyering]

On Tue, Jan 31, 2023 at 1:43 PM Pádraig Brady  wrote:
> s/accepted/supports/ in NEWS

Thanks. Good catch.
I prefer to use "accepts", matching the entry just below.
Will push soon.

Re: RFC: cksum --base64/-b support

[Jim Meyering]

On Tue, Jan 31, 2023 at 5:00 AM Pádraig Brady  wrote:
> On 31/01/2023 06:48, Jim Meyering wrote:
> > On Mon, Jan 30, 2023 at 11:29 AM Pádraig Brady  wrote:
> >> ...
> >
> > Thanks for the speedy feedback.
> >
> >> "If must be followed by white space." comment has a typo
> >> and also not enforced explicitly, so could be removed.
> >
> > Thanks. Removed.
> >
> >> valid_digits() may check beyond the end of the buffer
> >> in the case len != BASE64_LENGTH.
> >
> > Please share how that could happen.
> > I don't see how there could be a read-overrun there, since the input
> > string is always NUL-terminated.
>
> Oh right I missed that.
> Indeed the !isxdigit() would have terminated upon reaching the NUL.
> Thanks for using the len though as it's easier to read at least.
>
> Similarly I now see that the `while (!ISWHITE())` can write past the NUL 
> terminator,
> which could be triggered with a line full of spaces for example.
> Patch suggested below...

Good catch. An input of just three NUL bytes would trigger a read
buffer overrun:

  printf '\0\0\0' |valgrind src/cksum -a sha1 --check

V3 has a test to cover that.

> > V2 attached.
>
> We should also mention that --check autodetects the encoding.
> I wonder should we have the separate *sum utils autodetect the encoding also,
> but not support the --base64 option. Then `cksum -a blah -b` format
> would be supported by `blahsum -c`. Perhaps it's best to restrict to cksum,
> to aid in the deprecation of the separate utils.

I too prefer to limit this feature addition to cksum.

> So suggested changes...
>
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi

Thanks. Applied along with test addition.


cu-cksum-base64-v3.diff
Description: Binary data

Re: RFC: cksum --base64/-b support

[Jim Meyering]

On Mon, Jan 30, 2023 at 11:29 AM Pádraig Brady  wrote:
> ...

Thanks for the speedy feedback.

> "If must be followed by white space." comment has a typo
> and also not enforced explicitly, so could be removed.

Thanks. Removed.

> valid_digits() may check beyond the end of the buffer
> in the case len != BASE64_LENGTH.

Please share how that could happen.
I don't see how there could be a read-overrun there, since the input
string is always NUL-terminated.
However, I've taking your suggestion below:

> Perhaps change the "else" to "else if (len == digest_hex_bytes)".
> Similarly it may be more defensive / efficient to pass
> digest_len out of split_3().

I'd considered whether to make *split_3 return that length to the
caller, but erred on the side of keeping changes small.
However, given your feedback, I've made both bsd_split_3 and split_3
return digest length via *D_LEN, and thus avoid adding that strlen
call.

> non padded base64 encodings are not supported.
> Is that OK. Worth mentioning in texinfo if so.

IMHO, we must reject a digest that's had any change to its trailing
"=" (padding) bytes.
Done.

> A non padded negative test in cksum-c.sh would be useful.

Good idea.
I've added tests like that in the new test file, removing one or both
"=" bytes in each of those examples.

V2 attached.


cu-cksum-base64-v2.diff
Description: Binary data

Re: RFC: cksum --base64/-b support

[Jim Meyering]

On Mon, Jan 30, 2023 at 10:17 AM Pádraig Brady  wrote:
> On 29/01/2023 20:40, Jim Meyering wrote:
> > Hi Pádraig! Happy new year (belatedly ;-). Hope you're well.
> >
> > I'd like our generated announcements to be able to include
> > base64-encoded checksums without having to recommend verifying them
> > using openbsd's cksum, so...

Thanks for the long reply.

> This is so the checksums are shorter right?

That was my primary motivation.

> I.e. 4x/3 rather than the 2x for hex.
> Playing devil's advocate, is the complexity of
> generally using base64 for this worth it, since say a 512 bit checksum
> would only reduce from 128 chars to 86 chars?

IMHO, added complexity is small, so yes, it's worth it.

> Also related to this is the use of variable length algorithms
> (supported with the existing -l option).
> A quick scan of https://www.keylength.com/ suggests newer algorithms
> like blake2 blake3 sha3 with -l 256 may be sufficient for this use case
> in which case the difference would only be 64 chars to 44 chars.
> The following demos that, while also using existing tools:
>
>$ sha256sum < /dev/null | xxd -r -p | basenc --base16
>E3B0C44298FC1C149AFBF4C8996FB92427AE41E4649B934CA495991B7852B855
>$ sha256sum < /dev/null | xxd -r -p | basenc --base64
>47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=
>
> > I've begun writing the code to add --base64/-b support for GNU cksum,
> > prompted by this thread:
> > https://lists.gnu.org/r/diffutils-devel/2023-01/msg00015.html and the
> > fact that OpenBSD already has this option (as -b):
> > https://man.openbsd.org/cksum
>
> We originally considered this back at:
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=7313
> which showed ways to achieve this with existing tools.
> Though if it was a common operation (like it would be for your use case),
> and for the fact that openbsd already implements this,
> then it would be worth adding an option.
>
> BTW I noted the following possible option when I recently refactored cksum:
>
>--digest_format={int, hex, base64, binary}
>/* cksum output formats:
>   int (sum, and cksum default),
>   hex (rest default),
>   b64 (to reduce size),
>   bin (would auto suppress names? restrict to single argument? */

I'm not sure how --digest-format=int -a sha1 would work.
Or --digest-format=bin -a sum

But doing it that way does provide a way to override a prior --base64 option.
Without that, is a --hex option warranted?

Re: RFC: cksum --base64/-b support

[Jim Meyering]

On Sun, Jan 29, 2023 at 1:10 PM Jim Meyering  wrote:
> On Sun, Jan 29, 2023 at 12:40 PM Jim Meyering  wrote:
> ...
> > - I'm inclined to work like the openbsd cksum and accept invocations
> > like "cksum -a sha1x" and "cksum -a sha1b". Any objection?
>
> Actually, I am now **disinclined** to implement this part. It'd make
> sense only if we were able to compute multiple checksums in a single
> invocation.
> Allowing that would require a fundamental redesign, which feels out of
> scope, at least initially.

Here's a proposed patch for that.


cu-cksum-base64.diff
Description: Binary data

[PATCH] digest.c: remove a duplicate variable

[Jim Meyering]

FYI, while preparing the --base64 patch, I noticed this nit, so have
just pushed the attached:


cu-dup-local.diff
Description: Binary data

[PATCH] build: avoid spurious failures due to lack of EGREP definition

[Jim Meyering]

FYI, I noticed failures that arose due to lack of an EGREP definition,
so I pushed the attached to fix them. Possibly specific to me for
using the very latest autoconf and/or automake locally.


cu-EGREP.diff
Description: Binary data

Re: RFC: cksum --base64/-b support

[Jim Meyering]

On Sun, Jan 29, 2023 at 12:40 PM Jim Meyering  wrote:
...
> - I'm inclined to work like the openbsd cksum and accept invocations
> like "cksum -a sha1x" and "cksum -a sha1b". Any objection?

Actually, I am now **disinclined** to implement this part. It'd make
sense only if we were able to compute multiple checksums in a single
invocation.
Allowing that would require a fundamental redesign, which feels out of
scope, at least initially.

RFC: cksum --base64/-b support

[Jim Meyering]

Hi Pádraig! Happy new year (belatedly ;-). Hope you're well.

I'd like our generated announcements to be able to include
base64-encoded checksums without having to recommend verifying them
using openbsd's cksum, so...

I've begun writing the code to add --base64/-b support for GNU cksum,
prompted by this thread:
https://lists.gnu.org/r/diffutils-devel/2023-01/msg00015.html and the
fact that OpenBSD already has this option (as -b):
https://man.openbsd.org/cksum

Two questions:
- blake2b's tag is inconsistently capitalized. All of the other tags
are all-caps versions of their lower-case strings, but this one is
spelled BLAKE2b, with a lower-case "b" at the end. I presume this is
desired. Likely too late to change to make it consistent. Arose while
considering how to implement support for the "x" and "b"
option suffixes, to denote "use hex" or "use base64" as encoding,
while the usual default is of course hex, and --base64 changes that.
Leading to my second question:
- I'm inclined to work like the openbsd cksum and accept invocations
like "cksum -a sha1x" and "cksum -a sha1b". Any objection?

Also, comparing algorithms, openbsd has two that we don't: rmd160, sha512/256
I'm not interested in adding those in this diff, of course, but it may
be something to consider for compatibility.

What's the schedule for the next release?
Assuming this is desirable, want to include it there?

My own ETA is variable, depending on pressure/desire.
I've written most of the code (but not yet suffix support) and minimal
tests, but no documentation or NEWS.

Re: [PATCH] Re: wc --no-total option

[Jim Meyering]

On Mon, Sep 12, 2022, 14:53 Pádraig Brady  wrote:

> On 12/09/2022 10:20, David Pinto wrote:
> > On Sun, 26 Jun 2022 at 00:49, David Pinto 
> wrote:
> >> Hi
> >>
> >> A couple of years ago [1] someone made a feature request for a wc
> >> option that would skip the total line when processing multiple files.
> >> I didn't see anyone commenting against it and it is something that I'm
> >> constantly hacking with `head -n-1`.
> >>
> >> I've attached a patch that implements a new `--no-total` option to wc.
> >> I believe this patch to be trivial enough that I can't claim copyright
> >> for anything.
> >>
> >> Best regards
> >> David
> >>
> >> [1] https://lists.gnu.org/archive/html/coreutils/2015-11/msg00064.html
> >
> > Hi
> >
> > It's been almost 3 months without any reply.  I hope it's OK to bump
> > it again.  I've just changed the subject line to make it clear there
> > is a patch attached.
>
> Thanks for the bump.
>
> This is one of those marginal ones since
> there is no extra functionality provided
> by bringing the logic within the utility.
>
> The following function would achieve the desired functionality:
>
>wc-no-total() { wc "$@" /dev/null | head -n-2; }
>
> Since it's easy enough to achieve with a single extra processing step,
> Given the above, I'd be 60:40 against adding a --no-total option.
> But thinking more, the above is awkward to combine with the --files0-from
> option.
> So you'd need a separate invocation in that case like:
>
>{ find files -print0; printf '%s\0' /dev/null; } |
>wc --files0-from=- |
>head -n2
>
> Even though there is still no extra functionality,
> the above is starting to get a bit obtuse.
>
> If we lifted the restriction with --files0-from
> to also allow file names to be specified on the command line
> (and for those to be processed after stdin),
> it would mean the wc-no-total() function above would be general,
> and would work for all wc invocations.
>
> Though a --no-total option is looking more appealing
> given the above considerations. I.e. that the
> wc-no-total() implementation isn't obvious,
> and we'd have to change wc anyway to make it general.
>
> So I'd be 55:45 for adding this option.
>

Good arguments. +1

>

Re: [PATCH] ls: support explicit --time=modification selection

[Jim Meyering]

On Fri, Aug 12, 2022 at 6:39 AM Pádraig Brady  wrote:
> The attached adds support for explicitly specifying --time=mtime.
> I'm not sure about adding this, but it does give symmetry to the
> description of the --time arguments.  Also it would allow
> overriding a previously selected timestamp selection in an alias etc.

Thanks! That looks like a fine addition. Glad you noticed.

FYI: two small diffs

[Jim Meyering]

I've just pushed these two small changes:

>From 7e6c39cc02f461c45ef51544a512a40791d6d603 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Jun 2022 08:07:18 -0700
Subject: [PATCH 1/2] maint: remove unnecessary inclusion of hash.h

* src/cut.c: Don't include hash.h. The implementation was
changed not to need that in v8.21-43-g3e466ad05.
---
 src/cut.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/cut.c b/src/cut.c
index 6fd8978a6..2a45eedff 100644
--- a/src/cut.c
+++ b/src/cut.c
@@ -33,7 +33,6 @@
 #include "error.h"
 #include "fadvise.h"
 #include "getndelim2.h"
-#include "hash.h"

 #include "set-fields.h"

-- 
2.35.1.677.gabf474a5dd


>From 6c03e8fbb25d52b5fda62a95090568f0ba5fca70 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Jun 2022 07:33:08 -0700
Subject: [PATCH 2/2] cp: avoid -Wmaybe-uninitialized warning from GCC13

* src/copy.c (infer_scantype): Always set scan_inference.ext_start,
to make the code match the comment.
---
 src/copy.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/copy.c b/src/copy.c
index edc822134..0c368d0e4 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1045,7 +1045,10 @@ infer_scantype (int fd, struct stat const *sb,
   if (! (HAVE_STRUCT_STAT_ST_BLOCKS
  && S_ISREG (sb->st_mode)
  && ST_NBLOCKS (*sb) < sb->st_size / ST_NBLOCKSIZE))
-return PLAIN_SCANTYPE;
+{
+  scan_inference->ext_start = -1;
+  return PLAIN_SCANTYPE;
+}

 #ifdef SEEK_HOLE
   scan_inference->ext_start = lseek (fd, 0, SEEK_DATA);
@@ -1053,6 +1056,8 @@ infer_scantype (int fd, struct stat const *sb,
 return LSEEK_SCANTYPE;
   else if (errno != EINVAL && !is_ENOTSUP (errno))
 return ERROR_SCANTYPE;
+#else
+  scan_inference->ext_start = -1;
 #endif

   return ZERO_SCANTYPE;
-- 
2.35.1.677.gabf474a5dd

Re: [PATCH] maint: update tests/init.sh from gnulib

[Jim Meyering]

On Wed, Dec 22, 2021 at 4:38 PM Bernhard Voelker
 wrote:
>
> On 12/22/21 18:15, Jim Meyering wrote:
> > Thanks. Please push.
>
> Thanks for the review, done:
> https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=1435e8e5c5
>
> > I like the idea of adding a syntax-check rule for that, too. Long overdue.
> > However, what do you think about making it work also for projects like sed
> > for which init.sh is in a differently-named subdir (there, it's 
> > "testsuite/")?
> > Then, we could consider moving it to maint.mk.
>
> 'gnulib/tests/init.sh' is just one file, and I think each package has a 
> different
> set of files physically copied from gnulib, e.g.:
>
> * coreutils:
>   + gnulib/doc/COPYINGv3   -> COPYING
>   + gnulib/build-aux/bootstrap -> bootstrap
>   + gnulib/tests/init.sh   -> tests/init.sh
>
> * findutils:
>   + gnulib/doc/COPYINGv3   -> COPYING
>   + gnulib/doc/fdl.texi-> doc/fdl.texi
>   + gnulib/build-aux/bootstrap -> bootstrap
>   + gnulib/tests/init.sh   -> tests/init.sh
>
> Hmm, that reminds me that 'fdl.texi' seems to be outdated in coreutils ... 
> which is
> also a physical copy since aefd434e56c6 (May 2020), but Paul updated the file 
> in gnulib
> in Aug 2020 (gnulib commit b5d9bcdf0348).
>
> Hence the list of files to copy for coreutils would be the same as that for 
> findutils.

Good point. That suggests that there should be a list of file names,
e.g. including fdl.texi.
Then the code would prune that list, removing files that are not VC'd
(and hence don't need to be updated).

> Another approach is to provide a make target 'update-gnulib-to-latest' via 
> gnulib's top/maint.mk
> which I proposed a while ago; each package could then specify in a make 
> target which files have
> to be copied.  See the 2 attachments in:
>   https://lists.gnu.org/r/coreutils/2018-12/msg7.html

Sorry that's been languishing for so long.

IMHO, we need both a syntax-check rule AND a turn-key "update if
anything's out of date" rule
that both copies any updated files into place and creates a proper
commit for them.

Re: [PATCH] maint: update tests/init.sh from gnulib

[Jim Meyering]

On Mon, Dec 20, 2021 at 2:31 PM Bernhard Voelker
 wrote:
> Patch attached.
>
> Additionally, it may be worth adding a syntax-check rule to ensure the files
> physically copied from gnulib stay in sync.
> WDYT?
>
> Have a nice day,
> Berny
>
> diff --git a/cfg.mk b/cfg.mk
> index 6d6c37dc2..f06223f2e 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -138,6 +138,14 @@ sc_ensure_gl_diffs_apply_cleanly:
>  'make refresh-gnulib-patches' >&2; exit 1; }   \
> done
>
> +# Ensure all physically copied files from gnulib are in sync
> +sc_ensure_copied_gnulib_files_are_in_sync:
> +   @diff -u $(srcdir)/gnulib/doc/COPYINGv3 $(srcdir)/COPYING \
> + && diff -u $(srcdir)/gnulib/build-aux/bootstrap $(srcdir)/bootstrap 
> \
> + && diff -u $(srcdir)/gnulib/tests/init.sh $(srcdir)/tests/init.sh \
> + || { echo '$(ME): please update copied files from gnulib' 1>&2; \
> +  exit 1; }\
> +
>  # Avoid :>file which doesn't propagate errors
>  sc_prohibit_colon_redirection:
> @cd $(srcdir)/tests && GIT_PAGER= git grep -En ': *>.*\|\|' \

Thanks. Please push.
I like the idea of adding a syntax-check rule for that, too. Long overdue.
However, what do you think about making it work also for projects like sed
for which init.sh is in a differently-named subdir (there, it's "testsuite/")?
Then, we could consider moving it to maint.mk.

Technically, we could run "git ls-files" and search for '/init\.sh$', but
simplest might be to just add a variable whose default value is "tests"
and may be overridden. If someone has two or more init.sh files (or none),
then searching via git ls-files may make more sense.

fix two syntax-check failures

[Jim Meyering]

A few syntax-check failures snuck in, so I've just fixed them with
these two just-pushed commits.


sc.diff
Description: Binary data

FYI: [PATCH] maint: factor.c: avoid new GCC 12 warning

[Jim Meyering]

I've just pushed this:

maint: factor.c: avoid new GCC 12 warning
* src/factor.c (millerrabin2): Mark as ATTRIBUTE_PURE,
per advice from GCC 12.
---
 src/factor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/factor.c b/src/factor.c
index caa97cbd2..7fc30717a 100644
--- a/src/factor.c
+++ b/src/factor.c
@@ -1137,7 +1137,7 @@ millerrabin (uintmax_t n, uintmax_t ni,
uintmax_t b, uintmax_t q,
   return false;
 }

-static bool
+ATTRIBUTE_PURE static bool
 millerrabin2 (const uintmax_t *np, uintmax_t ni, const uintmax_t *bp,
   const uintmax_t *qp, unsigned int k, const uintmax_t *one)
 {

Re: tests: env-s.pl: avoid spurious failure on OS X

[Jim Meyering]

On Mon, Sep 20, 2021 at 8:18 AM Pádraig Brady  wrote:
> On 20/09/2021 15:19, Jim Meyering wrote:
> > On Mon, Sep 20, 2021 at 3:14 AM Pádraig Brady  wrote:
> >> On 20/09/2021 06:46, Jim Meyering wrote:
> >>> I noticed/fixed a spurious test failure on OS X:
> >>
> >> Oh interesting.
> >> This seems like a more general issue.
> >> Should we do this more centrally by adding to the list in
> >> tests/envvar-check
> >
> > Adding that name to the list in tests/envvar-check would have no
> > effect, since this case is not about inheriting from the original
> > environment, but rather about something that's inserted upon each
> > "env" invocation.
>
> I don't fully understand where that env var is being set,
> but don't see a specific issue with the patch, so please push.

Done.

> BTW that test should not be dependent on the host env(1) since:
> https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=c8539d36c

Right. it's not.
I confess I did not root-cause, but it seems to be something deeper.
That envvar, __CF_USER_TEXT_ENCODING is referenced e.g., here
https://developer.apple.com/library/archive/technotes/tn2228/_index.html
and associated with a file, ~/.CFUserTextEncoding.

Re: cksum --version and --help suggestions

[Jim Meyering]

On Mon, Sep 20, 2021 at 6:02 AM Pádraig Brady  wrote:
> On 20/09/2021 11:03, Pádraig Brady wrote:
> > On 18/09/2021 17:34, Jim Meyering wrote:
> >> I didn't see an explanation for the single quotes in cksum --help, so
> >> propose to remove them as unnecessary syntax.
> >
> > Fair enough. We should probably also remove the quotes from usage() in tee.c
> >
> >> Also, IMHO, you should now be listed as a coauthor of the umbrella
> >> "cksum" program.
> >
> > +1
>
> That got me looking at usage(), which needed a few other improvements.
> I'll apply the attached on top.

Good catches.

Re: tests: env-s.pl: avoid spurious failure on OS X

[Jim Meyering]

On Mon, Sep 20, 2021 at 3:14 AM Pádraig Brady  wrote:
> On 20/09/2021 06:46, Jim Meyering wrote:
> > I noticed/fixed a spurious test failure on OS X:
>
> Oh interesting.
> This seems like a more general issue.
> Should we do this more centrally by adding to the list in
> tests/envvar-check

Adding that name to the list in tests/envvar-check would have no
effect, since this case is not about inheriting from the original
environment, but rather about something that's inserted upon each
"env" invocation.

tests: env-s.pl: avoid spurious failure on OS X

[Jim Meyering]

I noticed/fixed a spurious test failure on OS X:


0001-tests-env-s.pl-avoid-spurious-failure-on-OS-X.patch
Description: Binary data

cksum --version and --help suggestions

[Jim Meyering]

I didn't see an explanation for the single quotes in cksum --help, so
propose to remove them as unnecessary syntax.

Also, IMHO, you should now be listed as a coauthor of the umbrella
"cksum" program.


0002-cksum-list-P-draig-as-coauthor.patch
Description: Binary data


0001-cksum-drop-single-quotes-in-help.patch
Description: Binary data

Re: [PATCH] rmdir: fix uninitialized memory causing incorrect error

[Jim Meyering]

On Thu, Sep 16, 2021 at 3:35 PM Pádraig Brady  wrote:
> * src/rmdir.c (main): Only inspect the returned stat structure,
> when stat(2) returns success.
> ---
>  src/rmdir.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/rmdir.c b/src/rmdir.c
> index 149d4659a..c6e2aba0f 100644
> --- a/src/rmdir.c
> +++ b/src/rmdir.c
> @@ -262,7 +262,8 @@ main (int argc, char **argv)
>struct stat st;
>int ret = stat (dir, &st);
>/* Some other issue following, or is actually a directory. 
> */
> -  if ((ret != 0 && errno != ENOTDIR) || S_ISDIR (st.st_mode))
> +  if ((ret != 0 && errno != ENOTDIR)
> +  || (ret == 0 && S_ISDIR (st.st_mode)))

Good one!
Found via valgrind?

Re: [PATCH] build: avoid new chmod.c warnings from upcoming GCC12

[Jim Meyering]

On Thu, Sep 16, 2021 at 2:44 PM Pádraig Brady  wrote:
> On 16/09/2021 21:30, Jim Meyering wrote:
> > I tried to build with upcoming GCC12 and with warnings + -Werror, and
> > hit one failure.
> > Here's a patch to avoid that:
>
> Cool. Small nit; it's probably best to use a trailing ,
> for our standard from of aggregate initialization. I.e.,
>
> -  struct change_status ch;
> +  struct change_status ch = { 0, };

Good catch, indeed. Thank you. Pushed with that correction.

[PATCH] build: avoid new chmod.c warnings from upcoming GCC12

[Jim Meyering]

I tried to build with upcoming GCC12 and with warnings + -Werror, and
hit one failure.
Here's a patch to avoid that:


0001-build-avoid-new-chmod.c-warnings-from-upcoming-GCC12.patch
Description: Binary data

Re: [PATCH V2] Add support for cksum --algorithm [sm3]

[Jim Meyering]

On Tue, Sep 14, 2021 at 5:36 AM Pádraig Brady  wrote:
> On 12/09/2021 23:01, Pádraig Brady wrote:
> > On 12/09/2021 19:13, Pádraig Brady wrote:
> >> This patch set refactors all digest implementations
> >> to their own modules, all interfaced through digest.c.
> >> All file operations and diagnostics are done in digest.c.
> >> All digests are made available through `cksum -a`.
> >> Also we add support for SM3 through `cksum -a sm3` only.
> >>
> >> V2 changes:
> >>
> >> - Various small fixes to previous patch set.
> >> - Simplify b2sum specific code.
> >> - Add support for `cksum -c` to infer the algorithm from tagged checksums.
> >
> > This is pretty much ready to land now I think.
> > I hope to land it tomorrow.
> >
> > There is a question though re default format to use for cksum -a.
> > I.e. should we use --tag format by default for cksum -a, as that
> > is now directly consumable by cksum -c.
> > If we did that though, we'd have to have the opposite option
> > for cksum, so something like `cksum --untagged` to produce
> > the traditional coreutils output format of "$hash  $file".
> > I'm undecided.
>
> I thought about it a bit more, I'm going to change the default format
> for cksum to tagged, and add an --untagged option. Reasons:
>
> - It's a now or never change, since cksum -a is new it doesn't have backward 
> compat issues.
> - It's simpler as don't need to specify --tag on generation or -a on checking 
> invocations.
> - It's a more general format supporting mixed and length adjusted digests.
>
> Now the tagged format doesn't support encoding --binary or --text mode,
> but that got me investigating whether cksum should support these at all.
> My conclusion is that it should not, and just use binary everywhere.
> I.e. cksum should not support --binary or --text. Reasons:
>
>   - cygwin is the main consideration here, and it seems to be defaulting to 
> binary these days
> - i.e. text/binary is a confusing distraction for the vast majority of 
> cases
>   - The cygwin model seems to be being subsumed by the WSL model anyway
>   - Shared checksum files for text files stored in system native format seems 
> quite edge case
> - Even then, one can always convert to system native after verification
>   - The functionality is retained in the standalone utils if needed
>
> Interestingly one place where we might care about processing in text mode,
> is for the checksum files themselves, but we don't actually handle that at 
> all :/
> Looking back I see many users having issues with \r chars messing up --check.
> Eric Blake had a good suggestion to encode \r in file names and then ignore
> real \r chars in checksum files. I'll implement that now while were working 
> on this.

Good ideas, all! Thanks for working though all that and making the leap.

Re: [PATCH 7/8] cksum: add --algorithm option to select digest mode

[Jim Meyering]

Re this:

+#if HASH_ALGO_CKSUM
+  /* TODO: Remove these restrictions.  */
+  switch (cksum_algorithm)
+{
+case bsd:
+case sysv:
+case crc:
+if (delim != '\n')
+  die (EXIT_FAILURE, 0,
+  _("--zero is not supported with --algorithm={bsd,sysv,crc}"));
+if (prefix_tag)
+  die (EXIT_FAILURE, 0,
+  _("--tag is not supported with --algorithm={bsd,sysv,crc}"));
+if (do_check)
+  die (EXIT_FAILURE, 0,
+  _("--check is not supported with --algorithm={bsd,sysv,crc}"));
+break;
+default:
+break;
+}

I like the new table-driven code. Thanks!
Depending on when you want to address that TODO, you may want to
diagnose multiple errors before exiting, so as to diagnose two or
three errors when using more than one of these: --zero --tag --check

Re: [PATCH 7/8] cksum: add --algorithm option to select digest mode

[Jim Meyering]

On Tue, Sep 7, 2021 at 10:39 PM Pádraig Brady  wrote:
> I need to add some NEWS for this one:
>
> diff --git a/NEWS b/NEWS
> index d9ab04529..7c245f1a6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -88,6 +88,11 @@ GNU coreutils NEWS-*- 
> outline -*-
>
>   ** New Features
>
> +  cksum now supports the -a (--algorithm) option to select any
> +  of the exiting sum, md5sum, b2sum, sha*sum implementations etc.
> +  cksum now has the superset functionality of all these utilities,
> +  and there will be no future standalone checksumming utilities introduced.
> +
> expr and factor now support bignums on all platforms.
>
> ls --classify now supports the "always", "auto", or "never" flags,

Nit: s/exiting/existing/

Also, maybe shorten and reword the final sentence to be active-voice?
> cksum now subsumes all of these programs,
> and coreutils will introduce no future standalone checksum utility.

Re: Add support for cksum --algorithm [sm3]

[Jim Meyering]

On Tue, Sep 7, 2021 at 5:47 PM Pádraig Brady  wrote:
> This patch set refactors all digest implementations
> to their own modules, all interfaced through digest.c.
> All file operations and diagnostics are done in digest.c.
> All digests are made available through `cksum -a`.
> Also we add support for SM3 through `cksum -a sm3` only.

Nice work.
I've begun reading through these.
So far it all looks impeccable.

Re: [PATCH] sum: always output a file name if one passed

[Jim Meyering]

On Mon, Aug 30, 2021 at 11:23 AM Pádraig Brady  wrote:
> I'm working on consolidating digest algorithms
> (interfaced through cksum --algorithm),
> and I noticed this inconsistency in sum(1),
> which seems appropriate to address before consolidation
> (see attached).

Looks fine to me. Thanks!

FYI: maint: bootstrap: remove reference to unused hash-pjw module

[Jim Meyering]

I've just pushed this:

maint: bootstrap: remove reference to unused hash-pjw module
* bootstrap.conf (gnulib_modules): Remove hash-pjw. No longer used.

diff --git a/bootstrap.conf b/bootstrap.conf
index f55da99db..daea2824c 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -128,7 +128,6 @@ gnulib_modules="
   group-member
   hard-locale
   hash
-  hash-pjw
   hash-triple
   heap
   host-os

Re: [PATCH] copy: exit immediately upon failure to allocate hash memory

[Jim Meyering]

On Mon, May 3, 2021 at 10:59 AM Pádraig Brady  wrote:
> I noticed this issue while looking at allocation of hash structures for:
> https://bugs.gnu.org/48189
>
> Addressed with the attached.

Good catch! Thanks. That is an old bug.
Looks like I introduced it here:
TEXTUTILS-2_0_15-40-g9d0fbdaf3
https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=9d0fbdaf3

Re: removing fiemap logic from copy.c

[Jim Meyering]

On Sun, May 2, 2021 at 2:50 PM Pádraig Brady  wrote:
> FYI tomorrow I'm going to push a change to
> remove the fiemap extent handling logic from cp/mv/install etc.
> (Assuming that ok with folks of course).
>
> This is only used on 10 year old linux kernels,
> (where SEEK_HOLE is not available).
>
> Also with fiemap we perform an fsync before each copy,
> which is best avoided.
>
> The patch is largish, but boring as just removes lines,
> so not attaching here.

That code brings back not-so-fond memories.
Good riddance :-)

Re: [PATCH] ls: add --sort=width (-W) option to sort by filename width

[Jim Meyering]

On Sun, Apr 11, 2021 at 12:48 PM Jim Meyering  wrote:
>
> On Sun, Apr 11, 2021 at 10:38 AM Pádraig Brady  wrote:
> ...
> > The changes didn't introduce any new instances
> > of that const declaration style (apart from the new forward declaration),
> > but you're right we should be consistent here.
> > This seems like the perfect thing for a syntax check,
> > so I added a simple one as follows:
> >
> > +# Prefer the const declaration form, with const following the type
> > +sc_prohibit-const-char:
> > +   @prohibit='const char \*'   \
> > +   in_vc_files='\.[ch]$$'  \
> > +   halt='Use char const *, not const char *'   \
> > + $(_sc_search_regexp)
> >
> > I adjusted all files to pass and pushed.
>
> Great. Thank you!

Incidentally, running on Fedora 34 built with gcc-11(upcoming,
probably irrelevant), and with an update-to-latest-gnulib patch, this
ls test fails: tests/ls/stat-free-color
Here's the log:


stat-free-color.log
Description: Binary data

Re: [PATCH] ls: add --sort=width (-W) option to sort by filename width

[Jim Meyering]

On Sun, Apr 11, 2021 at 10:38 AM Pádraig Brady  wrote:
...
> The changes didn't introduce any new instances
> of that const declaration style (apart from the new forward declaration),
> but you're right we should be consistent here.
> This seems like the perfect thing for a syntax check,
> so I added a simple one as follows:
>
> +# Prefer the const declaration form, with const following the type
> +sc_prohibit-const-char:
> +   @prohibit='const char \*'   \
> +   in_vc_files='\.[ch]$$'  \
> +   halt='Use char const *, not const char *'   \
> + $(_sc_search_regexp)
>
> I adjusted all files to pass and pushed.

Great. Thank you!

Re: [PATCH] ls: add --sort=width (-W) option to sort by filename width

[Jim Meyering]

On Sun, Apr 11, 2021 at 9:10 AM Jim Meyering  wrote:
>
> On Sat, Apr 10, 2021 at 8:22 PM Pádraig Brady  wrote:
> > On 09/04/2021 23:51, Pádraig Brady wrote:
> > > On 09/04/2021 13:02, Carl Edquist wrote:
> > >> Dear Coreutils Maintainers,
> > >>
> > >> I'd like to introduce my favorite 'ls' option, '-W', which I have been
> > >> enjoying using regularly over the last few years.
> > >>
> > >> The concept is just to sort filenames by their printed widths.
> > >>
> > >>
> > >> (If this sounds odd, I invite you hear it out, try and see for yourself!)
> > >>
> > >>
> > >> I am including a patch with my implementation and accompanying tests - as
> > >> well as some sample output.  And I'll happily field any requests for
> > >> improvements.
> > >
> > > I quite like this. It seems useful.
> > > Also doing outside of ls is quite awkward,
> > > especially considering multi column output.
> > >
> > > I would avoid the -W short option, as that would clash with ls from 
> > > FreeBSD for example.
> > > It's probably best to not provide a short option for this at all.
> >
> > Playing around with this a bit more,
> > I really like how much more concise it makes output in general.
> >
> > I've attached two patches which I'll apply tomorrow at some stage.
> >
> > The first adjusts your patch to:
> >Remove -W short option.
> >Fix crash on systems with statx().
> >s/filename/file name/.
> >Expand in texinfo that --sort=width is most useful with -C (the default)
> >
> > The second is a performance improvement,
> > as we're now calling quote_name_width a lot more.
>
> Nice. I like this new option, too! Who would have thought :-)
>
> Two minor stylistic suggestions:
>
> move this declaration of "i" down into the loop where it's used, to
> save two lines:
>
> +static void
> +update_current_files_cache (void)
> +{
> +  size_t i;
> +
> +  for (i = 0; i < cwd_n_used; i++)
>
> And please use "char const *" (not const char *) in the added code of
> ls.c. Admittedly this is a really small nit, especially since the
> existing code in that file is not yet self-consistent. But at least
> the preferred spelling outnumbers the others by about 3 to 1.

Oops. I see that you've pushed already.
I'll adjust if you don't beat me to it again. Thanks!

Re: [PATCH] ls: add --sort=width (-W) option to sort by filename width

[Jim Meyering]

On Sat, Apr 10, 2021 at 8:22 PM Pádraig Brady  wrote:
> On 09/04/2021 23:51, Pádraig Brady wrote:
> > On 09/04/2021 13:02, Carl Edquist wrote:
> >> Dear Coreutils Maintainers,
> >>
> >> I'd like to introduce my favorite 'ls' option, '-W', which I have been
> >> enjoying using regularly over the last few years.
> >>
> >> The concept is just to sort filenames by their printed widths.
> >>
> >>
> >> (If this sounds odd, I invite you hear it out, try and see for yourself!)
> >>
> >>
> >> I am including a patch with my implementation and accompanying tests - as
> >> well as some sample output.  And I'll happily field any requests for
> >> improvements.
> >
> > I quite like this. It seems useful.
> > Also doing outside of ls is quite awkward,
> > especially considering multi column output.
> >
> > I would avoid the -W short option, as that would clash with ls from FreeBSD 
> > for example.
> > It's probably best to not provide a short option for this at all.
>
> Playing around with this a bit more,
> I really like how much more concise it makes output in general.
>
> I've attached two patches which I'll apply tomorrow at some stage.
>
> The first adjusts your patch to:
>Remove -W short option.
>Fix crash on systems with statx().
>s/filename/file name/.
>Expand in texinfo that --sort=width is most useful with -C (the default)
>
> The second is a performance improvement,
> as we're now calling quote_name_width a lot more.

Nice. I like this new option, too! Who would have thought :-)

Two minor stylistic suggestions:

move this declaration of "i" down into the loop where it's used, to
save two lines:

+static void
+update_current_files_cache (void)
+{
+  size_t i;
+
+  for (i = 0; i < cwd_n_used; i++)

And please use "char const *" (not const char *) in the added code of
ls.c. Admittedly this is a really small nit, especially since the
existing code in that file is not yet self-consistent. But at least
the preferred spelling outnumbers the others by about 3 to 1.

Re: Is the 'diacrit' module still needed?

[Jim Meyering]

On Mon, Mar 22, 2021 at 4:20 PM Bernhard Voelker
 wrote:
>
> On 3/21/21 10:24 PM, Paul Eggert wrote:
> > Today I updated Coreutils to current Gnulib, [...].
>
> Thanks for getting rid of the diacrit module.
>
> The attached also updates the bootstrap script.
>
> BTW: what about a syntax-check rule to ensure we don't forget
> to sync files physically copied into 'coreutils.git',
> something (not much tested) like the following?
>
> Have a nice day,
> Berny
>
>
> diff --git a/cfg.mk b/cfg.mk
> index d65bda2fd..0d103f66b 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -797,6 +797,15 @@ sc_gitignore_missing:
>   sort | uniq -u | grep . && { echo '$(ME): Add above'  \
> 'entries to .gitignore' >&2; exit 1; } || :
>
> +# Ensure that physical copies of gnulib files are in sync.
> +sc_gnulib_copies_compare:
> +   @cd $(srcdir) \
> + && diff COPYING  gnulib/doc/COPYINGv3 \
> + && diff bootstrap gnulib/build-aux/bootstrap \
> + && diff tests/init.sh gnulib/tests/init.sh \

Hi Berny,
I like the idea.
However, to impose it on everyone via a new sc_ rule, it'd have to work for
arbitrarily-named gnulib (easy, use maint.mk's $(gnulib_dir)) and
"tests" directories.
Handling "tests" is harder: sed has "testsuite", and automake has "t",
but with a
loop over a few hard-coded names like those, you could find a good
candidate for this, too.
In the search for a test directory, I'd default to a user-overridable
(either "tests" or deliberately empty by default)
dir name and if that's not specified, set it via the search through
that hard-coded list of likely names.

For extra credit (to be run manually when out of sync), add a companion rule
that will perform the update and git-commit the result with a proper log entry.

Please use diff -u, not just "diff".

Re: [PATCH] cksum: Use pclmul hardware instruction for CRC32 calculation

[Jim Meyering]

On Sat, Mar 13, 2021 at 10:19 AM Pádraig Brady  wrote:
> On 13/03/2021 16:13, Pádraig Brady wrote:
>
> FYI testing on an older i3-2310M system
> shows the bottleneck is not near I/O (cat is much faster).
> A 500MiB file improves from 1.40s to 0.67s on the i3-2310M.
>
> $ time src/cksum file.in
>3404199294 524288000 file.in
>real 0m0.672s
>user 0m0.584s
>sys  0m0.084s
>
> I'm also considering applying the attached
> to add a --debug option (present on a few other coreutils),
> which will diagnose the implementation used
> (since it's build time and run time variable).

I like the new option, and the patch looks fine.
I assume you'll mention the addition in NEWS.

Re: Bug#982208: coreutils: cat -E, --show-ends display strangely when CR (\ r) is included.

[Jim Meyering]

On Tue, Feb 9, 2021 at 3:01 PM Pádraig Brady  wrote:
...
> I agree the current behavior is less than useful because:
>
>* \r\n is common a line end combination
>* catting such a file without options causes it to display normally
>* overwriting the first char with $, loses info
>
> I propose to change the upstream coreutils project
> as per the attached, to extend the --show-ends option
> to show \r as ^M when the following character is \n.

+1 patch looks fine. Thanks!

Re: Another appeal for `uniq --stream`

[Jim Meyering]

On Fri, Jan 22, 2021 at 10:37 AM Tony Fischetti
 wrote:
>
> For a while, I've been using a small program I wrote (with help from a
> GPL AVL-library) to filter unsorted duplicate lines. I thought I might
> see if this can be added to `uniq` (or some other way) but I saw that
> a nearly identical proposal
> (https://lists.gnu.org/archive/html/coreutils/2011-11/msg00016.html)
> was already put forth and rejected.
>
> I thought it might be worth it to make the case again, with an expanded
> rationale, and especially as I already have a proof of concept (available
> below) and I'm willing to write the code, documentation, translation,
> etc...
>
> It was said in the replies to the original proposal that it's up to
> the user to decide whether they want to run `sort` and then pipe it
> to `uniq`. But in all the years I've used coreutils, I've never once
> used `uniq` without `sort`. I've spoken to many others, and their
> experience comports with mine.
>
> But this was not because I wanted the output to be sorted; in fact,
> I specifically didn't. Most times, I want (and even require that) the
> duplicated lines be stripped as soon as the data becomes available,
> and remain in the original order. This is especially useful for log
> files, journals, output from statistical software, etc...

I'm sure it's been mentioned in previous threads, but didn't see an
alternate-language implementation listed in the 2011 coreutils thread
you linked above, so here's one:

  perl -ne '$seen{$_}++ or print'

You can also wrap a function around that, e.g.,

  uniq_stream() { perl -ne '$seen{$_}++ or print' "$@"; }

Re: [PATCH] ls: fix crash printing SELinux context for unstatable files

[Jim Meyering]

On Wed, Nov 11, 2020 at 9:34 AM Pádraig Brady  wrote:
> This crash was identified by Cyber Independent Testing Lab:
> https://cyber-itl.org/2020/10/28/citl-7000-defects.html
> and was introduced with commit v6.9.90-11-g4245876e2
>
> * src/ls.c (gobble_file): Ensure scontext is initialized
> in the case where files are not statable.
> * tests/ls/selinux-segfault.sh: Renamed from proc-selinux-segfault.sh,
> and added test case for broken symlinks.
> * tests/local.mk: Adjust for the renamed test.
> * NEWS: Mention the bug fix.

Good one. Looks fine. Thanks!

Re: [PATCH] timeout: support sub-second timeouts on macOS

[Jim Meyering]

On Sun, Nov 8, 2020 at 1:26 PM Pádraig Brady  wrote:
> On 08/11/2020 18:52, Jim Meyering wrote:
> > On Sat, Nov 7, 2020 at 2:08 PM Pádraig Brady  wrote:
> >> * m4/jm-macros.m4: Check for setitimer.
> >> * src/timeout.c: Use setitimer if timer_settime is not available.
> >> * NEWS: Mention the improvement.
> >
> > Thanks!  The patch looks fine and I confirmed it works.
> > I encountered a few hurdles to make everything build using Xcode-12.1:
> > - I had brew's bison-2.3, but that didn't work (failed to create
> > lib/parse-datetime.c). fixed by installing bison-3.7.3
> > - compile error due to gnulib's lib/mgetgroups.c. I worked around it
> > by adding a gross cast. I doubt that's proper, but I haven't dug
> > enough to say more.
> >
> > diff --git a/lib/mgetgroups.c b/lib/mgetgroups.c
> > index 3377d7bb2..a27da05e5 100644
> > --- a/lib/mgetgroups.c
> > +++ b/lib/mgetgroups.c
> > @@ -93,7 +93,7 @@ mgetgroups (char const *username, gid_t gid, gid_t 
> > **groups)
> > int last_n_groups = max_n_groups;
> >
> > /* getgrouplist updates max_n_groups to num required.  */
> > -  ng = getgrouplist (username, gid, g, &max_n_groups);
> > +  ng = getgrouplist (username, gid, (int *) g, &max_n_groups);
> >
> > /* Some systems (like Darwin) have a bug where they
> >never increase max_n_groups.  */
> >
> > - finally, I normally configure with --enable-gcc-warnings, but that
> > evokes the attached errors, so I got past that by building with
> > WERROR_CFLAGS=
> >
>
> Thanks for all the checking.
> I just used a dist tarball to check.
>
> Re getgrouplist, that was looked extensively at in:
> https://bugs.gnu.org/20923
> Is the __GNUC__ define used there different for your case?

Yes, this version of xcode declares itself as GCC 4.2.1:
[Two other solutions: use GCC or do not to use --enable-gcc-warnings]

$ :|gcc -E -dD -|grep GNUC
#define __GNUC__ 4
#define __GNUC_MINOR__ 2
#define __GNUC_PATCHLEVEL__ 1
#define __GNUC_STDC_INLINE__ 1

Re: [PATCH] timeout: support sub-second timeouts on macOS

[Jim Meyering]

On Sat, Nov 7, 2020 at 2:08 PM Pádraig Brady  wrote:
> * m4/jm-macros.m4: Check for setitimer.
> * src/timeout.c: Use setitimer if timer_settime is not available.
> * NEWS: Mention the improvement.

Thanks!  The patch looks fine and I confirmed it works.
I encountered a few hurdles to make everything build using Xcode-12.1:
- I had brew's bison-2.3, but that didn't work (failed to create
lib/parse-datetime.c). fixed by installing bison-3.7.3
- compile error due to gnulib's lib/mgetgroups.c. I worked around it
by adding a gross cast. I doubt that's proper, but I haven't dug
enough to say more.

diff --git a/lib/mgetgroups.c b/lib/mgetgroups.c
index 3377d7bb2..a27da05e5 100644
--- a/lib/mgetgroups.c
+++ b/lib/mgetgroups.c
@@ -93,7 +93,7 @@ mgetgroups (char const *username, gid_t gid, gid_t **groups)
   int last_n_groups = max_n_groups;

   /* getgrouplist updates max_n_groups to num required.  */
-  ng = getgrouplist (username, gid, g, &max_n_groups);
+  ng = getgrouplist (username, gid, (int *) g, &max_n_groups);

   /* Some systems (like Darwin) have a bug where they
  never increase max_n_groups.  */

- finally, I normally configure with --enable-gcc-warnings, but that
evokes the attached errors, so I got past that by building with
WERROR_CFLAGS=
  GEN  lib/alloca.h
  GEN  lib/byteswap.h
  GEN  lib/configmake.h
  GEN  lib/ctype.h
  GEN  lib/dirent.h
  GEN  lib/arpa/inet.h
  GEN  lib/fcntl.h
  GEN  lib/fnmatch.h
echo '#include "mini-gmp.h"' >lib/gmp.h-t
  GEN  lib/getopt.h
  GEN  lib/getopt-cdefs.h
  GEN  lib/iconv.h
  GEN  lib/inttypes.h
  GEN  lib/langinfo.h
mv lib/gmp.h-t lib/gmp.h
  GEN  lib/limits.h
  GEN  lib/locale.h
  GEN  lib/netdb.h
  GEN  lib/math.h
  GEN  lib/pthread.h
  GEN  lib/sched.h
  GEN  lib/selinux/selinux.h
  GEN  lib/signal.h
  GEN  lib/stdint.h
  GEN  lib/selinux/context.h
  GEN  lib/stdio.h
  GEN  lib/stdlib.h
  GEN  lib/string.h
  GEN  lib/sys/ioctl.h
  GEN  lib/sys/random.h
  GEN  lib/sys/resource.h
  GEN  lib/sys/select.h
  GEN  lib/sys/socket.h
  GEN  lib/sys/stat.h
  GEN  lib/sys/time.h
  GEN  lib/sys/types.h
  GEN  lib/sys/uio.h
  GEN  lib/sys/utsname.h
  GEN  lib/termios.h
  GEN  lib/sys/wait.h
  GEN  lib/time.h
  GEN  lib/unistd.h
  GEN  lib/unistr.h
  GEN  lib/unitypes.h
  GEN  lib/uniwidth.h
  GEN  lib/utime.h
  GEN  lib/wchar.h
  GEN  lib/wctype.h
make[1]: Entering directory '/Users/meyering/w/co/cu'
  YACC generate-parse-datetime
make[1]: Leaving directory '/Users/meyering/w/co/cu'
make  all-recursive
make[1]: Entering directory '/Users/meyering/w/co/cu'
Making all in po
make[2]: Entering directory '/Users/meyering/w/co/cu/po'
make[2]: Nothing to be done for 'all'.
make[2]: Leaving directory '/Users/meyering/w/co/cu/po'
Making all in .
make[2]: Entering directory '/Users/meyering/w/co/cu'
  GEN  doc/constants.texi
  CC   src/ls-dir.o
  CC   src/dircolors.o
  CC   src/dirname.o
  CC   src/echo.o
  CC   src/du.o
  CC   src/env.o
  CC   src/expand.o
  CC   src/expand-common.o
  CC   src/expr.o
  CC   src/factor.o
  CC   src/false.o
  CC   src/fmt.o
  CC   src/fold.o
  CC   src/ginstall-install.o
  CC   src/ginstall-prog-fprintf.o
  CC   src/ginstall-copy.o
  CC   src/ginstall-cp-hash.o
  CC   src/ginstall-extent-scan.o
  CC   src/ginstall-selinux.o
  CC   src/ginstall-force-link.o
  CC   src/groups.o
  CC   src/group-list.o
  CC   src/head.o
  CC   src/id.o
  CC   src/join.o
  CC   src/kill.o
  CC   src/link.o
  CC   src/ln.o
  CC   src/relpath.o
  CC   src/logname.o
  CC   src/ls-ls.o
  CC   src/md5sum-md5sum.o
  CC   src/mkdir.o
  CC   src/prog-fprintf.o
  CC   src/mkfifo.o
  CC   src/mknod.o
  CC   src/mktemp.o
  CC   src/mv.o
  CC   src/remove.o
  CC   src/nl.o
  CC   src/nproc.o
  CC   src/nohup.o
  CC   src/numfmt.o
  CC   src/od.o
  CC   src/paste.o
  CC   src/pathchk.o
  CC   src/pr.o
  CC   src/printenv.o
  CC   src/printf.o
  CC   src/ptx.o
src/numfmt.c:396:26: error: implicit conversion increases floating-point 
precision: 'double' to 'long double' [-Werror,-Wdouble-promotion]
  return val < 0 ? val - 0.5 : val + 0.5;
   ~ ^~~
src/numfmt.c:396:38: error: implicit conversion increases floating-point 
precision: 'double' to 'long double' [-Werror,-Wdouble-promotion]
  return val < 0 ? val - 0.5 : val + 0.5;
   ~ ^~~
  CC   src/pwd.o
src/numfmt.c:787:22: error: implicit conversion increases floating-point 
precision: 'double' to 'long double' [-Werror,-Wdouble-promotion]
  if (absld (val) >= scale_base)
  ~~ ^~
src/numfmt.c:789:14: error: implicit conv

Re: [PATCH] maint: cleanup operation of fs-magic-compare

[Jim Meyering]

On Tue, Oct 27, 2020 at 1:12 PM Pádraig Brady  wrote:
> * src/local.mk: Ensure we map 2 hext digits to 4,
> so that we don't output already handled Z3FOLD file system (0x33).
> Also hide the generation command for src/fs.h.
> ---
>  src/local.mk | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Diff looks fine. Nit in the log entry: s/hext/hex/

FYI, avoid new GCC11 warning

[Jim Meyering]

FYI, I've just pushed this:

maint: avoid new sort.c warning from upcoming GCC11

gcc version 11.0.0 20201025 (experimental) warns that
src/sort.c:1655:1: warning: function might be candidate for attribute \
  'pure' if it is known to return normally [-Wsuggest-attribute=pure]
* src/sort.c (limfield): Mark as pure.

diff --git a/src/sort.c b/src/sort.c
index 0163ba18a..8671ea767 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -1651,7 +1651,7 @@ begfield (struct line const *line, struct
keyfield const *key)
 /* Return the limit of (a pointer to the first character after) the field
in LINE specified by KEY. */

-static char *
+static char * _GL_ATTRIBUTE_PURE
 limfield (struct line const *line, struct keyfield const *key)
 {
   char *ptr = line->text, *lim = ptr + line->length - 1;

Re: Safety option "--preserve-root" in chown/chgrp

[Jim Meyering]

While POSIX (now) actually specifies the desired behavior for rm, it
does not (yet) for chown or chgrp.

IMHO, it would be a welcome change to make chown and chgrp reject an
attempt to operate recursively on a root file system, even though
POSIX has not yet required that behavior. In a way, adding this
protection to chown and chgrp feels even more important than adding it
for rm: these tools typically process files at a significantly higher
rate than rm, so can inflict more damage in the time it might take an
interactive user to realize the error and hit ^C.

On Thu, Jun 18, 2020 at 3:14 PM Harald Koch  wrote:
>
> Hello,
>
> first, thank you to all of you making it possible to bring Unix to everyone 
> of us!
> As a technical supporter, we see situations each day where we ask ourself how 
> this could happen. In the last seven days, we had to support our customers 
> who made big mistakes, and two times it was a very big effort to revert the 
> backups and make the system functional. These two situations both occured 
> (independently from each other) by changing permissions due to misconfigured 
> NFS and CIFS shares. The remote administrator tried to solve it by a simple 
> „chown“ or „chgrp“ recursively, which is wrong to solve the situation, but 
> that’s another point. The problem is, that they made a "chown -R www-data /" 
> - ok, bad idea afterwards.
> My colleague (here in CC) tries to find out how this could easily enhanced, 
> and found in the man pages the section:
>
>--no-preserve-root
>   do not treat '/' specially (the default)
>
>--preserve-root
>   fail to operate recursively on ‚/'
>
>
> So, there is an option to disallow this behavior. Would have been this set in 
> the call of chown, we would have saved much time (and customer’s money, which 
> flows into our pockets). The question is: if there is such a safety option, 
> why is it reverted to „by default unsafe“? In my understanding, it would be 
> better to have „--preserve-root“ be the default and to allow operation on „/" 
> only by option. I know this would have a big impact on existing scripts, but 
> I feel a bit disappointed by the administrator-friendlyness of these options. 
> It’s like having an airbag in a car, but you must enable it in exactly the 
> situation of an accident.
> How do you feel about this?
>
>
> Freundliche Grüße/Best regards,
>
> Harald Koch
>
> c-works GmbH
> Otto-Lilienthal-Str. 36
> 71034 Böblingen
>
> E-Mail: h.k...@c-works.de 
> Tel.:  +49-(0)7031-714-9440
> Fax: +49-(0)7031-714-9442
>
> Geschäftsführer/Managing Director: Harald Koch
> Sitz und Registergericht/Domicile and Court of Registry: Stuttgart
> HRB-Nr./ Commercial Register No. 725882
>
> —
> Due to corona we moved to remote office, leading to possible telephone 
> quality degradation.
>
>

Re: [PATCH 1/2] echo: pacify Oracle Studio 12.6

[Jim Meyering]

On Mon, Jun 1, 2020 at 4:45 PM Bob Proulx  wrote:
> Paul Eggert wrote:
> > * src/echo.c (main): Don’t assign pointer to bool.
> > This is well-defined in C99, but is arguably bad style
> > and Oracle Studio 12.6 complains.
> ...
> > +  bool posixly_correct = !!getenv ("POSIXLY_CORRECT");
>
> Of course this is fine.  But because char *getenv() returns a pointer
> it just has this feeling of expressing frustration with the compiler
> seeing the !! there.  It feels like an obvious cast silencing a
> compiler warning.  Perhaps that is the intent? :-)
>
>   RETURN VALUE
>The getenv() function returns a pointer to the value in the
>environment, or NULL if there is no match.
>
> Just as a soft comment that isn't very strong if it were me I would
> use a comparison against a pointer which produced a boolean result and
> that boolean result used to assign to a bool.  It just feels to me
> like more of the types are more obvious this way.  And no cast of any
> type is either implicit or explicit.
>
>   bool posixly_correct = getenv ("POSIXLY_CORRECT") != NULL;

Hi Bob,

I had the same aversion to "!!", ... up until maybe a decade ago when
I realized we could view "!!" as a pointer-to-bool "operator".
It's an idiom that you have to use a few times before it starts to
become natural, then you won't want to go back.

Jim

Re: Does -s apply to -m in sort?

[Jim Meyering]

On Mon, May 11, 2020 at 5:43 PM Peng Yu  wrote:
> Are you the author of -m? If not, maybe the author of -m should knows how
> it works with -s? If not, maybe this should be documented anyway?

Peng Yu,
Given the context of Eric's patiently instructive reply[1], your own
reply here comes across as exceptionally rude. If I were otherwise
inclined to try to help you, just seeing your manner exhibited here
would convince me not to even try.

[1] https://lists.gnu.org/archive/html/coreutils/2020-05/msg7.html

Re: failing CI jobs

[Jim Meyering]

On Wed, Mar 18, 2020 at 11:03 AM Kaz Kylheku (Coreutils)
<962-396-1...@kylheku.com> wrote:
>
> On 2020-03-18 04:27, "Toni Uhlig (Smartphone)" via GNU coreutils General
> Discussion wrote:
> > There are a lot of failing CI jobs and nobody seems to care about.
> > Some of them seem to fail since two+ years ago.
> > Why not disable them, if nobody cares about?
> >
> > Source:
> > https://hydra.nixos.org/job/gnu/coreutils-master
>
> Firstly, this specific page is not found; 404 error.
>
> Secondly, more generally, surely NixOS is not hosting CI for GNU
> Coreutils development?

They provide a useful service. Here's a working link:
https://hydra.nixos.org/jobset/gnu/coreutils-master#tabs-jobs

Re: RFC: du reports a 1.2PB file on a 1TB btrfs disk

[Jim Meyering]

On Tue, Mar 10, 2020 at 12:24 PM Kaz Kylheku (Coreutils)
<962-396-1...@kylheku.com> wrote:
> On 2020-03-10 11:52, Jim Meyering wrote:
> > Otherwise, du provides no way of seeing how much of the actual disk
> > space is being used by such FS-compressed files.
>
> If you stat the file, what are the values of st_size, st_blksize and
> st_blocks?

That particular file is long gone, but I've just created a 1.8T file
on a 700G file system.
Before I began this experiment, "Avail" was 524G, so it appears to
occupy about 60G actual space.
FTR, I created the file by running this: yes $(printf '%065535d\n' 0) > big

$ stat big
  File: big
  Size: 1957123607586   Blocks: 3822507048 IO Block: 4096   regular file
Device: 1bh/27d Inode: 788544108   Links: 1
Access: (0644/-rw-r--r--)  Uid: ( 6405/meyering)   Gid: (  100/   users)
Access: 2020-03-10 21:01:17.710555709 -0700
Modify: 2020-03-10 21:26:05.137122240 -0700
Change: 2020-03-10 21:26:05.137122240 -0700
 Birth: -
$ du -sh big; df -hT .
1.8Tbig
Filesystem Type   Size  Used Avail Use% Mounted on
/dev/vda3  btrfs  698G  221G  464G  33% /home/meyering

RFC: du reports a 1.2PB file on a 1TB btrfs disk

[Jim Meyering]

[note: this was not a sparse file and I wasn't using --apparent-size]

On a 1TB local btrfs file system, I found a log file for which du
reported a size of 1.2 petabytes. Quite surprising for a moment, then
I remembered btrfs's "compression".

Should we teach du to report compressed size? Via a new option? Or
make it report *that* size by default, and add a new option to report
the other number. Or should we just view this as a btrfs+compression
infelicity?

Otherwise, du provides no way of seeing how much of the actual disk
space is being used by such FS-compressed files.

Re: gcc10's -Wreturn-local-addr gives FP warning about lib/careadlinkat

[Jim Meyering]

On Sat, Feb 8, 2020 at 5:54 PM Jim Meyering  wrote:
...
> > > Hence, my last resort suggestion: disable that warning option for all
> > > gnulib compiles. Note that simply omitting the -W... option was
> > > insufficient (probably pulled in by some high-level one), so I
> > > resorted to adding -Wno-return-local-addr:
> >
> > +1
> >
> > This is a significant enough regression that it should be reported.
> > I didn't see a gcc bug report, only this meta bug:
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90556
>
> I've pushed that commit.
> Thanks for looking. I'll file a gcc report tomorrow.

Filed as https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93644
and connected it to 90556.

Re: gcc10's -Wreturn-local-addr gives FP warning about lib/careadlinkat

[Jim Meyering]

On Fri, Feb 7, 2020 at 5:47 AM Pádraig Brady  wrote:
>
> On 06/02/2020 17:05, Jim Meyering wrote:
> > On Thu, Feb 6, 2020 at 6:03 AM Pádraig Brady  wrote:
> >> On 06/02/2020 00:27, Jim Meyering wrote:
> >>> Building latest latest coreutils using latest-from-git gcc10 evokes
> >>> this false positive:
> >>>
> >>> lib/careadlinkat.c: In function 'careadlinkat':
> >>> cc1: error: function may return address of local variable
> >>> [-Werror=return-local-addr]
> >>> lib/careadlinkat.c:73:8: note: declared here
> >>>  73 |   char stack_buf[1024];
> >>>
> >>> I'm guessing improved flow analysis will eventually suppress this. I
> >>> hesitate to turn off the useful and normally-high-S/N
> >>> -Wreturn-local-addr globally. Maybe just disable it in that one file,
> >>> temporarily?
> >>
> >> The logic of the function looks fine.
> >> Would an `assure (buf != stack_buf)` before the `return buf`
> >> indicate that constraint to gcc with minimal runtime overhead?
> >
> > I would have preferred that, but it has no effect.
>
> surprised.
>
> > I then tried to suppress that warning in the affected file by adding
> > these lines:
> >
> > /* Without this pragma, gcc 10.0.1 20200205 reports that
> > the "function may return address of local variable".  */
> > # pragma GCC diagnostic ignored "-Wreturn-local-addr"
> >
> > But, surprisingly, that didn't help, either.
>
> surprised++
> That suggests something major is up with that error
> which might be easier for gcc folks to find.
>
> > Also tried Kaz Kylheku's return-early suggestion, to no avail.
> >
> > Hence, my last resort suggestion: disable that warning option for all
> > gnulib compiles. Note that simply omitting the -W... option was
> > insufficient (probably pulled in by some high-level one), so I
> > resorted to adding -Wno-return-local-addr:
>
> +1
>
> This is a significant enough regression that it should be reported.
> I didn't see a gcc bug report, only this meta bug:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90556

I've pushed that commit.
Thanks for looking. I'll file a gcc report tomorrow.

Re: gcc10's -Wreturn-local-addr gives FP warning about lib/careadlinkat

[Jim Meyering]

On Thu, Feb 6, 2020 at 6:03 AM Pádraig Brady  wrote:
> On 06/02/2020 00:27, Jim Meyering wrote:
> > Building latest latest coreutils using latest-from-git gcc10 evokes
> > this false positive:
> >
> > lib/careadlinkat.c: In function 'careadlinkat':
> > cc1: error: function may return address of local variable
> > [-Werror=return-local-addr]
> > lib/careadlinkat.c:73:8: note: declared here
> > 73 |   char stack_buf[1024];
> >
> > I'm guessing improved flow analysis will eventually suppress this. I
> > hesitate to turn off the useful and normally-high-S/N
> > -Wreturn-local-addr globally. Maybe just disable it in that one file,
> > temporarily?
>
> The logic of the function looks fine.
> Would an `assure (buf != stack_buf)` before the `return buf`
> indicate that constraint to gcc with minimal runtime overhead?

I would have preferred that, but it has no effect.
I then tried to suppress that warning in the affected file by adding
these lines:

/* Without this pragma, gcc 10.0.1 20200205 reports that
   the "function may return address of local variable".  */
# pragma GCC diagnostic ignored "-Wreturn-local-addr"

But, surprisingly, that didn't help, either.
Also tried Kaz Kylheku's return-early suggestion, to no avail.

Hence, my last resort suggestion: disable that warning option for all
gnulib compiles. Note that simply omitting the -W... option was
insufficient (probably pulled in by some high-level one), so I
resorted to adding -Wno-return-local-addr:


gcc10-FP-vs-careadlinkat.diff
Description: Binary data

gcc10's -Wreturn-local-addr gives FP warning about lib/careadlinkat

[Jim Meyering]

Building latest latest coreutils using latest-from-git gcc10 evokes
this false positive:

lib/careadlinkat.c: In function 'careadlinkat':
cc1: error: function may return address of local variable
[-Werror=return-local-addr]
lib/careadlinkat.c:73:8: note: declared here
   73 |   char stack_buf[1024];

I'm guessing improved flow analysis will eventually suppress this. I
hesitate to turn off the useful and normally-high-S/N
-Wreturn-local-addr globally. Maybe just disable it in that one file,
temporarily?

Re: building old coreutils versions on new glibc systems

[Jim Meyering]

On Tue, Aug 6, 2019 at 5:56 PM Assaf Gordon  wrote:
> On Tue, Aug 06, 2019 at 09:35:01PM +0200, Bernhard Voelker wrote:
> > On 8/2/19 9:05 AM, Jim Meyering wrote:
> > > Nice work. I've had to go through this process a few times over the
> > > years, and having these handy patch files checked in and maintained
> > > would make it easier to automate the process.
>
> > While this work is definitely worth keeping, I'm only 20:80 to add
> > something to the current (and future) version which belongs to older
> > versions.
> >
> > What about either uploading it to the FTP, or even better to add it
> > to the web pages' CVS?
>
> Adding it as a page to the website sounds good (it will also be easy
> for people to find using common search engines).
> I don't like the FTP idea so much - not very accesible unless you
> know exactly what you're looking for.
>
> Attached is a possible HTML page (and the patches in a subdirectory).

Since it is something that may contribute to binaries I build (with
the handy related build target), it feels like it belongs in
version-control. Otherwise, if downloading from some web site, we'd
have to verify the patches via gpg-signature or checksum before using
them to build new binaries.

Re: building old coreutils versions on new glibc systems

[Jim Meyering]

On Thu, Aug 1, 2019 at 7:48 PM Assaf Gordon  wrote:
> While trying to find out the first version with the 'seq' bug
> (my previous email), I realized it has become quite hard to build
> old coreutils version on newer glibc system.
>
> In particular:
> 1. At some point 'gets' was removed from glibc, but old sources refer it.
> 2. Older gnulib used internal glibc symbols (libio.h) and the detection
> method changed (_IO_ftrylockfile vs _IO_EOF_SEEN).
> See:  https://git.sv.gnu.org/cgit/gnulib.git/commit/?id=74d9d6a2
> 3. Old coreutils defined 'futimens','tee','eaccess' functions which conflict
> with later glibc functions of same name.
>
> In short, it's not trivial to download a tarball from
> https://ftp.gnu.org/gnu/coreutils/ and build it on modern systems
> (and it seems even more complicated to build from git).
>
> The attached patches enable building old tarballs on modern systems
> (tested on Debian 10 with GLIBC 2.28-10, gcc 8.3.0-6).
>
> The sequence should be:
>
> wget https://ftp.gnu.org/gnu/coreutils/coreutils-5.97.tar.gz
> tar -xf coreutils-5.97.tar.gz
> cd coreutils-5.97
> patch -p1 < ../coreutils-5.97-on-glibc-2.28.patch
> ./configure
> make
>
> Coreutils Versions Patch file
> 5.0coreutils-5.0-on-glibc-2.28.patch
> 5.97 to 6.9coreutils-5.97-on-glibc-2.28.patch
> 6.10   coreutils-6.10-on-glibc-2.28.patch
> 6.11   coreutils-6.11-on-glibc-2.28.patch
> 6.12   coreutils-6.12-on-glibc-2.28.patch
> 7.2  to 8.3coreutils-7.2-on-glibc-2.28.patch
> 8.4  to 8.12   coreutils-8.4-on-glibc-2.28.patch
> 8.13 to 8.16   coreutils-8.13-on-glibc-2.28.patch
> 8.17   coreutils-8.17-on-glibc-2.28.patch
> 8.18 to 8.23   coreutils-8.18-on-glibc-2.28.patch
> 8.24 to 8.29   coreutils-8.24-on-glibc-2.28.patch
> 8.30 and newer [builds without patching]

Nice work. I've had to go through this process a few times over the
years, and having these handy patch files checked in and maintained
would make it easier to automate the process. I'm on the fence as to
whether it's worth checking them in, given how few of us end up
building all old versions like that. Selfishly, I want it. Now that I
write this, I conclude it's worth the small cost. No need to
distribute those files, of course, and anything that makes a
maintainer's job easier (for such a small cost) is worthwhile.

Re: [PATCH] doc: fix description of tail -f on truncated files

[Jim Meyering]

On Sun, Jun 9, 2019, 04:43 Pádraig Brady  wrote:

> * doc/coreutils.texi (tail invocation): Update to match
> the new behavior following commit v8.23-189-gb28ff6a
> ---
> ...
>  No matter which method you use, if the tracked file is determined to have
>  shrunk, @command{tail} prints a message saying the file has been truncated
> -and resumes tracking the end of the file from the newly-determined
> endpoint.
> +and resumes tracking from the start of the file, assuming it has been
> +truncated to 0, which is the usual truncation operation for log files.
>

Good point.

Re: new snapshot available: coreutils-8.30.87-66e2d.tar.xz

[Jim Meyering]

On Wed, Mar 6, 2019 at 10:55 PM Pádraig Brady  wrote:
> We plan to release coreutils-8.31 in the coming week
> so any testing you can do on various different systems between now and then
> would be most welcome.

Thanks for all the work and for yet another release.
I tried to build on Fedora 29 using bleeding edge gcc and hit two
build failures. One from gnulib's userspec-test.c (just pushed the
fix) and the other here.
With these two changes, all of the tests and distcheck passed.

I wrote the attached patch solely to ensure that the tests all passed,
and am posting only for the record.
I would not apply this patch, since I like that warning and expect the
gcc regression will be fixed promptly.


coreutils-vs-gccs-buggy-Wnull-dereference.diff
Description: Binary data

Re: baseN: new program suggestion (various 'base' encoding)

[Jim Meyering]

On Sat, Jan 19, 2019 at 9:27 PM Pádraig Brady  wrote:
> On 13/01/19 22:16, Pádraig Brady wrote:
> > Ok I've sent a gnulib patch to support disbling VLA use with GNULIB_NO_VLA,
> > which is enabled in coreutils with the attached.
>
> Both now pushed at:
>
> https://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=8da562a
> https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=d26dece

Thanks!
I'm doing the same for each of grep, diffutils, gzip and sed.

Re: coreutils code structure overview

[Jim Meyering]

On Sat, Jan 5, 2019, 05:56 Pádraig Brady  FYI I've added a link on the main coreutils page on gnu.org
> to this useful code structure overview for the coreutils
>
> http://www.maizure.org/projects/decoded-gnu-coreutils/


Nice!

Re: copyright year update

[Jim Meyering]

On Tue, Jan 1, 2019 at 4:13 PM Assaf Gordon  wrote:
>
> Hello all,
>
> I took the liberty to update the copyright years,
> based on the same patch from last year (commit ece71579):
>
>  maint: update all copyright year number ranges
>
>  Run "make update-copyright" and then...
>
>  * gnulib: Update to latest with copyright year adjusted.
>  * tests/init.sh: Sync with gnulib to pick up copyright year.
>  * bootstrap: Likewise.
>  * tests/sample-test: Adjust to use the single most recent year.
>
> https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=8dfcf38af1280c34d7c02b1bbed151368f6de211
>
> Note that the 'bootstrap' sync changed more lines than just
> the copyright year, it pulled the latest version.
> Otherwise, changes are just copyright years.

Thanks, Assaf.
I've just pushed an additional commit (update gnulib to latest) that
causes "make syntax-check" to succeed once again, avoiding this
failure:

maint.mk: out of date copyright in ./gnulib/lib/version-etc.c; update it
make: *** [maint.mk;1199: sc_copyright_check] Error 1

Re: [PATCH] maintainer-makefile: fix syntax-check rule for "same.h" [was: [INSTALLED 2/2] ln: use linkat and symlinkat]

[Jim Meyering]

On Sun, Oct 28, 2018 at 12:59 PM Bernhard Voelker
 wrote:
> On 10/28/18 1:25 PM, Jim Meyering wrote:
> > Thanks for fixing that, but...
> >
> >> -   @h='same.h' re='\ > +   @h='same.h' re='\ >
> > please use the "?" operator instead of an empty alternation, i.e.,
> >
> >@h='same.h' re='\
> okay, thanks.  Would you push for me with that little change, please?
> I don't have push permissions on 'gnulib.git' anyway.

Sure. Done.

Re: [PATCH] maintainer-makefile: fix syntax-check rule for "same.h" [was: [INSTALLED 2/2] ln: use linkat and symlinkat]

[Jim Meyering]

On Sun, Oct 28, 2018 at 12:22 PM Bernhard Voelker
 wrote:
>
> sorry, I forgot to include bug-gnulib, sending again ...
> For reference, the original change in coreutils was published here:
>   https://lists.gnu.org/r/coreutils/2018-10/msg00032.html
>
> On 10/28/18 9:48 AM, Paul Eggert wrote:
> > @@ -291,7 +269,8 @@ do_link (const char *source, const char *dest, int 
> > link_errno)
> >if (source_status == 0
> >&& SAME_INODE (source_stats, dest_stats)
> >&& (source_stats.st_nlink == 1
> > -  || same_name (source, dest)))
> > +  || same_nameat (AT_FDCWD, source,
> > +  destdir_fd, dest_base)))
>
> This change leads leads to a FP syntax-check failure:
>
>   $ make syntax-check
>   ...
>   prohibit_same_without_use
>   src/ln.c
>   maint.mk: the above files include same.h but don't use it
>   make: *** [maint.mk:575: sc_prohibit_same_without_use] Error 1
>
> The attached gnulib patch fixes the SC rule - pls. check.

Thanks for fixing that, but...

>-   @h='same.h' re='\

Re: [PATCH] test: avoid FP in chroot-credentials.sh for different group list order

[Jim Meyering]

On Sat, Oct 20, 2018 at 8:22 PM Bernhard Voelker
 wrote:
> On 10/20/18 11:32 AM, Jim Meyering wrote:
> > If you adjust your function to be:
> >
> >   num_sort() { tr -s ' ' '\n' |sort -n; }
> >
> > then you can make less invasive changes like this:
> >
> > test "$(chroot --userspec=$NON_ROOT_USERNAME / id -G|num_sort)" = \
> >  "$(id -G $NON_ROOT_USERNAME|num_sort)" || fail=1
>
> Fair enough.  My intention was to catch more errors (while they are
> swallowed by the pipe) vs. yours is the smaller change, indeed.
> New version attached, pushing soon.

Thank you. That looks fine.

Re: [PATCH] test: avoid FP in chroot-credentials.sh for different group list order

[Jim Meyering]

On Sat, Oct 20, 2018 at 12:04 AM Bernhard Voelker
 wrote:
>
> On my openSUSE:Tumbleweed system, I get a false positive test failure
> in the above 'check-root' test because the group lists inside and
> outside the chroot have a different order:
>
>   ++ chroot --userspec=berny / id -G
>   ++ id -G berny
>   + test '100 454 457 480 492' = '100 480 492 457 454'
>   + fail=1
>
> * tests/misc/chroot-credentials.sh (numSort): Add function to sort
> group list files, and use it in the test cases which test multiple
> groups.
> ---
>  tests/misc/chroot-credentials.sh | 30 --
>  1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/tests/misc/chroot-credentials.sh 
> b/tests/misc/chroot-credentials.sh
> index 89d68451e..1e18ef70b 100755
> --- a/tests/misc/chroot-credentials.sh
> +++ b/tests/misc/chroot-credentials.sh
> @@ -69,13 +69,29 @@ id_G_after_chroot=$(
>  )
>  test "$id_G_after_chroot" = $NON_ROOT_GID || fail=1
>
> +# Sort whitespace-separated list of groups for all file arguments
> +# (and convert them to newline-delimited lists).
> +numSort()
> +{
> +  for f in "$@"; do
> +tr ' ' '\n' < "$f" > "$f.x" \
> +  && sort -no "$f" "$f.x" \
> +  && rm -f "$f.x" \
> +  || return 1
> +  done
> +}
> +
>  # Verify that when specifying only the user name we get all their groups
> -test "$(chroot --userspec=$NON_ROOT_USERNAME / id -G)" = \
> - "$(id -G $NON_ROOT_USERNAME)" || fail=1

Thanks, Bernie.
If you adjust your function to be:

  num_sort() { tr -s ' ' '\n' |sort -n; }

then you can make less invasive changes like this:

test "$(chroot --userspec=$NON_ROOT_USERNAME / id -G|num_sort)" = \
 "$(id -G $NON_ROOT_USERNAME|num_sort)" || fail=1

[PATCH] maint: init.cfg: fix a minor quoting bug

[Jim Meyering]

To trigger the bug, you'd have to run "make check" as a user with
membership in no more than one group.

I noticed that syntax highlighting was all messed up for many lines
starting there.

>From 7ff3e79312f0a9b6bf2c3198cee5c32793ca3bf8 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Sun, 1 Jul 2018 09:05:53 -0700
Subject: [PATCH] maint: init.cfg: fix a minor quoting bug

* init.cfg (require_membership_in_two_groups_): This fixes a bug
introduced by me in v8.15-8-gdd0e4c562.  Luckily, the consequence
of triggering the bug was the mere added backslash in the diagnostic:
"...but running id -G\ either...".
---
 init.cfg | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/init.cfg b/init.cfg
index 4259715c6..749d921c2 100644
--- a/init.cfg
+++ b/init.cfg
@@ -501,7 +501,7 @@ require_membership_in_two_groups_()
 *' '*) ;;
 *) skip_ 'requires membership in two groups
 this test requires that you be a member of more than one group,
-but running 'id -G'\'' either failed or found just one.  If you really
+but running '\''id -G'\'' either failed or found just one.  If you really
 are a member of at least two groups, then rerun this test with
 COREUTILS_GROUPS set in your environment to the space-separated list
 of group names or numbers.  E.g.,
--

Re: [PATCH] tests: standardize perl usage in tests

[Jim Meyering]

On Sat, Jun 30, 2018 at 6:55 PM, Pádraig Brady  wrote:
> * tests/cp/fiemap-FMR.sh: Ensure perl is parameterized to $PERL,
> and ensure require_perl_ is used, so tests are skipped appropriately.
> * tests/cp/preserve-gid.sh: Likewise.
> * tests/du/long-from-unreadable.sh: Likewise.
> * tests/misc/env-S-script.sh: Likewise.
> * tests/misc/sort-benchmark-random.sh: Likewise.
> * tests/rm/deep-2.sh: Likewise.
...
> -perl -e '
> +$PERL -e '
...
> -perl -e '
> +$PERL -e '
...
> -: ${PERL=perl}
>  $PERL \
>  -e 'my $d = "x" x 200; foreach my $i (1..52)' \
>  -e '  { mkdir ($d, 0700) && chdir $d or die "$!" }' \

Thanks for all the clean-up.
That looks fine.

For a further (separate) diff, what do you think about also defining a
"perl" function?
Then, the majority of those invocations would have the more natural
"perl -e ..." look.

Re: [PATCH] tests: fix false failures when perl not available

[Jim Meyering]

On Tue, Jun 26, 2018 at 12:44 AM, Pádraig Brady  wrote:
> * tests/local.mk: Reference the stub that skips perl tests,
> with the correct path.
> ---
>  tests/local.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tests/local.mk b/tests/local.mk
> index 307ade9..47c6b95 100644
> --- a/tests/local.mk
> +++ b/tests/local.mk
> @@ -28,7 +28,7 @@ TEST_EXTENSIONS = .sh .pl .xpl
>  if HAVE_PERL
>  TESTSUITE_PERL = $(PERL)
>  else
> -TESTSUITE_PERL = $(SHELL) $(srcdir)/no-perl
> +TESTSUITE_PERL = $(SHELL) $(srcdir)/tests/no-perl

Good one. Thanks!

Re: error: jump skips variable initialization

[Jim Meyering]

On Mon, Jun 25, 2018 at 12:07 PM, Bruno Haible  wrote:
...
> OK, done as follows:
>
> 2018-06-25  Bruno Haible  
>
> manywarnings: Don't enable -Wjump-misses-init warnings by default.
> * build-aux/gcc-warning.spec: Add -Wjump-misses-init.
> * m4/manywarnings.m4 (gl_MANYWARN_ALL_GCC(C)): Remove
> -Wjump-misses-init.
...

Thank you for the fine follow-up.

Re: FYI: address coreutils' gcc9 build failures

[Jim Meyering]

On Sun, Jun 24, 2018 at 7:43 PM, Bruno Haible  wrote:
> Jim Meyering asks:
>> I presume that by reporting this, you think gcc-4.8.5 is still worth 
>> supporting?
>
> Sure. This version of GCC is the default one on RHEL 7.4 / PowerPC, which is a
> mainstream Linux distro that is less than 1 year old. [1]
>
> In gnulib we support GCC versions down to 3.1. They are all working reasonably
> well.

Thanks. I pushed this:


parse-datetime--accommodate-gcc-4.8.5.diff
Description: Binary data

Re: FYI: address coreutils' gcc9 build failures

[Jim Meyering]

On Sun, Jun 24, 2018 at 6:36 PM, Bruno Haible  wrote:
> Jim Meyering wrote:
>> diff --git a/lib/parse-datetime.y b/lib/parse-datetime.y
>> index f14bb31dc..3d8666a4d 100644
>> --- a/lib/parse-datetime.y
>> +++ b/lib/parse-datetime.y
>> @@ -1766,6 +1766,11 @@ parse_datetime2 (struct timespec *result, char const 
>> *p,
>>
>>timezone_t tz = tzdefault;
>>
>> +  /* Store a local copy prior to first "goto".  Without this, a prior use
>> + below of RELATIVE_TIME_0 on the RHS might translate to an assignment-
>> + to-temporary, which would trigger a -Wjump-misses-init warning.  */
>> +  static const relative_time rel_time_0 = RELATIVE_TIME_0;
>> +
>>if (strncmp (p, "TZ=\"", 4) == 0)
>>  {
>>char const *tzbase = p + 4;
>
> With gcc 4.8.5, this produces a compilation error:
>
>   CC   lib/parse-datetime.o
> parse-datetime.y: In function 'parse_datetime2':
> parse-datetime.y:1772:3: error: initializer element is not constant
> make[2]: *** [lib/parse-datetime.o] Error 1
>
> Workaround: Remove the word 'static' from line 1772.

I presume that by reporting this, you think gcc-4.8.5 is still worth supporting?

FYI, even gcc-5.4 is no longer supported by upstream.

Re: error: jump skips variable initialization

[Jim Meyering]

On Sun, Jun 24, 2018 at 5:00 PM, Bruno Haible  wrote:
> Hi Jim, Paul,
>
>> parse-datetime.y: In function 'parse_datetime2':
>> parse-datetime.y:1791:19: error: jump skips variable initialization \
>>   [-Werror=jump-misses-init]

Hi Bruno,

Didn't this warning or one very much like it catch real bugs, e.g.,
where the initialization would be omitted, and the result would be to
use an uninitialized variable somewhere after the jumped-to label?

> Is the point merely "the standard says that a warning is possible, so let's
> implement it"?
>
> Can someone answer this?
>
> The next question then is: Why don't we silence this warning in the
> 'manywarnings' module?

If there is clearly no more utility in using this warning for C code,
then we're better off deleting it. Certainly in today's case, there
was no bug.

Re: FYI: address coreutils' gcc9 build failures

[Jim Meyering]

On Sun, Jun 24, 2018 at 4:20 PM, Pádraig Brady  wrote:
> On 24/06/18 16:10, Jim Meyering wrote:
>> +/* Without this pragma, gcc version 9.0.0 20180616 suggests that
>> +   the canon_* functions might be candidateifor attribute 'malloc'  */
>> +#if 9 <= __GNUC__
>> +# pragma GCC diagnostic ignored "-Wsuggest-attribute=malloc"
>> +#endif
>
> typo in candidatefor
>
> These functions basically wrap strdup() so would seem like
> it would be valid to tag them with _GL_ATTRIBUTE_MALLOC ?

Indeed. Thanks!
Pushing your suggested fix momentarily.

FYI: address coreutils' gcc9 build failures

[Jim Meyering]

I tried to build coreutils using recent gcc, built from git, so it
calls itself "gcc version 9.0.0 20180616 (experimental) (GCC)".
There were three gnulib-related build failures.  These were errors
solely because I also configured with --enable-gcc-warnings.

With these three changes to gnulib files, the build succeeded:

>From 593c1703712ec68b1e59bd7f1764d990e030ec69 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Sun, 24 Jun 2018 11:31:50 -0700
Subject: [PATCH 1/3] manywarnings: accommodate GCC 9.0-pre: remove -Wchkp and
 -Wabi

* build-aux/gcc-warning.spec: Add them here, each with an explanation.
* m4/manywarnings.m4: Remove them.
Otherwise, building coreutils, I would see this:
cc1: error: deprecated command line option '-Wchkp' [-Werror]
cc1: error: -Wabi won't warn about anything [-Werror=abi]
cc1: note: -Wabi warns about differences from the most up-to-date ABI,\
  which is also used by default
cc1: note: use e.g. -Wabi=11 to warn about changes from GCC 7
---
 ChangeLog  | 12 
 build-aux/gcc-warning.spec |  2 ++
 m4/manywarnings.m4 |  2 --
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 110487657..546da3218 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2018-06-24  Jim Meyering  
+
+   manywarnings: accommodate GCC 9.0-pre: remove -Wchkp and -Wabi
+   * build-aux/gcc-warning.spec: Add them here, each with an explanation.
+   * m4/manywarnings.m4: Remove them.
+   Otherwise, building coreutils, I would see this:
+   cc1: error: deprecated command line option '-Wchkp' [-Werror]
+   cc1: error: -Wabi won't warn about anything [-Werror=abi]
+   cc1: note: -Wabi warns about differences from the most up-to-date ABI,\
+ which is also used by default
+   cc1: note: use e.g. -Wabi=11 to warn about changes from GCC 7
+
 2018-06-03  Paul Eggert  

Port crypto/af_alg to GCC 4.8.4
diff --git a/build-aux/gcc-warning.spec b/build-aux/gcc-warning.spec
index 2ffdb2ba4..e2625ea9d 100644
--- a/build-aux/gcc-warning.spec
+++ b/build-aux/gcc-warning.spec
@@ -3,6 +3,7 @@
 --extra-warnings   alias for -Wextra
 -Wabi-tag  c++
 -Wabi= c++
+-Wabi  this is now a no-op
 -Waggregate-return obsolescent
 -Waliasing fortran
 -Walign-commonsfortran
@@ -30,6 +31,7 @@
 -Wcatch-value  c++
 -Wcatch-value=<0,3>c++
 -Wcharacter-truncation fortran
+-Wchkp deprecated
 -Wclass-memaccess  c++
 -Wcompare-realsfortran
 -Wconditionally-supported  c++ and objc++
diff --git a/m4/manywarnings.m4 b/m4/manywarnings.m4
index 60c0e4051..350c1355d 100644
--- a/m4/manywarnings.m4
+++ b/m4/manywarnings.m4
@@ -113,7 +113,6 @@ m4_defun([gl_MANYWARN_ALL_GCC(C)],
   gl_manywarn_set=
   for gl_manywarn_item in -fno-common \
 -W \
--Wabi \
 -Waddress \
 -Waggressive-loop-optimizations \
 -Wall \
@@ -128,7 +127,6 @@ m4_defun([gl_MANYWARN_ALL_GCC(C)],
 -Wcast-align=strict \
 -Wcast-function-type \
 -Wchar-subscripts \
--Wchkp \
 -Wclobbered \
 -Wcomment \
 -Wcomments \
--
2.18.0.rc2


>From 03fc6e299b3bdb255ee5f70fbec31feaf69717e8 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Sun, 24 Jun 2018 11:51:48 -0700
Subject: [PATCH 2/3] canon-host.c: avoid spurious GCC 9 warning

* lib/canon-host.c: Suppress GCC9's -Wsuggest-attribute=malloc.
---
 ChangeLog| 3 +++
 lib/canon-host.c | 6 ++
 2 files changed, 9 insertions(+)

diff --git a/ChangeLog b/ChangeLog
index 546da3218..d325d42ed 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,8 @@
 2018-06-24  Jim Meyering  

+   canon-host.c: avoid spurious GCC 9 warning
+   * lib/canon-host.c: Suppress GCC9's -Wsuggest-attribute=malloc.
+
manywarnings: accommodate GCC 9.0-pre: remove -Wchkp and -Wabi
* build-aux/gcc-warning.spec: Add them here, each with an explanation.
* m4/manywarnings.m4: Remove them.
diff --git a/lib/canon-host.c b/lib/canon-host.c
index f2a16a758..64afe1a0a 100644
--- a/lib/canon-host.c
+++ b/lib/canon-host.c
@@ -17,6 +17,12 @@
You should have received a copy of the GNU General Public License
along with this program.  If not, see <https://www.gnu.org/licenses/>.  */

+/* Without this pragma, gcc version 9.0.0 20180616 suggests that
+   the canon_* functions might be candidateifor attribute 'malloc'  */
+#if 9 <= __GNUC__
+# pragma GCC diagnostic ignored "-Wsuggest-attribute=malloc"
+#endif
+
 #include 

 #include "canon-host.h"
--
2.18.0.rc2


>From 131b984c708d174366d5e6d224c5164a7225ee7c Mon Sep 17 00:00:00 200

[PATCH] maint: don't pull in gnulib's now-unused ftello module

[Jim Meyering]

I noticed this is no longer used:

>From cb1f253e630c41e2f6b278a6a2592edb9cad97ea Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Tue, 19 Jun 2018 16:49:39 -0700
Subject: [PATCH] maint: do not depend directly on gnulib's now-unused ftello
 module

* bootstrap.conf (gnulib_modules): Remove ftello, since it is
no longer used directly, since v8.9-11-geab97b307.
---
 bootstrap.conf | 1 -
 1 file changed, 1 deletion(-)

diff --git a/bootstrap.conf b/bootstrap.conf
index 8c2265f95..23b83ab31 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -100,7 +100,6 @@ gnulib_modules="
   fstatat
   fsusage
   fsync
-  ftello
   ftoastr
   ftruncate
   fts
--

Re: [PATCH] rm: add --preserve-root=all to protect mounts

[Jim Meyering]

On Mon, Jun 18, 2018 at 8:16 AM, Jim Meyering  wrote:
> On Sun, Jun 17, 2018 at 9:41 PM, Pádraig Brady  wrote:
>> On 17/06/18 21:16, Jim Meyering wrote:
>>> On Sun, Jun 17, 2018 at 8:44 PM, Pádraig Brady  wrote:
>>>> I'll apply the attached full patch soon.
>>>
>>> Thanks for doing all that. I like it.
>>> One nit: this says "on a separate device":
>>>
>>> +  --preserve-root[=all]  do not remove '/' (default); with 'all', 
>>> skip\n\
>>> +  any command line argument on a separate 
>>> device\n\
>>>
>>> Perhaps it should say something like "on a separate device from its
>>> parent" ? Otherwise, one might erroneously interpret "separate" as
>>> meaning "different from the device of other command line arguments".
>>>
>>> I wondered how the code tested for this attribute, and had to read the
>>> code to find that it stats both the command-line argument, A, and
>>> "A/..", and if they have different device IDs, then A is eligible to
>>> be skipped when this new option is in effect.
>>
>> Yes the texinfo mentions the parent.
>> I'll expand --help to mention that also:
>>
>>   --preserve-root[=all]  do not remove '/' (default);
>>   with 'all', skip any command line argument
>>   on a separate device from its parent
>
> Thanks, clearly I missed the texinfo addition :-)
> Speaking of that, please use the active voice, not passive, i.e., change this
>
>   When @samp{all} is specified, any command line argument that
>   is on a separate file system to its parent is also skipped.
>
> to this:
>
>   When @samp{all} is specified, skip any command line argument that
>   is not on the same file system as its parent.

Actually, please also change s/skip/reject/; that is more appropriate,
since that rejection results in a nonzero exit status.

Re: [PATCH] rm: add --preserve-root=all to protect mounts

[Jim Meyering]

On Sun, Jun 17, 2018 at 9:41 PM, Pádraig Brady  wrote:
> On 17/06/18 21:16, Jim Meyering wrote:
>> On Sun, Jun 17, 2018 at 8:44 PM, Pádraig Brady  wrote:
>>> I'll apply the attached full patch soon.
>>
>> Thanks for doing all that. I like it.
>> One nit: this says "on a separate device":
>>
>> +  --preserve-root[=all]  do not remove '/' (default); with 'all', 
>> skip\n\
>> +  any command line argument on a separate 
>> device\n\
>>
>> Perhaps it should say something like "on a separate device from its
>> parent" ? Otherwise, one might erroneously interpret "separate" as
>> meaning "different from the device of other command line arguments".
>>
>> I wondered how the code tested for this attribute, and had to read the
>> code to find that it stats both the command-line argument, A, and
>> "A/..", and if they have different device IDs, then A is eligible to
>> be skipped when this new option is in effect.
>
> Yes the texinfo mentions the parent.
> I'll expand --help to mention that also:
>
>   --preserve-root[=all]  do not remove '/' (default);
>   with 'all', skip any command line argument
>   on a separate device from its parent

Thanks, clearly I missed the texinfo addition :-)
Speaking of that, please use the active voice, not passive, i.e., change this

  When @samp{all} is specified, any command line argument that
  is on a separate file system to its parent is also skipped.

to this:

  When @samp{all} is specified, skip any command line argument that
  is not on the same file system as its parent.

Re: [PATCH] rm: add --preserve-root=all to protect mounts

[Jim Meyering]

On Sun, Jun 17, 2018 at 8:44 PM, Pádraig Brady  wrote:
> I'll apply the attached full patch soon.

Thanks for doing all that. I like it.
One nit: this says "on a separate device":

+  --preserve-root[=all]  do not remove '/' (default); with 'all', skip\n\
+  any command line argument on a separate device\n\

Perhaps it should say something like "on a separate device from its
parent" ? Otherwise, one might erroneously interpret "separate" as
meaning "different from the device of other command line arguments".

I wondered how the code tested for this attribute, and had to read the
code to find that it stats both the command-line argument, A, and
"A/..", and if they have different device IDs, then A is eligible to
be skipped when this new option is in effect.