The following commands manifests the problem. In old versions before commit 4762ae9d60 ("column: use libsmartcols for --table"), both of them should output with 2 "|" echo '||' | column -o '|' -s '|' -t echo '|| ' | column -o '|' -s '|' -t Fixes: 4762ae9d60 ("column: use libsmartcols for --table") Signed-off-by: Yousong Zhou <zhouyousong@yunionyun.com> --- tests/expected/column/table-empty-column-at-eol | 1 + tests/expected/column/table-empty-column-at-eol2 | 1 + tests/ts/column/table | 8 ++++++++ text-utils/column.c | 15 ++++++++++----- 4 files changed, 20 insertions(+), 5 deletions(-) create mode 100644 tests/expected/column/table-empty-column-at-eol create mode 100644 tests/expected/column/table-empty-column-at-eol2 diff --git a/tests/expected/column/table-empty-column-at-eol b/tests/expected/column/table-empty-column-at-eol new file mode 100644 index 000000000..948cf947f --- /dev/null +++ b/tests/expected/column/table-empty-column-at-eol @@ -0,0 +1 @@ +| diff --git a/tests/expected/column/table-empty-column-at-eol2 b/tests/expected/column/table-empty-column-at-eol2 new file mode 100644 index 000000000..7c4378506 --- /dev/null +++ b/tests/expected/column/table-empty-column-at-eol2 @@ -0,0 +1 @@ +|| diff --git a/tests/ts/column/table b/tests/ts/column/table index bd1f16f3f..e64dee746 100755 --- a/tests/ts/column/table +++ b/tests/ts/column/table @@ -116,4 +116,12 @@ ts_init_subtest "empty-column" printf ':a:b\n' | $TS_CMD_COLUMN --table --separator ':' --output-separator ':' >> $TS_OUTPUT 2>&1 ts_finalize_subtest +ts_init_subtest "empty-column-at-eol" +printf '|' | $TS_CMD_COLUMN --separator '|' --output-separator '|' --table >> $TS_OUTPUT 2>&1 +ts_finalize_subtest + +ts_init_subtest "empty-column-at-eol2" +printf '||' | $TS_CMD_COLUMN --separator '|' --output-separator '|' --table >> $TS_OUTPUT 2>&1 +ts_finalize_subtest + ts_finalize diff --git a/text-utils/column.c b/text-utils/column.c index 13b39537e..64f1cf7e9 100644 --- a/text-utils/column.c +++ b/text-utils/column.c @@ -169,8 +169,9 @@ static wchar_t *local_wcstok(wchar_t *p, const wchar_t *separator, int greedy, w return strtok_r(p, separator, state); #endif if (!p) { - if (!*state || !**state) + if (!*state) { return NULL; + } p = *state; } result = p; @@ -435,7 +436,7 @@ static int add_line_to_table(struct column_control *ctl, wchar_t *wcs) if (!ctl->tab) init_table(ctl); - while ((wcdata = local_wcstok(wcs, ctl->input_separator, ctl->greedy, &sv))) { + while ((wcdata = local_wcstok(wcs, ctl->input_separator, ctl->greedy, &sv)) || sv) { char *data; if (scols_table_get_ncols(ctl->tab) < n + 1) { @@ -452,9 +453,13 @@ static int add_line_to_table(struct column_control *ctl, wchar_t *wcs) err(EXIT_FAILURE, _("failed to allocate output line")); } - data = wcs_to_mbs(wcdata); - if (!data) - err(EXIT_FAILURE, _("failed to allocate output data")); + if (wcdata) { + data = wcs_to_mbs(wcdata); + if (!data) + err(EXIT_FAILURE, _("failed to allocate output data")); + } else { + data = NULL; + } if (scols_line_refer_data(ln, n, data)) err(EXIT_FAILURE, _("failed to add output data")); n++;
On Thu, 27 Jun 2019 at 17:22, Yousong Zhou <yszhou4tech@gmail.com> wrote: > > The following commands manifests the problem. In old versions before > commit 4762ae9d60 ("column: use libsmartcols for --table"), both of them > should output with 2 "|" > > echo '||' | column -o '|' -s '|' -t > echo '|| ' | column -o '|' -s '|' -t > > Fixes: 4762ae9d60 ("column: use libsmartcols for --table") > Signed-off-by: Yousong Zhou <zhouyousong@yunionyun.com> > --- Hi, do you think this is a real bug and the fix a proper one? should I post a pull request through the github project page? Regards, yousong > tests/expected/column/table-empty-column-at-eol | 1 + > tests/expected/column/table-empty-column-at-eol2 | 1 + > tests/ts/column/table | 8 ++++++++ > text-utils/column.c | 15 ++++++++++----- > 4 files changed, 20 insertions(+), 5 deletions(-) > create mode 100644 tests/expected/column/table-empty-column-at-eol > create mode 100644 tests/expected/column/table-empty-column-at-eol2 > > diff --git a/tests/expected/column/table-empty-column-at-eol b/tests/expected/column/table-empty-column-at-eol > new file mode 100644 > index 000000000..948cf947f > --- /dev/null > +++ b/tests/expected/column/table-empty-column-at-eol > @@ -0,0 +1 @@ > +| > diff --git a/tests/expected/column/table-empty-column-at-eol2 b/tests/expected/column/table-empty-column-at-eol2 > new file mode 100644 > index 000000000..7c4378506 > --- /dev/null > +++ b/tests/expected/column/table-empty-column-at-eol2 > @@ -0,0 +1 @@ > +|| > diff --git a/tests/ts/column/table b/tests/ts/column/table > index bd1f16f3f..e64dee746 100755 > --- a/tests/ts/column/table > +++ b/tests/ts/column/table > @@ -116,4 +116,12 @@ ts_init_subtest "empty-column" > printf ':a:b\n' | $TS_CMD_COLUMN --table --separator ':' --output-separator ':' >> $TS_OUTPUT 2>&1 > ts_finalize_subtest > > +ts_init_subtest "empty-column-at-eol" > +printf '|' | $TS_CMD_COLUMN --separator '|' --output-separator '|' --table >> $TS_OUTPUT 2>&1 > +ts_finalize_subtest > + > +ts_init_subtest "empty-column-at-eol2" > +printf '||' | $TS_CMD_COLUMN --separator '|' --output-separator '|' --table >> $TS_OUTPUT 2>&1 > +ts_finalize_subtest > + > ts_finalize > diff --git a/text-utils/column.c b/text-utils/column.c > index 13b39537e..64f1cf7e9 100644 > --- a/text-utils/column.c > +++ b/text-utils/column.c > @@ -169,8 +169,9 @@ static wchar_t *local_wcstok(wchar_t *p, const wchar_t *separator, int greedy, w > return strtok_r(p, separator, state); > #endif > if (!p) { > - if (!*state || !**state) > + if (!*state) { > return NULL; > + } > p = *state; > } > result = p; > @@ -435,7 +436,7 @@ static int add_line_to_table(struct column_control *ctl, wchar_t *wcs) > if (!ctl->tab) > init_table(ctl); > > - while ((wcdata = local_wcstok(wcs, ctl->input_separator, ctl->greedy, &sv))) { > + while ((wcdata = local_wcstok(wcs, ctl->input_separator, ctl->greedy, &sv)) || sv) { > char *data; > > if (scols_table_get_ncols(ctl->tab) < n + 1) { > @@ -452,9 +453,13 @@ static int add_line_to_table(struct column_control *ctl, wchar_t *wcs) > err(EXIT_FAILURE, _("failed to allocate output line")); > } > > - data = wcs_to_mbs(wcdata); > - if (!data) > - err(EXIT_FAILURE, _("failed to allocate output data")); > + if (wcdata) { > + data = wcs_to_mbs(wcdata); > + if (!data) > + err(EXIT_FAILURE, _("failed to allocate output data")); > + } else { > + data = NULL; > + } > if (scols_line_refer_data(ln, n, data)) > err(EXIT_FAILURE, _("failed to add output data")); > n++;
On Sat, Jul 13, 2019 at 01:30:27PM +0800, Yousong Zhou wrote: > On Thu, 27 Jun 2019 at 17:22, Yousong Zhou <yszhou4tech@gmail.com> wrote: > > > > The following commands manifests the problem. In old versions before > > commit 4762ae9d60 ("column: use libsmartcols for --table"), both of them > > should output with 2 "|" > > > > echo '||' | column -o '|' -s '|' -t > > echo '|| ' | column -o '|' -s '|' -t > > > > Fixes: 4762ae9d60 ("column: use libsmartcols for --table") > > Signed-off-by: Yousong Zhou <zhouyousong@yunionyun.com> > > --- > > Hi, do you think this is a real bug and the fix a proper one? should > I post a pull request through the github project page? Sorry for delay, I had vacation last weekend. I think your patch seems fine at first glance and send it to mailing list is good enough. You do not have to use github PR. Thanks. Karel -- Karel Zak <kzak@redhat.com> http://karelzak.blogspot.com
On Mon, 15 Jul 2019 at 16:00, Karel Zak <kzak@redhat.com> wrote: > > On Sat, Jul 13, 2019 at 01:30:27PM +0800, Yousong Zhou wrote: > > On Thu, 27 Jun 2019 at 17:22, Yousong Zhou <yszhou4tech@gmail.com> wrote: > > > > > > The following commands manifests the problem. In old versions before > > > commit 4762ae9d60 ("column: use libsmartcols for --table"), both of them > > > should output with 2 "|" > > > > > > echo '||' | column -o '|' -s '|' -t > > > echo '|| ' | column -o '|' -s '|' -t > > > > > > Fixes: 4762ae9d60 ("column: use libsmartcols for --table") > > > Signed-off-by: Yousong Zhou <zhouyousong@yunionyun.com> > > > --- > > > > Hi, do you think this is a real bug and the fix a proper one? should > > I post a pull request through the github project page? > > > Sorry for delay, I had vacation last weekend. > > I think your patch seems fine at first glance and send it to mailing > list is good enough. You do not have to use github PR. Thanks. > > Karel Does the patch still look good now ;) yousong > > -- > Karel Zak <kzak@redhat.com> > http://karelzak.blogspot.com
On Thu, 27 Jun 2019 at 10:24, Yousong Zhou <yszhou4tech@gmail.com> wrote: > > The following commands manifests the problem. In old versions before > commit 4762ae9d60 ("column: use libsmartcols for --table"), both of them > should output with 2 "|" > > echo '||' | column -o '|' -s '|' -t > echo '|| ' | column -o '|' -s '|' -t > > Fixes: 4762ae9d60 ("column: use libsmartcols for --table") > Signed-off-by: Yousong Zhou <zhouyousong@yunionyun.com> > --- > tests/expected/column/table-empty-column-at-eol | 1 + > tests/expected/column/table-empty-column-at-eol2 | 1 + > tests/ts/column/table | 8 ++++++++ > text-utils/column.c | 15 ++++++++++----- > 4 files changed, 20 insertions(+), 5 deletions(-) > create mode 100644 tests/expected/column/table-empty-column-at-eol > create mode 100644 tests/expected/column/table-empty-column-at-eol2 > > diff --git a/tests/expected/column/table-empty-column-at-eol b/tests/expected/column/table-empty-column-at-eol > new file mode 100644 > index 000000000..948cf947f > --- /dev/null > +++ b/tests/expected/column/table-empty-column-at-eol > @@ -0,0 +1 @@ > +| > diff --git a/tests/expected/column/table-empty-column-at-eol2 b/tests/expected/column/table-empty-column-at-eol2 > new file mode 100644 > index 000000000..7c4378506 > --- /dev/null > +++ b/tests/expected/column/table-empty-column-at-eol2 > @@ -0,0 +1 @@ > +|| > diff --git a/tests/ts/column/table b/tests/ts/column/table > index bd1f16f3f..e64dee746 100755 > --- a/tests/ts/column/table > +++ b/tests/ts/column/table > @@ -116,4 +116,12 @@ ts_init_subtest "empty-column" > printf ':a:b\n' | $TS_CMD_COLUMN --table --separator ':' --output-separator ':' >> $TS_OUTPUT 2>&1 > ts_finalize_subtest > > +ts_init_subtest "empty-column-at-eol" > +printf '|' | $TS_CMD_COLUMN --separator '|' --output-separator '|' --table >> $TS_OUTPUT 2>&1 > +ts_finalize_subtest > + > +ts_init_subtest "empty-column-at-eol2" > +printf '||' | $TS_CMD_COLUMN --separator '|' --output-separator '|' --table >> $TS_OUTPUT 2>&1 > +ts_finalize_subtest > + > ts_finalize > diff --git a/text-utils/column.c b/text-utils/column.c > index 13b39537e..64f1cf7e9 100644 > --- a/text-utils/column.c > +++ b/text-utils/column.c > @@ -169,8 +169,9 @@ static wchar_t *local_wcstok(wchar_t *p, const wchar_t *separator, int greedy, w > return strtok_r(p, separator, state); > #endif > if (!p) { > - if (!*state || !**state) > + if (!*state) { > return NULL; > + } This would be better without unnecessary curly braces. > p = *state; > } > result = p; > @@ -435,7 +436,7 @@ static int add_line_to_table(struct column_control *ctl, wchar_t *wcs) > if (!ctl->tab) > init_table(ctl); > > - while ((wcdata = local_wcstok(wcs, ctl->input_separator, ctl->greedy, &sv))) { > + while ((wcdata = local_wcstok(wcs, ctl->input_separator, ctl->greedy, &sv)) || sv) { I don't think '|| sv' is needed. > char *data; > > if (scols_table_get_ncols(ctl->tab) < n + 1) { > @@ -452,9 +453,13 @@ static int add_line_to_table(struct column_control *ctl, wchar_t *wcs) > err(EXIT_FAILURE, _("failed to allocate output line")); > } > > - data = wcs_to_mbs(wcdata); > - if (!data) > - err(EXIT_FAILURE, _("failed to allocate output data")); > + if (wcdata) { > + data = wcs_to_mbs(wcdata); > + if (!data) > + err(EXIT_FAILURE, _("failed to allocate output data")); > + } else { > + data = NULL; > + } Another pair of unnecessary braces. > if (scols_line_refer_data(ln, n, data)) > err(EXIT_FAILURE, _("failed to add output data")); > n++; If resubmitted feel free to add the following. Even if not resubmitted with fixes; looks pretty good. Reviewed-by: Sami Kerola <kerolasa@iki.fi> -- Sami Kerola http://www.iki.fi/kerolasa/
Hi Sami, On Sun, 28 Jul 2019 at 02:49, Sami Kerola <kerolasa@iki.fi> wrote: > > On Thu, 27 Jun 2019 at 10:24, Yousong Zhou <yszhou4tech@gmail.com> wrote: > > > > The following commands manifests the problem. In old versions before > > commit 4762ae9d60 ("column: use libsmartcols for --table"), both of them > > should output with 2 "|" > > > > echo '||' | column -o '|' -s '|' -t > > echo '|| ' | column -o '|' -s '|' -t > > > > Fixes: 4762ae9d60 ("column: use libsmartcols for --table") > > Signed-off-by: Yousong Zhou <zhouyousong@yunionyun.com> > > --- > > tests/expected/column/table-empty-column-at-eol | 1 + > > tests/expected/column/table-empty-column-at-eol2 | 1 + > > tests/ts/column/table | 8 ++++++++ > > text-utils/column.c | 15 ++++++++++----- > > 4 files changed, 20 insertions(+), 5 deletions(-) > > create mode 100644 tests/expected/column/table-empty-column-at-eol > > create mode 100644 tests/expected/column/table-empty-column-at-eol2 > > > > diff --git a/tests/expected/column/table-empty-column-at-eol b/tests/expected/column/table-empty-column-at-eol > > new file mode 100644 > > index 000000000..948cf947f > > --- /dev/null > > +++ b/tests/expected/column/table-empty-column-at-eol > > @@ -0,0 +1 @@ > > +| > > diff --git a/tests/expected/column/table-empty-column-at-eol2 b/tests/expected/column/table-empty-column-at-eol2 > > new file mode 100644 > > index 000000000..7c4378506 > > --- /dev/null > > +++ b/tests/expected/column/table-empty-column-at-eol2 > > @@ -0,0 +1 @@ > > +|| > > diff --git a/tests/ts/column/table b/tests/ts/column/table > > index bd1f16f3f..e64dee746 100755 > > --- a/tests/ts/column/table > > +++ b/tests/ts/column/table > > @@ -116,4 +116,12 @@ ts_init_subtest "empty-column" > > printf ':a:b\n' | $TS_CMD_COLUMN --table --separator ':' --output-separator ':' >> $TS_OUTPUT 2>&1 > > ts_finalize_subtest > > > > +ts_init_subtest "empty-column-at-eol" > > +printf '|' | $TS_CMD_COLUMN --separator '|' --output-separator '|' --table >> $TS_OUTPUT 2>&1 > > +ts_finalize_subtest > > + > > +ts_init_subtest "empty-column-at-eol2" > > +printf '||' | $TS_CMD_COLUMN --separator '|' --output-separator '|' --table >> $TS_OUTPUT 2>&1 > > +ts_finalize_subtest > > + > > ts_finalize > > diff --git a/text-utils/column.c b/text-utils/column.c > > index 13b39537e..64f1cf7e9 100644 > > --- a/text-utils/column.c > > +++ b/text-utils/column.c > > @@ -169,8 +169,9 @@ static wchar_t *local_wcstok(wchar_t *p, const wchar_t *separator, int greedy, w > > return strtok_r(p, separator, state); > > #endif > > if (!p) { > > - if (!*state || !**state) > > + if (!*state) { > > return NULL; > > + } > > This would be better without unnecessary curly braces. > > > p = *state; > > } > > result = p; > > @@ -435,7 +436,7 @@ static int add_line_to_table(struct column_control *ctl, wchar_t *wcs) > > if (!ctl->tab) > > init_table(ctl); > > > > - while ((wcdata = local_wcstok(wcs, ctl->input_separator, ctl->greedy, &sv))) { > > + while ((wcdata = local_wcstok(wcs, ctl->input_separator, ctl->greedy, &sv)) || sv) { > > I don't think '|| sv' is needed. Thanks, you are right. Now I recall it it must be a leftover before I removed the "&& !**data" check in local_wcstok() > > > char *data; > > > > if (scols_table_get_ncols(ctl->tab) < n + 1) { > > @@ -452,9 +453,13 @@ static int add_line_to_table(struct column_control *ctl, wchar_t *wcs) > > err(EXIT_FAILURE, _("failed to allocate output line")); > > } > > > > - data = wcs_to_mbs(wcdata); > > - if (!data) > > - err(EXIT_FAILURE, _("failed to allocate output data")); > > + if (wcdata) { > > + data = wcs_to_mbs(wcdata); > > + if (!data) > > + err(EXIT_FAILURE, _("failed to allocate output data")); > > + } else { > > + data = NULL; > > + } > > Another pair of unnecessary braces. > > > if (scols_line_refer_data(ln, n, data)) > > err(EXIT_FAILURE, _("failed to add output data")); > > n++; > > If resubmitted feel free to add the following. Even if not resubmitted > with fixes; looks pretty good. > > Reviewed-by: Sami Kerola <kerolasa@iki.fi> Just sent a v2 patch. It's simplified to only 1 line change. Thank you for the review ;) Regards, yousong > > -- > Sami Kerola > http://www.iki.fi/kerolasa/