* epg--status-GET-HIDDEN cleanup suggestion
@ 2008-08-16 13:17 Ted Zlatanov
2008-08-16 13:41 ` Daiki Ueno
0 siblings, 1 reply; 4+ messages in thread
From: Ted Zlatanov @ 2008-08-16 13:17 UTC (permalink / raw)
To: emacs-devel; +Cc: Daiki Ueno
[-- Attachment #1: Type: text/plain, Size: 232 bytes --]
The function epg--status-GET-HIDDEN in epg.el had lots of repetition, so
I cleaned it up a little with a let*. I don't know the protocol for
comitting these cleanups, so I offer the patch for review. It works for
me.
Thanks
Ted
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: epg.el.patch --]
[-- Type: text/x-patch, Size: 4419 bytes --]
Index: epg.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/epg.el,v
retrieving revision 1.6
diff -c -r1.6 epg.el
*** epg.el 6 May 2008 07:57:34 -0000 1.6
--- epg.el 16 Aug 2008 13:14:25 -0000
***************
*** 1228,1282 ****
(defun epg--status-GET_HIDDEN (context string)
(when (and epg-key-id
(string-match "\\`passphrase\\." string))
! (unless (epg-context-passphrase-callback context)
! (error "passphrase-callback not set"))
! (let (inhibit-quit
! passphrase
! passphrase-with-new-line
! encoded-passphrase-with-new-line)
! (unwind-protect
! (condition-case nil
! (progn
! (setq passphrase
! (funcall
! (if (consp (epg-context-passphrase-callback context))
! (car (epg-context-passphrase-callback context))
! (epg-context-passphrase-callback context))
! context
! epg-key-id
! (if (consp (epg-context-passphrase-callback context))
! (cdr (epg-context-passphrase-callback context)))))
! (when passphrase
! (setq passphrase-with-new-line (concat passphrase "\n"))
! (epg--clear-string passphrase)
! (setq passphrase nil)
! (if epg-passphrase-coding-system
! (progn
! (setq encoded-passphrase-with-new-line
! (epg--encode-coding-string
! passphrase-with-new-line
! (coding-system-change-eol-conversion
! epg-passphrase-coding-system 'unix)))
! (epg--clear-string passphrase-with-new-line)
! (setq passphrase-with-new-line nil))
! (setq encoded-passphrase-with-new-line
! passphrase-with-new-line
! passphrase-with-new-line nil))
! (process-send-string (epg-context-process context)
! encoded-passphrase-with-new-line)))
! (quit
! (epg-context-set-result-for
! context 'error
! (cons '(quit)
! (epg-context-result-for context 'error)))
! (delete-process (epg-context-process context))))
! (if passphrase
! (epg--clear-string passphrase))
! (if passphrase-with-new-line
! (epg--clear-string passphrase-with-new-line))
! (if encoded-passphrase-with-new-line
! (epg--clear-string encoded-passphrase-with-new-line))))))
!
(defun epg--prompt-GET_BOOL (context string)
(let ((entry (assoc string epg-prompt-alist)))
(y-or-n-p (if entry (cdr entry) (concat string "? ")))))
--- 1228,1278 ----
(defun epg--status-GET_HIDDEN (context string)
(when (and epg-key-id
(string-match "\\`passphrase\\." string))
! (let* ((context-callback (epg-context-passphrase-callback context))
! (callback (or (car-safe context-callback) context-callback))
! (file (cdr-safe context-callback)))
! (unless context-callback
! (error "passphrase-callback not set"))
! (let (inhibit-quit
! passphrase
! passphrase-with-new-line
! encoded-passphrase-with-new-line)
! (unwind-protect
! (condition-case nil
! (progn
! (setq passphrase
! (funcall callback context epg-key-id file))
! (when passphrase
! (setq passphrase-with-new-line (concat passphrase "\n"))
! (epg--clear-string passphrase)
! (setq passphrase nil)
! (if epg-passphrase-coding-system
! (progn
! (setq encoded-passphrase-with-new-line
! (epg--encode-coding-string
! passphrase-with-new-line
! (coding-system-change-eol-conversion
! epg-passphrase-coding-system 'unix)))
! (epg--clear-string passphrase-with-new-line)
! (setq passphrase-with-new-line nil))
! (setq encoded-passphrase-with-new-line
! passphrase-with-new-line
! passphrase-with-new-line nil))
! (process-send-string (epg-context-process context)
! encoded-passphrase-with-new-line)))
! (quit
! (epg-context-set-result-for
! context 'error
! (cons '(quit)
! (epg-context-result-for context 'error)))
! (delete-process (epg-context-process context))))
! (if passphrase
! (epg--clear-string passphrase))
! (if passphrase-with-new-line
! (epg--clear-string passphrase-with-new-line))
! (if encoded-passphrase-with-new-line
! (epg--clear-string encoded-passphrase-with-new-line)))))))
!
(defun epg--prompt-GET_BOOL (context string)
(let ((entry (assoc string epg-prompt-alist)))
(y-or-n-p (if entry (cdr entry) (concat string "? ")))))
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: epg--status-GET-HIDDEN cleanup suggestion
2008-08-16 13:17 epg--status-GET-HIDDEN cleanup suggestion Ted Zlatanov
@ 2008-08-16 13:41 ` Daiki Ueno
2008-08-16 16:05 ` Ted Zlatanov
0 siblings, 1 reply; 4+ messages in thread
From: Daiki Ueno @ 2008-08-16 13:41 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: emacs-devel
Ted Zlatanov <tzz@lifelogs.com> writes:
> The function epg--status-GET-HIDDEN in epg.el had lots of repetition, so
> I cleaned it up a little with a let*.
Lots of? It seems that your change binds only a few variables, and
doesn't reduce the lines of code.
> I don't know the protocol for comitting these cleanups, so I offer the
> patch for review. It works for me.
I don't like to bind variables which is used only once at run time.
Regards,
--
Daiki Ueno
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: epg--status-GET-HIDDEN cleanup suggestion
2008-08-16 13:41 ` Daiki Ueno
@ 2008-08-16 16:05 ` Ted Zlatanov
2008-08-18 1:16 ` Daiki Ueno
0 siblings, 1 reply; 4+ messages in thread
From: Ted Zlatanov @ 2008-08-16 16:05 UTC (permalink / raw)
To: emacs-devel; +Cc: Daiki Ueno
On Sat, 16 Aug 2008 22:41:42 +0900 Daiki Ueno <ueno@unixuser.org> wrote:
DU> Ted Zlatanov <tzz@lifelogs.com> writes:
>> The function epg--status-GET-HIDDEN in epg.el had lots of repetition, so
>> I cleaned it up a little with a let*.
DU> Lots of? It seems that your change binds only a few variables, and
DU> doesn't reduce the lines of code.
You called (epg-context-passphrase-callback context) 6 times. My change
reduces it to 1 call, bound to a variable from that point on. Also, I
changed
(funcall
(if (consp (epg-context-passphrase-callback context))
(car (epg-context-passphrase-callback context))
(epg-context-passphrase-callback context))
context
epg-key-id
(if (consp (epg-context-passphrase-callback context))
(cdr (epg-context-passphrase-callback context))))
to
(let* ((context-callback (epg-context-passphrase-callback context))
(callback (or (car-safe context-callback) context-callback))
(file (cdr-safe context-callback)))
...
(funcall callback context epg-key-id file))
which to me seems much more readable (using car-safe and cdr-safe makes
the logic simpler too). Lines of code are not the only measure of code
cleanliness; in this specific case I found your logic hard to read while
debugging an unrelated problem so I tried to clean it up.
>> I don't know the protocol for comitting these cleanups, so I offer the
>> patch for review. It works for me.
DU> I don't like to bind variables which is used only once at run time.
Your choice, it's your code. I find my version more readable, but
that's certainly a matter of opinion.
Ted
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: epg--status-GET-HIDDEN cleanup suggestion
2008-08-16 16:05 ` Ted Zlatanov
@ 2008-08-18 1:16 ` Daiki Ueno
0 siblings, 0 replies; 4+ messages in thread
From: Daiki Ueno @ 2008-08-18 1:16 UTC (permalink / raw)
To: Ted Zlatanov; +Cc: emacs-devel
[-- Attachment #1: Type: text/plain, Size: 712 bytes --]
>>>>> In <m2skt53pyr.fsf@lifelogs.com>
>>>>> Ted Zlatanov <tzz@lifelogs.com> wrote:
> Lines of code are not the only measure of code cleanliness; in this
> specific case I found your logic hard to read while debugging an
> unrelated problem so I tried to clean it up.
You appear to be shortsighted. epg-context-* and epg-context-set-* are
close to simple getter/setter functions of defstruct in CL (see the
definition of epg-context-*). It looks even unnatural for me to use a
temporary variable just to refer a slot value.
Anyway, I'm going to commit the following change, which ensures
the PASSPHRASE-CALLBACK slot have a cons-cell, and reduces the call to
`epg-context-passphrase-callback' to 3 times.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: epg-callbacks.diff --]
[-- Type: text/x-diff, Size: 2967 bytes --]
diff --git a/lisp/epg.el b/lisp/epg.el
index b7d9732..7ad0d34 100644
--- a/lisp/epg.el
+++ b/lisp/epg.el
@@ -185,7 +185,7 @@
(cons 'epg-context
(vector (or protocol 'OpenPGP) armor textmode include-certs
cipher-algorithm digest-algorithm compress-algorithm
- #'epg-passphrase-callback-function
+ (list #'epg-passphrase-callback-function)
nil
nil nil nil nil nil nil)))
@@ -328,7 +328,9 @@ This function is for internal use only."
"Set the function used to query passphrase."
(unless (eq (car-safe context) 'epg-context)
(signal 'wrong-type-argument (list 'epg-context-p context)))
- (aset (cdr context) 7 passphrase-callback))
+ (aset (cdr context) 7 (if (consp passphrase-callback)
+ passphrase-callback
+ (list passphrase-callback))))
(defun epg-context-set-progress-callback (context
progress-callback)
@@ -336,7 +338,9 @@ This function is for internal use only."
If optional argument HANDBACK is specified, it is passed to PROGRESS-CALLBACK."
(unless (eq (car-safe context) 'epg-context)
(signal 'wrong-type-argument (list 'epg-context-p context)))
- (aset (cdr context) 8 progress-callback))
+ (aset (cdr context) 8 (if (consp progress-callback)
+ progress-callback
+ (list progress-callback))))
(defun epg-context-set-signers (context signers)
"Set the list of key-id for signing."
@@ -1239,13 +1243,10 @@ This function is for internal use only."
(progn
(setq passphrase
(funcall
- (if (consp (epg-context-passphrase-callback context))
- (car (epg-context-passphrase-callback context))
- (epg-context-passphrase-callback context))
+ (car (epg-context-passphrase-callback context))
context
epg-key-id
- (if (consp (epg-context-passphrase-callback context))
- (cdr (epg-context-passphrase-callback context)))))
+ (cdr (epg-context-passphrase-callback context))))
(when passphrase
(setq passphrase-with-new-line (concat passphrase "\n"))
(epg--clear-string passphrase)
@@ -1493,16 +1494,13 @@ This function is for internal use only."
(if (and (epg-context-progress-callback context)
(string-match "\\`\\([^ ]+\\) \\([^ ]\\) \\([0-9]+\\) \\([0-9]+\\)"
string))
- (funcall (if (consp (epg-context-progress-callback context))
- (car (epg-context-progress-callback context))
- (epg-context-progress-callback context))
+ (funcall (car (epg-context-progress-callback context))
context
(match-string 1 string)
(match-string 2 string)
(string-to-number (match-string 3 string))
(string-to-number (match-string 4 string))
- (if (consp (epg-context-progress-callback context))
- (cdr (epg-context-progress-callback context))))))
+ (cdr (epg-context-progress-callback context)))))
(defun epg--status-ENC_TO (context string)
(if (string-match "\\`\\([0-9A-Za-z]+\\) \\([0-9]+\\) \\([0-9]+\\)" string)
[-- Attachment #3: Type: text/plain, Size: 25 bytes --]
Regards,
--
Daiki Ueno
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-08-18 1:16 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-08-16 13:17 epg--status-GET-HIDDEN cleanup suggestion Ted Zlatanov
2008-08-16 13:41 ` Daiki Ueno
2008-08-16 16:05 ` Ted Zlatanov
2008-08-18 1:16 ` Daiki Ueno
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).