Re: [PATCH] split: adding --exec, --exec-wait, and --pause

[Jim Meyering]

Chris Frey <address@hidden> wrote:
> I haven't seen any comments on this patch yet.  What can I do to help get
> this patch included in coreutils?

Thanks for the feature-adding patch.

Adding a new feature to a classic program in the coreutils package is
not something we do lightly.  There has to be a pretty good reason and
no easy workaround or alternative tool.

In this case, what's the motivation?  Saving disk space?
Please provide examples showing how this is useful to you.

In your typical applications, are you splitting an existing
file or data from an input pipe?

If it's always an existing file, then perhaps a simpler change (not
involving exec, user interaction, etc.) would be to provide an option
to split telling it to extract only the Nth hunk from its input file.
Then the iteration and user interaction could happen outside of split.

Here are some guidelines:

  - base your changes on the latest code in CVS, here:
      http://savannah.gnu.org/cvs/?group=coreutils

  - add tests to exercise the new functionality, as well as tests that
      exercise it in combination with related options.  The more the better.

  - follow the guidelines in the GNU Coding Standards (standards.info)
      which is distributed as part of the autoconf package.

  - include changes to the texinfo documentation, and be sure to update
      the --help output.

  - finally, if the change is `significant' you'll have to send signed
      copyright assignment papers to the FSF

If a few people say that your patch is essential to them, that
helps to accelerate the process.

For now, I'll probably start a cvs-only patches/ directory and
begin putting candidate *.diff files there.

------------------
Regarding your patch, here's a relatively shallow review:

We rarely add new short-named options (-e, -w, -p),
so if you want to continue with this, please remove those.

You should handle a few more failed system/library calls.
Uses of at least read, write, close, dup2 and asprintf are unchecked.

Don't hard-code /bin/sh.
> +      execl ("/bin/sh", "/bin/sh", "-c", cmd_line, NULL);
At least pull it out into a #define:

Is ttyname_r portable?
Here, it's probably better to use ttyname.
An alternative would be to undo the recent change that made
split reuse stdin.  That'd probably be better, since most other
programs in coreutils read user input from stdin.

Re this:
> +      /* build the command line, passing a few filenames so the user
> +       * can use multiple %s if he needs to... the cheap way */
> +      asprintf (&cmd_line, exec_command, outfile, outfile, outfile, outfile,
> +               outfile, outfile, outfile);
The file names may have to be quoted.  The prefix may contain
shell meta-characters.  What if exec_command contains more than
seven %s's?


Given the relative complexity (in number of syscalls) of the
exec-and-prompt-from-within-split approach, I think you can see
why I'd prefer to consider the simpler approach.

Finally, don't expect quick feedback.
Adding a feature like this is low priority.