Re: [PATCH] org-protocol: fixes open-source and extends rewriting of URLs

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Mario Martelli <tlmtr@schnuddelhuddel.de> writes:

> Sorry for producing noise. Just read the contribution guidelines once again ;)
> Hope everything is now according to the guidelines.

Thank you. Comments follow.

> Subject: [PATCH 1/4] org-protocol.el: Fix for failing open-source subprotocol
>
> *  (org-protocol-open-source): make sure url is
> sanitised before processing

Applied.

> Subject: [PATCH 2/4] org-protocol.el: Fix for silently failing open-source
>  subprotocol
>
> * (org-protocol-open-source): tests URL against
> base-url and not the filename

Applied.

> Subject: [PATCH 3/4] org-protocol.el sources with date URL are supported
>
> * (org-protocol-project-alist): date-URL is added as example
> * (org-protocol-open-source): first match is processed in rewrite

Not applied. See below.

> +          :rewrites ((\"org/?$\" . \"index.php\")))
> +         (\"Hugo based blog\"
> +          :base-url \"https://themes.gohugo.io/theme/hugo-icarus/\"
> +          :working-directory \"~/Documents/MyBlog/themes/hugo-icarus-theme/exampleSite/content/post/\"
> +          :online-suffix \".html\"
> +          :working-suffix \".md\"
> +          :rewrites ((\"\\(https://themes.gohugo.io/theme/hugo-icarus/[0-9]+/[0-9]+/[0-9]+/\\)\" . \".md\")))))

Would it be possible to make the path involved smaller so that the
example can fit in 80 columns?

>     The last line tells `org-protocol-open-source' to open
>     /home/user/org/index.php, if the URL cannot be mapped to an existing
> @@ -554,8 +561,13 @@ The location for a browser's bookmark should look like this:
>  		      ;; Try to match a rewritten URL and map it to
>  		      ;; a real file.  Compare redirects without
>  		      ;; suffix.
> -		      (when (string-match-p (car rewrite) f1)
> -			(throw 'result (concat wdir (cdr rewrite))))))))
> +		      (when (string-match (car rewrite) f1)
> +			(setq replacement (cdr rewrite))

Here, `replacement' isn't lexically bound, so compiling
"org-protocol.el" should return an error. `replacement' should be
let-bound instead.

> +			(if (match-string 0 f1)

AFAIU, (match-string 0 f1) is necessarily true, per the (string-match
(car rewrite) f1) above, so the test can be removed.

> +			    (setq replacement (concat
> +					       (directory-file-name  (replace-match "" nil nil f1 1))
> +					       replacement )))))

Spurious white space character before the parenthesis.

> +		    (throw 'result (concat wdir replacement)))))

Shouldn't the (throw ...) be within the (when (string-match ...)) as it
was before? I may be wrong. I didn't check the high level logic of the
patch since I don't know "org-protocol.el".

> Applied for it just now. Have marked the changes as tiny.

Great. Let me know when the process is complete.

Regards,

-- 
Nicolas Goaziou                                                0x80A93738