[bug#58812] [PATCH 0/5] Add --symlink option to 'guix shell'.

[Maxim Cournoyer <maxim.cournoyer@gmail.com>]

Hi again,

Ludovic Courtès <ludo@gnu.org> writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> * gnu/build/install.scm (evaluate-populate-directive): By default, error when
>> the target of a symlink doesn't exist.  Always ensure TARGET ends with "/".
>> (populate-root-file-system): Call evaluate-populate-directive with
>>  #:error-on-dangling-symlink #t and add comment.
>
> [...]
>
>> +  (define target* (if (string-suffix? "/" target)
>> +                      target
>> +                      (string-append target "/")))
>
> Maybe make it:
>
>   (let ((target (if …)))
>     …)
>
> so there’s only one ‘target’ in scope (and no ‘target*’); otherwise it’s
> easy to forget the ‘*’ and refer to wrong one.

It's a pattern I've used at other places; I find it more hygienic to not
shadow existing variables; it signal to the reader "be careful, this is
not the same as the argument-bound one, though they are closely
related".

>> +                   (when error-on-dangling-symlink?
>> +                     ;; When the symbolic link points to a relative path,
>> +                     ;; checking if its target exists must be done relative to
>> +                     ;; the link location.
>> +                     (with-directory-excursion (if (string-prefix? "/" old)
>> +                                                   (getcwd)
>> +                                                   (dirname new*)) ;relative
>> +                       (unless (file-exists? old)
>> +                         (error (format #f "symlink `~a' points to nonexistent \
>> +file `~a'" new* old)))))
>> +                   (symlink old new*))
>
> I would avoid the directory excursion when unnecessary:
>
>   (unless (if (string-prefix? "/" old)
>               (file-exists? old)
>               (with-directory-excursion (dirname new)
>                 (file-exists? old)))
>     …)

Done:

--8<---------------cut here---------------start------------->8---
modified   gnu/build/install.scm
@@ -99,14 +99,14 @@ (define target* (if (string-suffix? "/" target)
                  (lambda ()
                    (when error-on-dangling-symlink?
                      ;; When the symbolic link points to a relative path,
-                     ;; checking if its target exists must be done relative to
-                     ;; the link location.
-                     (with-directory-excursion (if (string-prefix? "/" old)
-                                                   (getcwd)
-                                                   (dirname new*)) ;relative
-                       (unless (file-exists? old)
-                         (error (format #f "symlink `~a' points to nonexistent \
-file `~a'" new* old)))))
+                     ;; checking if its target exists must be done relatively
+                     ;; to the link location.
+                     (unless (if (string-prefix? "/" old)
+                                 (file-exists? old)
+                                 (with-directory-excursion (dirname new*)
+                                   (file-exists? old)))
+                       (error (format #f "symlink `~a' points to nonexistent \
+file `~a'" new* old))))
                    (symlink old new*))
--8<---------------cut here---------------end--------------->8---

> (We could use ‘lstat’ instead of ‘file-exists?’ if we want to allow
> symlinks to dangling symlinks…)

It seems better to leave it as-is; the odd use case of symlinking to a
dangling symlink can be accomplished via "#:error-on-dangling-symlink
#f" :-).

-- 
Thanks,
Maxim