Re: [PATCH 1/2] Refactor digest authentication in url-auth

[Eli Zaretskii]

> From: Jarno Malmari <address@hidden>
> Date: Sun, 13 Nov 2016 13:36:20 +0200
> 
> Additionally, this refactoring fixed a bug where duplicate key entries
> were continuously added in `url-digest-auth-storage' each time
> authenticated.
> * lisp/url/url-auth.el (url-digest-auth, url-digest-auth-create-key):
> (url-digest-auth-build-response, url-digest-auth-directory-id-assoc,
> url-digest-auth-name-value-string, url-digest-auth-source-creds,
> url-digest-cached-key, url-digest-cache-key, url-digest-find-creds,
> url-digest-find-new-key, url-digest-prompt-creds): Add new functions to
> simplify code and aid in unit testing.

This is not how we format commit logs of changes to several functions
whose list spans several lines.  The entry should look like this:

* lisp/url/url-auth.el (url-digest-auth, url-digest-auth-create-key):
(url-digest-auth-build-response, url-digest-auth-directory-id-assoc)
(url-digest-auth-name-value-string, url-digest-auth-source-creds)
(url-digest-cached-key, url-digest-cache-key, url-digest-find-creds)
(url-digest-find-new-key, url-digest-prompt-creds): Add new functions to
simplify code and aid in unit testing.

IOW, each line begins is separately parenthesized.

> +(defsubst url-digest-auth-kd (data secret)
> +  "Apply digest algorithm MD5 to DATA using SECRET as described in RFC 2617."
> +  (md5 (url-digest-auth-colonjoin secret data)))
> +
> +(defsubst url-digest-auth-make-ha1 (user realm password)
> +  "Compute hashed A1 value as described in RFC 2617.
> +USER, REALM, and PASSWORD are strings used to create the hash
> +from."
> +  (md5 (url-digest-auth-colonjoin user realm password)))

Hear and elsewhere, I find the doc strings impenetrable without having
RFC 2617 around.  I wonder if it would make sense to describe the
arguments in a bit more detail, such that consulting the RFC each time
these are used would not be necessary.  Is that practical?

> +(defun url-digest-auth-directory-id-assoc (dirkey keylist)
> +  "Find the best match in key list using a path or a realm.
> +
> +The string DIRKEY is either a path or a realm.  The key list to
> +search through is the alist KEYLIST where car of each element is
> +either a path or a realm.  Realms are searched for an exact
> +match.  For paths, an ancestor is sufficient for a match."

GNU coding standards frown on using "path" for anything but PATH-style
directory lists.  Please use "file name" or "directory name" instead.

> +   ;; no partial matches for non-path, i.e. realm
> +   (and (string-match "/" dirkey)

This will fail with Windows file names that use backslashes.

I also find the test to be too loose: does having a slash in a string
really make it a directory name?  At least on Windows, a string with
embedded slashes or backslashes can be an invalid file name.

> +(defun url-digest-cached-key (url realm)
> +  "Find best match for URL and REALM from `url-digest-auth-storage'.
> +The return value is a list consisting of a realm (or a directory)
> +a user name, and hashed authentication tokens HA1 and HA2,
> +defined in RFC 2617. Modifying the contents of the returned list

Two spaces between sentences, please.

> +  "Create a key for digest authentication method.
> +The USERNAME and PASSWORD are the credentials for REALM and are
> +used in making a hashed value named HA1. The HTTP METHOD and URI
> +makes a second hashed value HA2. These hashes are used in making
> +the authentication key that can be stored without saving the
> +password in plain text.  The return value is a list consisting of
> +hashed authentication tokens HA1 and HA2, defined in RFC 2617.

Same here.

> +Primary method for finding credentials is from Emacs auth-source.
> +If password isn't found, and PROMPT is non-nil, allow to query
> +credentials via minibuffer.

"Allow to query" or "query"?

> +    ;; if incomplete and prompt allowed, prompt the user

Comments should begin with a capital letter and end with a period, as
normal sentences are (here and elsewhere in the patch).

Thanks.