[PATCH v6 0/6] More ls improvements

[PATCH v6 0/6] More ls improvements

[Glenn Washburn]

Considering Daniel's feedback on the v2 of this patch series, I've broken up
patches #1 and #2 and dropped patch #3 from the original series. These changes
make the output of the ls command a little more like the GNU ls output.

Glenn Washburn

v6:
 * Rebase onto current master
 * Change ctx->filename to ctx->filename != NULL in tertiary conditional in
   patch 5 and add a comment explaining why ctx->filename is needed

v4: Add missing parenthesis

Glenn Washburn (6):
  commands/ls: Return proper GRUB_ERR_* for functions returning type
    grub_err_t
  commands/ls: Merge print_files_long and print_files into print_file
  commands/ls: Show modification time for file paths
  commands/ls: Output path for single file arguments given with path
  commands/ls: Print full paths for file args
  commands/ls: Add directory header for dir args

 grub-core/commands/ls.c | 103 +++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 49 deletions(-)

Range-diff against v4:
1:  9086cccd8a0f = 1:  f8081dbd262d commands/ls: Return proper GRUB_ERR_* for functions returning type grub_err_t
2:  3192dc2471d0 = 2:  310a52b42d6d commands/ls: Merge print_files_long and print_files into print_file
3:  889e1e05a355 ! 3:  01532280401b commands/ls: Show modification time for file paths
    @@ grub-core/commands/ls.c: grub_ls_list_files (char *dirname, int longlist, int al
      
     -	  grub_file_close (file);
     -
    --	  p = grub_strrchr (dirname, '/') + 1;
    --	  ctx.dirname = grub_strndup (dirname, p - dirname);
    +-	  p = grub_strrchr (dirname, '/');
    +-	  if (p == NULL)
     +	  /* PATH might be a regular file.  */
    -+	  ctx.filename = grub_strrchr (dirname, '/') + 1;
    ++	  ctx.filename = grub_strrchr (dirname, '/');
    ++	  if (ctx.filename == NULL)
    + 	    goto fail;
    +-	  ++p;
    ++	  ++(ctx.filename);
    + 
    +-	  ctx.dirname = grub_strndup (dirname, p - dirname);
     +	  ctx.dirname = grub_strndup (dirname, ctx.filename - dirname);
      	  if (ctx.dirname == NULL)
      	    goto fail;
4:  d63e007f635b = 4:  fd4378a9302a commands/ls: Output path for single file arguments given with path
5:  39adcb21e320 ! 5:  16c2203f04dc commands/ls: Print full paths for file args
    @@ grub-core/commands/ls.c: print_file (const char *filename, const struct grub_dir
      		     datetime.minute, datetime.second);
          }
     -  grub_printf ("%s%s\n", filename, info->dir ? "/" : "");
    -+  grub_printf ("%s%s\n", (ctx->filename) ? pathname : filename,
    ++  /*
    ++   * Only print the full path when listing a file path given as an argument
    ++   * to ls (ie. when ctx->filename != NULL). File listings that are printed
    ++   * due to showing the contents of a directory do not need a full path because
    ++   * the full path to the directory will have already been printed.
    ++   */
    ++  grub_printf ("%s%s\n", (ctx->filename != NULL) ? pathname : filename,
     +			 info->dir ? "/" : "");
     +
     +  grub_free (pathname);
6:  0ebbfd3b0e15 ! 6:  ec9120ac52b0 commands/ls: Add directory header for dir args
    @@ grub-core/commands/ls.c: grub_ls_list_files (char *dirname, int longlist, int al
      
      	  /* PATH might be a regular file.  */
     +	  ctx.print_dirhdr = 0;
    - 	  ctx.filename = grub_strrchr (dirname, '/') + 1;
    - 	  ctx.dirname = grub_strndup (dirname, ctx.filename - dirname);
    - 	  if (ctx.dirname == NULL)
    + 	  ctx.filename = grub_strrchr (dirname, '/');
    + 	  if (ctx.filename == NULL)
    + 	    goto fail;
     @@ grub-core/commands/ls.c: grub_cmd_ls (grub_extcmd_context_t ctxt, int argc, char **args)
          grub_ls_list_devices (state[0].set);
        else
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v6 1/6] commands/ls: Return proper GRUB_ERR_* for functions returning type grub_err_t

[Glenn Washburn]

Also, remove unused code.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index f660946a24e4..65b17768b605 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -250,7 +250,6 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
 	  if (ctx.dirname == NULL)
 	    goto fail;
 
-	  all = 1;
 	  grub_memset (&info, 0, sizeof (info));
 	  if (longlist)
 	    print_files_long (p, &info, &ctx);
@@ -272,7 +271,7 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
 
   grub_free (device_name);
 
-  return 0;
+  return GRUB_ERR_NONE;
 }
 
 static grub_err_t
@@ -288,7 +287,7 @@ grub_cmd_ls (grub_extcmd_context_t ctxt, int argc, char **args)
       grub_ls_list_files (args[i], state[0].set, state[2].set,
 			  state[1].set);
 
-  return 0;
+  return GRUB_ERR_NONE;
 }
 
 static grub_extcmd_t cmd;
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v6 2/6] commands/ls: Merge print_files_long and print_files into print_file

[Glenn Washburn]

Simplify the code by removing logic around which file printer to call.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 35 ++++++++++++-----------------------
 1 file changed, 12 insertions(+), 23 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 65b17768b605..2eaefaf9027a 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -89,24 +89,12 @@ struct grub_ls_list_files_ctx
   char *dirname;
   int all;
   int human;
+  int longlist;
 };
 
 /* Helper for grub_ls_list_files.  */
 static int
-print_files (const char *filename, const struct grub_dirhook_info *info,
-	     void *data)
-{
-  struct grub_ls_list_files_ctx *ctx = data;
-
-  if (ctx->all || filename[0] != '.')
-    grub_printf ("%s%s ", filename, info->dir ? "/" : "");
-
-  return 0;
-}
-
-/* Helper for grub_ls_list_files.  */
-static int
-print_files_long (const char *filename, const struct grub_dirhook_info *info,
+print_file (const char *filename, const struct grub_dirhook_info *info,
 		  void *data)
 {
   struct grub_ls_list_files_ctx *ctx = data;
@@ -114,6 +102,12 @@ print_files_long (const char *filename, const struct grub_dirhook_info *info,
   if ((! ctx->all) && (filename[0] == '.'))
     return 0;
 
+  if (! ctx->longlist)
+    {
+      grub_printf ("%s%s ", filename, info->dir ? "/" : "");
+      return 0;
+    }
+
   if (! info->dir)
     {
       grub_file_t file;
@@ -217,13 +211,11 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
       struct grub_ls_list_files_ctx ctx = {
 	.dirname = dirname,
 	.all = all,
-	.human = human
+	.human = human,
+	.longlist = longlist
       };
 
-      if (longlist)
-	(fs->fs_dir) (dev, path, print_files_long, &ctx);
-      else
-	(fs->fs_dir) (dev, path, print_files, &ctx);
+      (fs->fs_dir) (dev, path, print_file, &ctx);
 
       if (grub_errno == GRUB_ERR_BAD_FILE_TYPE
 	  && path[grub_strlen (path) - 1] != '/')
@@ -251,10 +243,7 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
 	    goto fail;
 
 	  grub_memset (&info, 0, sizeof (info));
-	  if (longlist)
-	    print_files_long (p, &info, &ctx);
-	  else
-	    print_files (p, &info, &ctx);
+	  print_file (p, &info, &ctx);
 
 	  grub_free (ctx.dirname);
 	}
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v6 3/6] commands/ls: Show modification time for file paths

[Glenn Washburn]

The modification time for paths to files was not being printed because
the grub_dirhook_info, which contains the mtime, was initialized to NULL.
Instead of calling print_file() directly, use fs->fs_dir() to call
print_file() with a properly filled in grub_dirhook_info. This has the
added benefit of reducing code complexity.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 2eaefaf9027a..1b447acf7c34 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -87,6 +87,7 @@ grub_ls_list_devices (int longlist)
 struct grub_ls_list_files_ctx
 {
   char *dirname;
+  char *filename;
   int all;
   int human;
   int longlist;
@@ -102,6 +103,9 @@ print_file (const char *filename, const struct grub_dirhook_info *info,
   if ((! ctx->all) && (filename[0] == '.'))
     return 0;
 
+  if ((ctx->filename != NULL) && (grub_strcmp (filename, ctx->filename) != 0))
+    return 0;
+
   if (! ctx->longlist)
     {
       grub_printf ("%s%s ", filename, info->dir ? "/" : "");
@@ -210,6 +214,7 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
     {
       struct grub_ls_list_files_ctx ctx = {
 	.dirname = dirname,
+	.filename = NULL,
 	.all = all,
 	.human = human,
 	.longlist = longlist
@@ -220,30 +225,23 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
       if (grub_errno == GRUB_ERR_BAD_FILE_TYPE
 	  && path[grub_strlen (path) - 1] != '/')
 	{
-	  /* PATH might be a regular file.  */
-	  char *p;
-	  grub_file_t file;
-	  struct grub_dirhook_info info;
-	  grub_errno = 0;
-
-	  file = grub_file_open (dirname, GRUB_FILE_TYPE_GET_SIZE
-				 | GRUB_FILE_TYPE_NO_DECOMPRESS);
-	  if (! file)
-	    goto fail;
+	  /*
+	   * Reset errno as it is currently set, but will cause subsequent code
+	   * to think there is an error.
+	   */
+	  grub_errno = GRUB_ERR_NONE;
 
-	  grub_file_close (file);
-
-	  p = grub_strrchr (dirname, '/');
-	  if (p == NULL)
+	  /* PATH might be a regular file.  */
+	  ctx.filename = grub_strrchr (dirname, '/');
+	  if (ctx.filename == NULL)
 	    goto fail;
-	  ++p;
+	  ++(ctx.filename);
 
-	  ctx.dirname = grub_strndup (dirname, p - dirname);
+	  ctx.dirname = grub_strndup (dirname, ctx.filename - dirname);
 	  if (ctx.dirname == NULL)
 	    goto fail;
 
-	  grub_memset (&info, 0, sizeof (info));
-	  print_file (p, &info, &ctx);
+	  (fs->fs_dir) (dev, ctx.dirname + (path - dirname), print_file, &ctx);
 
 	  grub_free (ctx.dirname);
 	}
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v6 4/6] commands/ls: Output path for single file arguments given with path

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 1b447acf7c34..886441575cac 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -108,6 +108,8 @@ print_file (const char *filename, const struct grub_dirhook_info *info,
 
   if (! ctx->longlist)
     {
+      if (ctx->filename != NULL)
+	grub_xputs (ctx->dirname);
       grub_printf ("%s%s ", filename, info->dir ? "/" : "");
       return 0;
     }
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v6 5/6] commands/ls: Print full paths for file args

[Glenn Washburn]

For arguments that are paths to files, print the full path of the file.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 886441575cac..09de23bb0b3a 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -98,6 +98,7 @@ static int
 print_file (const char *filename, const struct grub_dirhook_info *info,
 		  void *data)
 {
+  char *pathname = NULL;
   struct grub_ls_list_files_ctx *ctx = data;
 
   if ((! ctx->all) && (filename[0] == '.'))
@@ -117,7 +118,6 @@ print_file (const char *filename, const struct grub_dirhook_info *info,
   if (! info->dir)
     {
       grub_file_t file;
-      char *pathname;
 
       if (ctx->dirname[grub_strlen (ctx->dirname) - 1] == '/')
 	pathname = grub_xasprintf ("%s%s", ctx->dirname, filename);
@@ -143,7 +143,6 @@ print_file (const char *filename, const struct grub_dirhook_info *info,
       else
 	grub_xputs ("????????????");
 
-      grub_free (pathname);
       grub_errno = GRUB_ERR_NONE;
     }
   else
@@ -165,7 +164,16 @@ print_file (const char *filename, const struct grub_dirhook_info *info,
 		     datetime.day, datetime.hour,
 		     datetime.minute, datetime.second);
     }
-  grub_printf ("%s%s\n", filename, info->dir ? "/" : "");
+  /*
+   * Only print the full path when listing a file path given as an argument
+   * to ls (ie. when ctx->filename != NULL). File listings that are printed
+   * due to showing the contents of a directory do not need a full path because
+   * the full path to the directory will have already been printed.
+   */
+  grub_printf ("%s%s\n", (ctx->filename != NULL) ? pathname : filename,
+			 info->dir ? "/" : "");
+
+  grub_free (pathname);
 
   return 0;
 }
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v6 6/6] commands/ls: Add directory header for dir args

[Glenn Washburn]

Like the GNU ls, first print a line with the directory path before printing
files in the directory, which will not have a directory component, but only
if there is more than one argument.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/ls.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 09de23bb0b3a..3416ec97c871 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -91,6 +91,7 @@ struct grub_ls_list_files_ctx
   int all;
   int human;
   int longlist;
+  int print_dirhdr;
 };
 
 /* Helper for grub_ls_list_files.  */
@@ -107,6 +108,12 @@ print_file (const char *filename, const struct grub_dirhook_info *info,
   if ((ctx->filename != NULL) && (grub_strcmp (filename, ctx->filename) != 0))
     return 0;
 
+  if (ctx->print_dirhdr)
+    {
+      grub_printf ("%s:\n", ctx->dirname);
+      ctx->print_dirhdr = 0;
+    }
+
   if (! ctx->longlist)
     {
       if (ctx->filename != NULL)
@@ -179,7 +186,7 @@ print_file (const char *filename, const struct grub_dirhook_info *info,
 }
 
 static grub_err_t
-grub_ls_list_files (char *dirname, int longlist, int all, int human)
+grub_ls_list_files (char *dirname, int longlist, int all, int human, int dirhdr)
 {
   char *device_name;
   grub_fs_t fs;
@@ -227,7 +234,8 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
 	.filename = NULL,
 	.all = all,
 	.human = human,
-	.longlist = longlist
+	.longlist = longlist,
+	.print_dirhdr = dirhdr
       };
 
       (fs->fs_dir) (dev, path, print_file, &ctx);
@@ -242,6 +250,7 @@ grub_ls_list_files (char *dirname, int longlist, int all, int human)
 	  grub_errno = GRUB_ERR_NONE;
 
 	  /* PATH might be a regular file.  */
+	  ctx.print_dirhdr = 0;
 	  ctx.filename = grub_strrchr (dirname, '/');
 	  if (ctx.filename == NULL)
 	    goto fail;
@@ -281,8 +290,8 @@ grub_cmd_ls (grub_extcmd_context_t ctxt, int argc, char **args)
     grub_ls_list_devices (state[0].set);
   else
     for (i = 0; i < argc; i++)
-      grub_ls_list_files (args[i], state[0].set, state[2].set,
-			  state[1].set);
+      grub_ls_list_files (args[i], state[0].set, state[2].set, state[1].set,
+			  argc > 1);
 
   return GRUB_ERR_NONE;
 }
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v6 0/6] More ls improvements

[Daniel Kiper]

On Sat, Mar 01, 2025 at 11:15:31PM -0600, Glenn Washburn wrote:
> Considering Daniel's feedback on the v2 of this patch series, I've broken up
> patches #1 and #2 and dropped patch #3 from the original series. These changes
> make the output of the ls command a little more like the GNU ls output.
>
> Glenn Washburn
>
> v6:
>  * Rebase onto current master
>  * Change ctx->filename to ctx->filename != NULL in tertiary conditional in
>    patch 5 and add a comment explaining why ctx->filename is needed
>
> v4: Add missing parenthesis
>
> Glenn Washburn (6):
>   commands/ls: Return proper GRUB_ERR_* for functions returning type
>     grub_err_t
>   commands/ls: Merge print_files_long and print_files into print_file
>   commands/ls: Show modification time for file paths
>   commands/ls: Output path for single file arguments given with path
>   commands/ls: Print full paths for file args
>   commands/ls: Add directory header for dir args

For all patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel