Re: Patches for --vnlog support

[Erik Auerswald]

Hi Dima,

On 05.08.22 06:28, Dima Kogan wrote:
> I just dusted this off, and finished it. The patch series is attached.
> The controversial changes (treatment of trailing comments and trailing
> whitespace) are implemented ONLY if --vnlog.

Thanks for adjusting the vnlog patches.

> Let me know if there are issues.

I have looked at the patches, and played a little with "--vnlog"
after applying the patches.  I have a few questions and comments:

1) 0001-vnlog-support.patch

  a) Why do you delete the initialization of the "output_header"
     and "input_header" variables?  That should not be necessary.
     …
     Ah, you move those to text-options.c with extern declaration
     in text-options.h.  But it seems to me as if this is no
     longer required after patch number 3.  If you would move the
     contents of patch 3 into this patch, you could probably omit
     this change.  (I did not test this.)

  b) A small nit: I am not really fond of using the short option
     "-v" as alias for "--vnlog".  A "-v" option often is a short
     form of "--verbose", sometimes short for "--version".
     I would prefer to have no short option for "--vnlog" at all.
     Since long options can be abbreviated, "--vn" would suffice
     to activate "--vnlog" currently.

  c) I would prefer if it you could move the contents of patch
     0003-numerical-field-references-illegal-only-if-vnlog.patch
     into the first patch.  That would ease understanding and
     reviewing the patches.

  d) The macro "IS_COMMENT" results in a semicolon (';') as
     comment character inside vnlog data lines.  That is a
     deviation from the vnlog description given at
     <https://github.com/dkogan/vnlog/>.  I'd expect only
     the Number Sign ('#') to start a comment with "--vnlog".

  e) I'd prefer the contents of patch number 4
     (0004-I-ignore-trailing-comments-trailing-whitespace-ONLY-.patch)
     to be integrated into this patch.  That would make it
     easier to review the patches.

  f) Please omit the deactivated ("if(0)") tests from the patch.

2) 0002-vnlog-command-line-options-checks.patch

  a) This looks OK.  I would not mind if you moved the contents
     into the first patch, but having them in a separate one
     seems OK, too.

3) 0003-numerical-field-references-illegal-only-if-vnlog.patch

  a) I would prefer these changes to be moved into the first
     patch.

4) 0004-I-ignore-trailing-comments-trailing-whitespace-ONLY-.patch

  a) I would prefer these changes to be moved into the first
     patch.

5) 0005-vnlog-added-to-help.patch

  a) I think it would be more accurate to state that "--vnlog"
     requires the use of the field name.  I'd suggest to add
     a new sentence instead of just adding " or --vnlog".

  b) I'd like it if you could add "--vnlog" information to
     doc/datamash.texi as well.

  c) I think it would be nice to add a bit more information
     regarding the vnlog format to the man page (man/datamash.x)
     and the texinfo documentation (doc/datamash.texi).

6) 0006-rmdup-works-with-vnlog.patch

  a) This looks OK.  I would not mind if you moved the contents
     into the first patch, but having them in a separate one
     seems OK, too.

7) 0007-reverse-works-with-vnlog.patch

  a) This looks OK.  I would not mind if you moved the contents
     into the first patch, but having them in a separate one
     seems OK, too.

8) 0008-Tests-for-vnlog.patch

  a) I'd prefer if you removed the commented out tests, I think
     they distract from the actual contents.

  b) I'd prefer if you added a test that verifies that ';'
     does not start an inline comment with "--vnlog".
     Inline comments can be started with a semicolon with
     the current patch set.

  c) I'd prefer if you added tests that verify that ';'
     cannot be used as a replacement for '#' with "--vnlog".
     This does seems to work correctly with the current patch
     set, but I would like to have tests for this difference
     between existing GNU Datamash comment lines and "--vnlog"
     comment and header lines.

Thanks,
Erik