[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [elpa] scratch/add-vdiff 0f640d3 007/258: Add header info
From: |
Justin Burkett |
Subject: |
Re: [elpa] scratch/add-vdiff 0f640d3 007/258: Add header info |
Date: |
Wed, 17 May 2017 12:41:31 -0400 |
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))