Re: master 421eeff: Add support for multiple Gravatar services

Re: master 421eeff: Add support for multiple Gravatar services

[Michael Albinus]

rpluim@gmail.com (Robert Pluim) writes:

Hi Robert,

> diff --git a/etc/NEWS b/etc/NEWS
> index ba3e691..2150f49 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -186,6 +186,12 @@ key             binding
>  / v             package-menu-filter-by-version
>  / /             package-menu-filter-clear
>
> +** Gravatar
> +
> +===
> +*** New user option 'gravatar-service' for host to query for gravatars.
> +Defaults to Libravatar, with Unicornify and Gravatar as options.
> +
>  \f
>  * New Modes and Packages in Emacs 28.1

What does "===" mean?

Best regards, Michael.

Re: master 421eeff: Add support for multiple Gravatar services

[Robert Pluim]

>>>>> On Tue, 24 Mar 2020 20:11:41 +0100, Michael Albinus <michael.albinus@gmx.de> said:

    Michael> rpluim@gmail.com (Robert Pluim) writes:
    Michael> Hi Robert,

    >> diff --git a/etc/NEWS b/etc/NEWS
    >> index ba3e691..2150f49 100644
    >> --- a/etc/NEWS
    >> +++ b/etc/NEWS
    >> @@ -186,6 +186,12 @@ key             binding
    >> / v             package-menu-filter-by-version
    >> / /             package-menu-filter-clear
    >> 
    >> +** Gravatar
    >> +
    >> +===
    >> +*** New user option 'gravatar-service' for host to query for gravatars.
    >> +Defaults to Libravatar, with Unicornify and Gravatar as options.
    >> +
    >> \f
    >> * New Modes and Packages in Emacs 28.1

    Michael> What does "===" mean?

It means I forgot to commit my fixup. Fixed.

Robert

Re: master 421eeff: Add support for multiple Gravatar services

[Michael Albinus]

Robert Pluim <rpluim@gmail.com> writes:

Hi Robert,

>     >> +** Gravatar
>     >> +
>     >> +===
>     >> +*** New user option 'gravatar-service' for host to query for gravatars.
>     >> +Defaults to Libravatar, with Unicornify and Gravatar as options.
>
>     Michael> What does "===" mean?
>
> It means I forgot to commit my fixup. Fixed.

Perhaps we shall add this explanation to the head of the etc/NEWS file? :-)

> Robert

Best regards, Michael.

Re: master 421eeff: Add support for multiple Gravatar services

[Robert Pluim]

>>>>> On Wed, 25 Mar 2020 09:54:25 +0100, Michael Albinus <michael.albinus@gmx.de> said:

    Michael> Robert Pluim <rpluim@gmail.com> writes:
    Michael> Hi Robert,

    >> >> +** Gravatar
    >> >> +
    >> >> +===
    >> >> +*** New user option 'gravatar-service' for host to query for gravatars.
    >> >> +Defaults to Libravatar, with Unicornify and Gravatar as options.
    >> 
    Michael> What does "===" mean?
    >> 
    >> It means I forgot to commit my fixup. Fixed.

    Michael> Perhaps we shall add this explanation to the head of the etc/NEWS file? :-)

Like this you mean? :-)

diff --git a/etc/NEWS b/etc/NEWS
index c0d8f0ccc7..53f6c719ff 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -18,6 +18,7 @@ with a prefix argument or by typing 'C-u C-h C-n'.
 Temporary note:
 +++ indicates that all relevant manuals in doc/ have been updated.
 --- means no change in the manuals is needed.
+=== means Robert messed up, double check the commit before pushing.
 When you add a new item, use the appropriate mark if you are sure it
 applies, and please also update docstrings as needed.

Re: master 421eeff: Add support for multiple Gravatar services

[Michael Albinus]

Robert Pluim <rpluim@gmail.com> writes:

> Like this you mean? :-)
>
> diff --git a/etc/NEWS b/etc/NEWS
> index c0d8f0ccc7..53f6c719ff 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -18,6 +18,7 @@ with a prefix argument or by typing 'C-u C-h C-n'.
>  Temporary note:
>  +++ indicates that all relevant manuals in doc/ have been updated.
>  --- means no change in the manuals is needed.
> +=== means Robert messed up, double check the commit before pushing.
>  When you add a new item, use the appropriate mark if you are sure it
>  applies, and please also update docstrings as needed.

YES :-)

Maybe also a rule in emacs/.git/pre-commit ...

(Not fully a joke, with such a trick I protect myself from doing
nonsense in the Tramp git repo)

Best regards, Michael.

Re: master 421eeff: Add support for multiple Gravatar services

[Glenn Morris]

> branch: master
> commit 421eeff243af683bf0b7c6d9181650a1c6900f9b

>     Add support for multiple Gravatar services

This causes a test failure. Ref eg
https://hydra.nixos.org/build/115470595

  Test gravatar-build-url condition:
      (error "foo is not an email address")


(Could "if you change foo.el, run any foo.el tests before pushing"
become part of the standard workflow?)

Re: master 421eeff: Add support for multiple Gravatar services

[Philip K.]

It seems my last patch didn't include the test update from the patch
before! Should I resubmit the entire patch again, or just that part?

-- 
	Philip K.

Re: master 421eeff: Add support for multiple Gravatar services

[Robert Pluim]

>>>>> On Tue, 24 Mar 2020 16:45:08 -0400, Glenn Morris <rgm@gnu.org> said:

    >> branch: master
    >> commit 421eeff243af683bf0b7c6d9181650a1c6900f9b

    >> Add support for multiple Gravatar services

    GM> This causes a test failure. Ref eg
    GM> https://hydra.nixos.org/build/115470595

    GM>   Test gravatar-build-url condition:
    GM>       (error "foo is not an email address")

    GM> (Could "if you change foo.el, run any foo.el tests before pushing"
    GM> become part of the standard workflow?)

It normally is, Iʼm just astonished there are tests for gravatar.el

Should be fixed now.

Robert

Re: master 421eeff: Add support for multiple Gravatar services

[Philip K.]

[-- Attachment #1: Type: text/plain, Size: 161 bytes --]


This should fix it. It would be worth considering expanding the test
cases too, especially since it's not testing the current default
anymore.

-- 
	Philip K.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-gravatar-build-url-testcase.patch --]
[-- Type: text/x-diff, Size: 962 bytes --]

From 44014722a5fba37ac3b6625ba7730f125883d789 Mon Sep 17 00:00:00 2001
From: Philip K <philip@warpmail.net>
Date: Tue, 24 Mar 2020 22:19:01 +0100
Subject: [PATCH] Fix gravatar-build-url testcase

* test/lisp/image/gravatar-tests.el (gravatar-build-url): Set service
  type to gravatar
---
 test/lisp/image/gravatar-tests.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/lisp/image/gravatar-tests.el b/test/lisp/image/gravatar-tests.el
index e66b5c6803..77b364073d 100644
--- a/test/lisp/image/gravatar-tests.el
+++ b/test/lisp/image/gravatar-tests.el
@@ -65,7 +65,8 @@ gravatar-build-url
   "Test `gravatar-build-url'."
   (let ((gravatar-default-image nil)
         (gravatar-force-default nil)
-        (gravatar-size nil))
+        (gravatar-size nil)
+        (gravatar-service 'gravatar))
     (should (equal (gravatar-build-url "foo") "\
 https://www.gravatar.com/avatar/acbd18db4cc2f85cedef654fccc4a4d8?r=g"))))
 
-- 
2.20.1

Re: master 421eeff: Add support for multiple Gravatar services

[Robert Pluim]

>>>>> On Tue, 24 Mar 2020 22:23:16 +0100, philip@warpmail.net (Philip K.) said:

    Philip> This should fix it. It would be worth considering expanding the test
    Philip> cases too, especially since it's not testing the current default
    Philip> anymore.

I fixed it the other way: itʼs now testing the default value.

Robert

Re: master 421eeff: Add support for multiple Gravatar services

[Philip K.]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]


I was re-reading the libravatar specification today, and noticed that I
had missed a section that described what to do if a DNS query results in
multiple records. The current implementation just used a "simple"
(dns-query ...) but that is explicit not sufficient. Instead one must
choose the highest priority servers and then randomly select one of
these, according to their weights. The patch I have attached to this
mail implements this, and can be tested by evaluating

	(gravatar--service-libravatar "test@pusling.com")

It's rather hard to find a domain with multiple libravatar entries, but
the selection algorithm can be tested using this slightly modified
excerpt from the patch:

    (defun test-selection ()
      (catch 'found
        ;; res is a hardcoded and simplified (dns-query "..." t) result.
        ;; "target" would usually point to a domain.
        (let* ((res '(((target alpha) (priority 10) (weight 10))
                      ((target beta) (priority 10) (weight 0))
                      ((target gamma) (priority 10) (weight 30))
                      ((target delta) (priority 5) (weight 10))))
               (prio (mapcar (lambda (r) (dns-get 'priority r)) res))
               (max (apply #'max prio))
               (sum 0) top)
          (dolist (rec res)
            (when (= max (dns-get 'priority rec))
              (setq sum (+ sum (dns-get 'weight rec)))
              (push rec top)))
          (dolist (rec top)
            (when (<= (if (= 0 sum) -1 (random sum))
                      (dns-get 'weight rec))
              (throw 'found (dns-get 'target rec)))
            (setq sum (- sum (dns-get 'weight rec)))))))

    (/ (cl-loop repeat 1000 count (eq (test-selection) 'gamma)) 1000.0)

where the last expression should approximately equal 0.75 (3/4), because
only the records alpha, beta and gamma have the highest priority (10),
and between them gamma has a 30/(10 + 30) = 3/4'th chance of being
chosen. In case all records are weighted 0, the first one is chosen.

I once again apologise for not thoroughly enough testing my last patch,
and hope this at least somewhat makes up for it.

-- 
	Philip K.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-Libravatar-federation-handling.patch --]
[-- Type: text/x-diff, Size: 2924 bytes --]

From 6191aa6577db1056bb74a4359d87a119e9517aca Mon Sep 17 00:00:00 2001
From: Philip K <philip@warpmail.net>
Date: Tue, 31 Mar 2020 13:23:09 +0200
Subject: [PATCH] Fix Libravatar federation handling

Previous implementation didn't correctly handle the result of
dns-query, and didn't implement the necessary record selection
algorithm as specified on the wiki (https://wiki.libravatar.org/api/,
"Federated servers").

* lisp/image/gravatar.el (gravatar--service-libravatar): Fix
libravatar image host resolver algorithm.
---
 lisp/image/gravatar.el | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/lisp/image/gravatar.el b/lisp/image/gravatar.el
index ff59a72ac8..d800e5dc55 100644
--- a/lisp/image/gravatar.el
+++ b/lisp/image/gravatar.el
@@ -149,12 +149,33 @@ gravatar--service-libravatar
           (dolist (record '(("_avatars-sec" . "https")
                             ("_avatars" . "http")))
             (let* ((query (concat (car record) "._tcp." domain))
-                   (result (dns-query query 'SRV)))
+                   (result (dns-query query 'SRV t)))
               (when result
-                (throw 'found (format "%s://%s/avatar"
-                                      (cdr record)
-                                      result)))))
-          "https://seccdn.libravatar.org/avatar")))))
+                (let* ((res (mapcar (lambda (rec)
+                                      (dns-get 'data (cdr rec)))
+                                    (dns-get 'answers result)))
+                       (prio
+                        (mapcar (lambda (r) (dns-get 'priority r)) res))
+                       (max (apply #'max prio))
+                       (sum 0) top)
+                  ;; Attempt to find all records with the same maximal
+                  ;; priority, and calculate the sum of their weights.
+                  (dolist (rec res)
+                    (when (= max (dns-get 'priority rec))
+                      (setq sum (+ sum (dns-get 'weight rec)))
+                      (push rec top)))
+                  ;; In case there is more than one maximal priority
+                  ;; record, choose one at random, while taking the
+                  ;; individual record weights into consideration.
+                  (dolist (rec top)
+                    (when (<= (if (= 0 sum) -1 (random sum))
+                              (dns-get 'weight rec))
+                      (throw 'found (format "%s://%s:%s/avatar"
+                                            (cdr record)
+                                            (dns-get 'target rec)
+                                            (dns-get 'port rec))))
+                    (setq sum (- sum (dns-get 'weight rec)))))))
+            "https://seccdn.libravatar.org/avatar"))))))
 
 (defun gravatar-hash (mail-address)
   "Return the Gravatar hash for MAIL-ADDRESS."
-- 
2.20.1