From 3edb7d538fbf8f1ad88cf764033b19ed0486b7eb Mon Sep 17 00:00:00 2001
From: Fabian Homborg <FHomborg@gmail.com>
Date: Sun, 2 Apr 2017 17:02:55 +0200
Subject: [PATCH] Improve `bg` argument handling

- Error out if anything that is not a PID is given

- Otherwise background all matching existing jobs, even if not all
  PIDs exist (but print a message for the non-existing ones)

Fixes #3909.
---
 CHANGELOG.md    |  1 +
 doc_src/bg.txt  | 10 ++++++++++
 src/builtin.cpp | 37 ++++++++++++++++++++++++-------------
 tests/jobs.err  |  3 ++-
 tests/jobs.in   |  7 ++++---
 5 files changed, 41 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index acec7edf8..629cf02bb 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@
 - `type` now no longer requires `which`, which means fish no longer uses it anywhere. Packagers should remove the dependency (#3912).
 - Using symbolic permissions with the `umask` command now works (#738).
 - Command substitutions now have access to the terminal, allowing tools like `fzf` to work in them (#1362, #3922).
+- `bg`s argument parsing has been reworked. It now fails for invalid arguments but allows non-existent jobs (#3909).
 
 ---
 
diff --git a/doc_src/bg.txt b/doc_src/bg.txt
index 8e9ec2444..7195501ed 100644
--- a/doc_src/bg.txt
+++ b/doc_src/bg.txt
@@ -11,7 +11,17 @@ bg [PID...]
 
 The PID of the desired process is usually found by using <a href="index.html#expand-process">process expansion</a>.
 
+When at least one of the arguments isn't a valid job specifier (i.e. PID),
+`bg` will print an error without backgrounding anything.
+
+When all arguments are valid job specifiers, bg will background all matching jobs that exist.
 
 \subsection bg-example Example
 
 `bg %1` will put the job with job ID 1 in the background.
+
+`bg 123 456 789` will background 123, 456 and 789.
+
+If only 123 and 789 exist, it will still background them and print an error about 456.
+
+`bg 123 banana` or `bg banana 123` will complain that "banana" is not a valid job specifier.
diff --git a/src/builtin.cpp b/src/builtin.cpp
index f1ef3dbcc..e4a256a93 100644
--- a/src/builtin.cpp
+++ b/src/builtin.cpp
@@ -2988,11 +2988,8 @@ static int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
 
 /// Helper function for builtin_bg().
 static int send_to_bg(parser_t &parser, io_streams_t &streams, job_t *j, const wchar_t *name) {
-    if (j == 0) {
-        streams.err.append_format(_(L"%ls: Unknown job '%ls'\n"), L"bg", name);
-        builtin_print_help(parser, streams, L"bg", streams.err);
-        return STATUS_BUILTIN_ERROR;
-    } else if (!j->get_flag(JOB_CONTROL)) {
+    assert(j != NULL);
+    if (!j->get_flag(JOB_CONTROL)) {
         streams.err.append_format(
             _(L"%ls: Can't put job %d, '%ls' to background because it is not under job control\n"),
             L"bg", j->job_id, j->command_wcstr());
@@ -3028,16 +3025,30 @@ static int builtin_bg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
             res = send_to_bg(parser, streams, j, _(L"(default)"));
         }
     } else {
-        int i;
-        int pid;
+        std::vector<int> pids;
 
-        for (i = 1; argv[i]; i++) {
-            pid = fish_wcstoi(argv[i]);
-            if (errno || pid < 0 || !job_get_from_pid(pid)) {
-                streams.err.append_format(_(L"%ls: '%ls' is not a job\n"), argv[0], argv[i]);
-                return STATUS_BUILTIN_ERROR;
+        // If one argument is not a valid pid (i.e. integer >= 0), fail without backgrounding anything,
+        // but still print errors for all of them.
+        for (int i = 1; argv[i]; i++) {
+            int pid = fish_wcstoi(argv[i]);
+            if (errno || pid < 0) {
+                streams.err.append_format(_(L"%ls: '%ls' is not a valid job specifier\n"), argv[0], argv[i]);
+                res = STATUS_BUILTIN_ERROR;
+            }
+            pids.push_back(pid);
+        }
+        if (res == STATUS_BUILTIN_ERROR) {
+            return res;
+        }
+
+        // Background all existing jobs that match the pids.
+        // Non-existent jobs aren't an error, but information about them is useful.
+        for (auto p : pids) {
+            if (job_t* j = job_get_from_pid(p)) {
+                res |= send_to_bg(parser, streams, j, *argv);
+            } else {
+                streams.err.append_format(_(L"%ls: Could not find job '%d'\n"), argv[0], p);
             }
-            res |= send_to_bg(parser, streams, job_get_from_pid(pid), *argv);
         }
     }
 
diff --git a/tests/jobs.err b/tests/jobs.err
index 13e56fdf5..5a1052d09 100644
--- a/tests/jobs.err
+++ b/tests/jobs.err
@@ -1,2 +1,3 @@
-bg: '3' is not a job
+bg: '-23' is not a valid job specifier
 fg: No suitable job: 3
+bg: Could not find job '3'
diff --git a/tests/jobs.in b/tests/jobs.in
index ad0b002e7..e41b64537 100644
--- a/tests/jobs.in
+++ b/tests/jobs.in
@@ -1,6 +1,7 @@
-sleep 1 &
-sleep 1 &
+sleep 5 &
+sleep 5 &
 jobs -c
-bg 3
+bg -23 1
 fg 3
+bg 3
 or exit 0