Re: Increasing FreeBSD compatibility in mtree

[christos%astron.com@localhost (Christos Zoulas)]

In article <20121219184352.GC38760%lor.one-eyed-alien.net@localhost>,
Brooks Davis  <brooks%freebsd.org@localhost> wrote:
>
>Here's an improved patch that disables iflag and documents this
>behavior.  It also adds a COMPATIBILITY section which is incomplete
>pending resolution of the rest of the issues.
>
>
>/* FreeBSD compat, remove after 2018. */
>
>Index: mtree.c
>===================================================================
>--- mtree.c    (revision 244436)
>+++ mtree.c    (working copy)
>@@ -236,6 +236,12 @@
>       if ((cflag && Cflag) || (cflag && Dflag) || (Cflag && Dflag))
>               mtree_err("-c, -C and -D flags are mutually exclusive");
> 
>+      if (cflag && iflag) {
>+              warnx("-c and -i specified, setting -j and unsetting -i");
>+              iflag = 0;
>+              jflag = 1;
>+      }
>+
>       if (iflag && mflag)
>               mtree_err("-i and -m flags are mutually exclusive");

Yes, that is fine, but read on.

>Index: mtree.8
>===================================================================
>--- mtree.8    (revision 244436)
>+++ mtree.8    (working copy)
>@@ -166,6 +166,16 @@
> If no inclusion list is provided, the default is to display all files.
> .It Fl i
> If specified, set the schg and/or sappnd flags.
>+For compatibility with
>+.Fx
>+if the
>+.Fl c
>+and
>+.Fl i
>+option are both passed,
>+then the
>+.Fl j
>+option is implied and a warning is emitted.
> .It Fl j
> Indent the output 4 spaces each time a directory level is descended when
> creating a specification with the
>@@ -690,6 +700,37 @@
> or
> .Fl u
> to create directory hierarchies for, for example, distributions.
>+.Sh COMPATIBILITY
>+This version of the
>+.Nm utility differs from the version in
>+.Fx 9
>+and earlier in several ways:
>+.Bl -bullet -compact
>+.It
>+When the
>+.Fl c
>+option is specified a final ``..'' is not emitted to match the top
>+directory.
>+Files made to match by appending ``..\\n\\n''.
>+.It
>+The
>+.Fl U
>+and
>+.Fl u
>+options do not set the modification time or the schg and/or sappnd
>+file flags unless the
>+.Fl t
>+and
>+.Fl i
>+options are also passed.
>+.It
>+The
>+.Fl j
>+option is equivalent to the
>+.Fx
>+.Fl i
>+option and should be used in its place.
>+.El
> .Sh SEE ALSO
> .Xr chflags 1 ,
> .Xr chgrp 1 ,

The more I read this man page, the more difficult it is for me to
understand.  I think we are trying to achieve both compatibility
with the current versions of mtree, and make both the NetBSD
and FreeBSD versions similar/identical, while keeping the more
rational and informative output.

These are all conflicting goals and we are introducing extra
complexity to achieve them. This can cannibalize the future
maintenability and usability of the utility.

Why don't we do something like have a global flag/environment option
to preserve the current behavior, and then design the new behavior
as we want it to be without being constrained by compatibility?
Then at least hopefully we can keep the compat code in one place,
and remove them after the next release. Or even do it based on the
command name (omtree).

>Hmm, that's true.  The final .. looks more complete in -j output, but is
>basically a no op in practice and trivial to add after the fact which
>leads me to think I should just document the difference and move on.
>
>One compromise approach might be to enable it when -j is specified since
>that's a new option on NetBSD.  That could be accomplished as follows:
>
>Index: create.c
>===================================================================
>RCS file: /cvsroot/src/usr.sbin/mtree/create.c,v
>retrieving revision 1.67
>diff -u -r1.67 create.c
>--- create.c   15 Dec 2012 01:24:40 -0000      1.67
>+++ create.c   19 Dec 2012 18:20:24 -0000
>@@ -143,12 +143,12 @@
>                       statf(indent, p);
>                       break;
>               case FTS_DP:
>-                      if (p->fts_level > 0) {
>+                      if (p->fts_level > 0)
>                               if (!nflag)
>                                       printf("%*s# %s\n", indent, "",
>                                           p->fts_path);
>+                      if (p->fts_level > 0 || jflag)
>                               printf("%*s..\n\n", indent, "");
>-                      }
>                       break;
>               case FTS_DNR:
>               case FTS_ERR:
>

I don't like this much because it is weird to have the output change because
an unrelated option is set.

>> >>  - size keywords are printed for all file types.
>
>If you're willing to do this it would be helpful.  I know I will have to
>enable it to make this version acceptable on FreeBSD.  Otherwise every
>non-file line will change in the default case.  Obviously the reverse
>is that if you do make this change the files of NetBSD users change
>instead.  That being said, the time=%d.%09d bugfix means you already
>have done that in the default case so now is as good a time as any to
>make major changes.

I am ok with that.

>That could work.  What about the /set statement?  I think I'll probably
>need to change that on FreeBSD to avoid churning every line.  The
>type=file seems obviously wrong give that we aren't printing output for
>files.

Sounds good to me.

Best,

christos