xargs: various fixes.

Don't run the command with no arguments if we run out of input but
have already run the command at least once. The implementation of "run
the command at least once (unless -r was supplied)" wasn't taking into
account whether or not this was our first time round the loop.

Fix the exit value, and the -- already documented but not implemented --
behavior if a child exits with status 255.

Also extend the tests to cover these cases, plus cases I broke while
coming up with the fix. Add more tests to convince myself that we've
correctly interpreted how -s is supposed to behave, and fix the corner
cases at the bottom end of the range.

This fixes some issues we were seeing trying to build the Android SDK
for (and more importantly, on) macOS.

2 files changed, 50 insertions, 18 deletions

diff --git a/tests/xargs.test b/tests/xargs.test
index dce93ed3..827118ad 100644
--- a/tests/xargs.test
+++ b/tests/xargs.test
@@ -10,14 +10,22 @@ testing "spaces" "xargs" \
 
 testing "-n 0" "xargs -n 0 2>/dev/null || echo ok" "ok\n" \
 	"" "one \ntwo\n three"
+testing "-n 1" "xargs -n 1" "one\n" "" "one\n"
 testing "-n 2" "xargs -n 2" "one two\nthree\n" "" "one \ntwo\n three"
 testing "-n exact match" "xargs -n 3" "one two three\n" "" "one two three"
 testing "xargs2" "xargs -n2" "one two\nthree four\nfive\n" "" \
 	"one two three four five"
-testing "-s too long" "xargs -s 9 echo 2>/dev/null || echo ok" \
-	"one\ntwo\nok\n" "" "one two three"
+testing "-s too long" "xargs -s 9 echo 2>/dev/null; echo \$?" \
+	"one\ntwo\n1\n" "" "one two three"
 testing "-s 13" "xargs -s 13 echo" "one two\nthree\n" "" "one \ntwo\n three"
 testing "-s 12" "xargs -s 12 echo" "one\ntwo\nthree\n" "" "one \ntwo\n three"
+# Check that we're accounting for the command *and* its arguments correctly.
+testing "-s counts args" "xargs -s 13 echo ' ' ' '" "    one\n" "" "one\n"
+# 5 is the minimum allowed for the default "echo".
+testing "-s 4 fails" "xargs -s 4 2>/dev/null || echo bad" "bad\n" "" ""
+testing "-s 5 no input okay" "xargs -s 5" "\n" "" ""
+testing "-s 5 with input bad" "xargs -s 5 2>/dev/null || echo bad" "bad\n" \
+  "" "a\n"
 
 touch one two three
 testing "command -opt" "xargs -n2 ls -1" "one\ntwo\nthree\n" "" \
@@ -32,6 +40,17 @@ testing "-t" "xargs -t 2>stderr ; cat stderr ; rm stderr" "one two\necho one two
 testing "-E END" "xargs -E END" "a b\n" "" "a\nb\nEND\nc\nd\n"
 
 testing "-r" "xargs -r echo x" "" "" ""
+testing "no -r" "xargs echo x" "x\n" "" ""
+
+# Exit value madness. 0 and 126/127 are normal.
+testing "true" "xargs true; echo \$?" "0\n" "" ""
+testing "command not found" "xargs does-not-exist 2>/dev/null; echo \$?" \
+  "127\n" "" ""
+# But 1-125 are flattened to 123.
+testing "false" "xargs false; echo \$?" "123\n" "" ""
+# 255 is special "abort early" magic.
+testing "exit 255" "xargs sh -c 'exit 255' 2>&1; echo \$?" \
+  "xargs: sh: exited with status 255; aborting\n124\n" "" ""
 
 # TODO: what exactly is -x supposed to do? why does coreutils output "one"?
 #testing "-x" "xargs -x -s 9 || echo expected" "one\nexpected\n" "" "one\ntwo\nthree"
@@ -40,8 +59,4 @@ testing "-r" "xargs -r echo x" "" "" ""
 
 #testing "-n exact match"
 #testing "-s exact match"
-#testing "-s 0"
 #testing "-s impossible"
-
-# xargs command_not_found - returns 127
-# xargs false - returns 1
diff --git a/toys/posix/xargs.c b/toys/posix/xargs.c
index f0d2efe0..6ba5fb07 100644
--- a/toys/posix/xargs.c
+++ b/toys/posix/xargs.c
@@ -20,14 +20,14 @@ config XARGS
 
     Run command line one or more times, appending arguments from stdin.
 
-    If command exits with 255, don't launch another even if arguments remain.
+    If COMMAND exits with 255, don't launch another even if arguments remain.
 
     -0	Each argument is NULL terminated, no whitespace or quote processing
     -E	Stop at line matching string
     -n	Max number of arguments per command
     -o	Open tty for COMMAND's stdin (default /dev/null)
     -p	Prompt for y/n from tty before running each command
-    -r	Don't run command with empty input
+    -r	Don't run command with empty input (otherwise always run command once)
     -s	Size in bytes per command line
     -t	Trace, print command line to stderr
 */
@@ -44,8 +44,8 @@ GLOBALS(
   FILE *tty;
 )
 
-// If out==NULL count TT.bytes and TT.entries, stopping at max.
-// Otherwise, fill out out[]
+// If entry==NULL count TT.bytes and TT.entries, stopping at max.
+// Otherwise, fill out entry[].
 
 // Returning NULL means need more data.
 // Returning char * means hit data limits, start of data left over
@@ -104,7 +104,7 @@ static char *handle_entries(char *data, char **entry)
 void xargs_main(void)
 {
   struct double_list *dlist = NULL, *dtemp;
-  int entries, bytes, done = 0, status;
+  int entries, bytes, done = 0, ran_once = 0, status;
   char *data = NULL, **out;
   pid_t pid;
   long posix_max_bytes;
@@ -129,6 +129,8 @@ void xargs_main(void)
   for (entries = 0, bytes = -1; entries < toys.optc; entries++, bytes++)
     bytes += strlen(toys.optargs[entries]);
 
+  if (bytes >= TT.s) error_exit("can't fit single argument");
+
   // Loop through exec chunks.
   while (data || !done) {
     int doit = 1;
@@ -162,14 +164,14 @@ void xargs_main(void)
       break;
     }
 
-    if (TT.entries == 0 && FLAG(r)) continue;
-
-    // Accumulate cally thing
-
-    if (data && !TT.entries) error_exit("argument too long");
-    out = xzalloc((entries+TT.entries+1)*sizeof(char *));
+    if (TT.entries == 0) {
+      if (data) error_exit("argument too long");
+      else if (ran_once) xexit();
+      else if (FLAG(r)) continue;
+    }
 
     // Fill out command line to exec
+    out = xzalloc((entries+TT.entries+1)*sizeof(char *));
     memcpy(out, toys.optargs, entries*sizeof(char *));
     TT.entries = 0;
     TT.bytes = bytes;
@@ -196,8 +198,23 @@ void xargs_main(void)
         xexec(out);
       }
       waitpid(pid, &status, 0);
-      status = WIFEXITED(status) ? WEXITSTATUS(status) : WTERMSIG(status)+127;
+
+      // xargs is yet another weird collection of exit value special cases,
+      // different to all the others.
+      if (WIFEXITED(status)) {
+        if (WEXITSTATUS(status) == 126 || WEXITSTATUS(status) == 127) {
+          toys.exitval = WEXITSTATUS(status);
+          xexit();
+        } else if (WEXITSTATUS(status) >= 1 && WEXITSTATUS(status) <= 125) {
+          toys.exitval = 123;
+        } else if (WEXITSTATUS(status) == 255) {
+          error_msg("%s: exited with status 255; aborting", out[0]);
+          toys.exitval = 124;
+          xexit();
+        }
+      } else toys.exitval = 127;
     }
+    ran_once = 1;
 
     // Abritrary number of execs, can't just leak memory each time...
     while (dlist) {