branch: elpa/git-commit
commit 4bcb3031542deee77e5a5899e266fbe61f958ced
Author: Jonas Bernoulli <jo...@bernoul.li>
Commit: Jonas Bernoulli <jo...@bernoul.li>
Use and-let* for side-effects even more
The previous commit also replaces many instances of `when-let' with
`and-let'. In those cases I did not hesitate. This commit addresses
instances where side-effects are involved.
My current reasoning is that if the returned value matters, then
`and'/`and-let*' should be used, and whether there are side-effects is
only a secondary consideration. Of course there is a gray area; if it
is really important to emphasize the side-effect, then one might still
want to use `when'/`when-let*'.
The simplest case is the change to `magit-diff-visit--hunk'. That
function doesn't actually visit anything; instead it returns the hunk
which is relevant when visiting. But the function name could give the
false impression that it somehow visits a hunk. Preventing that false
impression is a good reason to use `and-let*'. Additionally the
side-effect of the first form in BODY is local to the BODY, to the
outside code this is an irrelevant implementation detail.
`magit-diff--dwim' call `deactivate-mark', which clearly is a
side-effect that affects the global buffer state. However this
side-effect is not relevant to the direct outside code, which on the
other hand cares very much about the returned value.
Finally `magit-hunk-goto-successor', which is very much about the
side-effect of "going to the successor". This is so obvious that I
don't think it is necessary to signal the side-effect-ness. Again the
returned value is used for flow-control, which should be made obvious,
so `and-let*' it is.
Unfortunately, due to a bug in Emacs 26, we cannot actually use
`and-let*' in some of these cases.
---
lisp/magit-diff.el | 91 +++++++++++++++++++++++++++---------------------------
lisp/magit-git.el | 6 ++--
lisp/magit-refs.el | 2 +-
3 files changed, 50 insertions(+), 49 deletions(-)
diff --git a/lisp/magit-diff.el b/lisp/magit-diff.el
index f942b42bdf..1141973675 100644
--- a/lisp/magit-diff.el
+++ b/lisp/magit-diff.el
@@ -1090,9 +1090,10 @@ The information can be in three forms:
If no DWIM context is found, nil is returned."
(cond
- ((--when-let (magit-region-values '(commit branch) t)
+ ((when-let* ((commits (magit-region-values '(commit branch) t)))
+ ;; Cannot use and-let* because of debbugs#31840.
(deactivate-mark)
- (concat (car (last it)) ".." (car it))))
+ (concat (car (last commits)) ".." (car commits))))
(magit-buffer-refname
(cons 'commit magit-buffer-refname))
((derived-mode-p 'magit-stash-mode)
@@ -1141,26 +1142,26 @@ If no DWIM context is found, nil is returned."
(t range)))
(defun magit-diff--region-range (&optional interactive mbase)
- (when-let ((commits (magit-region-values '(commit branch) t)))
- (let ((revA (car (last commits)))
- (revB (car commits)))
- (when interactive
- (deactivate-mark))
- (if mbase
- (let ((base (magit-git-string "merge-base" revA revB)))
- (cond
- ((string= (magit-rev-parse revA) base)
- (format "%s..%s" revA revB))
- ((string= (magit-rev-parse revB) base)
- (format "%s..%s" revB revA))
- (interactive
- (let ((main (magit-completing-read "View changes along"
- (list revA revB)
- nil t nil nil revB)))
- (format "%s...%s"
- (if (string= main revB) revA revB) main)))
- (t "%s...%s" revA revB)))
- (format "%s..%s" revA revB)))))
+ (when-let* ((commits (magit-region-values '(commit branch) t)) ;debbugs#31840
+ (revA (car (last commits)))
+ (revB (car commits)))
+ (when interactive
+ (deactivate-mark))
+ (if mbase
+ (let ((base (magit-git-string "merge-base" revA revB)))
+ (cond
+ ((string= (magit-rev-parse revA) base)
+ (format "%s..%s" revA revB))
+ ((string= (magit-rev-parse revB) base)
+ (format "%s..%s" revB revA))
+ (interactive
+ (let ((main (magit-completing-read "View changes along"
+ (list revA revB)
+ nil t nil nil revB)))
+ (format "%s...%s"
+ (if (string= main revB) revA revB) main)))
+ (t "%s...%s" revA revB)))
+ (format "%s..%s" revA revB))))
(defun magit-diff-read-range-or-commit (prompt &optional secondary-default
mbase)
"Read range or revision with special diff range treatment.
@@ -1677,24 +1678,24 @@ the Magit-Status buffer for DIRECTORY."
(list buf nil))))
(defun magit-diff-visit--hunk ()
- (when-let ((scope (magit-diff-scope)))
- (let ((section (magit-current-section)))
- (cl-case scope
- ((file files)
- (setq section (car (oref section children))))
- (list
- (setq section (car (oref section children)))
- (when section
- (setq section (car (oref section children))))))
- (and
- ;; Unmerged files appear in the list of staged changes
- ;; but unlike in the list of unstaged changes no diffs
- ;; are shown here. In that case `section' is nil.
- section
- ;; Currently the `hunk' type is also abused for file
- ;; mode changes, which we are not interested in here.
- (not (equal (oref section value) '(chmod)))
- section))))
+ (when-let* ((scope (magit-diff-scope)) ;debbugs#31840
+ (section (magit-current-section)))
+ (cl-case scope
+ ((file files)
+ (setq section (car (oref section children))))
+ (list
+ (setq section (car (oref section children)))
+ (when section
+ (setq section (car (oref section children))))))
+ (and
+ ;; Unmerged files appear in the list of staged changes
+ ;; but unlike in the list of unstaged changes no diffs
+ ;; are shown here. In that case `section' is nil.
+ section
+ ;; Currently the `hunk' type is also abused for file
+ ;; mode changes, which we are not interested in here.
+ (not (equal (oref section value) '(chmod)))
+ section)))
(defun magit-diff-visit--goto-from-p (section in-worktree)
(and magit-diff-visit-previous-blob
@@ -2843,7 +2844,7 @@ It the SECTION has a different type, then do nothing."
(defun magit-hunk-goto-successor (section arg)
(and (magit-hunk-section-p section)
- (when-let ((parent (magit-get-section
+ (and-let* ((parent (magit-get-section
(magit-section-ident
(oref section parent)))))
(let* ((children (oref parent children))
@@ -3191,10 +3192,10 @@ are highlighted."
(cond
((not magit-diff-adjust-tab-width)
tab-width)
- ((--when-let (find-buffer-visiting file)
- (cache (buffer-local-value 'tab-width it))))
- ((--when-let (assoc file magit-diff--tab-width-cache)
- (or (cdr it)
+ ((and-let* ((buffer (find-buffer-visiting file)))
+ (cache (buffer-local-value 'tab-width buffer))))
+ ((and-let* ((elt (assoc file magit-diff--tab-width-cache)))
+ (or (cdr elt)
tab-width)))
((or (eq magit-diff-adjust-tab-width 'always)
(and (numberp magit-diff-adjust-tab-width)
diff --git a/lisp/magit-git.el b/lisp/magit-git.el
index 46fc791ca4..d5b8d8aa6c 100644
--- a/lisp/magit-git.el
+++ b/lisp/magit-git.el
@@ -2119,9 +2119,9 @@ Return a list of two integers: (A>B B>A)."
(if rev (concat rev "^{commit}") "HEAD") "--"))
(defun magit-format-rev-summary (rev)
- (--when-let (magit-rev-format "%h %s" rev)
- (magit--put-face 0 (string-match " " it) 'magit-hash it)
- it))
+ (when-let* ((str (magit-rev-format "%h %s" rev))) ;debbugs#31840
+ (magit--put-face 0 (string-match " " str) 'magit-hash str)
+ str))
(defvar magit-ref-namespaces
'(("\\`HEAD\\'" . magit-head)
diff --git a/lisp/magit-refs.el b/lisp/magit-refs.el
index c6f9ef0598..c1216f919c 100644
--- a/lisp/magit-refs.el
+++ b/lisp/magit-refs.el
@@ -359,7 +359,7 @@ Type \\[magit-reset] to reset `HEAD' to the commit at point.
((eq major-mode 'magit-refs-mode)
(setq args magit-buffer-arguments))
((and (memq use-buffer-args '(always selected))
- (when-let ((buffer (magit-get-mode-buffer
+ (when-let* ((buffer (magit-get-mode-buffer ;debbugs#31840
'magit-refs-mode nil
(eq use-buffer-args 'selected))))
(setq args (buffer-local-value 'magit-buffer-arguments buffer))
