From: Justin Burkett <justin@burkett.cc>
To: Stefan Monnier <monnier@iro.umontreal.ca>
Cc: emacs-devel@gnu.org
Subject: Re: [elpa] scratch/add-vdiff 0f640d3 007/258: Add header info
Date: Wed, 17 May 2017 12:41:31 -0400 [thread overview]
Message-ID: <CAF5dqNL78f3tn-qYcHziAxi0yHxW4T0AJuLoA_dfZ=x-boChRA@mail.gmail.com> (raw)
In-Reply-To: <jwvinkza3fo.fsf-monnier+emacsdiffs@gnu.org>
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
<monnier@iro.umontreal.ca> 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, stash@{N}^2, 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))
next prev parent reply other threads:[~2017-05-17 16:41 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20170517121232.22782.37573@vcs0.savannah.gnu.org>
[not found] ` <20170517121311.B646A20E24@vcs0.savannah.gnu.org>
2017-05-17 12:42 ` [elpa] scratch/add-vdiff 0f640d3 007/258: Add header info Stefan Monnier
2017-05-17 14:45 ` Justin Burkett
2017-05-17 15:16 ` Michael Albinus
2017-05-17 15:54 ` Justin Burkett
2017-05-17 17:35 ` Michael Albinus
2017-05-17 15:38 ` Stefan Monnier
2017-05-17 16:17 ` Stefan Monnier
2017-05-17 16:41 ` Justin Burkett [this message]
2017-05-17 16:32 ` vdiff and smerge-refine-subst Stefan Monnier
2017-05-17 16:43 ` Justin Burkett
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAF5dqNL78f3tn-qYcHziAxi0yHxW4T0AJuLoA_dfZ=x-boChRA@mail.gmail.com' \
--to=justin@burkett.cc \
--cc=emacs-devel@gnu.org \
--cc=monnier@iro.umontreal.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.