Fix unnecessary addition of Expires header

proxy/http/HttpSM.cc

@@ -3055,7 +3055,7 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p)
       // the reason string being written to the client and a bad CL when reading from cache.
       // I didn't find anywhere this appended reason is being used, so commenting it out.
       /*
-        if (t_state.is_cacheable_and_negative_caching_is_enabled && p->bytes_read == 0) {
+        if (t_state.is_cacheable_due_to_negative_caching_configuration && p->bytes_read == 0) {
         int reason_len;
         const char *reason = t_state.hdr_info.server_response.reason_get(&reason_len);
         if (reason == NULL)
@@ -3111,8 +3111,8 @@ HttpSM::tunnel_handler_server(int event, HttpTunnelProducer *p)
   }
 
   // turn off negative caching in case there are multiple server contacts
-  if (t_state.is_cacheable_and_negative_caching_is_enabled) {
-    t_state.is_cacheable_and_negative_caching_is_enabled = false;
+  if (t_state.is_cacheable_due_to_negative_caching_configuration) {
+    t_state.is_cacheable_due_to_negative_caching_configuration = false;
   }
 
   // If we had a ground fill, check update our status
@@ -6735,7 +6735,7 @@ HttpSM::setup_server_transfer()
 
   nbytes = server_transfer_init(buf, hdr_size);
 
-  if (t_state.is_cacheable_and_negative_caching_is_enabled &&
+  if (t_state.is_cacheable_due_to_negative_caching_configuration &&
       t_state.hdr_info.server_response.status_get() == HTTP_STATUS_NO_CONTENT) {
     int s = sizeof("No Content") - 1;
     buf->write("No Content", s);

proxy/http/HttpTransact.cc

@@ -4402,7 +4402,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
     client_response_code = server_response_code;
     base_response        = &s->hdr_info.server_response;
 
-    s->is_cacheable_and_negative_caching_is_enabled = cacheable && s->txn_conf->negative_caching_enabled;
+    s->is_cacheable_due_to_negative_caching_configuration = cacheable && is_negative_caching_appropriate(s);
 
     // determine the correct cache action given the original cache action,
     // cacheability of server response, and request method
@@ -4464,7 +4464,7 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
     //   before issuing a 304
     if (s->cache_info.action == CACHE_DO_WRITE || s->cache_info.action == CACHE_DO_NO_ACTION ||
         s->cache_info.action == CACHE_DO_REPLACE) {
-      if (s->is_cacheable_and_negative_caching_is_enabled) {
+      if (s->is_cacheable_due_to_negative_caching_configuration) {
         HTTPHdr *resp;
         s->cache_info.object_store.create();
         s->cache_info.object_store.request_set(&s->hdr_info.client_request);
@@ -4500,8 +4500,8 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
           SET_VIA_STRING(VIA_PROXY_RESULT, VIA_PROXY_SERVER_REVALIDATED);
         }
       }
-    } else if (s->is_cacheable_and_negative_caching_is_enabled) {
-      s->is_cacheable_and_negative_caching_is_enabled = false;
+    } else if (s->is_cacheable_due_to_negative_caching_configuration) {
+      s->is_cacheable_due_to_negative_caching_configuration = false;
     }
 
     break;
@@ -4911,7 +4911,7 @@ HttpTransact::set_headers_for_cache_write(State *s, HTTPInfo *cache_info, HTTPHd
      sites yields no insight. So the assert is removed and we keep the behavior that if the response
      in @a cache_info is already set, we don't override it.
   */
-  if (!s->is_cacheable_and_negative_caching_is_enabled || !cache_info->response_get()->valid()) {
+  if (!s->is_cacheable_due_to_negative_caching_configuration || !cache_info->response_get()->valid()) {
     cache_info->response_set(response);
   }
 

proxy/http/HttpTransact.h

@@ -737,23 +737,15 @@ class HttpTransact
     bool client_connection_enabled = true;
     bool acl_filtering_performed   = false;
 
-    /// True if negative caching is enabled and the response is cacheable.
+    /// True if the response is cacheable because of negative caching configuration.
     ///
-    /// Note carefully that this being true does not necessarily imply that the
-    /// response code was negative. It means that (a) the response was
-    /// cacheable apart from response code considerations, and (b) concerning
-    /// the response code one of the following was true:
+    /// This being true implies the following:
     ///
-    ///   * The response was a negative response code configured cacheable
-    ///   by the user via negative response caching configuration, or ...
-    ///
-    ///   * The response code was an otherwise cacheable positive repsonse
-    ///   value (such as a 200 response, for example).
-    ///
-    /// TODO: We should consider refactoring this variable and its use. For now
-    /// I'm giving it an awkwardly long name to make sure the meaning of it is
-    /// clear in its various contexts.
-    bool is_cacheable_and_negative_caching_is_enabled = false;
+    /// * The response code was negative.
+    /// * Negative caching is enabled.
+    /// * The response is considered cacheable because of negative caching
+    ///   configuration.
+    bool is_cacheable_due_to_negative_caching_configuration = false;
     // for authenticated content caching
     CacheAuth_t www_auth_content = CACHE_AUTH_NONE;