[PATCH] Cygwin: utils: chattr: Improve option parsing.

[PATCH] Cygwin: utils: chattr: Improve option parsing.

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

This possibly improves the usability of chattr for some typical use cases:

Command         : Old  : New behavior
================================================
chattr -h       : help : help
chattr -h FILE  : help : chattr -- -h -- FILE
chattr -hs FILE : help : chattr -- -h -s -- FILE
chattr -sh FILE : fail : chattr -- -s -h -- FILE
chattr -ar FILE : fail : chattr -- -a -r -- FILE

Unrelated: there a two trivial block-copied-but-not-changed issues:

$ egrep 'ACL|--r' chattr.c
           "Get POSIX ACL information\n"
       "  -R, --recursive     recursively list attributes of directories 
and their \n"

Regards,
Christian


[-- Attachment #2: 0001-Cygwin-utils-chattr-Improve-option-parsing.patch --]
[-- Type: text/plain, Size: 1370 bytes --]

From 865a5a50501f3fd0cf5ed28500d3e6e45a6456de Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Wed, 19 May 2021 16:24:47 +0200
Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing.

Interpret '-h' as '--help' only if last argument.
Allow multiple characters in first '-mode' argument.

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/utils/chattr.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/winsup/utils/chattr.c b/winsup/utils/chattr.c
index 98f693aab..f6ce340b4 100644
--- a/winsup/utils/chattr.c
+++ b/winsup/utils/chattr.c
@@ -271,7 +271,7 @@ int
 main (int argc, char **argv)
 {
   int c, ret = 0;
-  int lastoptind = 0;
+  int lastoptind = 1;
   char *opt;
 
   opterr = 0;
@@ -295,15 +295,16 @@ main (int argc, char **argv)
 	  print_version ();
 	  return 0;
 	  break;
+	case 'h':
+	  /* Print help if -h is last argument or --help is used,
+	     otherwise interpret -h as 'remove hidden attribute'.  */
+	  if (optind >= argc || (optind > lastoptind && argv[optind-1][1] == '-'))
+	    usage (stdout);
+	  /*FALLTHRU*/
 	default:
 	  if (optind > lastoptind)
-	    {
-	      --optind;
-	      goto next;
-	    }
-	  /*FALLTHRU*/
-	case 'h':
-	  usage (c == 'h' ? stdout : stderr);
+	    --optind;
+	  goto next;
 	}
     }
 next:
-- 
2.31.1

Re: [PATCH] Cygwin: utils: chattr: Improve option parsing.

[Corinna Vinschen]

Hi Christian,

On May 19 17:46, Christian Franke wrote:
> This possibly improves the usability of chattr for some typical use cases:
> 
> Command         : Old  : New behavior
> ================================================
> chattr -h       : help : help
> chattr -h FILE  : help : chattr -- -h -- FILE
> chattr -hs FILE : help : chattr -- -h -s -- FILE
> chattr -sh FILE : fail : chattr -- -s -h -- FILE
> chattr -ar FILE : fail : chattr -- -a -r -- FILE
> 
> Unrelated: there a two trivial block-copied-but-not-changed issues:
> 
> $ egrep 'ACL|--r' chattr.c
>           "Get POSIX ACL information\n"
>       "  -R, --recursive     recursively list attributes of directories and
> their \n"

Oops.  Please patch while you're at it...

> 
> Regards,
> Christian
> 

> From 865a5a50501f3fd0cf5ed28500d3e6e45a6456de Mon Sep 17 00:00:00 2001
> From: Christian Franke <christian.franke@t-online.de>
> Date: Wed, 19 May 2021 16:24:47 +0200
> Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing.
> 
> Interpret '-h' as '--help' only if last argument.

Who was the idiot using -h for help *and* the hidden flag? *blush*

I'd vote for --help to be changed to -H for the single character
option.  The help output is very unlikely to be used in scripts,
so that shouldn't be a backward compat problem.

Would you mind to change the patch accordingly?


Thanks,
Corinna

Re: [PATCH] Cygwin: utils: chattr: Improve option parsing.

[Christian Franke]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

Corinna Vinschen wrote:
> Hi Christian,
>
> On May 19 17:46, Christian Franke wrote:
>> ...
>> $ egrep 'ACL|--r' chattr.c
>>            "Get POSIX ACL information\n"
>>        "  -R, --recursive     recursively list attributes of directories and
>> their \n"
> Oops.  Please patch while you're at it...
> ...
>>  From 865a5a50501f3fd0cf5ed28500d3e6e45a6456de Mon Sep 17 00:00:00 2001
>> From: Christian Franke<...>
>> Date: Wed, 19 May 2021 16:24:47 +0200
>> Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing.
>>
>> Interpret '-h' as '--help' only if last argument.
> Who was the idiot using -h for help *and* the hidden flag? *blush*
>
> I'd vote for --help to be changed to -H for the single character
> option.  The help output is very unlikely to be used in scripts,
> so that shouldn't be a backward compat problem.

New patch attached.

Note that there is now the possibly unexpected (& hidden) behavior that 
'chattr -h' without file argument clears the hidden attribute of cwd.

Regards,
Christian


[-- Attachment #2: 0001-Cygwin-utils-chattr-Improve-option-parsing-fix-some-.patch --]
[-- Type: text/plain, Size: 5050 bytes --]

From b838125f797c123d4a6d2f0ef30cccee8face50c Mon Sep 17 00:00:00 2001
From: Christian Franke <christian.franke@t-online.de>
Date: Thu, 20 May 2021 11:05:29 +0200
Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing, fix some
 messages.

Allow multiple characters also in first '-mode' argument.
Use '-H' instead of '-h' for '--help' to fix ambiguity with
hidden attribute.  Fix help and usage texts and documentation.

Signed-off-by: Christian Franke <christian.franke@t-online.de>
---
 winsup/doc/utils.xml  | 10 +++++-----
 winsup/utils/chattr.c | 32 ++++++++++++++------------------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/winsup/doc/utils.xml b/winsup/doc/utils.xml
index 22bd86904..69611b954 100644
--- a/winsup/doc/utils.xml
+++ b/winsup/doc/utils.xml
@@ -27,18 +27,18 @@
 
     <refsynopsisdiv>
       <screen>
-chattr [-RVfhv] [+-=mode]... [file]...
+chattr [-RVfHv] [+-=mode]... [file]...
       </screen>
     </refsynopsisdiv>
 
     <refsect1 id="chattr-options">
       <title>Options</title>
       <screen>
-  -R, --recursive     recursively list attributes of directories and their
-                      contents
+  -R, --recursive     recursively apply the changes to directories and
+                      their contents
   -V, --verbose       Be verbose during operation
   -f, --force         suppress error messages
-  -h, --help          this help text
+  -H, --help          this help text
   -v, --version       display the program version
 </screen>
     </refsect1>
@@ -60,7 +60,7 @@ chattr [-RVfhv] [+-=mode]... [file]...
     <para>Supported attributes:</para>
 
     <screen>
-  'r', 'Readonly':     file is read-only
+  'r', 'Readonly':      file is read-only
   'h', 'Hidden':        file or directory is hidden
   's', 'System':        file or directory that the operating system uses
   'a', 'Archive':       file or directory has the archive marker set
diff --git a/winsup/utils/chattr.c b/winsup/utils/chattr.c
index 98f693aab..c7dc649c2 100644
--- a/winsup/utils/chattr.c
+++ b/winsup/utils/chattr.c
@@ -28,12 +28,12 @@ struct option longopts[] = {
   { "recursive", no_argument, NULL, 'R' },
   { "verbose", no_argument, NULL, 'V' },
   { "force", no_argument, NULL, 'f' },
-  { "help", no_argument, NULL, 'h' },
+  { "help", no_argument, NULL, 'H' },
   { "version", no_argument, NULL, 'v' },
   { NULL, no_argument, NULL, 0}
 };
 
-const char *opts = "+RVfhv";
+const char *opts = "+RVfHv";
 
 struct
 {
@@ -211,7 +211,7 @@ static void
 print_version ()
 {
   printf ("%s (cygwin) %d.%d.%d\n"
-	  "Get POSIX ACL information\n"
+	  "Change file attributes\n"
 	  "Copyright (C) 2018 - %s Cygwin Authors\n"
 	  "This is free software; see the source for copying conditions.  "
 	  "There is NO\n"
@@ -227,7 +227,7 @@ print_version ()
 static void __attribute__ ((__noreturn__))
 usage (FILE *stream)
 {
-  fprintf (stream, "Usage: %s [-RVfhv] [+-=mode]... [file]...\n",
+  fprintf (stream, "Usage: %s [-RVfHv] [+-=mode]... [file]...\n",
 	   program_invocation_short_name);
   if (stream == stderr)
     fprintf (stream, "Try '%s --help' for more information\n",
@@ -236,11 +236,11 @@ usage (FILE *stream)
     fprintf (stream, "\n"
       "Change file attributes\n"
       "\n"
-      "  -R, --recursive     recursively list attributes of directories and their \n"
+      "  -R, --recursive     recursively apply the changes to directories and their\n"
       "                      contents\n"
       "  -V, --verbose       Be verbose during operation\n"
       "  -f, --force         suppress error messages\n"
-      "  -h, --help          this help text\n"
+      "  -H, --help          this help text\n"
       "  -v, --version       display the program version\n"
       "\n"
       "The format of 'mode' is {+-=}[acCehnrsSt]\n"
@@ -251,7 +251,7 @@ usage (FILE *stream)
       "\n"
       "Supported attributes:\n"
       "\n"
-      "  'r', 'Readonly':     file is read-only\n"
+      "  'r', 'Readonly':      file is read-only\n"
       "  'h', 'Hidden':        file or directory is hidden\n"
       "  's', 'System':        file or directory that the operating system uses\n"
       "  'a', 'Archive':       file or directory has the archive marker set\n"
@@ -271,7 +271,7 @@ int
 main (int argc, char **argv)
 {
   int c, ret = 0;
-  int lastoptind = 0;
+  int lastoptind = 1;
   char *opt;
 
   opterr = 0;
@@ -281,15 +281,15 @@ main (int argc, char **argv)
 	{
 	case 'R':
 	  Ropt = 1;
-	  lastoptind = optind;
 	  break;
 	case 'V':
 	  Vopt = 1;
-	  lastoptind = optind;
 	  break;
 	case 'f':
 	  fopt = 1;
-	  lastoptind = optind;
+	  break;
+	case 'H':
+	  usage (stdout);
 	  break;
 	case 'v':
 	  print_version ();
@@ -297,14 +297,10 @@ main (int argc, char **argv)
 	  break;
 	default:
 	  if (optind > lastoptind)
-	    {
-	      --optind;
-	      goto next;
-	    }
-	  /*FALLTHRU*/
-	case 'h':
-	  usage (c == 'h' ? stdout : stderr);
+	    --optind;
+	  goto next;
 	}
+      lastoptind = optind;
     }
 next:
   while (optind < argc)
-- 
2.31.1

Re: [PATCH] Cygwin: utils: chattr: Improve option parsing.

[Corinna Vinschen]

Hi Christian,

On May 20 12:01, Christian Franke wrote:
> Corinna Vinschen wrote:
> > On May 19 17:46, Christian Franke wrote:
> > > ...
> > > $ egrep 'ACL|--r' chattr.c
> > >            "Get POSIX ACL information\n"
> > >        "  -R, --recursive     recursively list attributes of directories and
> > > their \n"
> > Oops.  Please patch while you're at it...
> > ...
> > >  From 865a5a50501f3fd0cf5ed28500d3e6e45a6456de Mon Sep 17 00:00:00 2001
> > > From: Christian Franke<...>
> > > Date: Wed, 19 May 2021 16:24:47 +0200
> > > Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing.
> > > 
> > > Interpret '-h' as '--help' only if last argument.
> > Who was the idiot using -h for help *and* the hidden flag? *blush*
> > 
> > I'd vote for --help to be changed to -H for the single character
> > option.  The help output is very unlikely to be used in scripts,
> > so that shouldn't be a backward compat problem.
> 
> New patch attached.
> 
> Note that there is now the possibly unexpected (& hidden) behavior that
> 'chattr -h' without file argument clears the hidden attribute of cwd.

Uhm... why?  Isn't that easily avoidable?


Thanks,
Corinna

Re: [PATCH] Cygwin: utils: chattr: Improve option parsing.

[Christian Franke]

Hi Corinna,

Corinna Vinschen wrote:
> Hi Christian,
>
> On May 20 12:01, Christian Franke wrote:
>> Corinna Vinschen wrote:
>>> On May 19 17:46, Christian Franke wrote:
>>>> ...
>>>> $ egrep 'ACL|--r' chattr.c
>>>>             "Get POSIX ACL information\n"
>>>>         "  -R, --recursive     recursively list attributes of directories and
>>>> their \n"
>>> Oops.  Please patch while you're at it...
>>> ...
>>>>   From 865a5a50501f3fd0cf5ed28500d3e6e45a6456de Mon Sep 17 00:00:00 2001
>>>> From: Christian Franke<...>
>>>> Date: Wed, 19 May 2021 16:24:47 +0200
>>>> Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing.
>>>>
>>>> Interpret '-h' as '--help' only if last argument.
>>> Who was the idiot using -h for help *and* the hidden flag? *blush*
>>>
>>> I'd vote for --help to be changed to -H for the single character
>>> option.  The help output is very unlikely to be used in scripts,
>>> so that shouldn't be a backward compat problem.
>> New patch attached.
>>
>> Note that there is now the possibly unexpected (& hidden) behavior that
>> 'chattr -h' without file argument clears the hidden attribute of cwd.
> Uhm... why?

Because chattr uses '.' as default if no FILE argument is specified. The 
same applies to all other '+-=MODE' arguments. The patch does not change 
this behavior but of course enables it for '-h'.


>    Isn't that easily avoidable?

Yes: make FILE argument mandatory - but this would break backward 
compatibility.

Another behavior (possibly inherited from lsattr) is also not very useful:
'chattr +-=MODE DIRECTORY' also changes the attributes of all elements 
in the directory (not recursively). It is not possible to solely change 
the attributes of a directory except if it is the current directory and 
no FILE argument is passed. Fixing this would again break backward 
compatibility.

Regards,
Christian

Re: [PATCH] Cygwin: utils: chattr: Improve option parsing.

[Corinna Vinschen]

On May 20 21:48, Christian Franke wrote:
> Hi Corinna,
> 
> Corinna Vinschen wrote:
> > Hi Christian,
> > 
> > On May 20 12:01, Christian Franke wrote:
> > > Corinna Vinschen wrote:
> > > > On May 19 17:46, Christian Franke wrote:
> > > > > ...
> > > > > $ egrep 'ACL|--r' chattr.c
> > > > >             "Get POSIX ACL information\n"
> > > > >         "  -R, --recursive     recursively list attributes of directories and
> > > > > their \n"
> > > > Oops.  Please patch while you're at it...
> > > > ...
> > > > >   From 865a5a50501f3fd0cf5ed28500d3e6e45a6456de Mon Sep 17 00:00:00 2001
> > > > > From: Christian Franke<...>
> > > > > Date: Wed, 19 May 2021 16:24:47 +0200
> > > > > Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing.
> > > > > 
> > > > > Interpret '-h' as '--help' only if last argument.
> > > > Who was the idiot using -h for help *and* the hidden flag? *blush*
> > > > 
> > > > I'd vote for --help to be changed to -H for the single character
> > > > option.  The help output is very unlikely to be used in scripts,
> > > > so that shouldn't be a backward compat problem.
> > > New patch attached.
> > > 
> > > Note that there is now the possibly unexpected (& hidden) behavior that
> > > 'chattr -h' without file argument clears the hidden attribute of cwd.
> > Uhm... why?
> 
> Because chattr uses '.' as default if no FILE argument is specified. The
> same applies to all other '+-=MODE' arguments. The patch does not change
> this behavior but of course enables it for '-h'.

Oh, right.  I can't reproduce this behaviour with the most recent chattr
from e2fsprogs in F33.  Sorry, I'm not sure anymore why I did that.  But
given the entire behaviour is rather unexpected, and it's not even
documented, maybe we should drop it, too?  Not sure if backward compat
counts in this case...

> Another behavior (possibly inherited from lsattr) is also not very useful:
> 'chattr +-=MODE DIRECTORY' also changes the attributes of all elements in
> the directory (not recursively). It is not possible to solely change the
> attributes of a directory except if it is the current directory and no FILE
> argument is passed. Fixing this would again break backward compatibility.

Ouch, no!  That's a bug.  The expression in line 354 should have been

    if (S_ISDIR (st.st_mode) && Ropt && chattr_dir (argv[optind]))
		ret = 1;

the Ropt check is just missing.


Corinna

Re: [PATCH] Cygwin: utils: chattr: Improve option parsing.

[Corinna Vinschen]

On May 20 12:01, Christian Franke wrote:
> Corinna Vinschen wrote:
> > Hi Christian,
> > 
> > On May 19 17:46, Christian Franke wrote:
> > > ...
> > > $ egrep 'ACL|--r' chattr.c
> > >            "Get POSIX ACL information\n"
> > >        "  -R, --recursive     recursively list attributes of directories and
> > > their \n"
> > Oops.  Please patch while you're at it...
> > ...
> > >  From 865a5a50501f3fd0cf5ed28500d3e6e45a6456de Mon Sep 17 00:00:00 2001
> > > From: Christian Franke<...>
> > > Date: Wed, 19 May 2021 16:24:47 +0200
> > > Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing.
> > > 
> > > Interpret '-h' as '--help' only if last argument.
> > Who was the idiot using -h for help *and* the hidden flag? *blush*
> > 
> > I'd vote for --help to be changed to -H for the single character
> > option.  The help output is very unlikely to be used in scripts,
> > so that shouldn't be a backward compat problem.
> 
> New patch attached.
> 
> Note that there is now the possibly unexpected (& hidden) behavior that
> 'chattr -h' without file argument clears the hidden attribute of cwd.
> 
> Regards,
> Christian
> 

> From b838125f797c123d4a6d2f0ef30cccee8face50c Mon Sep 17 00:00:00 2001
> From: Christian Franke <christian.franke@t-online.de>
> Date: Thu, 20 May 2021 11:05:29 +0200
> Subject: [PATCH] Cygwin: utils: chattr: Improve option parsing, fix some
>  messages.
> 
> Allow multiple characters also in first '-mode' argument.
> Use '-H' instead of '-h' for '--help' to fix ambiguity with
> hidden attribute.  Fix help and usage texts and documentation.
> 
> Signed-off-by: Christian Franke <christian.franke@t-online.de>
> ---
>  winsup/doc/utils.xml  | 10 +++++-----
>  winsup/utils/chattr.c | 32 ++++++++++++++------------------
>  2 files changed, 19 insertions(+), 23 deletions(-)

Pushed.


Thanks,
Corinna