Re: [PATCH] org-contacts.el: Catch 'nextfile in org-contacts-db

Re: [PATCH] org-contacts.el: Catch 'nextfile in org-contacts-db

[phcrama.ebiz]

[-- Attachment #1.1: Type: text/plain, Size: 390 bytes --]

Hi Nicolas and Greg,

Thank you for applying my initial patch.

Inspired by Greg's improvements to my patch, I'd like to propose the attached (presumptuously named) better-patch that I believe fixes the TODO of my initial patch.

Sorry it isn't quite in the format of the previous one/recommended by the project but I'm not on my main machine now.

Best regards,
--
Philippe Crama

[-- Attachment #1.2: Type: text/html, Size: 446 bytes --]

[-- Attachment #2: 0001-org-contacts.el-Handle-missing-files-in-org-contacts(1).patch --]
[-- Type: application/octet-stream, Size: 1967 bytes --]

From b3bb252c6e2138fa65b7c960060d68ea8c260873 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gr=C3=A9goire=20Jadi?= <gregoire.jadi@univ-nantes.fr>
Date: Mon, 15 Jun 2015 09:36:36 +0200
Subject: [PATCH] org-contacts.el: Handle missing files in `org-contacts-files'
 gracefully

* contrib/lisp/org-contacts.el (org-contacts-files): Catch 'nextfile
From `org-check-agenda-file' and update `org-contacts-files' if
modified.

(org-contacts-db): Don't call `org-check-agenda-file' now called in
`org-contacts-files'.

When `org-agenda-files' contains non-existent files, remove them if
the user asks for it instead of failing.

Modified from a patch proposal by Philippe Crama
---
 contrib/lisp/org-contacts.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/contrib/lisp/org-contacts.el b/contrib/lisp/org-contacts.el
index bc07fc1..3e14c7e 100644
--- a/contrib/lisp/org-contacts.el
+++ b/contrib/lisp/org-contacts.el
@@ -227,7 +227,12 @@ A regexp matching strings of whitespace, `,' and `;'.")
 
 (defun org-contacts-files ()
   "Return list of Org files to use for contact management."
-  (or org-contacts-files (org-agenda-files t 'ifmode)))
+  (org-remove-if (lambda (file)
+		   (when (catch 'nextfile
+			   (org-check-agenda-file file))
+		     (customize-save-variable 'org-contacts-files
+					      (delete file org-contacts-files))))
+		 (or org-contacts-files (org-agenda-files t 'ifmode))))
 
 (defun org-contacts-db-need-update-p ()
   "Determine whether `org-contacts-db' needs to be refreshed."
@@ -261,7 +266,6 @@ to dead or no buffer."
 	     (make-progress-reporter "Updating Org Contacts Database..." 0 (length org-contacts-files)))
 	    (i 0))
 	(dolist (file (org-contacts-files))
-	  (org-check-agenda-file file)
 	  (with-current-buffer (org-get-agenda-file-buffer file)
 	    (unless (eq major-mode 'org-mode)
 	      (error "File %s is not in `org-mode'" file))
-- 
1.9.1


[-- Attachment #3: better-patch --]
[-- Type: application/octet-stream, Size: 2648 bytes --]

--- org.el	Fri Jun 19 12:07:26 2015
+++ org-modif.el	Wed Jun 24 09:13:10 2015
@@ -18097,17 +18097,28 @@
 (defun org-file-menu-entry (file)
   (vector file (list 'find-file file) t))
 
-(defun org-check-agenda-file (file)
-  "Make sure FILE exists.  If not, ask user what to do."
+(defun org-check-agenda-file-1 (file)
+  "Make sure FILE exists.  If not, ask user what to do.
+
+Possible outcomes:
+1. File exists -> return NIL.
+2. File doesn't exist and user requests to remove it -> throws 'nextfile.
+3. File doesn't exist and user requests to remove it -> raise error \"Abort\".
+
+You probably want `org-check-agenda-file'."
   (when (not (file-exists-p file))
     (message "Non-existent agenda file %s.  [R]emove from list or [A]bort?"
 	     (abbreviate-file-name file))
     (let ((r (downcase (read-char-exclusive))))
       (cond
        ((equal r ?r)
-	(org-remove-file file)
 	(throw 'nextfile t))
        (t (error "Abort"))))))
+
+(defun org-check-agenda-file (file)
+  "Make sure FILE exists, else if user agrees, remove FILE from `org-agenda-files'."
+  (when (catch 'nextfile (org-check-agenda-file-1 file))
+    (org-remove-file file)))
 
 (defun org-get-agenda-file-buffer (file)
   "Get a buffer visiting FILE.  If the buffer needs to be created, add


--- org-contacts.el	Fri Jun 19 12:07:20 2015
+++ org-contacts-modif.el	Wed Jun 24 09:13:56 2015
@@ -216,7 +216,18 @@
 
 (defun org-contacts-files ()
   "Return list of Org files to use for contact management."
-  (or org-contacts-files (org-agenda-files t 'ifmode)))
+  (if org-contacts-files
+      (org-remove-if (lambda (file)
+                       (when (catch 'nextfile
+                               (org-check-agenda-file-1 file))
+                         (customize-save-variable 'org-contacts-files
+                                                  (delete file org-contacts-files))))
+                     org-contacts-files)
+    (let (result)
+      (dolist (file (org-agenda-files t 'ifmode) (nreverse result))
+        (unless (catch 'nextfile
+                  (org-check-agenda-file file))
+          (push file result))))))
 
 (defun org-contacts-db-need-update-p ()
   "Determine whether `org-contacts-db' needs to be refreshed."
@@ -250,7 +261,6 @@
 	     (make-progress-reporter "Updating Org Contacts Database..." 0 (length org-contacts-files)))
 	    (i 0))
 	(dolist (file (org-contacts-files))
-	  (org-check-agenda-file file)
 	  (with-current-buffer (org-get-agenda-file-buffer file)
 	    (unless (eq major-mode 'org-mode)
 	      (error "File %s is no in `org-mode'" file))

Re: [PATCH] org-contacts.el: Catch 'nextfile in org-contacts-db

[Daimrod]

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

phcrama.ebiz@gmx.com writes:

> Hi Nicolas and Greg,

Hi Philippe, sorry for the late reply :(

> Thank you for applying my initial patch.
>
> Inspired by Greg's improvements to my patch, I'd like to propose the attached (presumptuously named) better-patch that I believe fixes the TODO of my initial patch.
>
> Sorry it isn't quite in the format of the previous one/recommended by the project but I'm not on my main machine now.

> --- org.el	Fri Jun 19 12:07:26 2015
> +++ org-modif.el	Wed Jun 24 09:13:10 2015
> @@ -18097,17 +18097,28 @@
>  (defun org-file-menu-entry (file)
>    (vector file (list 'find-file file) t))
>  
> -(defun org-check-agenda-file (file)
> -  "Make sure FILE exists.  If not, ask user what to do."
> +(defun org-check-agenda-file-1 (file)
> +  "Make sure FILE exists.  If not, ask user what to do.
> +
> +Possible outcomes:
> +1. File exists -> return NIL.
> +2. File doesn't exist and user requests to remove it -> throws 'nextfile.
> +3. File doesn't exist and user requests to remove it -> raise error \"Abort\".
> +
> +You probably want `org-check-agenda-file'."
>    (when (not (file-exists-p file))
>      (message "Non-existent agenda file %s.  [R]emove from list or [A]bort?"
>  	     (abbreviate-file-name file))
>      (let ((r (downcase (read-char-exclusive))))
>        (cond
>         ((equal r ?r)
> -	(org-remove-file file)
>  	(throw 'nextfile t))
>         (t (error "Abort"))))))

The 3. item looks wrong to me. I think it should be:
3. File doesn't exist and user requests to abort -> raise error \"Abort\".

> +(defun org-check-agenda-file (file)
> +  "Make sure FILE exists, else if user agrees, remove FILE from `org-agenda-files'."
> +  (when (catch 'nextfile (org-check-agenda-file-1 file))
> +    (org-remove-file file)))
>  
>  (defun org-get-agenda-file-buffer (file)
>    "Get a buffer visiting FILE.  If the buffer needs to be created, add
> 
> 
> --- org-contacts.el	Fri Jun 19 12:07:20 2015

Moreover, I'm not sure about changing the behaviour of an existing
function. Though the change looks good to me, it is very likely users
(or maybe even other parts of org-mode, I'haven't check) rely on it, and
expect to catch exceptions.

> +++ org-contacts-modif.el	Wed Jun 24 09:13:56 2015
> [snip]

I agree with the modifications of org-contacts.el.


Maybe org-contacts.el should have its own version of
`org-check-agenda-file' (e.g. named `org-contacts-check-contacts-file')
until we find a better solution...

WDYT?


Best,

-- 
Daimrod/Greg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] org-contacts.el: Catch 'nextfile in org-contacts-db

[phcrama.ebiz]

[-- Attachment #1: Type: text/html, Size: 547 bytes --]

[-- Attachment #2: 0001-Refactor-org-check-agenda-file-to-use-only-its-inter.patch --]
[-- Type: application/octet-stream, Size: 6309 bytes --]

From a8b92e49d0adfba0e7532be15d7d659bbb3159f5 Mon Sep 17 00:00:00 2001
From: "U-mathilde\\philippe" <phcrama.ebiz@gmx.com>
Date: Sat, 4 Jul 2015 23:01:56 +0200
Subject: [PATCH] Refactor org-check-agenda-file to use only its interactive
 part for org-contacts-files list

The org-contacts module uses org-check-agenda-file to filter the
`org-contacts-files' list.  However, when a file in that list was not
found, `org-check-agenda-file' would try to remove it from the
`org-agenda-files' list instead of the `org-contacts-files' list.

To keep one central place deciding what a valid agenda file
is (currently only file existence is checked, but who knows what other
checks might be added), I split off the UI part which will return NIL if
there's no problem, 'nextfile if the user decided to remove the file
from the list (note that the message doesn't say which list) and raises
a user-error if the user requests it.

I've then rewritten the `org-contacts-files' function to process the
`org-contacts-files' list using `org-check-agenda-file-1' and applying
the changes or (if `org-contacts-files' isn't define) to use
`org-check-agenda-file' on that list.
---
 contrib/lisp/org-contacts.el | 53 ++++++++++++++++++--------------------------
 lisp/org.el                  | 27 ++++++++++++++++++----
 2 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/contrib/lisp/org-contacts.el b/contrib/lisp/org-contacts.el
index edc09fe..fa0f7dc 100644
--- a/contrib/lisp/org-contacts.el
+++ b/contrib/lisp/org-contacts.el
@@ -227,7 +227,18 @@ A regexp matching strings of whitespace, `,' and `;'.")
 
 (defun org-contacts-files ()
   "Return list of Org files to use for contact management."
-  (or org-contacts-files (org-agenda-files t 'ifmode)))
+  (if org-contacts-files
+      (org-remove-if (lambda (file)
+                       (when (eq 'nextfile (org-check-agenda-file-1 file))
+                         (customize-save-variable
+			  'org-contacts-files
+			  (delete file org-contacts-files))))
+                     org-contacts-files)
+    (let (result)
+      (dolist (file (org-agenda-files t 'ifmode) (nreverse result))
+        (unless (catch 'nextfile
+                  (org-check-agenda-file file))
+          (push file result)))))))
 
 (defun org-contacts-db-need-update-p ()
   "Determine whether `org-contacts-db' needs to be refreshed."
@@ -261,37 +272,15 @@ to dead or no buffer."
 	     (make-progress-reporter "Updating Org Contacts Database..." 0 (length org-contacts-files)))
 	    (i 0))
 	(dolist (file (org-contacts-files))
-	  (if (catch 'nextfile
-                ;; if file doesn't exist and the user agrees to removing it
-                ;; from org-agendas-list, 'nextfile is thrown.  Catch it here
-                ;; and skip processing the file.
-                ;;
-                ;; TODO: suppose that the user has set an org-contacts-files
-                ;; list that contains an element that doesn't exist in the
-                ;; file system: in that case, the org-agenda-files list could
-                ;; be updated (and saved to the customizations of the user) if
-                ;; it contained the same file even though the org-agenda-files
-                ;; list wasn't actually used.  I don't think it is normal that
-                ;; org-contacts updates org-agenda-files in this case, but
-                ;; short of duplicating org-check-agenda-files and
-                ;; org-remove-files, I don't know how to avoid it.
-                ;;
-                ;; A side effect of the TODO is that the faulty
-                ;; org-contacts-files list never gets updated and thus the
-                ;; user is always queried about the missing files when
-                ;; org-contacts-db-need-update-p returns true.
-                (org-check-agenda-file file))
-              (message "Skipped %s removed from org-agenda-files list."
-                       (abbreviate-file-name file))
-	    (with-current-buffer (org-get-agenda-file-buffer file)
-	      (unless (eq major-mode 'org-mode)
-		(error "File %s is not in `org-mode'" file))
-	      (setf result
-		    (append result
-			    (org-scan-tags
-			     'org-contacts-at-point
-			     contacts-matcher
-			     todo-only)))))
+	  (with-current-buffer (org-get-agenda-file-buffer file)
+	    (unless (eq major-mode 'org-mode)
+	      (error "File %s is not in `org-mode'" file))
+	    (setf result
+		  (append result
+			  (org-scan-tags
+			   'org-contacts-at-point
+			   contacts-matcher
+			   todo-only))))
 	  (progress-reporter-update progress-reporter (setq i (1+ i))))
 	(setf org-contacts-db result
 	      org-contacts-last-update (current-time))
diff --git a/lisp/org.el b/lisp/org.el
index 085b763..26fc22e 100755
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -18559,18 +18559,37 @@ Optional argument FILE means use this file instead of the current."
 (defun org-file-menu-entry (file)
   (vector file (list 'find-file file) t))
 
-(defun org-check-agenda-file (file)
-  "Make sure FILE exists.  If not, ask user what to do."
+(defun org-check-agenda-file-1 (file)
+  "Make sure FILE exists.  If not, ask user what to do.
+
+This is an internal function.  See also `org-check-agenda-file'.  There are
+three possibilities:
+1. The file exists -> return NIL.
+2. The file doesn't exist and the user asks to remove it from the list
+   -> return 'next-file
+3. The file doesn't exist and the user asks to abort
+   -> raise error"
   (when (not (file-exists-p file))
     (message "Non-existent agenda file %s.  [R]emove from list or [A]bort?"
 	     (abbreviate-file-name file))
     (let ((r (downcase (read-char-exclusive))))
       (cond
        ((equal r ?r)
-	(org-remove-file file)
-	(throw 'nextfile t))
+	'nextfile)
        (t (user-error "Abort"))))))
 
+(defun org-check-agenda-file (file)
+  "Make sure FILE exists.  If not, ask user what to do."
+  (let ((problem (org-check-agenda-file-1 file)))
+    (cond
+     ((null problem) nil)
+     ((eql problem 'nextfile)
+      (org-remove-file file)
+      (throw 'nextfile t))
+     ;; NOTREAVHED
+     ;; (t (user-error "Abort"))
+     )))
+
 (defun org-get-agenda-file-buffer (file)
   "Get an agenda buffer visiting FILE.
 If the buffer needs to be created, add it to the list of buffers
-- 
2.3.5

[PATCH] org-contacts.el: Catch 'nextfile in org-contacts-db

[Philippe Crama]

Hi,

This is a small patch to catch the 'nextfile thrown by
`org-check-agenda-file' in `org-contacts-db'.

I don't know how to fix the TODO in the comment myself, sorry.  At
least, with the patch, the execution doesn't abort anymore.

Regards,
--
Philippe Crama

===File c:/msys64/tmp/org-mode/0001-org-contacts.el-Catch-nextfile-in-org-contacts-db.patch===
From 6f9679f2118fa8990386a0ed27a34d329cfc579f Mon Sep 17 00:00:00 2001
From: "U-mathilde\\philippe" <phcrama.ebiz@gmx.com>
Date: Wed, 10 Jun 2015 11:05:47 +0200
Subject: [PATCH] org-contacts.el: Catch 'nextfile in org-contacts-db

* org-contacts.el (org-contacts-db): Catch 'nextfile in org-contacts-db
(org-contacts-db): when a file in the list returned by the
`org-contacts-files' function doesn't exist and the user selects the
option to attempt to remove it from the `org-agenda-files' list, 'nextfile
is thrown.  Catch it and skip processing that file instead of failing.

TINYCHANGE
---
 contrib/lisp/org-contacts.el | 41 +++++++++++++++++++++++++++++++----------
 1 file changed, 31 insertions(+), 10 deletions(-)

diff --git a/contrib/lisp/org-contacts.el b/contrib/lisp/org-contacts.el
index bc07fc1..edc09fe 100644
--- a/contrib/lisp/org-contacts.el
+++ b/contrib/lisp/org-contacts.el
@@ -261,16 +261,37 @@ to dead or no buffer."
 	     (make-progress-reporter "Updating Org Contacts Database..." 0 (length org-contacts-files)))
 	    (i 0))
 	(dolist (file (org-contacts-files))
-	  (org-check-agenda-file file)
-	  (with-current-buffer (org-get-agenda-file-buffer file)
-	    (unless (eq major-mode 'org-mode)
-	      (error "File %s is not in `org-mode'" file))
-	    (setf result
-		  (append result
-			  (org-scan-tags
-			   'org-contacts-at-point
-			   contacts-matcher
-			   todo-only))))
+	  (if (catch 'nextfile
+                ;; if file doesn't exist and the user agrees to removing it
+                ;; from org-agendas-list, 'nextfile is thrown.  Catch it here
+                ;; and skip processing the file.
+                ;;
+                ;; TODO: suppose that the user has set an org-contacts-files
+                ;; list that contains an element that doesn't exist in the
+                ;; file system: in that case, the org-agenda-files list could
+                ;; be updated (and saved to the customizations of the user) if
+                ;; it contained the same file even though the org-agenda-files
+                ;; list wasn't actually used.  I don't think it is normal that
+                ;; org-contacts updates org-agenda-files in this case, but
+                ;; short of duplicating org-check-agenda-files and
+                ;; org-remove-files, I don't know how to avoid it.
+                ;;
+                ;; A side effect of the TODO is that the faulty
+                ;; org-contacts-files list never gets updated and thus the
+                ;; user is always queried about the missing files when
+                ;; org-contacts-db-need-update-p returns true.
+                (org-check-agenda-file file))
+              (message "Skipped %s removed from org-agenda-files list."
+                       (abbreviate-file-name file))
+	    (with-current-buffer (org-get-agenda-file-buffer file)
+	      (unless (eq major-mode 'org-mode)
+		(error "File %s is not in `org-mode'" file))
+	      (setf result
+		    (append result
+			    (org-scan-tags
+			     'org-contacts-at-point
+			     contacts-matcher
+			     todo-only)))))
 	  (progress-reporter-update progress-reporter (setq i (1+ i))))
 	(setf org-contacts-db result
 	      org-contacts-last-update (current-time))
-- 
2.3.5

============================================================

Re: [PATCH] org-contacts.el: Catch 'nextfile in org-contacts-db

[Daimrod]

[-- Attachment #1.1: Type: text/plain, Size: 395 bytes --]

Philippe Crama <phcrama.ebiz@gmx.com> writes:

> Hi,

Hello Philippe,

> This is a small patch to catch the 'nextfile thrown by
> `org-check-agenda-file' in `org-contacts-db'.
>
> I don't know how to fix the TODO in the comment myself, sorry.  At
> least, with the patch, the execution doesn't abort anymore.

Thanks for the report and the patch. I've attached a patch inspired by
yours. WDYT?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-org-contacts.el-Handle-missing-files-in-org-contacts.patch --]
[-- Type: text/x-diff, Size: 1967 bytes --]

From b3bb252c6e2138fa65b7c960060d68ea8c260873 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gr=C3=A9goire=20Jadi?= <gregoire.jadi@univ-nantes.fr>
Date: Mon, 15 Jun 2015 09:36:36 +0200
Subject: [PATCH] org-contacts.el: Handle missing files in `org-contacts-files'
 gracefully

* contrib/lisp/org-contacts.el (org-contacts-files): Catch 'nextfile
From `org-check-agenda-file' and update `org-contacts-files' if
modified.

(org-contacts-db): Don't call `org-check-agenda-file' now called in
`org-contacts-files'.

When `org-agenda-files' contains non-existent files, remove them if
the user asks for it instead of failing.

Modified from a patch proposal by Philippe Crama
---
 contrib/lisp/org-contacts.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/contrib/lisp/org-contacts.el b/contrib/lisp/org-contacts.el
index bc07fc1..3e14c7e 100644
--- a/contrib/lisp/org-contacts.el
+++ b/contrib/lisp/org-contacts.el
@@ -227,7 +227,12 @@ A regexp matching strings of whitespace, `,' and `;'.")
 
 (defun org-contacts-files ()
   "Return list of Org files to use for contact management."
-  (or org-contacts-files (org-agenda-files t 'ifmode)))
+  (org-remove-if (lambda (file)
+		   (when (catch 'nextfile
+			   (org-check-agenda-file file))
+		     (customize-save-variable 'org-contacts-files
+					      (delete file org-contacts-files))))
+		 (or org-contacts-files (org-agenda-files t 'ifmode))))
 
 (defun org-contacts-db-need-update-p ()
   "Determine whether `org-contacts-db' needs to be refreshed."
@@ -261,7 +266,6 @@ to dead or no buffer."
 	     (make-progress-reporter "Updating Org Contacts Database..." 0 (length org-contacts-files)))
 	    (i 0))
 	(dolist (file (org-contacts-files))
-	  (org-check-agenda-file file)
 	  (with-current-buffer (org-get-agenda-file-buffer file)
 	    (unless (eq major-mode 'org-mode)
 	      (error "File %s is not in `org-mode'" file))
-- 
1.9.1


[-- Attachment #1.3: Type: text/plain, Size: 37 bytes --]



> Regards,

-- 
Daimrod/Greg

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] org-contacts.el: Catch 'nextfile in org-contacts-db

[Nicolas Goaziou]

Hello,

Philippe Crama <phcrama.ebiz@gmx.com> writes:

> This is a small patch to catch the 'nextfile thrown by
> `org-check-agenda-file' in `org-contacts-db'.

Applied. Thank you.

Regards,

-- 
Nicolas Goaziou