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

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

[Stefan Monnier]

> +;;; vdiff.el --- Like vimdiff  -*- lexical-binding: t; -*-
> +
> +;; Copyright (C) 2016 Justin Burkett

The copyright should be "Free Software Foundation, Inc".
Run "make check_copyright" from the elpa GNUmakefile.

> +;;; Commentary:
> +
> +;;; Code:

I think it's safe to assume that many/most Emacs users haven't used Vim
much, and hence aren't familiar with vimdiff.  Please describe briefly what
it does (especially comparing it to ediff, I guess).


        Stefan

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

[Justin Burkett]

> The copyright should be "Free Software Foundation, Inc".
> Run "make check_copyright" from the elpa GNUmakefile.

Thanks Stefan. I changed the copyright.

Here is a rewritten introduction for those unfamiliar with vimdiff.

vdiff compares two or three buffers on the basis of the output from
the diff tool. The buffers are kept synchronized so that as you move
through one of the buffers the top of the active buffer aligns with
the corresponding top of the other buffer(s). This is similar to how
ediff works, but in ediff you use a third “control buffer” to move
through the diffed buffers. The key difference is that in vdiff you
are meant to actively edit one of the buffers and the display will
update automatically for the other buffer. Similar to ediff, vdiff
provides commands to “send” and “receive” hunks from one buffer to the
other as well as commands to traverse the diff hunks, which are useful
if you are trying to merge changes. In contrast to ediff, vdiff also
provides folding capabilities to fold sections of the buffers that
don’t contain changes. This folding occurs automatically. Finally, you
are encouraged to bind a key to `vdiff-hydra/body’, which will use
hydra.el (in ELPA) to create a convenient transient keymap containing
most of the useful vdiff commands.

This functionality is all inspired by (but not equivalent to) the
vimdiff tool from vim.

Justin

On Wed, May 17, 2017 at 8:42 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> +;;; vdiff.el --- Like vimdiff  -*- lexical-binding: t; -*-
>> +
>> +;; Copyright (C) 2016 Justin Burkett
>
> The copyright should be "Free Software Foundation, Inc".
> Run "make check_copyright" from the elpa GNUmakefile.
>
>> +;;; Commentary:
>> +
>> +;;; Code:
>
> I think it's safe to assume that many/most Emacs users haven't used Vim
> much, and hence aren't familiar with vimdiff.  Please describe briefly what
> it does (especially comparing it to ediff, I guess).
>
>
>         Stefan

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

[Michael Albinus]

Justin Burkett <justin@burkett.cc> writes:

> Here is a rewritten introduction for those unfamiliar with vimdiff.

Does it also support remote files, like ediff? This is ediff's killer
feature for me.

> Justin

Best regards, Michael.

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

[Stefan Monnier]

> Here is a rewritten introduction for those unfamiliar with vimdiff.

Yes, sorry, for some reason when I read that commit, the rest hadn't yet
arrived in my mailbox.
It looks fine, so feel free to merge it to master.  Thank you.


        Stefan

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

[Justin Burkett]

>Does it also support remote files, like ediff? This is ediff's killer feature for me.

Michael,

I don't believe there should be many differences (if any) between
vdiff and ediff with respect to remote files. vdiff has a similar
design to ediff here. It loads the content of the files into the
buffers and performs the diff on the buffers' content. Using tramp
remote files should be handled transparently. I can't do any testing
right now though.

Best, Justin

On Wed, May 17, 2017 at 11:16 AM, Michael Albinus
<michael.albinus@gmx.de> wrote:
> Justin Burkett <justin@burkett.cc> writes:
>
>> Here is a rewritten introduction for those unfamiliar with vimdiff.
>
> Does it also support remote files, like ediff? This is ediff's killer
> feature for me.
>
>> Justin
>
> Best regards, Michael.

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, 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))

vdiff and smerge-refine-subst

[Stefan Monnier]

Hi Justin,

Here's another question: any particular reason why you use your own
vdiff--diff-words instead of smerge-refine-subst?  Oh, wait, I think
I see why: smerge-refine-subst all works in the current buffer whereas
you have the two parts in separate buffers.  Would you be interested in
extending smerge-refine-subst so vdiff can use it?
[ We could rename it at the same time since I can't find
  a justification for the `subst` part of the name.  ]

While I'm here: I see you have a vdiff-default-refinement-syntax-code
custom variable, just like ediff has a ediff-forward-word-function.
Have you actually ever needed to change its value (or found users who
requested to be able to change this value)?  I resisted the original
temptation to introduce such a configuration into smerge-refine-subst
and never had to look back (although I occasionally feel like it would
be neat to refine the refinements (with char-granularity refinement
within words)).


        Stefan

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
<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))

Re: vdiff and smerge-refine-subst

[Justin Burkett]

> Would you be interested in extending smerge-refine-subst so vdiff can use it?

Yes, I could do that when I get a chance.

> Have you actually ever needed to change its value (or found users who requested to be able to change this value)?

No, I just did that because it was easy to do and ediff offered
something similar.

On Wed, May 17, 2017 at 12:32 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
> Hi Justin,
>
> Here's another question: any particular reason why you use your own
> vdiff--diff-words instead of smerge-refine-subst?  Oh, wait, I think
> I see why: smerge-refine-subst all works in the current buffer whereas
> you have the two parts in separate buffers.  Would you be interested in
> extending smerge-refine-subst so vdiff can use it?
> [ We could rename it at the same time since I can't find
>   a justification for the `subst` part of the name.  ]
>
> While I'm here: I see you have a vdiff-default-refinement-syntax-code
> custom variable, just like ediff has a ediff-forward-word-function.
> Have you actually ever needed to change its value (or found users who
> requested to be able to change this value)?  I resisted the original
> temptation to introduce such a configuration into smerge-refine-subst
> and never had to look back (although I occasionally feel like it would
> be neat to refine the refinements (with char-granularity refinement
> within words)).
>
>
>         Stefan

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

[Michael Albinus]

Justin Burkett <justin@burkett.cc> writes:

> Michael,

Hi Justin,

> I don't believe there should be many differences (if any) between
> vdiff and ediff with respect to remote files. vdiff has a similar
> design to ediff here. It loads the content of the files into the
> buffers and performs the diff on the buffers' content. Using tramp
> remote files should be handled transparently. I can't do any testing
> right now though.

Sounds good, I'll test it when available via ELPA.

IIRC, ediff uses local copies of remote files prior applying diff. This
is an explicit step ediff performs, so it isn't as transparent as it
might look like.

Anyway, in case of problems it shouldn't be too hard to add this also to
vdiff.

> Best, Justin

Best regards, Michael.