[PATCH 1/4] quilt.el: Refactor config reading functions

Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/4] quilt.el: Refactor config reading functions

Ondřej Lysoněk
quilt-patches-directory is a copy-paste of
quilt-pc-directory. Refactor the common code into a separate
function.

Signed-off-by: Ondřej Lysoněk <[hidden email]>
---
 lib/quilt.el | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/lib/quilt.el b/lib/quilt.el
index ae73f3d..66fb41a 100644
--- a/lib/quilt.el
+++ b/lib/quilt.el
@@ -29,12 +29,12 @@
   "Return t if there is on the bottom of patch stack, return nil if otherwise."
   (if (> (call-process "quilt" nil nil nil "applied") 0) 1))
 
-(defun quilt-patches-directory ()
-  "Return the location of patch files."
+(defun quilt--get-config-variable (var)
+  "Return the value of a configuration variable. Return nil if it is unset."
   (or (with-current-buffer (generate-new-buffer " *cmd")
         (shell-command
          (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
-                 "echo -n $QUILT_PATCHES")
+                 "echo -n $" var)
          t)
         (unwind-protect
             (let ((v (buffer-string)))
@@ -42,24 +42,17 @@
                   nil
                 v))
           (kill-buffer (current-buffer))))
-      (or (getenv "QUILT_PATCHES")
-          "patches")))
+      (getenv var)))
+
+(defun quilt-patches-directory ()
+  "Return the location of patch files."
+  (or (quilt--get-config-variable "QUILT_PATCHES")
+      "patches"))
 
 (defun quilt-pc-directory ()
   "Return the location of patch files."
-  (or (with-current-buffer (generate-new-buffer " *cmd")
-        (shell-command
-         (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
-                 "echo -n $QUILT_PC")
-         t)
-        (unwind-protect
-            (let ((v (buffer-string)))
-              (if (string= "" (buffer-string))
-                  nil
-                v))
-          (kill-buffer (current-buffer))))
-      (or (getenv "QUILT_PC")
-          ".pc")))
+  (or (quilt--get-config-variable "QUILT_PC")
+      ".pc"))
 
 (defun quilt-find-dir (fn &optional prefn)
   "Return the top level dir of quilt from FN."
--
2.25.4


_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/4] Document that quilt loads /etc/quilt.quiltrc

Ondřej Lysoněk
quilt loads /etc/quilt.quiltrc if ~/.quiltrc doesn't exist. Document
it.

Signed-off-by: Ondřej Lysoněk <[hidden email]>
---
 doc/main.tex   | 3 ++-
 doc/quilt.1.in | 7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/doc/main.tex b/doc/main.tex
index 24f863e..363d6f6 100644
--- a/doc/main.tex
+++ b/doc/main.tex
@@ -672,7 +672,8 @@ the series file in this case.
 \section{Customizing Quilt}
 
 Upon startup, quilt evaluates the file \textsf{.quiltrc} in the user's
-home directory, or the file specified with the \textsf{-{}-quiltrc} option.
+home directory, \textsf{/etc/quilt.quiltrc} if the former file does not
+exist, or the file specified with the \textsf{-{}-quiltrc} option.
 This file is a regular bash script. Default options can be passed to
 any command by defining a \textsf{QUILT\_\textit{COMMAND}\_ARGS} variable
 (for example, \textsf{QUILT\_DIFF\_ARGS="-{}-color=auto"} causes the output
diff --git a/doc/quilt.1.in b/doc/quilt.1.in
index c0e8b05..3c23043 100644
--- a/doc/quilt.1.in
+++ b/doc/quilt.1.in
@@ -178,9 +178,10 @@ Please refer to the pdf documentation for a full example of use.
 .SH CONFIGURATION FILE
 
 Upon startup, quilt evaluates the file .quiltrc in the user's home
-directory, or the file specified with the --quiltrc option.  This file
-is a regular bash script. Default options can be passed to any COMMAND
-by defining a QUILT_${COMMAND}_ARGS variable.  For example,
+directory, /etc/quilt.quiltrc if the former file does not exist, or
+the file specified with the --quiltrc option.  This file is a regular
+bash script. Default options can be passed to any COMMAND by defining
+a QUILT_${COMMAND}_ARGS variable.  For example,
 QUILT_DIFF_ARGS="--color=auto" causes the output of quilt diff to be
 syntax colored when writing to a terminal.
 
--
2.25.4


_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/4] quilt.el: Load /etc/quilt.quiltrc if ~/.quiltrc doesn't exist

Ondřej Lysoněk
In reply to this post by Ondřej Lysoněk
quilt loads /etc/quilt.quiltrc if ~/.quiltrc doesn't exist. Do the
same in quilt.el.

Signed-off-by: Ondřej Lysoněk <[hidden email]>
---
 lib/quilt.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/quilt.el b/lib/quilt.el
index 66fb41a..a872aab 100644
--- a/lib/quilt.el
+++ b/lib/quilt.el
@@ -33,7 +33,11 @@
   "Return the value of a configuration variable. Return nil if it is unset."
   (or (with-current-buffer (generate-new-buffer " *cmd")
         (shell-command
-         (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
+         (concat "if [ -f ~/.quiltrc ]; then"
+ "  . ~/.quiltrc ;"
+ "elif [ -f /etc/quilt.quiltrc ]; then"
+ "  . /etc/quilt.quiltrc ;"
+ "fi ;"
                  "echo -n $" var)
          t)
         (unwind-protect
--
2.25.4


_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

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

Ondřej Lysoněk
In reply to this post by Ondřej Lysoněk
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 <[hidden email]>
---
 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)
+ (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 (quilt-patches-prefix)
+     (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))
--
2.25.4


_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] quilt.el: Refactor config reading functions

Jean Delvare-3
In reply to this post by Ondřej Lysoněk
Hi Ondřej,

On Thu, 2020-05-14 at 21:59 +0200, Ondřej Lysoněk wrote:

> quilt-patches-directory is a copy-paste of
> quilt-pc-directory. Refactor the common code into a separate
> function.
>
> Signed-off-by: Ondřej Lysoněk <[hidden email]>
> ---
>  lib/quilt.el | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/lib/quilt.el b/lib/quilt.el
> index ae73f3d..66fb41a 100644
> --- a/lib/quilt.el
> +++ b/lib/quilt.el
> @@ -29,12 +29,12 @@
>    "Return t if there is on the bottom of patch stack, return nil if otherwise."
>    (if (> (call-process "quilt" nil nil nil "applied") 0) 1))
>  
> -(defun quilt-patches-directory ()
> -  "Return the location of patch files."
> +(defun quilt--get-config-variable (var)
> +  "Return the value of a configuration variable. Return nil if it is unset."
>    (or (with-current-buffer (generate-new-buffer " *cmd")
>          (shell-command
>           (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
> -                 "echo -n $QUILT_PATCHES")
> +                 "echo -n $" var)
>           t)
>          (unwind-protect
>              (let ((v (buffer-string)))
> @@ -42,24 +42,17 @@
>                    nil
>                  v))
>            (kill-buffer (current-buffer))))
> -      (or (getenv "QUILT_PATCHES")
> -          "patches")))
> +      (getenv var)))
> +
> +(defun quilt-patches-directory ()
> +  "Return the location of patch files."
> +  (or (quilt--get-config-variable "QUILT_PATCHES")
> +      "patches"))
>  
>  (defun quilt-pc-directory ()
>    "Return the location of patch files."
> -  (or (with-current-buffer (generate-new-buffer " *cmd")
> -        (shell-command
> -         (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
> -                 "echo -n $QUILT_PC")
> -         t)
> -        (unwind-protect
> -            (let ((v (buffer-string)))
> -              (if (string= "" (buffer-string))
> -                  nil
> -                v))
> -          (kill-buffer (current-buffer))))
> -      (or (getenv "QUILT_PC")
> -          ".pc")))
> +  (or (quilt--get-config-variable "QUILT_PC")
> +      ".pc"))
>  
>  (defun quilt-find-dir (fn &optional prefn)
>    "Return the top level dir of quilt from FN."

Disclaimer: not an emacs user, so I can't test this nor other patches
in this series.

Looks good to me, although it could be the right time to fix the
description of function quilt-pc-directory. "Return the location of
patch files." looks like a copy-and-paste error from function quilt-
patches-directory in commit f7b69c58d21903baacb290840e7bed9282e357e2.

The ".pc" directory contains quilt's working state.

--
Jean Delvare
SUSE L3 Support

_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/4] Document that quilt loads /etc/quilt.quiltrc

Jean Delvare-3
In reply to this post by Ondřej Lysoněk
On Thu, 2020-05-14 at 21:59 +0200, Ondřej Lysoněk wrote:

> quilt loads /etc/quilt.quiltrc if ~/.quiltrc doesn't exist. Document
> it.
>
> Signed-off-by: Ondřej Lysoněk <[hidden email]>
> ---
>  doc/main.tex   | 3 ++-
>  doc/quilt.1.in | 7 ++++---
>  2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/doc/main.tex b/doc/main.tex
> index 24f863e..363d6f6 100644
> --- a/doc/main.tex
> +++ b/doc/main.tex
> @@ -672,7 +672,8 @@ the series file in this case.
>  \section{Customizing Quilt}
>  
>  Upon startup, quilt evaluates the file \textsf{.quiltrc} in the user's
> -home directory, or the file specified with the \textsf{-{}-quiltrc} option.
> +home directory, \textsf{/etc/quilt.quiltrc} if the former file does not
> +exist, or the file specified with the \textsf{-{}-quiltrc} option.
>  This file is a regular bash script. Default options can be passed to
>  any command by defining a \textsf{QUILT\_\textit{COMMAND}\_ARGS} variable
>  (for example, \textsf{QUILT\_DIFF\_ARGS="-{}-color=auto"} causes the output
> diff --git a/doc/quilt.1.in b/doc/quilt.1.in
> index c0e8b05..3c23043 100644
> --- a/doc/quilt.1.in
> +++ b/doc/quilt.1.in
> @@ -178,9 +178,10 @@ Please refer to the pdf documentation for a full example of use.
>  .SH CONFIGURATION FILE
>  
>  Upon startup, quilt evaluates the file .quiltrc in the user's home
> -directory, or the file specified with the --quiltrc option.  This file
> -is a regular bash script. Default options can be passed to any COMMAND
> -by defining a QUILT_${COMMAND}_ARGS variable.  For example,
> +directory, /etc/quilt.quiltrc if the former file does not exist, or
> +the file specified with the --quiltrc option.  This file is a regular
> +bash script. Default options can be passed to any COMMAND by defining
> +a QUILT_${COMMAND}_ARGS variable.  For example,
>  QUILT_DIFF_ARGS="--color=auto" causes the output of quilt diff to be
>  syntax colored when writing to a terminal.

The support of /etc/quilt.quiltrc was already mentioned in section
COMMON OPTIONS TO ALL COMMANDS of the man page, but I agree it could be
documented in a more visible way.

Applied, thanks.

--
Jean Delvare
SUSE L3 Support

_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] quilt.el: Load /etc/quilt.quiltrc if ~/.quiltrc doesn't exist

Jean Delvare-3
In reply to this post by Ondřej Lysoněk
On Thu, 2020-05-14 at 21:59 +0200, Ondřej Lysoněk wrote:

> quilt loads /etc/quilt.quiltrc if ~/.quiltrc doesn't exist. Do the
> same in quilt.el.
>
> Signed-off-by: Ondřej Lysoněk <[hidden email]>
> ---
>  lib/quilt.el | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/quilt.el b/lib/quilt.el
> index 66fb41a..a872aab 100644
> --- a/lib/quilt.el
> +++ b/lib/quilt.el
> @@ -33,7 +33,11 @@
>    "Return the value of a configuration variable. Return nil if it is unset."
>    (or (with-current-buffer (generate-new-buffer " *cmd")
>          (shell-command
> -         (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
> +         (concat "if [ -f ~/.quiltrc ]; then"
> + "  . ~/.quiltrc ;"
> + "elif [ -f /etc/quilt.quiltrc ]; then"
> + "  . /etc/quilt.quiltrc ;"
> + "fi ;"
>                   "echo -n $" var)
>           t)
>          (unwind-protect

Nit-picking: alignment seems off by one char.

Other than that, I'm fine with the change, it's hard to believe it went
unnoticed for so long. Well I suppose most advanced quilt users have
their own customized ~/.quiltrc, but still.

--
Jean Delvare
SUSE L3 Support

_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

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

Jean Delvare-3
In reply to this post by Ondřej Lysoněk
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 <[hidden email]>
> ---
>  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

_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] quilt.el: Refactor config reading functions

Ondřej Lysoněk
In reply to this post by Jean Delvare-3
Hi Jean,

sorry for the late reply.

Jean Delvare <[hidden email]> writes:

> Hi Ondřej,
>
> On Thu, 2020-05-14 at 21:59 +0200, Ondřej Lysoněk wrote:
>> quilt-patches-directory is a copy-paste of
>> quilt-pc-directory. Refactor the common code into a separate
>> function.
>>
>> Signed-off-by: Ondřej Lysoněk <[hidden email]>
>> ---
>>  lib/quilt.el | 29 +++++++++++------------------
>>  1 file changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/lib/quilt.el b/lib/quilt.el
>> index ae73f3d..66fb41a 100644
>> --- a/lib/quilt.el
>> +++ b/lib/quilt.el
>> @@ -29,12 +29,12 @@
>>    "Return t if there is on the bottom of patch stack, return nil if otherwise."
>>    (if (> (call-process "quilt" nil nil nil "applied") 0) 1))
>>  
>> -(defun quilt-patches-directory ()
>> -  "Return the location of patch files."
>> +(defun quilt--get-config-variable (var)
>> +  "Return the value of a configuration variable. Return nil if it is unset."
>>    (or (with-current-buffer (generate-new-buffer " *cmd")
>>          (shell-command
>>           (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
>> -                 "echo -n $QUILT_PATCHES")
>> +                 "echo -n $" var)
>>           t)
>>          (unwind-protect
>>              (let ((v (buffer-string)))
>> @@ -42,24 +42,17 @@
>>                    nil
>>                  v))
>>            (kill-buffer (current-buffer))))
>> -      (or (getenv "QUILT_PATCHES")
>> -          "patches")))
>> +      (getenv var)))
>> +
>> +(defun quilt-patches-directory ()
>> +  "Return the location of patch files."
>> +  (or (quilt--get-config-variable "QUILT_PATCHES")
>> +      "patches"))
>>  
>>  (defun quilt-pc-directory ()
>>    "Return the location of patch files."
>> -  (or (with-current-buffer (generate-new-buffer " *cmd")
>> -        (shell-command
>> -         (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
>> -                 "echo -n $QUILT_PC")
>> -         t)
>> -        (unwind-protect
>> -            (let ((v (buffer-string)))
>> -              (if (string= "" (buffer-string))
>> -                  nil
>> -                v))
>> -          (kill-buffer (current-buffer))))
>> -      (or (getenv "QUILT_PC")
>> -          ".pc")))
>> +  (or (quilt--get-config-variable "QUILT_PC")
>> +      ".pc"))
>>  
>>  (defun quilt-find-dir (fn &optional prefn)
>>    "Return the top level dir of quilt from FN."
>
> Disclaimer: not an emacs user, so I can't test this nor other patches
> in this series.

I'm aware. I hope you didn't have to learn eLisp just to review these
patches :).

> Looks good to me, although it could be the right time to fix the
> description of function quilt-pc-directory. "Return the location of
> patch files." looks like a copy-and-paste error from function quilt-
> patches-directory in commit f7b69c58d21903baacb290840e7bed9282e357e2.
>
> The ".pc" directory contains quilt's working state.

Yes, I'll fix it in v2.

Ondrej


_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] quilt.el: Load /etc/quilt.quiltrc if ~/.quiltrc doesn't exist

Ondřej Lysoněk
In reply to this post by Jean Delvare-3
Jean Delvare <[hidden email]> writes:

> On Thu, 2020-05-14 at 21:59 +0200, Ondřej Lysoněk wrote:
>> quilt loads /etc/quilt.quiltrc if ~/.quiltrc doesn't exist. Do the
>> same in quilt.el.
>>
>> Signed-off-by: Ondřej Lysoněk <[hidden email]>
>> ---
>>  lib/quilt.el | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/quilt.el b/lib/quilt.el
>> index 66fb41a..a872aab 100644
>> --- a/lib/quilt.el
>> +++ b/lib/quilt.el
>> @@ -33,7 +33,11 @@
>>    "Return the value of a configuration variable. Return nil if it is unset."
>>    (or (with-current-buffer (generate-new-buffer " *cmd")
>>          (shell-command
>> -         (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
>> +         (concat "if [ -f ~/.quiltrc ]; then"
>> + "  . ~/.quiltrc ;"
>> + "elif [ -f /etc/quilt.quiltrc ]; then"
>> + "  . /etc/quilt.quiltrc ;"
>> + "fi ;"
>>                   "echo -n $" var)
>>           t)
>>          (unwind-protect
>
> Nit-picking: alignment seems off by one char.

Not sure where you mean exactly, but I noticed that there are tabs in the
added lines instead of spaces, which is inconsistent with the rest of
the function. Will fix.

> Other than that, I'm fine with the change, it's hard to believe it went
> unnoticed for so long. Well I suppose most advanced quilt users have
> their own customized ~/.quiltrc, but still.

Indeed. I was quite surprised.

Ondrej


_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

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

Ondřej Lysoněk
In reply to this post by Jean Delvare-3
Jean Delvare <[hidden email]> 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 <[hidden email]>
>> ---
>>  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


_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] quilt.el: Refactor config reading functions

Jean Delvare-3
In reply to this post by Ondřej Lysoněk
On Sat, 30 May 2020 17:57:08 +0200, Ondřej Lysoněk wrote:
> Hi Jean,
>
> sorry for the late reply.

No worry, I'm being slower than that on a regular basis so I can't
complain really.

> Jean Delvare <[hidden email]> writes:
> > Disclaimer: not an emacs user, so I can't test this nor other patches
> > in this series.  
>
> I'm aware. I hope you didn't have to learn eLisp just to review these
> patches :).

I learned some LISP in engineering school 20 years ago. That's how I
did not run away crying when reading the patch ;-)

--
Jean Delvare
SUSE L3 Support

_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/4] quilt.el: Load /etc/quilt.quiltrc if ~/.quiltrc doesn't exist

Jean Delvare-3
In reply to this post by Ondřej Lysoněk
On Sat, 30 May 2020 18:00:42 +0200, Ondřej Lysoněk wrote:

> Jean Delvare <[hidden email]> writes:
>
> > On Thu, 2020-05-14 at 21:59 +0200, Ondřej Lysoněk wrote:  
> >> quilt loads /etc/quilt.quiltrc if ~/.quiltrc doesn't exist. Do the
> >> same in quilt.el.
> >>
> >> Signed-off-by: Ondřej Lysoněk <[hidden email]>
> >> ---
> >>  lib/quilt.el | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/quilt.el b/lib/quilt.el
> >> index 66fb41a..a872aab 100644
> >> --- a/lib/quilt.el
> >> +++ b/lib/quilt.el
> >> @@ -33,7 +33,11 @@
> >>    "Return the value of a configuration variable. Return nil if it is unset."
> >>    (or (with-current-buffer (generate-new-buffer " *cmd")
> >>          (shell-command
> >> -         (concat "test -f ~/.quiltrc && . ~/.quiltrc ;"
> >> +         (concat "if [ -f ~/.quiltrc ]; then"
> >> + "  . ~/.quiltrc ;"
> >> + "elif [ -f /etc/quilt.quiltrc ]; then"
> >> + "  . /etc/quilt.quiltrc ;"
> >> + "fi ;"
> >>                   "echo -n $" var)
> >>           t)
> >>          (unwind-protect  
> >
> > Nit-picking: alignment seems off by one char.  
>
> Not sure where you mean exactly, but I noticed that there are tabs in the
> added lines instead of spaces, which is inconsistent with the rest of
> the function. Will fix.

Yes, that's what I meant. The mix of spaces and tabs breaks the
alignment in the patch, even if things look good in the file itself.

For your defense, the indentation is not consistent throughout the
file. But let's try to not make it worse, by being consistent within
every given function at least. Maybe we can make it fully consistent in
a separate patch, but I'm not the one making the decision of what the
proper style would be.

Thanks,
--
Jean Delvare
SUSE L3 Support

_______________________________________________
Quilt-dev mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/quilt-dev