Re: [Quilt-dev] [PATCH 4/4] quilt.el: Fix quilt-editable when QUILT_PATCHES_PREFIX is set

[Jean Delvare]

On Thu, 2020-05-14 at 21:59 +0200, Ondřej Lysoněk wrote:
> This patch fixes a bug in quilt-editable: if QUILT_PATCHES_PREFIX is
> set, quilt-editable will always return nil, even if the file being
> edited is part of the topmost patch.
> 
> If QUILT_PATCHES_PREFIX is set, then 'quilt top' prints the patch name
> as a relative path to the patch. Since in quilt-editable we're running
> 'quilt top' from the top level directory, the printed patch path is in
> the form $QUILT_PATCHES/patch-name.
> 
> Later on, we're looking for a cached version of the file that we're
> editing in .pc/. The patch directories are stored directly under .pc/,
> rather than .pc/$QUILT_PATCHES/, so we must remove the $QUILT_PATCHES/
> prefix from the patch path.
> 
> Signed-off-by: Ondřej Lysoněk <address@hidden>
> ---
>  lib/quilt.el | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/quilt.el b/lib/quilt.el
> index a872aab..4c31bd2 100644
> --- a/lib/quilt.el
> +++ b/lib/quilt.el
> @@ -58,6 +58,10 @@
>    (or (quilt--get-config-variable "QUILT_PC")
>        ".pc"))
>  
> +(defun quilt-patches-prefix ()
> +  "Return the value of the QUILT_PATCHES_PREFIX config variable. Return nil 
> if it is unset."
> +  (quilt--get-config-variable "QUILT_PATCHES_PREFIX"))
> +
>  (defun quilt-find-dir (fn &optional prefn)
>    "Return the top level dir of quilt from FN."
>    (if (or (not fn) (equal fn "/") (equal fn prefn))
> @@ -178,6 +182,13 @@
>        (setq n (1+ n))))
>    (completing-read p l nil t))
>  
> +(defun quilt--strip-patchname (pn)
> +  "Return the name of patch PN sans the path to the patches directory."
> +  (let ((patches-path (concat (quilt-patches-directory) "/")))
> +    (if (string-prefix-p patches-path pn)

Considering that you only call this function when quilt-patches-
directory is true, I think this test is pretty much guaranteed to
succeed?

Note that stripping the prefix "just in case" is generally not a good
idea, as nothing prevents users from creating a patches/ subdirectory
under the main patches/ directory (would be confusing, but is legal and
must be supported).

> +     (substring pn (length patches-path))
> +      pn)))
> +
>  (defvar quilt-edit-top-only 't)
>  (defun quilt-editable (f)
>    "Return t if F is editable in terms of current patch.  Return nil if 
> otherwise."
> @@ -188,7 +199,10 @@
>       (if (quilt-bottom-p)
>           (quilt-cmd "applied")       ; to print error message
>         (setq dirs (if quilt-edit-top-only
> -                      (list (substring (quilt-cmd-to-string "top")  0 -1))
> +                      (list (let ((patch (substring (quilt-cmd-to-string 
> "top")  0 -1)))
> +                              (if (cquilt-patches-prefix)

Please note that QUILT_PATCHES_PREFIX is considered true if not empty.
That means that quilt will print the prefix if QUILT_PATCHES_PREFIX if
set to "0" for example (admittedly confusing, but that's how it is
implemented). As I read your code, this stupid setting would be
interpreted incorrectly by emacs.

> +                                  (quilt--strip-patchname patch)
> +                                patch)))
>                        (cdr (cdr (directory-files (concat qd 
> (quilt-pc-directory) "/"))))))
>         (while (and (not result) dirs)
>           (if (file-exists-p (concat qd (quilt-pc-directory) "/" (car dirs) 
> "/" fn))

-- 
Jean Delvare
SUSE L3 Support