[PATCHES] Fix some ‘guix edit’ bad assumptions.

[PATCHES] Fix some ‘guix edit’ bad assumptions.

[Mathieu Lirzin]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

Hi,

As every sane person I use Emacs for my editing.  In order to have the
features of ‘emacsclient’ (fast startup + buffer sharing) and the
simplicity of ‘emacs’ (no daemon management), I have in my environment:

  export VISUAL="emacsclient --alternate-editor=''"

this automatically starts the daemon if it is not already running and
then launches ‘emacsclient’.  However, this does not work with ‘guix
edit’ because a single word command is assumed for $VISUAL and $EDITOR.

so here is a fix:


[-- Attachment #2: 0001-edit-Allow-command-line-arguments-in-VISUAL-and-EDIT.patch --]
[-- Type: text/x-diff, Size: 1744 bytes --]

From 9252f698348db42431264441a5e97fd5a414c001 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <mthl@gnu.org>
Date: Sat, 21 Nov 2015 14:37:54 +0100
Subject: [PATCH 1/2] edit: Allow command line arguments in $VISUAL and
 $EDITOR.

* guix/scripts/edit.scm (guix-edit): Fix the assumption that %editor is
  a one word command.
---
 guix/scripts/edit.scm | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/guix/scripts/edit.scm b/guix/scripts/edit.scm
index 73a5bb7..f894953 100644
--- a/guix/scripts/edit.scm
+++ b/guix/scripts/edit.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2015 Mathieu Lirzin <mthl@gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -22,6 +23,7 @@
   #:use-module (guix utils)
   #:use-module (guix packages)
   #:use-module (gnu packages)
+  #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-37)
   #:export (%editor
@@ -83,8 +85,13 @@ line."
 
       (catch 'system-error
         (lambda ()
-          (apply execlp (%editor) (%editor)
-                 (append-map package->location-specification packages)))
+          (let ((editor-args (string-split (%editor) #\space))
+                (file-names  (append-map package->location-specification
+                                         packages)))
+            (match editor-args
+              ((editor . args)
+               (apply execlp editor (append editor-args file-names)))
+              (_ #f))))
         (lambda args
           (let ((errno (system-error-errno args)))
             (leave (_ "failed to launch '~a': ~a~%")
-- 
2.6.2


[-- Attachment #3: Type: text/plain, Size: 438 bytes --]


Moreover I think that assuming a running emacs daemon by default if
$VISUAL and $EDITOR are not set is not an obvious decision.  My first
intuition would be to use GNU Nano which is often installed on GNU
systems.  But after trying to edit a package with it... I have come to
the conclusion that it was more reasonable to use 'emacs' instead.

I have no strong opinion on this (other than being against defaulting to
VI of course).  ;)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-edit-Don-t-assume-that-an-emacs-daemon-is-running.patch --]
[-- Type: text/x-diff, Size: 1121 bytes --]

From 7988fd52a23811720c82f20b5b65eb527564a7f6 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <mthl@gnu.org>
Date: Sat, 21 Nov 2015 15:05:07 +0100
Subject: [PATCH 2/2] edit: Don't assume that an emacs daemon is running.

* guix/scripts/edit.scm (%editor): Use Emacs as a default value.
---
 guix/scripts/edit.scm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/guix/scripts/edit.scm b/guix/scripts/edit.scm
index f894953..5dbb58d 100644
--- a/guix/scripts/edit.scm
+++ b/guix/scripts/edit.scm
@@ -50,8 +50,10 @@ Start $VISUAL or $EDITOR to edit the definitions of PACKAGE...\n"))
   (show-bug-report-information))
 
 (define %editor
-  (make-parameter (or (getenv "VISUAL") (getenv "EDITOR")
-                      "emacsclient")))
+  ;; XXX: It would be better to default to something more likely to be
+  ;; pre-installed on an average GNU system.  Since Nano is not suited for
+  ;; editing Scheme, Emacs is used instead.
+  (make-parameter (or (getenv "VISUAL") (getenv "EDITOR") "emacs")))
 
 (define (search-path* path file)
   "Like 'search-path' but exit if FILE is not found."
-- 
2.6.2


[-- Attachment #5: Type: text/plain, Size: 25 bytes --]


TIA,

--
Mathieu Lirzin

Re: [PATCHES] Fix some ‘guix edit’ bad assumptions.

[Ludovic Courtès]

Mathieu Lirzin <mthl@gnu.org> skribis:

>   export VISUAL="emacsclient --alternate-editor=''"

I didn’t know that including arguments in $VISUAL was supposed to work.
Does it actually work with other programs?

> +          (let ((editor-args (string-split (%editor) #\space))
> +                (file-names  (append-map package->location-specification
> +                                         packages)))
> +            (match editor-args
> +              ((editor . args)
> +               (apply execlp editor (append editor-args file-names)))

The problem is that this doesn’t correspond to shell tokenization.
I think the right fix would be to do:

  (system (string-append editor (string-join file-names)))

so that the thing is invoked via /bin/sh.  (We must exit with the value
that ‘system’ returns.)

> Moreover I think that assuming a running emacs daemon by default if
> $VISUAL and $EDITOR are not set is not an obvious decision.  My first
> intuition would be to use GNU Nano which is often installed on GNU
> systems.  But after trying to edit a package with it... I have come to
> the conclusion that it was more reasonable to use 'emacs' instead.

It could also be Zile or whatever.  The thing is, whatever choice we
make, it remains a last resort, and something that may not actually
work.

> From 7988fd52a23811720c82f20b5b65eb527564a7f6 Mon Sep 17 00:00:00 2001
> From: Mathieu Lirzin <mthl@gnu.org>
> Date: Sat, 21 Nov 2015 15:05:07 +0100
> Subject: [PATCH 2/2] edit: Don't assume that an emacs daemon is running.
>
> * guix/scripts/edit.scm (%editor): Use Emacs as a default value.

This is fine with me.

Thanks,
Ludo’.

Re: [PATCHES] Fix some ‘guix edit’ bad assumptions.

[Mathieu Lirzin]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

ludo@gnu.org (Ludovic Courtès) writes:

> Mathieu Lirzin <mthl@gnu.org> skribis:
>
>>   export VISUAL="emacsclient --alternate-editor=''"
>
> I didn’t know that including arguments in $VISUAL was supposed to work.
> Does it actually work with other programs?

It works with ‘git commit/rebase’ and using the 'v' key in ‘less’.
After a second thought, I must admit that the fact that this “should”
work is not obvious.  ;)

>> +          (let ((editor-args (string-split (%editor) #\space))
>> +                (file-names  (append-map package->location-specification
>> +                                         packages)))
>> +            (match editor-args
>> +              ((editor . args)
>> +               (apply execlp editor (append editor-args file-names)))
>
> The problem is that this doesn’t correspond to shell tokenization.
> I think the right fix would be to do:
>
>   (system (string-append editor (string-join file-names)))
>
> so that the thing is invoked via /bin/sh.  (We must exit with the value
> that ‘system’ returns.)

This is definitely a better solution.  Is this updated patch OK?


[-- Attachment #2: 0001-edit-Allow-command-line-arguments-in-VISUAL-and-EDIT.patch --]
[-- Type: text/x-diff, Size: 1350 bytes --]

From de988b953de96a17f79d5748541611fbb3d554f9 Mon Sep 17 00:00:00 2001
From: Mathieu Lirzin <mthl@gnu.org>
Date: Sat, 21 Nov 2015 14:37:54 +0100
Subject: [PATCH 1/2] edit: Allow command line arguments in $VISUAL and
 $EDITOR.

* guix/scripts/edit.scm (guix-edit): Fix the assumption that %editor is
  a one word command.
---
 guix/scripts/edit.scm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/guix/scripts/edit.scm b/guix/scripts/edit.scm
index 73a5bb7..c6d48ef 100644
--- a/guix/scripts/edit.scm
+++ b/guix/scripts/edit.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2015 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2015 Mathieu Lirzin <mthl@gnu.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -83,8 +84,9 @@ line."
 
       (catch 'system-error
         (lambda ()
-          (apply execlp (%editor) (%editor)
-                 (append-map package->location-specification packages)))
+          (let ((file-names (append-map package->location-specification
+                                        packages)))
+            (exit (system (string-join (cons (%editor) file-names))))))
         (lambda args
           (let ((errno (system-error-errno args)))
             (leave (_ "failed to launch '~a': ~a~%")
-- 
2.6.2


[-- Attachment #3: Type: text/plain, Size: 48 bytes --]


Thanks for your suggestion,

--
Mathieu Lirzin

Re: [PATCHES] Fix some ‘guix edit’ bad assumptions.

[Alex Kost]

Mathieu Lirzin (2015-11-21 18:31 +0300) wrote:

> Hi,
>
> As every sane person I use Emacs for my editing.

Hi, it's not related, but just in case you don't know, there is "M-x
guix-edit" command (or "M-x guix e" which is the same); package names
are completed.

If instead of the default package location directory
("~/.config/guix/latest" or whatever), you want to go to another
directory — for instance, to your guix git repo — you can use:

(setq guix-directory "~/src/guix")

-- 
Alex

Re: [PATCHES] Fix some ‘guix edit’ bad assumptions.

[Mathieu Lirzin]

Hi Alex,

Alex Kost <alezost@gmail.com> writes:

> If instead of the default package location directory
> ("~/.config/guix/latest" or whatever), you want to go to another
> directory — for instance, to your guix git repo — you can use:
>
> (setq guix-directory "~/src/guix")

This is definitely a convenient trick, thanks for sharing it.

--
Mathieu Lirzin

Re: [PATCHES] Fix some ‘guix edit’ bad assumptions.

[Ludovic Courtès]

Mathieu Lirzin <mthl@gnu.org> skribis:

> Hi Alex,
>
> Alex Kost <alezost@gmail.com> writes:
>
>> If instead of the default package location directory
>> ("~/.config/guix/latest" or whatever), you want to go to another
>> directory — for instance, to your guix git repo — you can use:
>>
>> (setq guix-directory "~/src/guix")
>
> This is definitely a convenient trick, thanks for sharing it.

Indeed, I had overlooked ‘guix-directory’.

Ludo’.

Re: [PATCHES] Fix some ‘guix edit’ bad assumptions.

[Ludovic Courtès]

Mathieu Lirzin <mthl@gnu.org> skribis:

> From de988b953de96a17f79d5748541611fbb3d554f9 Mon Sep 17 00:00:00 2001
> From: Mathieu Lirzin <mthl@gnu.org>
> Date: Sat, 21 Nov 2015 14:37:54 +0100
> Subject: [PATCH 1/2] edit: Allow command line arguments in $VISUAL and
>  $EDITOR.
>
> * guix/scripts/edit.scm (guix-edit): Fix the assumption that %editor is
>   a one word command.

[...]

> +          (let ((file-names (append-map package->location-specification
> +                                        packages)))
> +            (exit (system (string-join (cons (%editor) file-names))))))

Maybe add a comment saying why we use ‘system’ rather than ‘execlp’.

OK with this change, thanks!

Ludo’.