On Sun, Jan 30, 2011 at 08:38:25PM +0100, Sami Kerola wrote:
> This fix will also introduce warning. More has been setting
> option --no-scroll on if binary is has been called `page'. Since
> this was not documented, and rather well hide to the source, the
> functionality should be removed in future.
I don't think that we need the warning here. The feature is there for
years and would be better to improve documentation than print an
unexpected warning.
> Environment variable PAGE is after this commit examined by
$MORE
> getopt_long, which will mean that some previously broken ways to
> execute the command now work. For example one bellow.
>
> MORE='+10' more /etc/services
[...]
> +char short_opt_lines[] = "--lines";
why not:
char *short_opt_lines = "--lines";
> +char short_opt_start_line[] = "--start-line";
> +char short_opt_search[] = "--search";
> + fprintf(out,
> + _("Options:\n"
> + " -d, --visible-bell display help instead of ring bell \n"
> + " -f, --logical-count count logical, rather than screen lines\n"
> + " -l, --ignore-form-feed suppress pause after form feed\n"
> + " -p, --no-scroll suppress scroll, clean screen and disblay text\n"
> + " -c, --clean-ends suppress scroll, display text and clean line ends\n"
> + " -u, --no-underlining suppress underlining\n"
> + " -s, --squeeze squeeze multiple blank lines into one\n"
> + " -NUM, --lines=NUM set number of screen lines\n"
more(1) is also described in POSIX and there is suggested -n <NUM>,
maybe we can support all three formats (-NUM, -n NUM, --lines NUM)
> + " +NUM, --start-line=NUM display beginning from line number NUM\n"
Please, don't use '=' in the usage string, use:
" -n, -NUM, --lines NUM set number of screen lines\n"
or so.
> + " +/STRING, --search=STRING display beginning where search match\n"
> + " --help display this help and exit \n"
> + " --version output version information and exit\n"));
Seems like FSFism :-) I think we can use '-h' for help and maybe -v
or -V for version.
> +int getoptify_old_format(char *argv, char **new_argv, int new_argc, int *new_argc_optsp, int *old_opt_formatp) {
I don't understand why we need new_argc_optsp and old_opt_formatp
pointers. What about:
add_argv_option(char *option, char **argv, int *argc)
and all without unnecessary new_ prefix.
> + if (argv[0] == '+') {
> + if (argv[1] == '/') {
> + new_argv[new_argc] = (char *) &short_opt_search;
> + new_argc++;
> + new_argv[new_argc] = (char *) &argv[2];
> + new_argc++;
> + ++*old_opt_formatp;
> + } else {
> + new_argv[new_argc] = (char *) &short_opt_start_line;
> + new_argc++;
> + new_argv[new_argc] = (char *) &argv[1];
> + new_argc++;
> + ++*old_opt_formatp;
> + }
> + } else if (argv[0] == '-' && isdigit(argv[1])) {
> + new_argv[new_argc] = (char *) &short_opt_lines;
> + new_argc++;
> + new_argv[new_argc] = (char *) &argv[1];
> + new_argc++;
> + } else {
> + new_argv[new_argc] = argv;
> + new_argc++;
> + }
> +
> + ++*new_argc_optsp;
> + return(new_argc);
return new_argc;
the returns is not function
> + if (!(strcmp(program_invocation_short_name, "page"))) {
> + warnx(_("option --no-scroll is set because the binary has name `page'"));
> + noscroll++;
> + }
Please, remove the warning.
> + /* FIXME make fully dynamic; think about linux/limits.h ARG_MAX */
> + new_argv = (char **) xmalloc(sizeof(char *) * 256);
the size of the new_argv is
+ current argv
+ number of strings in $MORE
+ 6 (-NUM, +NUM, +/STRING are translated to two new options (e.g. "-n" "NUM")
> + new_argc_optsp = &new_argc_opts;
> + old_opt_formatp = &old_opt_format;
It would be better to move all this crazy stuff around argv[] outside
main(). It's more readable if there is getopt stuff in main() only.
argv = prepare_argv(argv, &argc);
while ((c = getopt_long(argc, argv, ..........))) {
...
}
> +
> + /* Convert environment string to look like option from
> + * command line. */
> + s = getenv("MORE");
if (s)
s = xstrdup(s);
> + if (s != NULL) {
> + int i;
> + i = 0;
> + while (s[0] != '\0' && new_argc < 256) {
> + if(isblank(s[0])) {
> + s[0] = '\0';
man getenv:
As typically implemented, getenv() returns a pointer to a string
within the environment list. The caller must take care not to modify
^^^^^^^^^^^^^^^^^^^^^^^^
this string, since that would change the environment of the process.
[...]
> + /* Make old style arguments to be long options, and copy
> + * other options to new argv. */
> + for (int i = 0; i < argc && new_argc < 256; i++) {
> + if (i == 0) {
> + new_argv[0] = argv[i];
it would be better to set argv[0] immediately after allocation and
care about argc > 0 in the rest of the code
> + } else {
> + new_argc = getoptify_old_format(argv[i], new_argv, new_argc, new_argc_optsp, old_opt_formatp);
> + }
> + }
[...]
> + while (1) {
> + int optind = -1;
Why? The getopt_long() wasn't called yet, the default setting should
be enough.
> + int c = getopt_long (new_argc, new_argv, "dflpcus",
> + long_options, &optind);
> + if (c == -1)
> + break;
IMHO, all this is unnecessary
while((c = getopt_long( ....
> + switch (c) {
> + case 'd':
> + dum_opt = 1;
> + break;
You need extra Python lesson :-)
case 'd':
dum_opt = 1;
break;
> + case 'l':
> + stop_opt = 0;
> + break;
[...]
> }
> - else break;
> }
> + optind -= old_opt_format;
Why? I expect that our new argv[] array is parsable by getopt_long()
without any extra care.
Note that getopt_long() reorder argv[], so all non-option arguments
are at the end of the array,
> + fnames = xmalloc(sizeof(char *) * new_argc - optind);
> + for (nfiles = 0; optind < new_argc;)
> + fnames[nfiles++] = argv[optind++];
is it necessary? The fnames should be a pointer to argv[optind]
and nfiles = argc - optind.
> - while (fnum < nfiles) {
> + while (fnames[fnum] != NULL && fnum < nfiles) {
unnecessary if nfiles number is correct
Karel
--
Karel Zak <kzak@xxxxxxxxxx>
http://karelzak.blogspot.com
--
To unsubscribe from this list: send the line "unsubscribe util-linux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
