[PATCH] column(1): -r to right justify

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] column(1): -r to right justify

Job Snijders-2
Dear all,

Following some back and forth on how disklabel output should be
formatted, I proposed to Kenneth to extend the column(1) utility. All that
was missing is the ability to right justify. I've longed for this
feature for a while: I often use 'column -t' to prettify data coming
from an awk pipeline.

Example:

job@vurt ~$ netstat -r | column -t -r | tail -5
    ff01::%iwm0/32  fe80::4708:d2be:9a     Um     0        3      -     4     iwm0  
     ff01::%lo0/32           localhost     Um     0        1  32768     4      lo0  
         ff02::/16           localhost   UGRS     0        1  32768     8      lo0  
    ff02::%iwm0/32  fe80::4708:d2be:9a     Um     0        3      -     4     iwm0  
     ff02::%lo0/32           localhost     Um     0        1  32768     4      lo0  


Patch courtesy of Kenneth R Westerback. OK?

Index: column.1
===================================================================
RCS file: /cvs/src/usr.bin/column/column.1,v
retrieving revision 1.18
diff -u -p -r1.18 column.1
--- column.1 24 Oct 2016 13:53:05 -0000 1.18
+++ column.1 4 Jul 2018 10:27:54 -0000
@@ -40,6 +40,7 @@
 .Nm column
 .Op Fl tx
 .Op Fl c Ar columns
+.Op Fl r Op Ar list
 .Op Fl s Ar sep
 .Op Ar
 .Sh DESCRIPTION
@@ -66,6 +67,16 @@ The options are as follows:
 Output is formatted for a display
 .Ar columns
 wide.
+.It Fl r Op Ar list
+Table mode will right justify the column contents for the
+specified columns.
+.Ar list
+is a list of comma separated column numbers or column ranges.
+Column numbers start at 1.
+The list must not contain whitespace.
+If no
+.Ar list
+is provided then all columns will be right justified.
 .It Fl s Ar sep
 Specify a set of characters to delimit columns for the
 .Fl t
Index: column.c
===================================================================
RCS file: /cvs/src/usr.bin/column/column.c,v
retrieving revision 1.26
diff -u -p -r1.26 column.c
--- column.c 22 Jun 2018 12:27:00 -0000 1.26
+++ column.c 4 Jul 2018 10:28:00 -0000
@@ -47,7 +47,8 @@
 void  c_columnate(void);
 void *ereallocarray(void *, size_t, size_t);
 void  input(FILE *);
-void  maketbl(void);
+int   rightjustify(const char *, const int);
+void  maketbl(const int, const char *);
 void  print(void);
 void  r_columnate(void);
 __dead void usage(void);
@@ -69,8 +70,8 @@ main(int argc, char *argv[])
 {
  struct winsize win;
  FILE *fp;
- int ch, tflag, xflag;
- char *p;
+ int ch, rflag, tflag, xflag;
+ char *p, *rcols;
  const char *errstr;
 
  setlocale(LC_CTYPE, "");
@@ -87,14 +88,19 @@ main(int argc, char *argv[])
  if (pledge("stdio rpath", NULL) == -1)
  err(1, "pledge");
 
- tflag = xflag = 0;
- while ((ch = getopt(argc, argv, "c:s:tx")) != -1) {
+ rcols = NULL;
+ rflag = 0; tflag = xflag = 0;
+ while ((ch = getopt(argc, argv, "c:r::s:tx")) != -1) {
  switch(ch) {
  case 'c':
  termwidth = strtonum(optarg, 1, INT_MAX, &errstr);
  if (errstr != NULL)
  errx(1, "%s: %s", errstr, optarg);
  break;
+ case 'r':
+ rflag = 1;
+ rcols = optarg;
+ break;
  case 's':
  if ((separator = reallocarray(NULL, strlen(optarg) + 1,
     sizeof(*separator))) == NULL)
@@ -139,7 +145,7 @@ main(int argc, char *argv[])
  return eval;
 
  if (tflag)
- maketbl();
+ maketbl(rflag, rcols);
  else if (*maxwidths >= termwidth)
  print();
  else if (xflag)
@@ -207,18 +213,69 @@ print(void)
  puts(table[row]->content);
 }
 
+int
+rightjustify(const char *rcols, const int col)
+{
+ const char *errstr;
+ char c, *num, *temp;
+ long long ch, rangestart;
+ unsigned int i;
+
+ if (rcols == NULL)
+ return 1;
+
+ temp = strdup(rcols);
+ num = temp;
+ rangestart = -1;
+
+ c = 0;
+ for (i = 0; i <= strlen(rcols); i++) {
+ ch = temp[i];
+ if (ch == ',' || ch == '-')
+ temp[i] = '\0';
+ if (temp[i] != '\0')
+ continue;
+
+ c = strtonum(num, 1, INT_MAX, &errstr);
+ if (errstr != NULL)
+ break;
+ c--; /* Users are 1-based. Reality is 0-based. */
+
+ if (c == col)
+ goto found;
+ if (ch == '-') {
+ rangestart = c;
+ } else if ((ch == ',' || ch == '\0') && rangestart != -1) {
+ if (rangestart <= col && c >= col)
+ goto found;
+ rangestart = -1;
+ }
+ num = temp + i + 1;
+ }
+
+ free(temp);
+ return 0;
+found:
+ free(temp);
+ return 1;
+}
 
 void
-maketbl(void)
+maketbl(const int rflag, const char *rcols)
 {
  struct field **row;
  int col;
 
  for (row = table; entries--; ++row) {
- for (col = 0; (*row)[col + 1].content != NULL; ++col)
- printf("%s%*s  ", (*row)[col].content,
-    maxwidths[col] - (*row)[col].width, "");
- puts((*row)[col].content);
+ for (col = 0; (*row)[col].content != NULL; ++col) {
+ if (rflag && rightjustify(rcols, col))
+ printf("%*s  ", maxwidths[col],
+    (*row)[col].content);
+ else
+ printf("%-*s  ", maxwidths[col],
+    (*row)[col].content);
+ }
+ putchar('\n');
  }
 }

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] column(1): -r to right justify

Sebastian Benoit-3
Job Snijders([hidden email]) on 2018.07.04 14:09:56 +0200:
> Dear all,
>
> Following some back and forth on how disklabel output should be
> formatted, I proposed to Kenneth to extend the column(1) utility. All that
> was missing is the ability to right justify. I've longed for this
> feature for a while: I often use 'column -t' to prettify data coming
> from an awk pipeline.

funny, i use an awk script for that ;)

code reads ok.

as to -r:

Linux:
there is -R mentioned in http://man7.org/linux/man-pages/man1/column.1.html

    -R, --table-right columns
        Right align text in the specified columns.

altough i dont know since when thats supported, because i could notfind that
on any linux system i have access to and the util-linux change log does not
mention it.

They use -R because their -r is

       -r, --tree column
              Specify column to use tree-like output. Note that the circular
              dependencies and another anomalies in child and parent
              relation are silently ignored.

FreeBSD doesn't have a right align in column.

So i think, if a new feature and flag is acceptable, it should be -R

/Benno

> Example:
>
> job@vurt ~$ netstat -r | column -t -r | tail -5
>     ff01::%iwm0/32  fe80::4708:d2be:9a     Um     0        3      -     4     iwm0  
>      ff01::%lo0/32           localhost     Um     0        1  32768     4      lo0  
>          ff02::/16           localhost   UGRS     0        1  32768     8      lo0  
>     ff02::%iwm0/32  fe80::4708:d2be:9a     Um     0        3      -     4     iwm0  
>      ff02::%lo0/32           localhost     Um     0        1  32768     4      lo0  
>
>
> Patch courtesy of Kenneth R Westerback. OK?
>
> Index: column.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/column/column.1,v
> retrieving revision 1.18
> diff -u -p -r1.18 column.1
> --- column.1 24 Oct 2016 13:53:05 -0000 1.18
> +++ column.1 4 Jul 2018 10:27:54 -0000
> @@ -40,6 +40,7 @@
>  .Nm column
>  .Op Fl tx
>  .Op Fl c Ar columns
> +.Op Fl r Op Ar list
>  .Op Fl s Ar sep
>  .Op Ar
>  .Sh DESCRIPTION
> @@ -66,6 +67,16 @@ The options are as follows:
>  Output is formatted for a display
>  .Ar columns
>  wide.
> +.It Fl r Op Ar list
> +Table mode will right justify the column contents for the
> +specified columns.
> +.Ar list
> +is a list of comma separated column numbers or column ranges.
> +Column numbers start at 1.
> +The list must not contain whitespace.
> +If no
> +.Ar list
> +is provided then all columns will be right justified.
>  .It Fl s Ar sep
>  Specify a set of characters to delimit columns for the
>  .Fl t
> Index: column.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/column/column.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 column.c
> --- column.c 22 Jun 2018 12:27:00 -0000 1.26
> +++ column.c 4 Jul 2018 10:28:00 -0000
> @@ -47,7 +47,8 @@
>  void  c_columnate(void);
>  void *ereallocarray(void *, size_t, size_t);
>  void  input(FILE *);
> -void  maketbl(void);
> +int   rightjustify(const char *, const int);
> +void  maketbl(const int, const char *);
>  void  print(void);
>  void  r_columnate(void);
>  __dead void usage(void);
> @@ -69,8 +70,8 @@ main(int argc, char *argv[])
>  {
>   struct winsize win;
>   FILE *fp;
> - int ch, tflag, xflag;
> - char *p;
> + int ch, rflag, tflag, xflag;
> + char *p, *rcols;
>   const char *errstr;
>  
>   setlocale(LC_CTYPE, "");
> @@ -87,14 +88,19 @@ main(int argc, char *argv[])
>   if (pledge("stdio rpath", NULL) == -1)
>   err(1, "pledge");
>  
> - tflag = xflag = 0;
> - while ((ch = getopt(argc, argv, "c:s:tx")) != -1) {
> + rcols = NULL;
> + rflag = 0; tflag = xflag = 0;
> + while ((ch = getopt(argc, argv, "c:r::s:tx")) != -1) {
>   switch(ch) {
>   case 'c':
>   termwidth = strtonum(optarg, 1, INT_MAX, &errstr);
>   if (errstr != NULL)
>   errx(1, "%s: %s", errstr, optarg);
>   break;
> + case 'r':
> + rflag = 1;
> + rcols = optarg;
> + break;
>   case 's':
>   if ((separator = reallocarray(NULL, strlen(optarg) + 1,
>      sizeof(*separator))) == NULL)
> @@ -139,7 +145,7 @@ main(int argc, char *argv[])
>   return eval;
>  
>   if (tflag)
> - maketbl();
> + maketbl(rflag, rcols);
>   else if (*maxwidths >= termwidth)
>   print();
>   else if (xflag)
> @@ -207,18 +213,69 @@ print(void)
>   puts(table[row]->content);
>  }
>  
> +int
> +rightjustify(const char *rcols, const int col)
> +{
> + const char *errstr;
> + char c, *num, *temp;
> + long long ch, rangestart;
> + unsigned int i;
> +
> + if (rcols == NULL)
> + return 1;
> +
> + temp = strdup(rcols);
> + num = temp;
> + rangestart = -1;
> +
> + c = 0;
> + for (i = 0; i <= strlen(rcols); i++) {
> + ch = temp[i];
> + if (ch == ',' || ch == '-')
> + temp[i] = '\0';
> + if (temp[i] != '\0')
> + continue;
> +
> + c = strtonum(num, 1, INT_MAX, &errstr);
> + if (errstr != NULL)
> + break;
> + c--; /* Users are 1-based. Reality is 0-based. */
> +
> + if (c == col)
> + goto found;
> + if (ch == '-') {
> + rangestart = c;
> + } else if ((ch == ',' || ch == '\0') && rangestart != -1) {
> + if (rangestart <= col && c >= col)
> + goto found;
> + rangestart = -1;
> + }
> + num = temp + i + 1;
> + }
> +
> + free(temp);
> + return 0;
> +found:
> + free(temp);
> + return 1;
> +}
>  
>  void
> -maketbl(void)
> +maketbl(const int rflag, const char *rcols)
>  {
>   struct field **row;
>   int col;
>  
>   for (row = table; entries--; ++row) {
> - for (col = 0; (*row)[col + 1].content != NULL; ++col)
> - printf("%s%*s  ", (*row)[col].content,
> -    maxwidths[col] - (*row)[col].width, "");
> - puts((*row)[col].content);
> + for (col = 0; (*row)[col].content != NULL; ++col) {
> + if (rflag && rightjustify(rcols, col))
> + printf("%*s  ", maxwidths[col],
> +    (*row)[col].content);
> + else
> + printf("%-*s  ", maxwidths[col],
> +    (*row)[col].content);
> + }
> + putchar('\n');
>   }
>  }
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] column(1): -r to right justify

Ingo Schwarze
In reply to this post by Job Snijders-2
Hi Job,

Job Snijders wrote on Wed, Jul 04, 2018 at 02:09:56PM +0200:

> Following some back and forth on how disklabel output should be
> formatted, I proposed to Kenneth to extend the column(1) utility.
> All that was missing is the ability to right justify.

I'm not totally sure we should add the feature, but maybe you are
right that it's useful enough, and column(1) still remains simple
enough.  We have to pay attention not to turn it into a pig
resembling rs(1), though.

> Patch courtesy of Kenneth R Westerback. OK?

No.  I see at least two aspects of the patch that are clearly
not yet OK, see inline.


> Index: column.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/column/column.1,v
> retrieving revision 1.18
> diff -u -p -r1.18 column.1
> --- column.1 24 Oct 2016 13:53:05 -0000 1.18
> +++ column.1 4 Jul 2018 10:27:54 -0000
> @@ -40,6 +40,7 @@
>  .Nm column
>  .Op Fl tx
>  .Op Fl c Ar columns
> +.Op Fl r Op Ar list

This is unacceptable.  A few programs exist that take options with
optional arguments, but we certainly do not want to add new ones
because their syntax and semantics is inherently ambiguous and error
prone.  POSIX also discourages it in general.

An option can either take no argument or require an argument,
so make a decision in that respect.  If you opt for requiring
an argument, then you can use a cut(1)-like syntax:

  cut -b 2-3    # bytes 2 and 3
  cut -b 2-     # all but the first byte
  cut -b -2     # the first two bytes
  cut -b 1-     # all bytes, not cutting away anything
  cut -b -2,4-  # all but the third byte

[...]
> Index: column.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/column/column.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 column.c
> --- column.c 22 Jun 2018 12:27:00 -0000 1.26
> +++ column.c 4 Jul 2018 10:28:00 -0000
[...]
> + rflag = 0; tflag = xflag = 0;

That looks inconsistent.  s/0; //

[...]

> @@ -207,18 +213,69 @@ print(void)
>   puts(table[row]->content);
>  }
>  
> +int
> +rightjustify(const char *rcols, const int col)
> +{
> + const char *errstr;
> + char c, *num, *temp;
> + long long ch, rangestart;
> + unsigned int i;
> +
> + if (rcols == NULL)
> + return 1;
> +
> + temp = strdup(rcols);
> + num = temp;
> + rangestart = -1;
> +
> + c = 0;
> + for (i = 0; i <= strlen(rcols); i++) {
> + ch = temp[i];
> + if (ch == ',' || ch == '-')
> + temp[i] = '\0';
> + if (temp[i] != '\0')
> + continue;
> +
> + c = strtonum(num, 1, INT_MAX, &errstr);

This is not acceptable either.

Your algorithm is of the order

  O(rows) * O(columns) * O(strlen(rcols))

and then calling strdup(3) on the quadratic level and strtonum(3)
inside the cubic loop.  Even though performance optimization is
often over-valued, that is not an excuse for selecting a basic
algorithm with an excessively high algorithmic order.

You really have to do argument parsing once before entering the
quadratic main loop rather than redoing it from scratch for each
and every table cell.

[...]
>  void
> -maketbl(void)
> +maketbl(const int rflag, const char *rcols)
>  {
>   struct field **row;
>   int col;
>  
>   for (row = table; entries--; ++row) {
> - for (col = 0; (*row)[col + 1].content != NULL; ++col)
[...]
> + for (col = 0; (*row)[col].content != NULL; ++col) {

Here, you are losing the feature that column(1) refrains from
printing trailing blanks.

I'm putting the audit on hold at this points because fixing the
issues found so far is going to change the patch substantially.

Yours,
  Ingo

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] column(1): -r to right justify

Ingo Schwarze
An afterthought...

Ingo Schwarze wrote on Sun, Jul 08, 2018 at 08:22:34PM +0200:

> If you opt for requiring an argument,
> then you can use a cut(1)-like syntax:

Alternatively, you can use a syntax similar to tbl(7) "Layout" lines:

  rrll

which allows later extensions like

  r|rclnl

That's much more powerful, not harder to understand for the user,
and probably also easier to implement.

Yours,
  Ingo