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

[Ondřej Lysoněk]

Jean Delvare <jdelvare@suse.de> writes:

> 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 <olysonek@redhat.com>
>> ---
>>  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?

Yes, this is mostly correct. The test was meant to be a "safety check",
but to be fair, it was mostly laziness on my part. Thanks for not
letting me get away with it.

As it turns out, there is a small catch, though. The
quilt-patches-directory function currently reads only the quiltrc
files. However, quilt saves the patches directory to .pc/.quilt_patches
and uses that to read the directory name from there on. So if the
QUILT_PATCHES variable is later changed in quiltrc, the prefix stripping
would no longer work correctly.

I'll fix it in v2.

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

Agreed. That's not what I was trying to do.

>
>> +    (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.

It will work correctly in this case. In eLisp, "0" is a true value. In
fact, everything non-nil is considered true [1]. The following
expression evaluates to 42:

(if "0" 42 43)

Version 2 of the patch series coming right up...

[1] https://www.gnu.org/software/emacs/manual/html_node/elisp/Conditionals.html

Ondrej

>
>> +                                 (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