From 4d92046d7291004d567b0c90b23f8200556c2682 Mon Sep 17 00:00:00 2001
From: Andreas Metzler <ametzler@bebt.de>
Date: Wed, 11 Oct 2017 00:47:08 +0200
Subject: [PATCH] xargs: warn on conflicting options -L, -I, -n

* xargs/xargs.c (warn_mutually_exclusive): Add function to output the
new warning diagnostic.
(main): Call the above new function in order to show a warning message
if conflicting options (-L -l -I -i -n) are specified.
* xargs/xargs.1: Explicitly document the fact that -L -l -I -i -n
options are mutually exclusive and how xargs behaves when more than one
of these options are specified.
* doc/find.texi (node Conflicting xargs options): Likewise, add this
section with examples.
* xargs/testsuite/config/unix.exp (xargs_start): Replace the full path
of the xargs executable in the stderr output file by its basename.
* xargs/testsuite/xargs.gnu/P3-n1-IARG.xe: Add file containing the
expected output with the new warning diagnostic.
* xargs/testsuite/xargs.posix/L2-n2.xe: Likewise.
* xargs/testsuite/xargs.posix/rc-123.xe: Likewise.
* xargs/testsuite/xargs.sysv/l1n4.xe: Likewise.
* xargs/testsuite/xargs/testsuite/xargs.gnu/P3-n1-IARG.exp: Ensure the
error diagnostic is in C locale to avoid false positives in other locales.
* xargs/testsuite/xargs.posix/L2-n2.exp: Likewise.
* xargs/testsuite/xargs.posix/rc-123.exp: Likewise.
* xargs/testsuite/xargs.sysv/l1n4.exp: Likewise.
* tests/xargs/conflicting_opts.sh: Add test coverage for combinations
of the mutually exclusive options -I -L and -n.
* xargs/testsuite/Makefile.am (EXTRA_DIST_EXP): Reference the new tests.
(EXTRA_DIST_XO): Reference the new *.xo files.
(EXTRA_DIST_XE): Reference the new *.xe files.
* NEWS (Improvements): Mention the change.

Co-authored-by: Bernhard Voelker <mail@bernhard-voelker.de>

Discussed at https://savannah.gnu.org/bugs/?52137
Duplicate of https://savannah.gnu.org/bugs/?58156
---
 NEWS                                     |   4 +
 doc/find.texi                            |  28 +++++
 tests/local.mk                           |   1 +
 tests/xargs/conflicting_opts.sh          | 133 +++++++++++++++++++++++
 xargs/testsuite/Makefile.am              |   4 +
 xargs/testsuite/config/unix.exp          |   1 +
 xargs/testsuite/xargs.gnu/P3-n1-IARG.exp |   1 +
 xargs/testsuite/xargs.gnu/P3-n1-IARG.xe  |   1 +
 xargs/testsuite/xargs.posix/L2-n2.exp    |   1 +
 xargs/testsuite/xargs.posix/L2-n2.xe     |   1 +
 xargs/testsuite/xargs.posix/rc-123.exp   |   1 +
 xargs/testsuite/xargs.posix/rc-123.xe    |   1 +
 xargs/testsuite/xargs.sysv/l1n4.exp      |   1 +
 xargs/testsuite/xargs.sysv/l1n4.xe       |   1 +
 xargs/xargs.1                            |  38 ++++++-
 xargs/xargs.c                            |  68 ++++++++++--
 16 files changed, 267 insertions(+), 18 deletions(-)
 create mode 100755 tests/xargs/conflicting_opts.sh
 create mode 100644 xargs/testsuite/xargs.gnu/P3-n1-IARG.xe
 create mode 100644 xargs/testsuite/xargs.posix/L2-n2.xe
 create mode 100644 xargs/testsuite/xargs.posix/rc-123.xe
 create mode 100644 xargs/testsuite/xargs.sysv/l1n4.xe

diff --git a/NEWS b/NEWS
index cc74ef10..195f7863 100644
--- a/NEWS
+++ b/NEWS
@@ -4,6 +4,10 @@ GNU findutils NEWS - User visible changes.      -*- outline -*- (allout)
 
 ** Changes in xargs
 
+xargs now warns when more than one of the conflicting options --max-lines (-L,
+-l), --replace (-i/-I) and --max-args (-n) are specified on the command line.
+[#52137]
+
 'xargs -t' no longer outputs a trailing blank to stderr after the last argument
 of each constructed command line to be executed.  [#57291]
 
diff --git a/doc/find.texi b/doc/find.texi
index f8bfcba0..fbf59188 100644
--- a/doc/find.texi
+++ b/doc/find.texi
@@ -3742,6 +3742,7 @@ a program died due to a fatal signal.
 
 @menu
 * xargs options::
+* Conflicting xargs options::
 * Invoking the shell from xargs::
 @end menu
 
@@ -3891,6 +3892,33 @@ integer.  Values are reused once child processes exit.  This can be
 used in a rudimentary load distribution scheme, for example.
 @end table
 
+@node Conflicting xargs options
+@subsection Conflicting options
+The options @samp{--max-lines} (@samp{-L}, @samp{-l}), @samp{--replace}
+(@samp{-I}, @samp{-i}) and @samp{--max-args} (@samp{-n}) are mutually exclusive.
+
+If some of them are specified at the same time, then @code{xargs} will
+generally use the option specified last on the command line, i.e., it will
+reset the value of the offending option (given before) to its default value.
+Additionally, @code{xargs} will issue a warning diagnostic on @file{stderr}.
+
+@example
+$ seq 4 | xargs -l2 -n3
+xargs: warning: options --max-lines and --max-args/-n are \
+  mutually exclusive, ignoring previous --max-lines value
+1 2 3
+4
+@end example
+
+The exception to this rule is that the special @var{max-args} value @var{1} is
+ignored after the @samp{--replace} option and its short-option aliases @samp{-I}
+and @samp{-i}, because it would not actually conflict.
+@example
+$ seq 2 | xargs --replace -n1 echo a-@{@}-b
+a-1-b
+a-2-b
+@end example
+
 @node Invoking the shell from xargs
 @subsection Invoking the shell from xargs
 
diff --git a/tests/local.mk b/tests/local.mk
index 6e67bad4..95bc3df5 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -117,6 +117,7 @@ all_tests = \
   tests/find/refuse-noop.sh \
   tests/find/debug-missing-arg.sh \
   tests/find/used.sh \
+  tests/xargs/conflicting_opts.sh \
   tests/xargs/verbose-quote.sh \
   $(all_root_tests)
 
diff --git a/tests/xargs/conflicting_opts.sh b/tests/xargs/conflicting_opts.sh
new file mode 100755
index 00000000..d8bb5a39
--- /dev/null
+++ b/tests/xargs/conflicting_opts.sh
@@ -0,0 +1,133 @@
+#!/bin/sh
+# Test mutually exclusive options --max-args, --max-lines, --replace, and
+# their short-option aliases (-L, -l -I -i -n).
+
+# Copyright (C) 2021 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/tests/init.sh"
+print_ver_ xargs
+
+# The idea behind: when using the mutually exclusive options, the last one
+# specified overrides the values of the previous ones.  Or to put it in
+# other words: the resulting output (and exit code btw.) is the same as if
+# the earlier, conflicting options had not been passed - apart from the
+# error diagnostic for every overridden option.
+
+# Create some input file.
+cat <<\EOF > in || framework_failure_
+a b c
+d e f
+g h i
+EOF
+
+WARNFMT='xargs: warning: options %s and %s are mutually exclusive, ignoring previous %s value'
+
+str4opt() {
+  case "$1" in
+  -n*|--max-args*) echo '--max-args/-n';;
+  -l*|--max-lines*) echo '--max-lines/-l';;
+  -i|-I*|--replace*) echo '--replace/-I/-i';;
+  esac
+}
+lopt4opt() {
+  case "$1" in
+  -n*|--max-args*) echo '--max-args';;
+  -l*|--max-lines*) echo '--max-lines';;
+  -i|-I*|--replace*) echo '--replace';;
+  esac
+}
+
+gen_err() {  # see warn_mutually_exclusive in xargs.c.
+  s="$(str4opt "$1")" || framework_failure_
+  l="$(lopt4opt "$2")" || framework_failure_
+
+  printf "$WARNFMT\n" "$l" "$s" "$l"
+}
+
+# test_xargs a test case, using $1 as the last option which should finally override
+# any other of the mutually exclusive options.
+# Example: test_xargs -n2 OTHER_OPTION
+# This will invoke 'xargs' with that option as the last one:
+#   xargs OTHER_OPTION -n2
+test_xargs() {
+  rm -f out err err2 expout experr || framework_failure_
+
+  # Generate the expected output file for the option in "$1".
+  # test_xargs xargs with that option only; stderr should be empty
+  # and stdout of this test_xargs is the reference for the actual test.
+  last_opt="$1" \
+    && shift 1 \
+    && xargs "$last_opt" < in > expout 2>err \
+    && compare /dev/null err \
+    || framework_failure_
+
+  # Generate the expected error diagnostic for the mutually exclusive option
+  # passed earlier.  No diagnostic if the previous mutually-exclusive option
+  # is the same as the current one.
+  prev_excl_option=  # variable for previous mutually-exclusive option.
+  for o in "$@" "$last_opt"; do
+    lopt="$(lopt4opt "$o")" || framework_failure_
+    case $o in -n*|--max-args*|-l*|--max-lines*|-i|-I*|--replace*)
+      test "$prev_excl_option" != '' \
+        && test "$prev_excl_option" != "$lopt" \
+        && gen_err "$o" "$prev_excl_option"
+      prev_excl_option="$lopt"
+      ;;
+    esac
+  done > experr
+
+  # test_xargs the test.
+  xargs "$@" "$last_opt" < in > out 2>err || fail=1
+  sed 's/^.*\(xargs:\)/&/' err > err2 || framework_failure_
+  compare expout out || fail=1
+  compare experr err2 || fail=1
+}
+
+# test_xargs the tests for --max-args as last option.
+for last_opt in -n2 --max-args=2; do
+  for other_opt in --max-lines=2 -l2 --replace='REPL' -I'REPL' -i; do
+    test_xargs $last_opt $other_opt
+  done
+done
+
+# test_xargs the tests for --max-line as last option.
+for last_opt in -l2 --max-lines=2; do
+  for other_opt in --max-args=2 -n2 --replace='REPL' -I'REPL' -i; do
+    test_xargs $last_opt $other_opt
+  done
+done
+
+# test_xargs the tests for --replace as last option.
+for last_opt in --replace='REPL' -I'REPL' -i; do
+  for other_opt in --max-args=2 -n2 -l2 --max-lines=2; do
+    test_xargs $last_opt $other_opt
+  done
+done
+
+# Finally do some multi-option tests.
+# Test 'xargs -l2 -l3 -l4 -i -n2':
+# diagnostic for -l once, then for -i being overridden.
+test_xargs -n2   -l2 -l3 -l4 -i
+
+# Test 'xargs -n2 -i -l4 -l5': diagnostic for -n, then once for -i.
+test_xargs -l5   -n2 -i -l4
+
+# Test 'xargs -l2 --max-lines=3 --replace=4 -I5 --max-args=6 -n7 -n1':
+# Diagnostic for the change from MAX-LINES to REPLACE,
+# then from REPLACE to MAX-ARGS.
+test_xargs -n1 -l2 --max-lines=3 --replace=4 -I5 --max-args=6 -n7
+
+Exit $fail
diff --git a/xargs/testsuite/Makefile.am b/xargs/testsuite/Makefile.am
index 307bab22..8d85d185 100644
--- a/xargs/testsuite/Makefile.am
+++ b/xargs/testsuite/Makefile.am
@@ -213,9 +213,13 @@ xargs.sysv/trace.xo
 
 
 EXTRA_DIST_XE = \
+xargs.gnu/P3-n1-IARG.xe \
 xargs.gnu/space-t-0.xe \
+xargs.posix/L2-n2.xe \
+xargs.posix/rc-123.xe \
 xargs.sysv/empty_def-t.xe \
 xargs.sysv/empty-t.xe \
+xargs.sysv/l1n4.xe \
 xargs.sysv/s25-t.xe \
 xargs.sysv/space-t.xe \
 xargs.sysv/trace.xe
diff --git a/xargs/testsuite/config/unix.exp b/xargs/testsuite/config/unix.exp
index 52fc7c31..7dca8930 100644
--- a/xargs/testsuite/config/unix.exp
+++ b/xargs/testsuite/config/unix.exp
@@ -157,6 +157,7 @@ proc xargs_start { passfail options {infile ""} {errh ""} {command ""} } {
     # if stderr check is enabled,
     if {$errfile != ""} then {
 	if {[file exists $errfile]} then {
+            catch "exec sed -i s/^.*xargs:/xargs:/ xargs.err" tmp
 	    set cmp_cmd "cmp xargs.err $errfile"
 	    send_log "$cmp_cmd\n"
 	    catch "exec $cmp_cmd" cmperr
diff --git a/xargs/testsuite/xargs.gnu/P3-n1-IARG.exp b/xargs/testsuite/xargs.gnu/P3-n1-IARG.exp
index 75795892..5ff49d8e 100644
--- a/xargs/testsuite/xargs.gnu/P3-n1-IARG.exp
+++ b/xargs/testsuite/xargs.gnu/P3-n1-IARG.exp
@@ -1 +1,2 @@
+set env(LC_ALL) "C"
 xargs_start p {-P3 -n1 -IARG sh -c ARG} Pdata.xi
diff --git a/xargs/testsuite/xargs.gnu/P3-n1-IARG.xe b/xargs/testsuite/xargs.gnu/P3-n1-IARG.xe
new file mode 100644
index 00000000..b07e415c
--- /dev/null
+++ b/xargs/testsuite/xargs.gnu/P3-n1-IARG.xe
@@ -0,0 +1 @@
+xargs: warning: options --max-args and --replace/-I/-i are mutually exclusive, ignoring previous --max-args value
diff --git a/xargs/testsuite/xargs.posix/L2-n2.exp b/xargs/testsuite/xargs.posix/L2-n2.exp
index 88a939f7..9a979cc5 100644
--- a/xargs/testsuite/xargs.posix/L2-n2.exp
+++ b/xargs/testsuite/xargs.posix/L2-n2.exp
@@ -1 +1,2 @@
+set env(LC_ALL) "C"
 xargs_start p {-L2 -n2} ldata.xi
diff --git a/xargs/testsuite/xargs.posix/L2-n2.xe b/xargs/testsuite/xargs.posix/L2-n2.xe
new file mode 100644
index 00000000..f1983b9f
--- /dev/null
+++ b/xargs/testsuite/xargs.posix/L2-n2.xe
@@ -0,0 +1 @@
+xargs: warning: options --max-lines and --max-args/-n are mutually exclusive, ignoring previous --max-lines value
diff --git a/xargs/testsuite/xargs.posix/rc-123.exp b/xargs/testsuite/xargs.posix/rc-123.exp
index 217de0fa..1bcf4904 100644
--- a/xargs/testsuite/xargs.posix/rc-123.exp
+++ b/xargs/testsuite/xargs.posix/rc-123.exp
@@ -1 +1,2 @@
+set env(LC_ALL) "C"
 xargs_start 123 {-n1 -IARG sh -c ARG} ftt.xi
diff --git a/xargs/testsuite/xargs.posix/rc-123.xe b/xargs/testsuite/xargs.posix/rc-123.xe
new file mode 100644
index 00000000..b07e415c
--- /dev/null
+++ b/xargs/testsuite/xargs.posix/rc-123.xe
@@ -0,0 +1 @@
+xargs: warning: options --max-args and --replace/-I/-i are mutually exclusive, ignoring previous --max-args value
diff --git a/xargs/testsuite/xargs.sysv/l1n4.exp b/xargs/testsuite/xargs.sysv/l1n4.exp
index 4eef4666..26a586f4 100644
--- a/xargs/testsuite/xargs.sysv/l1n4.exp
+++ b/xargs/testsuite/xargs.sysv/l1n4.exp
@@ -1 +1,2 @@
+set env(LC_ALL) "C"
 xargs_start p {-l1 -n4} files.xi
diff --git a/xargs/testsuite/xargs.sysv/l1n4.xe b/xargs/testsuite/xargs.sysv/l1n4.xe
new file mode 100644
index 00000000..f1983b9f
--- /dev/null
+++ b/xargs/testsuite/xargs.sysv/l1n4.xe
@@ -0,0 +1 @@
+xargs: warning: options --max-lines and --max-args/-n are mutually exclusive, ignoring previous --max-lines value
diff --git a/xargs/xargs.1 b/xargs/xargs.1
index d5735c7e..66dcfde0 100644
--- a/xargs/xargs.1
+++ b/xargs/xargs.1
@@ -287,6 +287,38 @@ and exit.
 Print the version number of
 .B xargs
 and exit.
+.PP
+The options
+.B \-\-max-lines
+(\fB\-L\fP, \fB\-l\fP),
+.B \-\-replace
+(\fB\-I\fP, \fB\-i\fP)
+and
+.B \-\-max-args
+(\fB\-n\fP)
+are mutually exclusive. If some of them are specified at the same
+time, then
+.B xargs
+will generally use the option specified last on the command line,
+i.e., it will reset the value of the offending option (given before)
+to its default value.
+Additionally,
+.B xargs
+will issue a warning diagnostic on
+.IR stderr .
+The exception to this rule is that the special
+.I max-args
+value
+.I 1
+('\fB\-n\fP\fI1\fP')
+is ignored after the
+.B \-\-replace
+option and its aliases
+.B \-I
+and
+.BR \-i ,
+because it would not actually conflict.
+
 .
 .SH "EXAMPLES"
 .nf
@@ -383,12 +415,6 @@ option can be used to discover the actual limits in force on the
 current system.
 .
 .SH "BUGS"
-The
-.B \-L
-option is incompatible with the
-.B \-I
-option, but perhaps should not be.
-.P
 It is not possible for
 .B xargs
 to be used securely, since there will always be a time gap between the
diff --git a/xargs/xargs.c b/xargs/xargs.c
index 64066d38..7e3db67a 100644
--- a/xargs/xargs.c
+++ b/xargs/xargs.c
@@ -386,6 +386,14 @@ static FILE* fopen_cloexec_for_read_only (const char *file_name)
 }
 
 
+static void warn_mutually_exclusive (const char *option, const char *offending)
+{
+  error (0, 0, _("warning: options %s and %s are mutually exclusive, "
+	 "ignoring previous %s value"),
+	 offending, option, offending);
+}
+
+
 int
 main (int argc, char **argv)
 {
@@ -544,15 +552,31 @@ main (int argc, char **argv)
 	  else
 	    bc_ctl.replace_pat = "{}";
 	  /* -i excludes -n -l.  */
-	  bc_ctl.args_per_exec = 0;
-	  bc_ctl.lines_per_exec = 0;
+	  if (bc_ctl.args_per_exec != 0)
+	    {
+	      warn_mutually_exclusive ("--replace/-I/-i", "--max-args");
+	      bc_ctl.args_per_exec = 0;
+	    }
+	  if (bc_ctl.lines_per_exec != 0)
+	    {
+	      warn_mutually_exclusive ("--replace/-I/-i", "--max-lines");
+	      bc_ctl.lines_per_exec = 0;
+	    }
 	  break;
 
 	case 'L':		/* POSIX */
 	  bc_ctl.lines_per_exec = parse_num (optarg, 'L', 1L, -1L, 1);
 	  /* -L excludes -i -n.  */
-	  bc_ctl.args_per_exec = 0;
-	  bc_ctl.replace_pat = NULL;
+	  if (bc_ctl.args_per_exec != 0)
+	    {
+	      warn_mutually_exclusive ("-L", "--max-args");
+	      bc_ctl.args_per_exec = 0;
+	    }
+	  if (bc_ctl.replace_pat != NULL)
+	    {
+	      warn_mutually_exclusive ("-L", "--replace");
+	      bc_ctl.replace_pat = NULL;
+	    }
 	  break;
 
 	case 'l':		/* deprecated */
@@ -561,19 +585,39 @@ main (int argc, char **argv)
 	  else
 	    bc_ctl.lines_per_exec = 1;
 	  /* -l excludes -i -n.  */
-	  bc_ctl.args_per_exec = 0;
-	  bc_ctl.replace_pat = NULL;
+	  if (bc_ctl.args_per_exec != 0)
+	    {
+	      warn_mutually_exclusive ("--max-lines/-l", "--max-args");
+	      bc_ctl.args_per_exec = 0;
+	    }
+	  if (bc_ctl.replace_pat != NULL)
+	    {
+	      warn_mutually_exclusive ("--max-lines/-l", "--replace");
+	      bc_ctl.replace_pat = NULL;
+	    }
 	  break;
 
 	case 'n':
 	  bc_ctl.args_per_exec = parse_num (optarg, 'n', 1L, -1L, 1);
 	  /* -n excludes -i -l.  */
-	  bc_ctl.lines_per_exec = 0;
-	  if (bc_ctl.args_per_exec == 1 && bc_ctl.replace_pat)
-	    /* ignore -n1 in '-i -n1' */
-	    bc_ctl.args_per_exec = 0;
-	  else
-	    bc_ctl.replace_pat = NULL;
+	  if (bc_ctl.lines_per_exec != 0)
+	    {
+	      warn_mutually_exclusive ("--max-args/-n", "--max-lines");
+	      bc_ctl.lines_per_exec = 0;
+	    }
+	  if (bc_ctl.replace_pat != NULL)
+	    {
+	      if (bc_ctl.args_per_exec == 1)
+		{
+		  /* ignore -n1 in '-i -n1' - https://sv.gnu.org/patch/?1500 */
+		  bc_ctl.args_per_exec = 0;
+		}
+	      else
+		{
+		  warn_mutually_exclusive ("--max-args/-n", "--replace");
+		  bc_ctl.replace_pat = NULL;
+		}
+	    }
 	  break;
 
 	  /* The POSIX standard specifies that it is not an error
-- 
2.29.2

