Rework caching

Online/offline functionality is now controlled by the single Boolean
git variable "magithub.online", whose values are documented within
`magithub-settings-popup':

- `true': requests are made to GitHub for missing data
- `false': no requests are made to GitHub

RelNotes/0.1.6.org

@@ -47,7 +47,14 @@
 - =hub.host= is no longer respected and has been replaced by user option
   ~magithub-github-hosts~.  This most directly impacts GitHub Enterprise
   support.
-- ~magithub-cache-invalidate~ was not used, so it is no longer available.
+** Caching  [[PR:328]]
+Caching has been reworked mostly from the ground-up.  'Offline mode'
+is now manifest in a single, Boolean-valued git variable
+"magithub.cache", which see.
+
+- ~magithub-cache-invalidate~ was not used, so it is no longer
+  available.
+- ~magithub-issue-refresh~ no longer takes parameters.
 
 * New Features
 - Browse commits by using =w= on a commit section.  If the current

magithub-ci.el

@@ -103,7 +103,7 @@ remote counterpart."
   (setq branch (or branch (magit-get-current-branch)))
   (if-let ((pull-request
             (or (magithub-pull-request-branch->pr--gitconfig branch)
-                (and (not (magithub-offline-p))
+                (and (magithub-online-p)
                      (with-demoted-errors "Error: %S"
                        (magithub-pull-request-branch->pr--ghub branch))))))
       (let-alist pull-request .head.sha)
@@ -196,11 +196,9 @@ remote counterpart."
   "Keymap for `magithub-ci-status' header section.")
 
 (defun magithub-ci-refresh ()
-  "Invalidate the CI cache and refresh the buffer.
-If EVEN-IF-OFFLINE is non-nil, we'll still refresh (that is,
-we'll hit the API) if Magithub is offline."
+  "Invalidate the CI cache and refresh the buffer."
   (interactive)
-  (when (magithub-offline-p)
+  (unless (magithub-online-p)
     (magithub-confirm 'ci-refresh-when-offline))
   (magithub-cache-without-cache :ci-status
     (magithub-ci-status (magithub-ci-status--get-default-ref)))

magithub-core.el

@@ -104,10 +104,6 @@ created automatically."
 
 ;;; Caching; Online/Offline mode
 
-(defun magithub-offline-p ()
-  "Non-nil if Magithub is not supposed to make API requests."
-  (memq (magithub-settings-cache-behavior) '(t refreshing-when-offline)))
-
 (defcustom magithub-cache-file "cache"
   "Use this file for Magithub's persistent cache."
   :group 'magithub
@@ -136,54 +132,47 @@ Occasionally written to `magithub-cache-file' by
 When non-nil, the cache will be written to disk next time the
 idle timer runs.")
 
-(defvar magithub-cache-ignore-class nil
-  "Class to ignore in `magithub-cache'.
-See also `magithub-cache-without-cache'.
-
-If t, all classes are ignored.")
-
 (defvar magithub-cache--refreshed-forms nil
   "Forms that have been refreshed this session.
-See also `magithub-refresh'.")
+See also `magithub--refresh'.")
 
 (cl-defun magithub-cache (class form &key message after-update)
   "The cached value for FORM if available.
 
 If FORM has not been cached or its CLASS dictates the cache has
 expired, FORM will be re-evaluated.
 
-CLASS: The kind of data this is; see `magithub-cache-ignore-class'.
+CLASS: The kind of data this is; see `magithub-cache--refresh'.
 
 MESSAGE may be specified for intensive functions.  We'll display
 this with `with-temp-message' while the form is evaluating.
 
 AFTER-UPDATE is a function to run after the cache is updated."
   (declare (indent defun))
-  (let* ((behavior (magithub-settings-cache-behavior))
+  (let* ((no-value-sym (cl-gensym))
          (entry (list (ghubp-get-context) class form))
-         (refreshing (memq behavior '(refreshing refreshing-when-offline)))
-         (recalc (or (null behavior)
-                     (and refreshing
-                          (not (member entry magithub-cache--refreshed-forms)))
-                     (and magithub-cache-ignore-class
-                          (or (eq magithub-cache-ignore-class t)
-                              (eq magithub-cache-ignore-class class)))))
-         no-value-sym cached-value new-value)
-
-    (unless recalc
-      (setq no-value-sym (cl-gensym)
-            cached-value (gethash entry magithub-cache--cache no-value-sym)
-            recalc (and (not (eq behavior t))
-                        (eq cached-value no-value-sym))
-            cached-value (if (eq cached-value no-value-sym) nil cached-value)))
-
-    (or (and recalc
+         (online (magithub-online-p))
+         (cached-value (gethash entry magithub-cache--cache no-value-sym))
+         (value-does-not-exist (eq cached-value no-value-sym))
+         (cached-value (if value-does-not-exist nil cached-value))
+         make-request new-value)
+
+    (when online
+      (if (or (eq magithub-cache--refresh t)
+              (eq magithub-cache--refresh class))
+          ;; if we're refreshing (and we haven't seen this form
+          ;; before), go ahead and make the request if it's the class
+          ;; we're refreshing (or t, which encompasses all classes)
+          (setq make-request (not (member entry magithub-cache--refreshed-forms)))
+        (setq make-request value-does-not-exist)))
+
+    (or (and make-request
              (prog1 (setq new-value (with-temp-message message (eval form)))
                (puthash entry new-value magithub-cache--cache)
                (unless magithub-cache--needs-write
                  (setq magithub-cache--needs-write t)
                  (run-with-idle-timer 600 nil #'magithub-cache-write-to-disk))
-               (when refreshing
+               (when magithub-cache--refresh
                  (push entry magithub-cache--refreshed-forms))
                (if (functionp after-update)
                    (funcall after-update new-value)
@@ -197,7 +186,7 @@ insert a header notifying the user that all data shown is cached.
 To aid in determining if the cache should be refreshed, we report
 the age of the oldest cached information."
   (when (and (magithub-usable-p)
-             (magithub-offline-p))
+             (not (magithub-online-p)))
     (magit-insert-section (magithub nil t)
       (insert
        (format
@@ -258,9 +247,9 @@ The cache is written to `magithub-cache-file' in
 (defmacro magithub-cache-without-cache (class &rest body)
   "For CLASS, execute BODY without using CLASS's caches.
 Use t to ignore previously cached values completely.
-See also `magithub-cache-ignore-class'."
+See also `magithub-cache--refresh'."
   (declare (indent 1) (debug t))
-  `(let ((magithub-cache-ignore-class ,class))
+  `(let ((magithub-cache--refresh ,class))
      ,@body))
 
 (add-hook 'kill-emacs-hook
@@ -510,9 +499,8 @@ of the form `owner/name' (as in `vermiculus/magithub')."
                           sparse-repo))
                ;; Repo may not exist; ignore 404
                (ghub-404 nil)))
-          (when (memq (magithub-settings-cache-behavior)
-		      '(when-present refreshing-when-offline))
-            (let ((magithub-settings-cache-behavior-override nil))
+          (when (magithub-online-p)
+            (let ((magithub-cache--refresh t))
               (magithub-repo sparse-repo)))
           sparse-repo))))
 
@@ -1134,19 +1122,31 @@ COMPARE is used on the application of ACCESSOR to each argument."
   (interactive)
   (magit-section-show-level -5))
 
-(defun magithub-reset-settings-cache-behavior-override ()
+(defun magithub--refresh-reset ()
   "Reset everything to the defaults after refreshing.
 To be added to `magit-unwind-refresh-hook'."
-  (setq magithub-settings-cache-behavior-override 'none)
+  (setq magithub-cache--refresh nil)
+  ;; reclaim some memory
   (setq magithub-cache--refreshed-forms nil))
 
+(defvar magithub-cache--refresh nil
+  ;; Can also consider making this a list in the future to refresh
+  ;; multiple forms.  No current use-case for this, though.
+  "Non-nil when refreshing.
+If t, all form classes will be refreshed.  Otherwise, if non-nil,
+this variable is expected to be `eq' to the class of forms that
+should be selectively refreshed.")
+
+(make-obsolete 'magithub-refresh 'magithub--refresh "0.2")
 (defun magithub-refresh ()
+  (interactive (user-error (substitute-command-keys
+			    "This is no longer an interactive function; \
+use \\[universal-argument] \\[magit-refresh] instead :-)"))))
+
+(defun magithub--refresh ()
   "Refresh GitHub data.
 Use directly at your own peril; this is intended for use with
 `magit-pre-refresh-hook'."
-  (interactive (user-error (substitute-command-keys
-			    "This is no longer an interactive function; \
-use \\[universal-argument] \\[magit-refresh] instead :-)")))
   (when (and current-prefix-arg
              (memq this-command '(magit-refresh
                                   magit-refresh-all
@@ -1156,16 +1156,13 @@ use \\[universal-argument] \\[magit-refresh] instead :-)")))
              (magithub-confirm-no-error 'refresh)
              (or (magithub--api-available-p)
                  (magithub-confirm-no-error 'refresh-when-API-unresponsive)))
-    ;; `magithub-refresh' is part of `magit-pre-refresh-hook' and our requests
+    ;; `magithub--refresh' is part of `magit-pre-refresh-hook' and our requests
     ;; are made as part of `magit-refresh'.  There's no way we can let-bind
-    ;; `magithub-settings-cache-behavior-override' around that entire form, so
-    ;; we do the next best thing: use `magit-unwind-refresh-hook' to reset the
-    ;; override back to its old value.
-    (setq magithub-settings-cache-behavior-override
-          (pcase (magithub-settings-cache-behavior)
-            (`t 'refreshing-when-offline)
-            (`nil nil)
-            (`when-present 'refreshing)))))
+    ;; `magithub-settings--refresh' around that entire form, so  we do the next
+    ;; best thing: use `magit-unwind-refresh-hook' to reset the  override back
+    ;; to its old value.
+    (setq magithub-cache--refresh t)
+    (setq magithub-cache--refreshed-forms nil)))
 
 (defun magithub-wash-gfm (text)
   "Wash TEXT as it comes from the API."
@@ -1251,9 +1248,9 @@ Interactively, this is the commit at point."
   '(progn
      (dolist (hook '(magit-revision-mode-hook git-commit-setup-hook))
        (add-hook hook #'magithub-bug-reference-mode-on))
-     (add-hook 'magit-pre-refresh-hook #'magithub-refresh)
+     (add-hook 'magit-pre-refresh-hook #'magithub--refresh)
      (add-hook 'magit-unwind-refresh-hook
-	       #'magithub-reset-settings-cache-behavior-override)))
+	       #'magithub--refresh-reset)))
 
 (provide 'magithub-core)
 ;;; magithub-core.el ends here

magithub-issue.el

@@ -401,16 +401,11 @@ Each function takes two arguments:
 
 ;;; Magithub-Status stuff
 
-
-(defun magithub-issue-refresh (even-if-offline)
-  "Refresh issues for this repository.
-If EVEN-IF-OFFLINE is non-nil, we'll still refresh (that is,
-we'll hit the API) if Magithub is offline."
-  (interactive "P")
-  (let ((magithub-settings-cache-behavior-override
-         (if even-if-offline nil (magithub-settings-cache-behavior))))
-    (magithub-cache-without-cache :issues
-      (ignore (magithub--issue-list))))
+(defun magithub-issue-refresh ()
+  "Refresh issues for this repository."
+  (interactive)
+  (magithub-cache-without-cache :issues
+    (magithub--issue-list))
   (when (derived-mode-p 'magit-status-mode)
     (magit-refresh)))
 

magithub-settings.el

@@ -66,28 +66,23 @@
   (magithub-settings--value-or "magithub.enabled" t
     #'magit-get-boolean))
 
-(magithub-settings--simple magithub-settings-popup ?c "cache"
-  "Controls the cache.
-
-- `always': The cache will not be used, but it will be updated
-
-- `never': The cache is never used.  Data is always re-retrieved
-           when online.  When offline, there is simply no data.
-
-- `whenPresent': The cache is used if it exists.  If there is
-                 nothing cached, data is retrieved when online.
-                 When offline, there is simply no data."
-  '("always" "never" "whenPresent") "whenPresent")
-
-(defvar magithub-settings-cache-behavior-override 'none)
-(defun magithub-settings-cache-behavior ()
-  (if (not (eq magithub-settings-cache-behavior-override 'none))
-      magithub-settings-cache-behavior-override
-    (pcase (magithub-settings--value-or "magithub.cache" "whenPresent")
-      ("always" t)
-      ("never" nil)
-      ("whenPresent" 'when-present)
-      (_ 'when-present))))
+(magithub-settings--simple magithub-settings-popup ?o "online"
+  "Controls whether Magithub is online or offline.
+
+- `true': requests are made to GitHub for missing data
+- `false': no requests are made to GitHub
+
+In both cases, when there is data in the cache, that data is
+used.  Refresh the buffer with a prefix argument to disregard the
+cache while refreshing: \\<magit-mode-map>\\[universal-argument] \\[magit-refresh]"
+  '("true" "false") "true")
+
+(defun magithub-online-p ()
+  "See `magithub-settings--set-magithub.online'.
+Returns the value as t or nil."
+  (magithub-settings--value-or "magithub.online" t
+    #'magit-get-boolean))
+
 
 (magithub-settings--simple magithub-settings-popup ?s "status.includeStatusHeader"
   "When true, the project status header is included in

test/magithub-test.el

@@ -40,9 +40,8 @@ cached API calls."
   (let ((magithub--api-last-checked (current-time)))
     (should (magithub-source--sparse-repo))
     (should (magithub-repo))
-    (should (let ((magithub-settings-cache-behavior-override t))     ; force API call
+    (should (let ((magithub-cache--refresh t)) ; force API call
               (magithub-repo)))
-    (should (let ((magithub-settings-cache-behavior-override nil))   ; force cache read
-              (magithub-repo)))))
+    (should (magithub-repo))))                 ; force cache read
 
 ;;; magithub-test.el ends here

test/test-helper.el

@@ -32,8 +32,8 @@ variable is not set, offer to save a snapshot of the real API's
 response."
   (message "(mock-ghub-request %S %S %S :query %S :payload %S :headers %S :unpaginate %S :noerror %S :reader %S :username %S :auth %S :host %S)"
            method resource params query payload headers unpaginate noerror reader username auth host)
-  (when (eq t (magithub-settings-cache-behavior))
-    (error "Did not respect cache"))
+  (when (not (magithub-online-p))
+    (error "Did not respect online/offline"))
   (let* ((parts (cdr (s-split "/" resource)))
          (directory (mapconcat (lambda (s) (concat s ".d"))
                                (butlast parts) "/"))