Re: [elpa] scratch/add-vdiff 0f640d3 007/258: Add header info

[Stefan Monnier]

BTW, I just took another look at that branch and I see one problem with
it: when the user installs the vcdiff.tar ELPA package that will be
built from it, she'll (potentially) get the following byte-compiler
error:

vdiff-magit.el:27:1:Error: Cannot open load file: Aucun fichier ou dossier de 
ce type, magit

Until now all packages which had this kind of "soft dependency" were
made to work with a trick like

    (if t (require 'magit)) ;; Don't require at compile-time.

so the error is only signaled if/when the user tries to use
vdiff-magit package, at which point it's definitely OK to complain that
we can't find Magit.

But in the case at hand this trick doesn't quite work because
vdiff-magit.el uses Magit macros (e.g. magit-define-popup), so it
needs Magit.  There's also the fact that the file uses dash macros
without requiring dash (presumably because it's normally brought in via
Magit).

One way to solve this is to delay macroexpansion of those uses of Magit
macros, which is what I do in the patch below (which also switches to
using pcase instead of dash).  Another would be to just mark the file as
no-byte-compile.  Yet another would be to split this into its own
package (which would then depend on vdiff and magit).


        Stefan


diff --git a/packages/vdiff/vdiff-magit.el b/packages/vdiff/vdiff-magit.el
index 241df34ae..b0f9b2574 100644
--- a/packages/vdiff/vdiff-magit.el
+++ b/packages/vdiff/vdiff-magit.el
@@ -24,8 +24,8 @@
 ;;; Code:
 
 (require 'vdiff)
-(require 'magit)
-(require 'magit-ediff)
+(if t (require 'magit))
+(if t (require 'magit-ediff))
 
 (defgroup vdiff-magit nil
   "vdiff support for Magit."
@@ -38,7 +38,6 @@ If non-nil, `vdiff-magit-show-staged' or
 hunk is in.  Otherwise, `vdiff-magit-dwim' runs
 `vdiff-magit-stage' when point is on an uncommitted hunk."
   ;; :package-version '(magit . "2.2.0")
-  :group 'vdiff-magit
   :type 'boolean)
 
 (defcustom vdiff-magit-show-stash-with-index t
@@ -70,7 +69,6 @@ in the index commit, address@hidden, will be shown in this
 comparison unless they conflicted with changes in the working
 tree at the time of stashing."
   ;; :package-version '(magit . "2.6.0")
-  :group 'vdiff-magit
   :type 'boolean)
 
 (defcustom vdiff-magit-use-ediff-for-merges nil
@@ -82,19 +80,36 @@ requiring a 3-way merge it will abort and forward to
 `magit-ediff-resolve' instead. The purpose of this flag is to
 make the merge experience consistent across all types of
 merges."
-  :group 'vdiff-magit
   :type 'boolean)
 
 (defcustom vdiff-magit-stage-is-2way nil
   "If non-nil `vdiff-magit-stage' will only show two buffers, the
 file and the index with the HEAD omitted."
-  :group 'vdiff-magit
   :type 'boolean)
 
 ;; (defvar magit-ediff-previous-winconf nil)
 
+(defmacro vdiff-magit--delay-macro (form)
+  "Delay macro-expansion of FORM if needed.
+More specifically, if FORM's macro is not yet defined at compile-time,
+keep it unexpanded until runtime.
+BEWARE: FORM can't refer to its lexical context."
+  (if (fboundp (car form))
+      form
+    ;; This means form will be macro-expanded every time the code is run.
+    ;; We could try to be more clever to avoid repeated macroexpansion, e.g.
+    ;; `(let ((form ',form))
+    ;;    (when (macrop (car-safe form))
+    ;;      (let ((expanded-form (macroexpand-all form)))
+    ;;        (when (consp expanded-form)
+    ;;          (setcar form (car expanded-form))
+    ;;          (setcdr form (cdr expanded-form)))))
+    ;;    (eval form t))
+    `(eval ',form t)))
+
 ;;;###autoload (autoload 'vdiff-magit-popup "vdiff-magit" nil t)
-(magit-define-popup vdiff-magit-popup
+(vdiff-magit--delay-macro
+ (magit-define-popup vdiff-magit-popup
   "Popup console for vdiff commands."
   :actions '((?d "Dwim"          vdiff-magit-dwim)
              (?u "Show unstaged" vdiff-magit-show-unstaged)
@@ -105,7 +120,7 @@ file and the index with the HEAD omitted."
              (?r "Diff range"    vdiff-magit-compare)
              (?c "Show commit"   vdiff-magit-show-commit) nil
              (?z "Show stash"    vdiff-magit-show-stash))
-  :max-action-columns 2)
+  :max-action-columns 2))
 
 ;;;###autoload
 (defun vdiff-magit-resolve (file)
@@ -195,8 +210,9 @@ line of the region.  With a prefix argument, instead of 
diffing
 the revisions, choose a revision to view changes along, starting
 at the common ancestor of both revisions (i.e., use a \"...\"
 range)."
-  (interactive (-let [(rev-a rev-b) (magit-ediff-compare--read-revisions
-                                   nil current-prefix-arg)]
+  (interactive (pcase-let ((`(,rev-a ,rev-b)
+                            (magit-ediff-compare--read-revisions
+                             nil current-prefix-arg)))
                  (nconc (list rev-a rev-b)
                         (magit-ediff-read-files rev-a rev-b))))
   (magit-with-toplevel
@@ -225,7 +241,8 @@ always guess right, in which case the appropriate 
`vdiff-magit-*'
 command has to be used explicitly.  If it cannot read the user's
 mind at all, then it asks the user for a command to run."
   (interactive)
-  (magit-section-case
+  (vdiff-magit--delay-macro
+   (magit-section-case
     (hunk (save-excursion
             (goto-char (magit-section-start (magit-section-parent it)))
             (vdiff-magit-dwim)))
@@ -267,13 +284,13 @@ mind at all, then it asks the user for a command to run."
        (cond ((not command)
               (call-interactively
                (magit-read-char-case
-                   "Failed to read your mind; do you want to " t
-                 (?c "[c]ommit"  'vdiff-magit-show-commit)
-                 (?r "[r]ange"   'vdiff-magit-compare)
-                 (?s "[s]tage"   'vdiff-magit-stage)
-                 (?v "resol[v]e" 'vdiff-magit-resolve))))
+                "Failed to read your mind; do you want to " t
+                (?c "[c]ommit"  'vdiff-magit-show-commit)
+                (?r "[r]ange"   'vdiff-magit-compare)
+                (?s "[s]tage"   'vdiff-magit-stage)
+                (?v "resol[v]e" 'vdiff-magit-resolve))))
              ((eq command 'vdiff-magit-compare)
-              (apply 'vdiff-magit-compare rev-a rev-b
+              (apply #'vdiff-magit-compare rev-a rev-b
                      (magit-ediff-read-files rev-a rev-b file)))
              ((eq command 'vdiff-magit-show-commit)
               (vdiff-magit-show-commit rev-b))
@@ -282,7 +299,7 @@ mind at all, then it asks the user for a command to run."
              (file
               (funcall command file))
              (t
-              (call-interactively command)))))))
+              (call-interactively command))))))))
 
 ;; ;;;###autoload
 (defun vdiff-magit-show-staged (file)
@@ -355,11 +372,11 @@ FILE must be relative to the top directory of the 
repository."
 three-buffer vdiff is used in order to distinguish changes in the
 stash that were staged."
   (interactive (list (magit-read-stash "Stash")))
-  (-let* ((rev-a (concat stash "^1"))
-          (rev-b (concat stash "^2"))
-          (rev-c stash)
-          ((file-a file-c) (magit-ediff-read-files rev-a rev-c))
-          (file-b file-c))
+  (pcase-let* ((rev-a (concat stash "^1"))
+               (rev-b (concat stash "^2"))
+               (rev-c stash)
+               (`(,file-a ,file-c) (magit-ediff-read-files rev-a rev-c))
+               (file-b file-c))
     (if (and vdiff-magit-show-stash-with-index
              (member file-a (magit-changed-files rev-b rev-a)))
         (let ((buf-a (magit-get-revision-buffer rev-a file-a))