[PATCH v4 0/6] More ls improvements

[PATCH v4 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

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 | 93 ++++++++++++++++++++---------------------
 1 file changed, 46 insertions(+), 47 deletions(-)

Range-diff against v3:
1:  9086cccd8a0f = 1:  9086cccd8a0f commands/ls: Return proper GRUB_ERR_* for functions returning type grub_err_t
2:  3192dc2471d0 = 2:  3192dc2471d0 commands/ls: Merge print_files_long and print_files into print_file
3:  fa7adbeee4e0 ! 3:  889e1e05a355 commands/ls: Show modification time for file paths
    @@ grub-core/commands/ls.c: print_file (const char *filename, const struct grub_dir
        if ((! ctx->all) && (filename[0] == '.'))
          return 0;
      
    -+  if (ctx->filename != NULL) && (grub_strcmp (filename, ctx->filename) != 0)
    ++  if ((ctx->filename != NULL) && (grub_strcmp (filename, ctx->filename) != 0))
     +    return 0;
     +
        if (! ctx->longlist)
4:  5a1618235b6e = 4:  d63e007f635b commands/ls: Output path for single file arguments given with path
5:  026f37b35adb = 5:  39adcb21e320 commands/ls: Print full paths for file args
6:  5bf39d658422 ! 6:  0ebbfd3b0e15 commands/ls: Add directory header for dir args
    @@ grub-core/commands/ls.c: struct grub_ls_list_files_ctx
      
      /* Helper for grub_ls_list_files.  */
     @@ grub-core/commands/ls.c: print_file (const char *filename, const struct grub_dirhook_info *info,
    -   if (ctx->filename != NULL) && (grub_strcmp (filename, ctx->filename) != 0)
    +   if ((ctx->filename != NULL) && (grub_strcmp (filename, ctx->filename) != 0))
          return 0;
      
     +  if (ctx->print_dirhdr)
-- 
2.34.1


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

[PATCH v4 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 6a1c7f5d3626..9e69b19371d5 100644
--- a/grub-core/commands/ls.c
+++ b/grub-core/commands/ls.c
@@ -246,7 +246,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);
@@ -268,7 +267,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
@@ -284,7 +283,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 v4 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 9e69b19371d5..43ee6aca085a 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] != '/')
@@ -247,10 +239,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 v4 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 | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index 43ee6aca085a..ed296a536554 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,26 +225,19 @@ 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, '/') + 1;
-	  ctx.dirname = grub_strndup (dirname, p - dirname);
+	  /* PATH might be a regular file.  */
+	  ctx.filename = grub_strrchr (dirname, '/') + 1;
+	  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 v4 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 ed296a536554..e33c16158d63 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 v4 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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
index e33c16158d63..384d3e3cede8 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,10 @@ 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 ? "/" : "");
+  grub_printf ("%s%s\n", (ctx->filename) ? 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 v4 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 384d3e3cede8..ce128d0c84a6 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)
@@ -173,7 +180,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;
@@ -221,7 +228,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);
@@ -236,6 +244,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, '/') + 1;
 	  ctx.dirname = grub_strndup (dirname, ctx.filename - dirname);
 	  if (ctx.dirname == NULL)
@@ -271,8 +280,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 v4 5/6] commands/ls: Print full paths for file args

[Daniel Kiper]

On Mon, Jan 06, 2025 at 01:02:43AM -0600, Glenn Washburn wrote:
> 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 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
> index e33c16158d63..384d3e3cede8 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,10 @@ 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 ? "/" : "");
> +  grub_printf ("%s%s\n", (ctx->filename) ? pathname : filename,

"(pathname != NULL) ? pathname : filename" would not be more natural/correct?

Otherwise patches LGTM...

Daniel

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

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

[Glenn Washburn]

On Mon, 24 Feb 2025 18:14:59 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Jan 06, 2025 at 01:02:43AM -0600, Glenn Washburn wrote:
> > 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 | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
> > index e33c16158d63..384d3e3cede8 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,10 @@ 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 ? "/" : "");
> > +  grub_printf ("%s%s\n", (ctx->filename) ? pathname : filename,
> 
> "(pathname != NULL) ? pathname : filename" would not be more natural/correct?

I think this is equivalent. Yes, this does seem more intuitive. Do you
want me to update and send a v5?

Glenn

> 
> Otherwise patches LGTM...
> 
> Daniel

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

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

[Daniel Kiper]

On Thu, Feb 27, 2025 at 12:28:31AM -0600, Glenn Washburn wrote:
> On Mon, 24 Feb 2025 18:14:59 +0100 Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Mon, Jan 06, 2025 at 01:02:43AM -0600, Glenn Washburn wrote:
> > > 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 | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
> > > index e33c16158d63..384d3e3cede8 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,10 @@ 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 ? "/" : "");
> > > +  grub_printf ("%s%s\n", (ctx->filename) ? pathname : filename,
> >
> > "(pathname != NULL) ? pathname : filename" would not be more natural/correct?
>
> I think this is equivalent. Yes, this does seem more intuitive. Do you
> want me to update and send a v5?

Yes, please! And feel free to add my RB to all patches...

Daniel

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

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

[Glenn Washburn]

On Thu, 27 Feb 2025 17:27:29 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Feb 27, 2025 at 12:28:31AM -0600, Glenn Washburn wrote:
> > On Mon, 24 Feb 2025 18:14:59 +0100 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > On Mon, Jan 06, 2025 at 01:02:43AM -0600, Glenn Washburn wrote:
> > > > 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 | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
> > > > index e33c16158d63..384d3e3cede8 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,10 @@ 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 ? "/" : "");
> > > > +  grub_printf ("%s%s\n", (ctx->filename) ? pathname : filename,
> > >
> > > "(pathname != NULL) ? pathname : filename" would not be more natural/correct?
> >
> > I think this is equivalent. Yes, this does seem more intuitive. Do you
> > want me to update and send a v5?

Actually, I'm wrong here. I should've run the filesystem tests instead
of just trying to reason through the code. This is not equivalent and
the way I had it originally was correct. The reason is that we do not
want to print a full path for files that are listed from directory
recursion. (pathname != NULL) is true when we are printing a listing of
a file, but that listing could be because a full path to the file was
given as an argument to ls or because ls was listing the contents of
the files parent directory. In the former, ctx->filename is set and we
want to print the full path. In the latter, ctx->filename is not set
and we do not want to print the full path because the full path to the
parent directory will have been printed earlier.

So ignore the v5, I'll send a v6 reverting this change.

Glenn

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

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

[Daniel Kiper]

On Sat, Mar 01, 2025 at 03:54:25PM -0600, Glenn Washburn wrote:
> On Thu, 27 Feb 2025 17:27:29 +0100 > Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Thu, Feb 27, 2025 at 12:28:31AM -0600, Glenn Washburn wrote:
> > > On Mon, 24 Feb 2025 18:14:59 +0100 Daniel Kiper <dkiper@net-space.pl> wrote:
> > > > On Mon, Jan 06, 2025 at 01:02:43AM -0600, Glenn Washburn wrote:
> > > > > 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 | 8 +++++---
> > > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/grub-core/commands/ls.c b/grub-core/commands/ls.c
> > > > > index e33c16158d63..384d3e3cede8 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,10 @@ 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 ? "/" : "");
> > > > > +  grub_printf ("%s%s\n", (ctx->filename) ? pathname : filename,
> > > >
> > > > "(pathname != NULL) ? pathname : filename" would not be more natural/correct?
> > >
> > > I think this is equivalent. Yes, this does seem more intuitive. Do you
> > > want me to update and send a v5?
>
> Actually, I'm wrong here. I should've run the filesystem tests instead
> of just trying to reason through the code. This is not equivalent and
> the way I had it originally was correct. The reason is that we do not
> want to print a full path for files that are listed from directory
> recursion. (pathname != NULL) is true when we are printing a listing of
> a file, but that listing could be because a full path to the file was
> given as an argument to ls or because ls was listing the contents of
> the files parent directory. In the former, ctx->filename is set and we
> want to print the full path. In the latter, ctx->filename is not set
> and we do not want to print the full path because the full path to the
> parent directory will have been printed earlier.

So, I think this grub_printf() call begs for a few lines of comment then.
Otherwise it is confusing...

> So ignore the v5, I'll send a v6 reverting this change.

OK...

Daniel

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