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

[Justin Burkett]

Thank you for pointing that out, Stefan, and thank you for the patch.

After thinking about it, I think I would prefer to split
vdiff-magit.el into a separate package. The two reasons are that magit
is not on ELPA, and the second is that I think there's a chance that
vdiff-magit might eventually be incorporated into the magit package
instead (it's more appropriate for it to be there in my opinion).

Justin

On Wed, May 17, 2017 at 12:17 PM, Stefan Monnier
<address@hidden> wrote:
> 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))