From c3374edc59f1679be254b69e63360808e7cfeb99 Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Mon, 23 Dec 2019 11:45:55 +0100
Subject: [PATCH] Reject time with background jobs

This check could probably done earlier in the parser but it works.
---
 src/parse_constants.h   |  4 ++++
 src/parse_execution.cpp | 16 ++++++++++++++--
 tests/checks/time.fish  | 14 ++++++++++++--
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/parse_constants.h b/src/parse_constants.h
index 5d6ded51b..1df7cde39 100644
--- a/src/parse_constants.h
+++ b/src/parse_constants.h
@@ -295,4 +295,8 @@ void parse_error_offset_source_start(parse_error_list_t *errors, size_t amt);
 #define ERROR_BAD_COMMAND_ASSIGN_ERR_MSG \
     _(L"Unsupported use of '='. In fish, please use 'set %ls %ls'.")
 
+/// Error message for a command like `time foo &`.
+#define ERROR_TIME_BACKGROUND \
+    _(L"'time' is not supported for background jobs. Consider using 'command time'.")
+
 #endif
diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp
index bc7401315..6fe1b8409 100644
--- a/src/parse_execution.cpp
+++ b/src/parse_execution.cpp
@@ -977,7 +977,13 @@ eval_result_t parse_execution_context_t::populate_not_process(
     auto &flags = job->mut_flags();
     flags.negate = !flags.negate;
     auto optional_time = not_statement.require_get_child<g::optional_time, 2>();
-    if (optional_time.tag() == parse_optional_time_time) flags.has_time_prefix = true;
+    if (optional_time.tag() == parse_optional_time_time) {
+        flags.has_time_prefix = true;
+        if (!job->mut_flags().foreground) {
+            this->report_error(not_statement, ERROR_TIME_BACKGROUND);
+            return eval_result_t::error;
+        }
+    }
     return this->populate_job_process(
         job, proc, not_statement.require_get_child<g::statement, 3>(),
         not_statement.require_get_child<g::variable_assignments, 1>());
@@ -1121,7 +1127,13 @@ eval_result_t parse_execution_context_t::populate_job_from_job_node(
     // Create processes. Each one may fail.
     process_list_t processes;
     processes.emplace_back(new process_t());
-    if (optional_time.tag() == parse_optional_time_time) j->mut_flags().has_time_prefix = true;
+    if (optional_time.tag() == parse_optional_time_time) {
+        j->mut_flags().has_time_prefix = true;
+        if (job_node_is_background(job_node)) {
+            this->report_error(job_node, ERROR_TIME_BACKGROUND);
+            return eval_result_t::error;
+        }
+    }
     eval_result_t result =
         this->populate_job_process(j, processes.back().get(), statement, variable_assignments);
 
diff --git a/tests/checks/time.fish b/tests/checks/time.fish
index 38c13dc8e..e42ae9e24 100644
--- a/tests/checks/time.fish
+++ b/tests/checks/time.fish
@@ -1,5 +1,5 @@
-#RUN: %fish %s
-time sleep 1s
+#RUN: %fish -C 'set -l fish %fish' %s
+time sleep 0s
 
 # These are a tad awkward because it picks the correct unit and adapts whitespace.
 # The idea is that it's a table.
@@ -43,3 +43,13 @@ not time true
 #CHECKERR: {{.*}}
 #CHECKERR: {{.*}}
 #CHECKERR: {{.*}}
+
+$fish -c 'time true&'
+#CHECKERR: fish: {{.*}}
+#CHECKERR: time true&
+#CHECKERR: ^
+
+$fish -c 'not time true&'
+#CHECKERR: fish: {{.*}}
+#CHECKERR: not time true&
+#CHECKERR: ^