[PATCH] org-attach.el: Fetch attachments from git annex

[PATCH] org-attach.el: Fetch attachments from git annex

[Erik Hetzner]

* org-attach.el (org-attach-use-annex): New function to check if git
  annex should be used.
  (org-annex-open): Automatically fetch attached files from git annex when
  opening if appropriate.
---
 lisp/org-attach.el | 41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index b3cd7b7..b3fde48 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -270,6 +270,14 @@ the ATTACH_DIR property) their own attachment directory."
   (org-entry-put nil "ATTACH_DIR_INHERIT" "t")
   (message "Children will inherit attachment directory"))
 
+(defun org-attach-use-annex ()
+  "Return true if we should use git annex for attachments."
+  (let* ((dir (expand-file-name org-attach-directory))
+	 (git-dir (vc-git-root dir)))
+    (and org-attach-git-annex-cutoff
+         (or (file-exists-p (expand-file-name "annex" git-dir))
+             (file-exists-p (expand-file-name ".git/annex" git-dir))))))
+
 (defun org-attach-commit ()
   "Commit changes to git if `org-attach-directory' is properly initialized.
 This checks for the existence of a \".git\" directory in that directory."
@@ -279,20 +287,16 @@ This checks for the existence of a \".git\" directory in that directory."
     (when (and git-dir (executable-find "git"))
       (with-temp-buffer
 	(cd dir)
-	(let ((have-annex
-	       (and org-attach-git-annex-cutoff
-		    (or (file-exists-p (expand-file-name "annex" git-dir))
-			(file-exists-p (expand-file-name ".git/annex" git-dir))))))
-	  (dolist (new-or-modified
-		   (split-string
-		    (shell-command-to-string
-		     "git ls-files -zmo --exclude-standard") "\0" t))
-	    (if (and have-annex
-		     (>= (nth 7 (file-attributes new-or-modified))
-			 org-attach-git-annex-cutoff))
-		(call-process "git" nil nil nil "annex" "add" new-or-modified)
-	      (call-process "git" nil nil nil "add" new-or-modified))
-	    (incf changes)))
+        (dolist (new-or-modified
+                 (split-string
+                  (shell-command-to-string
+                   "git ls-files -zmo --exclude-standard") "\0" t))
+          (if (and (org-attach-use-annex)
+                   (>= (nth 7 (file-attributes new-or-modified))
+                       org-attach-git-annex-cutoff))
+              (call-process "git" nil nil nil "annex" "add" new-or-modified)
+            (call-process "git" nil nil nil "add" new-or-modified))
+	    (incf changes))
 	(dolist (deleted
 		 (split-string
 		  (shell-command-to-string "git ls-files -z --deleted") "\0" t))
@@ -465,8 +469,13 @@ If IN-EMACS is non-nil, force opening in Emacs."
 	 (file (if (= (length files) 1)
 		   (car files)
 		 (org-icompleting-read "Open attachment: "
-				       (mapcar 'list files) nil t))))
-    (org-open-file (expand-file-name file attach-dir) in-emacs)))
+				       (mapcar 'list files) nil t)))
+         (path (expand-file-name file attach-dir)))
+    (if (and (file-symlink-p path)
+             (org-attach-use-annex)
+             (not (file-exists-p (file-symlink-p path))))
+        (call-process "git" nil nil nil "annex" "get" path))
+    (org-open-file path in-emacs)))
 
 (defun org-attach-open-in-emacs ()
   "Open attachment, force opening in Emacs.
-- 
2.5.0

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Eric Abrahamsen]

Erik Hetzner <egh@e6h.org> writes:

> * org-attach.el (org-attach-use-annex): New function to check if git
>   annex should be used.
>   (org-annex-open): Automatically fetch attached files from git annex when
>   opening if appropriate.

WANT!

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Erik Hetzner]

On Mon, 04 Jan 2016 21:09:55 -0800,
Erik Hetzner <egh@e6h.org> wrote:
>
> * org-attach.el (org-attach-use-annex): New function to check if git
>   annex should be used.
>   (org-annex-open): Automatically fetch attached files from git annex when
>   opening if appropriate.

[…]

Hi all,

I couldn’t figure out how to add a comment to my patch, so here is a little more
commentary.

Right now, if you use git to sync your Org files and git annex to manage your
(large) attachments, and are on a different computer from the one you were using
when you added the attachment, you will typically have a dangling symlink to
your attachment until you call =get annex get PATH=. (This is how git annex is
intended to work.)

This patch automatically calls =git annex get PATH= when you open the attachment
from Org, if the file has not been fetched already.

(It would be nice to have Org do the same thing when exporting to HTML as well,
if the git annex file is an image that is being exported as an embedded image
link, but I can’t figure out how to do that.)

I have signed copyright assignment papers with the FSF.

best, Erik
--
Sent from my free software system <http://fsf.org/>.

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Kyle Meyer]

Erik Hetzner <egh@e6h.org> writes:

> I couldn’t figure out how to add a comment to my patch, so here is a little more
> commentary.

Extra text can be placed after the three dashes and before the diff.

-- 
Kyle

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Rasmus]

Hi Erik,

> I couldn’t figure out how to add a comment to my patch, so here is a 
little more
> commentary.

For long prose like this I'd just export the patch (git format-patch)
and attach it to an email like this.  But this is also fine...  Perhaps
Kyle's method is more pro.

> Right now, if you use git to sync your Org files and git annex to 
manage your
> (large) attachments, and are on a different computer from the one you 
were using
> when you added the attachment, you will typically have a dangling 
symlink to
> your attachment until you call =get annex get PATH=. (This is how git 
annex is
> intended to work.)
> 
> This patch automatically calls =git annex get PATH= when you open the 
attachment
> from Org, if the file has not been fetched already.

This is a good idea.  Though really this sounds like something that
should be done by the git annex assistant rather than per application.

Has Joey spoken for or against automatic fetching of content when
requested?

> (It would be nice to have Org do the same thing when exporting to HTML 
as well,
> if the git annex file is an image that is being exported as an 
embedded image
> link, but I can’t figure out how to do that.)

A stronger argument for for first figuring out whether it 



> +(defun org-attach-use-annex ()
> +  "Return true if we should use git annex for attachments."
> +  (let* ((dir (expand-file-name org-attach-directory))
> +	 (git-dir (vc-git-root dir)))
> +    (and org-attach-git-annex-cutoff
> +         (or (file-exists-p (expand-file-name "annex" git-dir))
> +             (file-exists-p (expand-file-name ".git/annex" git-
dir))))))

As Kyle points out there should be a defcustom as well to decide whether
annex should be used.

since you call (org-attach-use-annex) you should probably check this
variable here.

>  (defun org-attach-commit ()
>    "Commit changes to git if `org-attach-directory' is properly 
initialized.
>  This checks for the existence of a \".git\" directory in that 
directory."
>  <at>  <at>  -279,20 +287,16  <at>  <at>  This checks for the 
existence of a \".git\" directory in that directory."
>      (when (and git-dir (executable-find "git"))
>        (with-temp-buffer
>  	(cd dir)
> -	(let ((have-annex
> -	       (and org-attach-git-annex-cutoff
> -		    (or (file-exists-p (expand-file-name "annex" git-
dir))
> -			(file-exists-p (expand-file-name ".git/annex" 
git-dir))))))
> -	  (dolist (new-or-modified
> -		   (split-string
> -		    (shell-command-to-string
> -		     "git ls-files -zmo --exclude-standard") "\0" t))
> -	    (if (and have-annex
> -		     (>= (nth 7 (file-attributes new-or-modified))
> -			 org-attach-git-annex-cutoff))
> -		(call-process "git" nil nil nil "annex" "add" new-or-
modified)
> -	      (call-process "git" nil nil nil "add" new-or-modified))
> -	    (incf changes)))
> +        (dolist (new-or-modified
> +                 (split-string
> +                  (shell-command-to-string
> +                   "git ls-files -zmo --exclude-standard") "\0" t))
> +          (if (and (org-attach-use-annex)

> +                   (>= (nth 7 (file-attributes new-or-modified))
> +                       org-attach-git-annex-cutoff))

IMO we should get rid of org-attach-git-annex-cutoff and point to
preferred content:

          http://git-annex.branchable.com/preferred_content/

E.g. I might have a small mp3 file.  There's no point in storing it with
git rather than git annex.

When preferred content is set up, I think git annex add will do the
right thing.

> +              (call-process "git" nil nil nil "annex" "add" new-or-
modified)
> +            (call-process "git" nil nil nil "add" new-or-modified))
> +	    (incf changes))
>  	(dolist (deleted
>  		 (split-string
>  		  (shell-command-to-string "git ls-files -z --deleted") 
"\0" t))
>  <at>  <at>  -465,8 +469,13  <at>  <at>  If IN-EMACS is non-nil, force 
opening in Emacs."
>  	 (file (if (= (length files) 1)
>  		   (car files)
>  		 (org-icompleting-read "Open attachment: "
> -				       (mapcar 'list files) nil t))))
> -    (org-open-file (expand-file-name file attach-dir) in-emacs)))
> +				       (mapcar 'list files) nil t)))
> +         (path (expand-file-name file attach-dir)))
> +    (if (and (file-symlink-p path)
> +             (org-attach-use-annex)
> +             (not (file-exists-p (file-symlink-p path))))

Can you ask annex if it's an annexed file?  A symlink in an annexed 
folder,
to a usb disk that is not attached would pass this test.  But the 
indication would be wrong.

> +        (call-process "git" nil nil nil "annex" "get" path))
> +    (org-open-file path in-emacs)))

Thanks,
Rasmus

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Kyle Meyer]

Rasmus <rasmus@gmx.us> writes:

[...]

>> This patch automatically calls =git annex get PATH= when you open the
>> attachment from Org, if the file has not been fetched already.
>
> This is a good idea.  Though really this sounds like something that
> should be done by the git annex assistant rather than per application.

I think a good number of people use git-annex without git-annex
assistant.  There was a question related to this on the git-annex
survey:

http://git-annex-survey.branchable.com/polls/2015/command_line_vs_assistant/

> Has Joey spoken for or against automatic fetching of content when
> requested?

I think I'm missing something.  Why would he have an issue with running
"git annex get"?

[...]

> Can you ask annex if it's an annexed file?

There's probably a better way, but the return value of "git annex find
FILE" will signal this.

-- 
Kyle

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Rasmus]

Hi Kyle,

Kyle Meyer <kyle@kyleam.com> writes:

> Rasmus <rasmus@gmx.us> writes:
>
> [...]
>
>>> This patch automatically calls =git annex get PATH= when you open the
>>> attachment from Org, if the file has not been fetched already.
>>
>> This is a good idea.  Though really this sounds like something that
>> should be done by the git annex assistant rather than per application.
>
> I think a good number of people use git-annex without git-annex
> assistant.  There was a question related to this on the git-annex
> survey:
>
> http://git-annex-survey.branchable.com/polls/2015/command_line_vs_assistant/

That’s a fair point.

>> Has Joey spoken for or against automatic fetching of content when
>> requested?
>
> I think I'm missing something.  Why would he have an issue with running
> "git annex get"?

I don't have anything against it, but in OP he also talks about
implementing something similar for ox-$BACKEND output...  Which doesn’t
seem like a bad request.  Why should org-attachment support git-annex
other parts of Org not?  But I don’t think we want to support annexed
files in general.  (What about LFS’ed files?  Assuming a similar feature
exists)

Perhaps I’m abstracting too much here.

Rasmus

-- 
Even a three-legged dog has three good legs to lose

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Kyle Meyer]

Rasmus <rasmus@gmx.us> writes:

>>> Has Joey spoken for or against automatic fetching of content when
>>> requested?
>>
>> I think I'm missing something.  Why would he have an issue with running
>> "git annex get"?
>
> I don't have anything against it, but in OP he also talks about
> implementing something similar for ox-$BACKEND output...  Which doesn’t
> seem like a bad request.  Why should org-attachment support git-annex
> other parts of Org not?  But I don’t think we want to support annexed
> files in general.  (What about LFS’ed files?  Assuming a similar feature
> exists)

I think Joey is a vim user, so I doubt he's said anything about an Emacs
program choosing to call "git annex get" :)

But yeah, that's a good point about which parts of Org should support or
care about annexed files.  IMO export and nearly all parts of Org should
not.  I see org-attach as an exception because it is focused on managing
external files and already has some support for doing so with git and
git-annex.  (I think it would be fine for org-attach to support other
ways of storing files, should someone care to implement it.)  But I
don't actually use org-attach, so I may be off the mark here.

-- 
Kyle

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Rasmus]

Kyle Meyer <kyle@kyleam.com> writes:

> Rasmus <rasmus@gmx.us> writes:
>
>>>> Has Joey spoken for or against automatic fetching of content when
>>>> requested?
>>>
>>> I think I'm missing something.  Why would he have an issue with running
>>> "git annex get"?
>>
>> I don't have anything against it, but in OP he also talks about
>> implementing something similar for ox-$BACKEND output...  Which doesn’t
>> seem like a bad request.  Why should org-attachment support git-annex
>> other parts of Org not?  But I don’t think we want to support annexed
>> files in general.  (What about LFS’ed files?  Assuming a similar feature
>> exists)
>
> I think Joey is a vim user, so I doubt he's said anything about an Emacs
> program choosing to call "git annex get" :)

I want to know if e.g. the git annex (assistant?) could support
transparently getting files.  That is once I try to open file X, which is
annexed and stored somewhere else, X will automatically be fetched
/without/ the need to call "git annex get" first.

I don’t know enough about annex, or file systems, to make a guess.  [And I
always just call "git annex get ." in the root of the annexed folder].

> But yeah, that's a good point about which parts of Org should support or
> care about annexed files.  IMO export and nearly all parts of Org should
> not.  I see org-attach as an exception because it is focused on managing
> external files and already has some support for doing so with git and
> git-annex.  (I think it would be fine for org-attach to support other
> ways of storing files, should someone care to implement it.)  But I
> don't actually use org-attach, so I may be off the mark here.

The problem sounds like an Upstream Problemᵀᴹ (IMO), and I’d be happy to
get rid of org-attach-git-annex-cutoff, i.e. cutting back on the
"explicit" support (in favor of the better tools for this problem built
into git annex).

Rasmus

-- 
Together we will make the possible totalllly impossible!

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Kyle Meyer]

Rasmus <rasmus@gmx.us> writes:

> I want to know if e.g. the git annex (assistant?) could support
> transparently getting files.  That is once I try to open file X, which is
> annexed and stored somewhere else, X will automatically be fetched
> /without/ the need to call "git annex get" first.

No, I don't think git-annex has a feature like this, though it seems a
"git annex open" command was requested a while back:

  https://git-annex.branchable.com/todo/git_annex_open/

--
Kyle

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Erik Hetzner]

Hi Rasmus,

On Tue, 05 Jan 2016 01:56:39 -0800,
Rasmus <rasmus@gmx.us> wrote:
> 
> Hi Erik,
> 
> 
> For long prose like this I'd just export the patch (git format-patch)
> and attach it to an email like this.  But this is also fine...  Perhaps
> Kyle's method is more pro.

Thanks! I’m not sure which seems better either.

> […]
> 
> This is a good idea.  Though really this sounds like something that
> should be done by the git annex assistant rather than per application.
> 
> Has Joey spoken for or against automatic fetching of content when
> requested?

If a user prefers to use git annex assistant or preferred content that shouldn’t
interfere with this. But to my mind, opening an attachment is a pretty clear
indication of the user’s desire to fetch the file from git annex, so it would be
nice to do it.

> > (It would be nice to have Org do the same thing when exporting to HTML 
> as well,
> > if the git annex file is an image that is being exported as an 
> embedded image
> > link, but I can’t figure out how to do that.)
> 
> A stronger argument for for first figuring out whether it 

Well, I think it would have been nice, but the “preferred content” feature you
reference below seems like it could cover most of this. And, while I think that
trying to open an attachment is a pretty clear indication that the user wants to
fetch the file from git annex, exporting is much less clear. I’m pretty
convinced that Kyle is right that git annex support belongs in org-attach, but
not elsewhere.

> […]
> 
> IMO we should get rid of org-attach-git-annex-cutoff and point to
> preferred content:
> 
>           http://git-annex.branchable.com/preferred_content/
> 
> E.g. I might have a small mp3 file.  There's no point in storing it with
> git rather than git annex.
> 
> When preferred content is set up, I think git annex add will do the
> right thing.

I’m not certain how preferred content works, but it doesn’t seem to apply to
which content is added to git annex (as opposed to added to the git repo
itself), which is what org-attach-git-annex-cutoff does.

> Can you ask annex if it's an annexed file?  A symlink in an annexed 
> folder,
> to a usb disk that is not attached would pass this test.  But the 
> indication would be wrong.

Thanks! I’m going to use Kyle’s suggestion here, to use =git annex find PATH=.

best, Erik
--
Sent from my free software system <http://fsf.org/>.

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Rasmus]

Hi Erik,

Erik Hetzner <egh@e6h.org> writes:

> If a user prefers to use git annex assistant or preferred content that shouldn’t
> interfere with this. But to my mind, opening an attachment is a pretty clear
> indication of the user’s desire to fetch the file from git annex, so it would be
> nice to do it.

Yes, there’s no harm.  Thanks for the pointer to the bug "git annex open".
Combining "git annex find" and "git annex get" seems like a fine solution.


>> […]
>> 
>> IMO we should get rid of org-attach-git-annex-cutoff and point to
>> preferred content:
>> 
>>           http://git-annex.branchable.com/preferred_content/
>> 
>> E.g. I might have a small mp3 file.  There's no point in storing it with
>> git rather than git annex.
>> 
>> When preferred content is set up, I think git annex add will do the
>> right thing.
>
> I’m not certain how preferred content works, but it doesn’t seem to apply to
> which content is added to git annex (as opposed to added to the git repo
> itself), which is what org-attach-git-annex-cutoff does.

Sorry.  What I was thinking of is annex.largefiles, which /uses/ the
preferred contents syntax.

E.g. in my config.annex I use the following setting:

[annex]
        ...
	largefiles = (largerthan=200kb or include=*.otf or include=*.ttf) and not (include=*.json or include=*.el or include=*.org)

Files that are not largefiles are checked in as normal git files.
Example:

    $ git annex find .fonts/FiraMono-BoldItalic.ttf
      .fonts/FiraMono-BoldItalic.ttf
      # → checked in in annex.
    $ git annex find .emacs.d/init.el
    $ git log -n1 --oneline  -- .emacs.d/init.el
    9a8fd3a git-annex in W530: conf.annex
    # → checked in in vanilla git.

Rasmus

-- 
This message is brought to you by the department of redundant departments

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Kyle Meyer]

Thanks for the patch.

Erik Hetzner <egh@e6h.org> writes:

> +(defun org-attach-use-annex ()
> +  "Return true if we should use git annex for attachments."

s/true/non-nil/

> +  (let* ((dir (expand-file-name org-attach-directory))
> +	 (git-dir (vc-git-root dir)))

I'd prefer

    (let ((git-dir (vc-git-root (expand-file-name org-attach-directory)))) ...)

[...]

> +    (if (and (file-symlink-p path)

s/if/when/

> +             (org-attach-use-annex)
> +             (not (file-exists-p (file-symlink-p path))))
> +        (call-process "git" nil nil nil "annex" "get" path))

- Should this display a message before calling "git annex get" to let the
  user know what's happening in case fetching the file takes some time?

- Should there be a setting the controls whether attachments are
  automatically fetched?

--
Kyle

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Erik Hetzner]

Thanks for the feedback, Kyle! I will make these changes and resubmit.

And thanks for the pointer to =git annex find PATH=. I think I can use that to
check if a file needs to be fetched.

(more below)

On Mon, 04 Jan 2016 22:21:41 -0800,
Kyle Meyer <kyle@kyleam.com> wrote:
> 
> Thanks for the patch.
>
> […]
> 
> > +             (org-attach-use-annex)
> > +             (not (file-exists-p (file-symlink-p path))))
> > +        (call-process "git" nil nil nil "annex" "get" path))
> 
> - Should this display a message before calling "git annex get" to let the
>   user know what's happening in case fetching the file takes some time?

That would make sense. I’ll change it.

> - Should there be a setting the controls whether attachments are
>   automatically fetched?

I don’t see why. If a file is managed in git annex and the user is trying to
open it, I can’t imagine why they wouldn’t want it fetched.

best, Erik
--
Sent from my free software system <http://fsf.org/>.

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Erik Hetzner]

* org-attach.el (org-attach-use-annex): New function to check if git
  annex should be used.
  (org-attach-annex-fetch-maybe): New function to fetch a file from git
  annex if necessary.
  (org-annex-open): Automatically fetch attached files from git annex when
  opening if necessary.
* testing/lisp/test-org-annex.el: New file for testing org-attach.  Only
  contains code for testing org-attach with git annex at the moment.
---
I have finally updated this patch per the feedback on the list. I have also
added some tests of the git annex attach code.

Thanks for all the feedback.

 lisp/org-attach.el              | 46 +++++++++++++++--------
 testing/lisp/test-org-attach.el | 81 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 16 deletions(-)
 create mode 100644 testing/lisp/test-org-attach.el

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index e6ad4b1..3500e26 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -270,6 +270,22 @@ the ATTACH_DIR property) their own attachment directory."
   (org-entry-put nil "ATTACH_DIR_INHERIT" "t")
   (message "Children will inherit attachment directory"))
 
+(defun org-attach-use-annex ()
+  "Return non-nil if git annex can be used."
+  (let ((git-dir (vc-git-root (expand-file-name org-attach-directory))))
+    (and org-attach-git-annex-cutoff
+         (or (file-exists-p (expand-file-name "annex" git-dir))
+             (file-exists-p (expand-file-name ".git/annex" git-dir))))))
+
+(defun org-attach-annex-fetch-maybe (path)
+  "Fetch PATH from git annex if necessary."
+  (when (and (org-attach-use-annex)
+	     (not (string-equal "found"
+				(shell-command-to-string
+				 (format "git annex find --format=found --in=here %s" (shell-quote-argument path))))))
+    (message "Fetching \"%s\" using git annex." path)
+    (call-process "git" nil nil nil "annex" "get" path)))
+
 (defun org-attach-commit ()
   "Commit changes to git if `org-attach-directory' is properly initialized.
 This checks for the existence of a \".git\" directory in that directory."
@@ -279,20 +295,16 @@ This checks for the existence of a \".git\" directory in that directory."
     (when (and git-dir (executable-find "git"))
       (with-temp-buffer
 	(cd dir)
-	(let ((have-annex
-	       (and org-attach-git-annex-cutoff
-		    (or (file-exists-p (expand-file-name "annex" git-dir))
-			(file-exists-p (expand-file-name ".git/annex" git-dir))))))
-	  (dolist (new-or-modified
-		   (split-string
-		    (shell-command-to-string
-		     "git ls-files -zmo --exclude-standard") "\0" t))
-	    (if (and have-annex
-		     (>= (nth 7 (file-attributes new-or-modified))
-			 org-attach-git-annex-cutoff))
-		(call-process "git" nil nil nil "annex" "add" new-or-modified)
-	      (call-process "git" nil nil nil "add" new-or-modified))
-	    (incf changes)))
+        (dolist (new-or-modified
+                 (split-string
+                  (shell-command-to-string
+                   "git ls-files -zmo --exclude-standard") "\0" t))
+          (if (and (org-attach-use-annex)
+                   (>= (nth 7 (file-attributes new-or-modified))
+                       org-attach-git-annex-cutoff))
+              (call-process "git" nil nil nil "annex" "add" new-or-modified)
+            (call-process "git" nil nil nil "add" new-or-modified))
+	    (incf changes))
 	(dolist (deleted
 		 (split-string
 		  (shell-command-to-string "git ls-files -z --deleted") "\0" t))
@@ -465,8 +477,10 @@ If IN-EMACS is non-nil, force opening in Emacs."
 	 (file (if (= (length files) 1)
 		   (car files)
 		 (completing-read "Open attachment: "
-				  (mapcar #'list files) nil t))))
-    (org-open-file (expand-file-name file attach-dir) in-emacs)))
+				  (mapcar #'list files) nil t)))
+         (path (expand-file-name file attach-dir)))
+    (org-attach-annex-fetch-maybe path)
+    (org-open-file path in-emacs)))
 
 (defun org-attach-open-in-emacs ()
   "Open attachment, force opening in Emacs.
diff --git a/testing/lisp/test-org-attach.el b/testing/lisp/test-org-attach.el
new file mode 100644
index 0000000..7f435bd
--- /dev/null
+++ b/testing/lisp/test-org-attach.el
@@ -0,0 +1,81 @@
+;;; test-org-attach.el --- Tests for Org Attach
+;;
+;; Copyright (c) 2016 Erik Hetzner
+;; Authors: Erik Hetzner
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+(require 'org-attach)
+
+(cl-defmacro test-org-attach-annex/with-annex (&body body)
+  `(let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (shell-command "git annex init")
+	   ,@body))))
+
+(ert-deftest test-org-attach/use-annex ()
+  (org-test-for-executable "git-annex")
+  ;; (skip-unless (test-org-attach-annex/installed))
+  (test-org-attach-annex/with-annex
+   (let ((org-attach-git-annex-cutoff 1))
+     (should (org-attach-use-annex)))
+
+   (let ((org-attach-git-annex-cutoff nil))
+     (should-not (org-attach-use-annex))))
+
+  ;; test with non annex directory
+  (let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (should-not (org-attach-use-annex)))
+       (delete-directory tmpdir 'recursive))))
+
+(ert-deftest test-org-attach/fetch-maybe ()
+  (org-test-for-executable "git-annex")
+  ;; (skip-unless (test-org-attach-annex/installed))
+  (test-org-attach-annex/with-annex
+   (let ((path (expand-file-name "test-file"))
+	 (annex-dup (make-temp-file "org-annex-test" t)))
+     (with-temp-buffer
+       (insert "hello world\n")
+       (write-file path))
+     (shell-command "git annex add test-file")
+     (shell-command "git annex sync")
+     ;; Set up remote & copy files there
+     (let ((annex-original default-directory)
+	   (default-directory annex-dup))
+       (shell-command (format "git clone %s ." (shell-quote-argument annex-original)))
+       (shell-command "git annex init dup")
+       (shell-command (format "git remote add original %s" (shell-quote-argument annex-original)))
+       (shell-command "git annex get test-file")
+       (shell-command "git annex sync"))
+     (shell-command (format "git remote add dup %s" (shell-quote-argument annex-dup)))
+     (shell-command "git annex sync")
+     (shell-command "git annex drop --force test-file")
+     ;; test getting the file from the dup
+     (org-attach-annex-fetch-maybe (expand-file-name "test-file"))
+     ;; check that the file has the right contents
+     (with-temp-buffer
+       (insert-file-contents path)
+       (should (string-equal "hello world\n" (buffer-string)))))))
+
+;;; test-org-attach.el ends here
-- 
2.5.0

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Rasmus]

Hi Erik,

Thanks for the patch.

Note that between git-annex v6 and annex.largefiles most of the checking
is unnecessary.  In my opinion it would be much more useful to start
ripping out annex specific code, though the automatic fetching should be
added.

Erik Hetzner <egh@e6h.org> writes:

> +(defun org-attach-use-annex ()
> +  "Return non-nil if git annex can be used."
> +  (let ((git-dir (vc-git-root (expand-file-name org-attach-directory))))
> +    (and org-attach-git-annex-cutoff
> +         (or (file-exists-p (expand-file-name "annex" git-dir))
> +             (file-exists-p (expand-file-name ".git/annex" git-dir))))))

Seems fine, but I wonder if it wouldn’t be better to check the exit code
of say an annex command and relies on its checking.  E.g. on my system

    (zerop (shell-command "cd ~/annex/doc.annex/ && git annex info --fast" nil)) => t
    (zerop (shell-command "cd ~/src/code/org-mode && git annex info --fast" nil )) => nil

> +(defun org-attach-annex-fetch-maybe (path)
> +  "Fetch PATH from git annex if necessary."
> +  (when (and (org-attach-use-annex)
> +	     (not (string-equal "found"
> +				(shell-command-to-string
> +				 (format "git annex find --format=found --in=here %s" (shell-quote-argument path))))))
> +    (message "Fetching \"%s\" using git annex." path)
> +    (call-process "git" nil nil nil "annex" "get" path)))

AFAIK annex will check if get should get anything.  If that’s correct, I’d
prefer to just rely on however git annex get checks files.

> +        (dolist (new-or-modified
> +                 (split-string
> +                  (shell-command-to-string
> +                   "git ls-files -zmo --exclude-standard") "\0" t))

When I run this command in one of my annex repos I get:

     fatal: This operation must be run in a work tree

Maybe you are assuming indirect mode?

> +          (if (and (org-attach-use-annex)

wouldn’t it be better to bind the return value of (org-attach-use-annex)
rather than call it once per file to be added?  Since there’s no dir
argument, I guess it won’t change.

> +                   (>= (nth 7 (file-attributes new-or-modified))
> +                       org-attach-git-annex-cutoff))

If people want this they can use annex.largefiles.  Reimplementing
annex.largefiles is not within Org’s domain.  It’s even more the case with
annex v6.

> +              (call-process "git" nil nil nil "annex" "add" new-or-modified)
> +            (call-process "git" nil nil nil "add" new-or-modified))

In git annex v6 you don’t need to call "git annex add" (but can).  In git
annex v5 you don’t need to call "git add".

To be compatible between with both v5 and v6 you can just call "git annex
add", I guess.


> +++ b/testing/lisp/test-org-attach.el
> @@ -0,0 +1,81 @@
> +;;; test-org-attach.el --- Tests for Org Attach
> +;;
> +;; Copyright (c) 2016 Erik Hetzner
> +;; Authors: Erik Hetzner

I did not check this part.

Thanks,
Rasmus


-- 
Not everything that goes around comes back around, you know

[PATCH] org-attach.el: Fetch attachments from git annex

[Erik Hetzner]

* org-attach.el (org-attach-use-annex): New function to check if git
  annex should be used.
  (org-attach-annex-get-maybe): New function to get a file from git
  annex if necessary.
  (org-annex-open): Automatically fetch attached files from git annex when
  opening if necessary.
* testing/lisp/test-org-annex.el: New file for testing org-attach. Only
  contains code for testing org-attach with git annex at the moment.
* mk/targets.mk: Fix cleantest target to for deleting git annex repos.
---
Changes per Rasmus’s feedback.

lisp/org-attach.el              | 43 ++++++++++++++--------
 mk/targets.mk                   |  2 +
 testing/lisp/test-org-attach.el | 81 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 110 insertions(+), 16 deletions(-)
 create mode 100644 testing/lisp/test-org-attach.el

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index e6ad4b1..1085ad3 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -270,29 +270,38 @@ the ATTACH_DIR property) their own attachment directory."
   (org-entry-put nil "ATTACH_DIR_INHERIT" "t")
   (message "Children will inherit attachment directory"))
 
+(defun org-attach-use-annex ()
+  "Return non-nil if git annex can be used."
+  (let ((git-dir (vc-git-root (expand-file-name org-attach-directory))))
+    (and org-attach-git-annex-cutoff
+         (or (file-exists-p (expand-file-name "annex" git-dir))
+             (file-exists-p (expand-file-name ".git/annex" git-dir))))))
+
+(defun org-attach-annex-get-maybe (path)
+  "Call git annex get PATH if using git annex."
+  (if (org-attach-use-annex)
+      (call-process "git" nil nil nil "annex" "get" path)))
+
 (defun org-attach-commit ()
   "Commit changes to git if `org-attach-directory' is properly initialized.
 This checks for the existence of a \".git\" directory in that directory."
   (let* ((dir (expand-file-name org-attach-directory))
 	 (git-dir (vc-git-root dir))
+	 (use-annex (org-attach-use-annex))
 	 (changes 0))
     (when (and git-dir (executable-find "git"))
       (with-temp-buffer
 	(cd dir)
-	(let ((have-annex
-	       (and org-attach-git-annex-cutoff
-		    (or (file-exists-p (expand-file-name "annex" git-dir))
-			(file-exists-p (expand-file-name ".git/annex" git-dir))))))
-	  (dolist (new-or-modified
-		   (split-string
-		    (shell-command-to-string
-		     "git ls-files -zmo --exclude-standard") "\0" t))
-	    (if (and have-annex
-		     (>= (nth 7 (file-attributes new-or-modified))
-			 org-attach-git-annex-cutoff))
-		(call-process "git" nil nil nil "annex" "add" new-or-modified)
-	      (call-process "git" nil nil nil "add" new-or-modified))
-	    (incf changes)))
+        (dolist (new-or-modified
+                 (split-string
+                  (shell-command-to-string
+                   "git ls-files -zmo --exclude-standard") "\0" t))
+          (if (and use-annex
+                   (>= (nth 7 (file-attributes new-or-modified))
+                       org-attach-git-annex-cutoff))
+              (call-process "git" nil nil nil "annex" "add" new-or-modified)
+            (call-process "git" nil nil nil "add" new-or-modified))
+	    (incf changes))
 	(dolist (deleted
 		 (split-string
 		  (shell-command-to-string "git ls-files -z --deleted") "\0" t))
@@ -465,8 +474,10 @@ If IN-EMACS is non-nil, force opening in Emacs."
 	 (file (if (= (length files) 1)
 		   (car files)
 		 (completing-read "Open attachment: "
-				  (mapcar #'list files) nil t))))
-    (org-open-file (expand-file-name file attach-dir) in-emacs)))
+				  (mapcar #'list files) nil t)))
+         (path (expand-file-name file attach-dir)))
+    (org-attach-annex-get-maybe path)
+    (org-open-file path in-emacs)))
 
 (defun org-attach-open-in-emacs ()
   "Open attachment, force opening in Emacs.
diff --git a/mk/targets.mk b/mk/targets.mk
index d390fdb..8eb02fd 100644
--- a/mk/targets.mk
+++ b/mk/targets.mk
@@ -158,4 +158,6 @@ cleandocs:
 	-$(FIND) doc -name \*~ -exec $(RM) {} \;
 
 cleantest:
+	# git annex makes files 444, change to user writableso we can delete them
+	chmod u+w -R $(testdir)
 	$(RMR) $(testdir)
diff --git a/testing/lisp/test-org-attach.el b/testing/lisp/test-org-attach.el
new file mode 100644
index 0000000..7f02e2d
--- /dev/null
+++ b/testing/lisp/test-org-attach.el
@@ -0,0 +1,81 @@
+;;; test-org-attach.el --- Tests for Org Attach
+;;
+;; Copyright (c) 2016 Erik Hetzner
+;; Authors: Erik Hetzner
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+(require 'org-attach)
+
+(cl-defmacro test-org-attach-annex/with-annex (&body body)
+  `(let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (shell-command "git annex init")
+	   ,@body))))
+
+(ert-deftest test-org-attach/use-annex ()
+  (org-test-for-executable "git-annex")
+  ;; (skip-unless (test-org-attach-annex/installed))
+  (test-org-attach-annex/with-annex
+   (let ((org-attach-git-annex-cutoff 1))
+     (should (org-attach-use-annex)))
+
+   (let ((org-attach-git-annex-cutoff nil))
+     (should-not (org-attach-use-annex))))
+
+  ;; test with non annex directory
+  (let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (should-not (org-attach-use-annex)))
+       (delete-directory tmpdir 'recursive))))
+
+(ert-deftest test-org-attach/get-maybe ()
+  (org-test-for-executable "git-annex")
+  ;; (skip-unless (test-org-attach-annex/installed))
+  (test-org-attach-annex/with-annex
+   (let ((path (expand-file-name "test-file"))
+	 (annex-dup (make-temp-file "org-annex-test" t)))
+     (with-temp-buffer
+       (insert "hello world\n")
+       (write-file path))
+     (shell-command "git annex add test-file")
+     (shell-command "git annex sync")
+     ;; Set up remote & copy files there
+     (let ((annex-original default-directory)
+	   (default-directory annex-dup))
+       (shell-command (format "git clone %s ." (shell-quote-argument annex-original)))
+       (shell-command "git annex init dup")
+       (shell-command (format "git remote add original %s" (shell-quote-argument annex-original)))
+       (shell-command "git annex get test-file")
+       (shell-command "git annex sync"))
+     (shell-command (format "git remote add dup %s" (shell-quote-argument annex-dup)))
+     (shell-command "git annex sync")
+     (shell-command "git annex drop --force test-file")
+     ;; test getting the file from the dup
+     (org-attach-annex-get-maybe (expand-file-name "test-file"))
+     ;; check that the file has the right contents
+     (with-temp-buffer
+       (insert-file-contents path)
+       (should (string-equal "hello world\n" (buffer-string)))))))
+
+;;; test-org-attach.el ends here
-- 
2.5.0

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Kyle Meyer]

Erik Hetzner <egh@e6h.org> writes:

> * org-attach.el (org-attach-use-annex): New function to check if git
>   annex should be used.
>   (org-attach-annex-get-maybe): New function to get a file from git
>   annex if necessary.
>   (org-annex-open): Automatically fetch attached files from git annex when
>   opening if necessary.
> * testing/lisp/test-org-annex.el: New file for testing org-attach. Only
>   contains code for testing org-attach with git annex at the moment.
> * mk/targets.mk: Fix cleantest target to for deleting git annex repos.
                                        ^^^^^^
                                        Typo
> +(defun org-attach-use-annex ()
> +  "Return non-nil if git annex can be used."
> +  (let ((git-dir (vc-git-root (expand-file-name org-attach-directory))))
> +    (and org-attach-git-annex-cutoff
> +         (or (file-exists-p (expand-file-name "annex" git-dir))
> +             (file-exists-p (expand-file-name ".git/annex" git-dir))))))
> +
> +(defun org-attach-annex-get-maybe (path)
> +  "Call git annex get PATH if using git annex."
> +  (if (org-attach-use-annex)
> +      (call-process "git" nil nil nil "annex" "get" path)))

s/if/when/

[...]

> -				  (mapcar #'list files) nil t))))
> -    (org-open-file (expand-file-name file attach-dir) in-emacs)))
> +				  (mapcar #'list files) nil t)))
> +         (path (expand-file-name file attach-dir)))
> +    (org-attach-annex-get-maybe path)
> +    (org-open-file path in-emacs)))

I think it's a mistake to always run git annex get and to remove the
message, because this process can hang if all the repos with the file
are unavailable.

This is also one of the reasons why I think there should be an option to
turn off automatic fetching.  Users should be able to stop org-attach
from trying to make connections.

--
Kyle

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Erik Hetzner]

On Mon, 25 Jan 2016 23:40:33 -0800,
Kyle Meyer <kyle@kyleam.com> wrote:
> 
> Erik Hetzner <egh@e6h.org> writes:
> 
> […]
> > * mk/targets.mk: Fix cleantest target to for deleting git annex repos.
>                                         ^^^^^^
>                                         Typo

Thank you!


> […]
> 
> s/if/when/

I’m sorry, you said this before, but I’ve always used =if= except in the case
where I’d otherwise need =progn=. Is the principle here that =when= should be
used when there is no else block? Is there a style guide that I should be
referring to before submitting patches?

> [...]
> 
> > -				  (mapcar #'list files) nil t))))
> > -    (org-open-file (expand-file-name file attach-dir) in-emacs)))
> > +				  (mapcar #'list files) nil t)))
> > +         (path (expand-file-name file attach-dir)))
> > +    (org-attach-annex-get-maybe path)
> > +    (org-open-file path in-emacs)))
> 
> I think it's a mistake to always run git annex get and to remove the
> message, because this process can hang if all the repos with the file
> are unavailable.
> 
> This is also one of the reasons why I think there should be an option to
> turn off automatic fetching.  Users should be able to stop org-attach
> from trying to make connections.

I have to admit I am reluctant to add another option to org-mode. What do you
think about a y-or-n-p if the file needs fetching that will ask if the user
would like to get the file from git annex?

best, Erik
--
Sent from my free software system <http://fsf.org/>.

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Kyle Meyer]

Erik Hetzner <egh@e6h.org> writes:

> Kyle Meyer <kyle@kyleam.com> wrote:

>> s/if/when/
>
> I’m sorry, you said this before, but I’ve always used =if= except in the case
> where I’d otherwise need =progn=. Is the principle here that =when= should be
> used when there is no else block?

Yes, for an 'if' without an 'else', I'd prefer to use 'when', especially
with a 'then' that isn't executed for its return value.  But it's not a
strong preference.

[...]

>> > -				  (mapcar #'list files) nil t))))
>> > -    (org-open-file (expand-file-name file attach-dir) in-emacs)))
>> > +				  (mapcar #'list files) nil t)))
>> > +         (path (expand-file-name file attach-dir)))
>> > +    (org-attach-annex-get-maybe path)
>> > +    (org-open-file path in-emacs)))
>> 
>> I think it's a mistake to always run git annex get and to remove the
>> message, because this process can hang if all the repos with the file
>> are unavailable.
>> 
>> This is also one of the reasons why I think there should be an option to
>> turn off automatic fetching.  Users should be able to stop org-attach
>> from trying to make connections.
>
> I have to admit I am reluctant to add another option to org-mode. What do you
> think about a y-or-n-p if the file needs fetching that will ask if the user
> would like to get the file from git annex?

I think a y-or-n-p prompt would be fine.  It also has the advantage that
it would make it clear that any hanging is from the 'git annex get'
call.

-- 
Kyle

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Rasmus]

>> I have to admit I am reluctant to add another option to org-mode. What do you
>> think about a y-or-n-p if the file needs fetching that will ask if the user
>> would like to get the file from git annex?
>
> I think a y-or-n-p prompt would be fine.  It also has the advantage that
> it would make it clear that any hanging is from the 'git annex get'
> call.

How about a defcustom that default to y-or-n-p?  I think one should be
able to turn it off an on...

Rasmus

-- 
History is what should never happen again

[PATCH] org-attach.el: Get attachments from git annex

[Erik Hetzner]

* org-attach.el (org-attach-use-annex): New function to check if git
  annex should be used.
  (org-attach-annex-get-maybe): New function to get a file from git
  annex if necessary.
  (org-attach-annex-confirm-get-function): New defcustom to determine
  behavior of org-attach-annex-get-maybe.
  (org-attach-open): Automatically get attached files from git annex when
  opening if necessary.
* testing/lisp/test-org-annex.el: New file for testing org-attach. Only
  contains code for testing org-attach with git annex at the moment.
* mk/targets.mk: Fix cleantest target so it can delete git annex repos.
---
I believe this addresses all the issues that have been raised. Thanks for all
your help with this.

 lisp/org-attach.el              | 61 +++++++++++++++++++-------
 mk/targets.mk                   |  2 +
 testing/lisp/test-org-attach.el | 95 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 16 deletions(-)
 create mode 100644 testing/lisp/test-org-attach.el

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index e6ad4b1..ec68ac4 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -131,6 +131,16 @@ When set to `query', ask the user instead."
 	  (const :tag "Always delete attachments" t)
 	  (const :tag "Query the user" query)))
 
+(defcustom org-attach-annex-confirm-get-function #'y-or-n-p
+  "Function to call to confirm if Org should call git annex get if necessary.
+If t, always get, if nil, never get."
+  :group 'org-attach
+  :package-version '(Org . "8.3")
+  :type '(choice
+	  (const :tag "confirm with y-or-n" y-or-n-p)
+	  (const :tag "always get from annex if necessary" t)
+	  (const :tag "never get from annex" nil)))
+
 ;;;###autoload
 (defun org-attach ()
   "The dispatcher for attachment commands.
@@ -270,29 +280,46 @@ the ATTACH_DIR property) their own attachment directory."
   (org-entry-put nil "ATTACH_DIR_INHERIT" "t")
   (message "Children will inherit attachment directory"))
 
+(defun org-attach-use-annex ()
+  "Return non-nil if git annex can be used."
+  (let ((git-dir (vc-git-root (expand-file-name org-attach-directory))))
+    (and org-attach-git-annex-cutoff
+         (or (file-exists-p (expand-file-name "annex" git-dir))
+             (file-exists-p (expand-file-name ".git/annex" git-dir))))))
+
+(defun org-attach-annex-get-maybe (path)
+  "Call git annex get PATH if using git annex."
+  (when (and (org-attach-use-annex)
+	     (not (string-equal "found"
+				(shell-command-to-string
+				 (format "git annex find --format=found --in=here %s" (shell-quote-argument path))))))
+    (if (if (functionp org-attach-annex-confirm-get-function)
+	    (funcall org-attach-annex-confirm-get-function (format "Run git annex get %s? " path))
+	  org-attach-annex-confirm-get-function)
+	(progn (message "Running git annex get \"%s\"." path)
+	       (call-process "git" nil nil nil "annex" "get" path))
+      (error "File %s stored in git annex but it is not available, and was not retrieved" path))))
+
 (defun org-attach-commit ()
   "Commit changes to git if `org-attach-directory' is properly initialized.
 This checks for the existence of a \".git\" directory in that directory."
   (let* ((dir (expand-file-name org-attach-directory))
 	 (git-dir (vc-git-root dir))
+	 (use-annex (org-attach-use-annex))
 	 (changes 0))
     (when (and git-dir (executable-find "git"))
       (with-temp-buffer
 	(cd dir)
-	(let ((have-annex
-	       (and org-attach-git-annex-cutoff
-		    (or (file-exists-p (expand-file-name "annex" git-dir))
-			(file-exists-p (expand-file-name ".git/annex" git-dir))))))
-	  (dolist (new-or-modified
-		   (split-string
-		    (shell-command-to-string
-		     "git ls-files -zmo --exclude-standard") "\0" t))
-	    (if (and have-annex
-		     (>= (nth 7 (file-attributes new-or-modified))
-			 org-attach-git-annex-cutoff))
-		(call-process "git" nil nil nil "annex" "add" new-or-modified)
-	      (call-process "git" nil nil nil "add" new-or-modified))
-	    (incf changes)))
+        (dolist (new-or-modified
+                 (split-string
+                  (shell-command-to-string
+                   "git ls-files -zmo --exclude-standard") "\0" t))
+          (if (and use-annex
+                   (>= (nth 7 (file-attributes new-or-modified))
+                       org-attach-git-annex-cutoff))
+              (call-process "git" nil nil nil "annex" "add" new-or-modified)
+            (call-process "git" nil nil nil "add" new-or-modified))
+	    (incf changes))
 	(dolist (deleted
 		 (split-string
 		  (shell-command-to-string "git ls-files -z --deleted") "\0" t))
@@ -465,8 +492,10 @@ If IN-EMACS is non-nil, force opening in Emacs."
 	 (file (if (= (length files) 1)
 		   (car files)
 		 (completing-read "Open attachment: "
-				  (mapcar #'list files) nil t))))
-    (org-open-file (expand-file-name file attach-dir) in-emacs)))
+				  (mapcar #'list files) nil t)))
+         (path (expand-file-name file attach-dir)))
+    (org-attach-annex-get-maybe path)
+    (org-open-file path in-emacs)))
 
 (defun org-attach-open-in-emacs ()
   "Open attachment, force opening in Emacs.
diff --git a/mk/targets.mk b/mk/targets.mk
index d390fdb..cab65cb 100644
--- a/mk/targets.mk
+++ b/mk/targets.mk
@@ -158,4 +158,6 @@ cleandocs:
 	-$(FIND) doc -name \*~ -exec $(RM) {} \;
 
 cleantest:
+# git annex makes files 444, change to user writable so we can delete them
+	if [ -d $(testdir) ] ; then chmod u+w -R $(testdir) ; fi
 	$(RMR) $(testdir)
diff --git a/testing/lisp/test-org-attach.el b/testing/lisp/test-org-attach.el
new file mode 100644
index 0000000..b0cfb58
--- /dev/null
+++ b/testing/lisp/test-org-attach.el
@@ -0,0 +1,95 @@
+;;; test-org-attach.el --- Tests for Org Attach
+;;
+;; Copyright (c) 2016 Erik Hetzner
+;; Authors: Erik Hetzner
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+(require 'org-attach)
+
+(cl-defmacro test-org-attach-annex/with-annex (&body body)
+  `(let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (shell-command "git annex init")
+	   ,@body))))
+
+(ert-deftest test-org-attach/use-annex ()
+  (org-test-for-executable "git-annex")
+  (test-org-attach-annex/with-annex
+   (let ((org-attach-git-annex-cutoff 1))
+     (should (org-attach-use-annex)))
+
+   (let ((org-attach-git-annex-cutoff nil))
+     (should-not (org-attach-use-annex))))
+
+  ;; test with non annex directory
+  (let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (should-not (org-attach-use-annex)))
+       (delete-directory tmpdir 'recursive))))
+
+(ert-deftest test-org-attach/get-maybe ()
+  (org-test-for-executable "git-annex")
+  (test-org-attach-annex/with-annex
+   (let ((path (expand-file-name "test-file"))
+	 (annex-dup (make-temp-file "org-annex-test" t)))
+     (with-temp-buffer
+       (insert "hello world\n")
+       (write-file path))
+     (shell-command "git annex add test-file")
+     (shell-command "git annex sync")
+     ;; Set up remote & copy files there
+     (let ((annex-original default-directory)
+	   (default-directory annex-dup))
+       (shell-command (format "git clone %s ." (shell-quote-argument annex-original)))
+       (shell-command "git annex init dup")
+       (shell-command (format "git remote add original %s" (shell-quote-argument annex-original)))
+       (shell-command "git annex get test-file")
+       (shell-command "git annex sync"))
+     (shell-command (format "git remote add dup %s" (shell-quote-argument annex-dup)))
+     (shell-command "git annex sync")
+     (shell-command "git annex drop --force test-file")
+     ;; test getting the file from the dup when we should ALWAYS get
+     (should (not (file-exists-p (file-symlink-p (expand-file-name "test-file")))))
+     (let ((org-attach-annex-confirm-get-function t))
+       (org-attach-annex-get-maybe (expand-file-name "test-file"))
+       ;; check that the file has the right contents
+       (with-temp-buffer
+	 (insert-file-contents path)
+	 (should (string-equal "hello world\n" (buffer-string)))))
+     ;; test getting the file from the dup when we should NEVER get
+     (shell-command "git annex drop --force test-file")
+     (let ((org-attach-annex-confirm-get-function nil))
+       (should-error (org-attach-annex-get-maybe (expand-file-name "test-file"))))
+     (let* ((called nil)
+	    (org-attach-annex-confirm-get-function (lambda (prompt)
+						     (setq called 'was-called)
+						     t)))
+       (org-attach-annex-get-maybe (expand-file-name "test-file"))
+       ;; check that the file has the right contents
+       (with-temp-buffer
+	 (insert-file-contents path)
+	 (should (string-equal "hello world\n" (buffer-string))))
+       (should (eq called 'was-called))))))
+
+;;; test-org-attach.el ends here
-- 
2.5.0

Re: [PATCH] org-attach.el: Get attachments from git annex

[Rasmus]

Hi Erik,

Thanks for the updated patch!

A couple of more comments follow.
I hope I’m not being too annoying! 

Erik Hetzner <egh@e6h.org> writes:

> +(defcustom org-attach-annex-confirm-get-function #'y-or-n-p
> +  "Function to call to confirm if Org should call git annex get if necessary.
> +If t, always get, if nil, never get."


Please note that the function should accept one argument cf. your code
below.  Also, I wonder if there’s really a point in having the increased
flexibility of a function over just: t, nil and ’ask.

> +  :group 'org-attach
> +  :package-version '(Org . "8.3")
> +  :type '(choice
> +	  (const :tag "confirm with y-or-n" y-or-n-p)
> +	  (const :tag "always get from annex if necessary" t)
> +	  (const :tag "never get from annex" nil)))

Nitpick: package version should be org 9.  You should add :version tag as
well.  Probably "25.1" is good.  Then we can mass-update them all when we
are allowed to merge...

> +(defun org-attach-annex-get-maybe (path)
> +  "Call git annex get PATH if using git annex."
> +  (when (and (org-attach-use-annex)
> +	     (not (string-equal "found"
> +				(shell-command-to-string
> +				 (format "git annex find --format=found --in=here %s" (shell-quote-argument path))))))
> +    (if (if (functionp org-attach-annex-confirm-get-function)
> +	    (funcall org-attach-annex-confirm-get-function (format "Run git annex get %s? " path))
> +	  org-attach-annex-confirm-get-function)
> +	(progn (message "Running git annex get \"%s\"." path)
> +	       (call-process "git" nil nil nil "annex" "get" path))
> +      (error "File %s stored in git annex but it is not available, and was not retrieved" path))))

Can’t you factor out the inner "if", e.g. to an outer let?  Shouldn’t you
check the return of annex get and show a warning or an error if it fails?
It seems the error is only called if the inner if fails (in which case the
error message is not precise since we didn’t try to retrieve the file).

>  (defun org-attach-commit ()

Looks fine. 

>  cleantest:
> +# git annex makes files 444, change to user writable so we can delete them
> +	if [ -d $(testdir) ] ; then chmod u+w -R $(testdir) ; fi
>  	$(RMR) $(testdir)

I wonder if it would be better to directly target the files you use?  I
don’t think there’s a case where changing the mod of the testdir is a
problem though....


> +;;; test-org-attach.el --- Tests for Org Attach

Skipped again...

-- 
Slowly unravels in a ball of yarn and the devil collects it

Re: [PATCH] org-attach.el: Get attachments from git annex

[Erik Hetzner]

Hi Rasmus,

Thanks again for the feedback!

On Wed, 27 Jan 2016 14:20:59 -0800,
Rasmus <rasmus@gmx.us> wrote:
> 
> Hi Erik,
> 
> Thanks for the updated patch!
> 
> A couple of more comments follow.
> I hope I’m not being too annoying! 

Not at all, I appreciate it.

> Erik Hetzner <egh@e6h.org> writes:
> 
> > +(defcustom org-attach-annex-confirm-get-function #'y-or-n-p
> > +  "Function to call to confirm if Org should call git annex get if necessary.
> > +If t, always get, if nil, never get."
> 
> 
> Please note that the function should accept one argument cf. your code
> below.  Also, I wonder if there’s really a point in having the increased
> flexibility of a function over just: t, nil and ’ask.

This is a good point - I was following the pattern of
`org-confirm-shell-link-function' but I don’t think that in this case
it is necessary.

> > +  :group 'org-attach
> > +  :package-version '(Org . "8.3")
> > +  :type '(choice
> > +	  (const :tag "confirm with y-or-n" y-or-n-p)
> > +	  (const :tag "always get from annex if necessary" t)
> > +	  (const :tag "never get from annex" nil)))
> 
> Nitpick: package version should be org 9.  You should add :version tag as
> well.  Probably "25.1" is good.  Then we can mass-update them all when we
> are allowed to merge...

Thank you, I wasn’t sure what to use here.

> > +(defun org-attach-annex-get-maybe (path)
> > +  "Call git annex get PATH if using git annex."
> > +  (when (and (org-attach-use-annex)
> > +	     (not (string-equal "found"
> > +				(shell-command-to-string
> > +				 (format "git annex find --format=found --in=here %s" (shell-quote-argument path))))))
> > +    (if (if (functionp org-attach-annex-confirm-get-function)
> > +	    (funcall org-attach-annex-confirm-get-function (format "Run git annex get %s? " path))
> > +	  org-attach-annex-confirm-get-function)
> > +	(progn (message "Running git annex get \"%s\"." path)
> > +	       (call-process "git" nil nil nil "annex" "get" path))
> > +      (error "File %s stored in git annex but it is not available, and was not retrieved" path))))
> 
> Can’t you factor out the inner "if", e.g. to an outer let?  Shouldn’t you
> check the return of annex get and show a warning or an error if it fails?
> It seems the error is only called if the inner if fails (in which case the
> error message is not precise since we didn’t try to retrieve the file).

This has been since refactored, but the point about the error remains. The
reason I used `error' is that the user has been attempting to open the file. If
the content is unavailable, then surely the attempt to open the file will be
unsuccessful. Perhaps a more clear docstring in `org-attach-annex-get-maybe' is
in order, though.

> >  (defun org-attach-commit ()
> 
> Looks fine. 
> 
> >  cleantest:
> > +# git annex makes files 444, change to user writable so we can delete them
> > +	if [ -d $(testdir) ] ; then chmod u+w -R $(testdir) ; fi
> >  	$(RMR) $(testdir)
> 
> I wonder if it would be better to directly target the files you use?  I
> don’t think there’s a case where changing the mod of the testdir is a
> problem though....

Since it is about to be removed via =rm -rf= it doesn’t seem worth worrying
about it :)

best, Erik

> 
> > +;;; test-org-attach.el --- Tests for Org Attach
> 
> Skipped again...
> 
> -- 
> Slowly unravels in a ball of yarn and the devil collects it

Re: [PATCH] org-attach.el: Get attachments from git annex

[Kyle Meyer]

Erik Hetzner <egh@e6h.org> writes:

> I believe this addresses all the issues that have been raised. Thanks for all
> your help with this.

Thanks for the updates.  I just have two minor things to add to Rasmus's
comments.

- Nitpick: It'd be nice if you could re-flow some of your lines to not
  be as wide.

- What's the advantage of using cl-defmacro instead of defmacro below?

> +(cl-defmacro test-org-attach-annex/with-annex (&body body)
> +  `(let ((tmpdir (make-temp-file "org-annex-test" t)))
> +     (unwind-protect
> +	 (let ((default-directory tmpdir)

Thanks again.

-- 
Kyle

[PATCH] org-attach.el: Get attachments from git annex

[Erik Hetzner]

From: Erik Hetzner <egh@e6h.org>

* org-attach.el (org-attach-use-annex): New function to check if git
  annex should be used.
  (org-attach-annex-get-maybe): New function to get a file from git
  annex if necessary.
  (org-attach-annex-auto-get): New defcustom to determine behavior
  of org-attach-annex-get-maybe.
  (org-attach-open): Automatically get attached files from git annex when
  opening if necessary.
* testing/lisp/test-org-annex.el: New file for testing org-attach. Only
  contains code for testing org-attach with git annex at the moment.
* mk/targets.mk: Fix cleantest target so it can delete git annex repos.
---
 lisp/org-attach.el              | 69 ++++++++++++++++++++++-------
 mk/targets.mk                   |  2 +
 testing/lisp/test-org-attach.el | 97 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+), 16 deletions(-)
 create mode 100644 testing/lisp/test-org-attach.el

diff --git a/lisp/org-attach.el b/lisp/org-attach.el
index e6ad4b1..15d4841 100644
--- a/lisp/org-attach.el
+++ b/lisp/org-attach.el
@@ -131,6 +131,17 @@ When set to `query', ask the user instead."
 	  (const :tag "Always delete attachments" t)
 	  (const :tag "Query the user" query)))
 
+(defcustom org-attach-annex-auto-get 'ask
+  "Confirmation preference for automatically getting annex files.
+If \\='ask, prompt using `y-or-n-p'.  If t, always get.  If nil, never get."
+  :group 'org-attach
+  :package-version '(Org . "9")
+  :version "25.1"
+  :type '(choice
+	  (const :tag "confirm with `y-or-n-p'" ask)
+	  (const :tag "always get from annex if necessary" t)
+	  (const :tag "never get from annex" nil)))
+
 ;;;###autoload
 (defun org-attach ()
   "The dispatcher for attachment commands.
@@ -270,29 +281,53 @@ the ATTACH_DIR property) their own attachment directory."
   (org-entry-put nil "ATTACH_DIR_INHERIT" "t")
   (message "Children will inherit attachment directory"))
 
+(defun org-attach-use-annex ()
+  "Return non-nil if git annex can be used."
+  (let ((git-dir (vc-git-root (expand-file-name org-attach-directory))))
+    (and org-attach-git-annex-cutoff
+         (or (file-exists-p (expand-file-name "annex" git-dir))
+             (file-exists-p (expand-file-name ".git/annex" git-dir))))))
+
+(defun org-attach-annex-get-maybe (path)
+  "Call git annex get PATH (via shell) if using git annex.
+Signals an error if the file content is not available and it was not retrieved."
+  (when (and (org-attach-use-annex)
+	     (not
+	      (string-equal
+	       "found"
+	       (shell-command-to-string
+		(format "git annex find --format=found --in=here %s"
+			(shell-quote-argument path))))))
+    (let ((should-get
+	   (if (eq org-attach-annex-auto-get 'ask)
+	       (y-or-n-p (format "Run git annex get %s? " path))
+	     org-attach-annex-auto-get)))
+      (if should-get
+	  (progn (message "Running git annex get \"%s\"." path)
+		 (call-process "git" nil nil nil "annex" "get" path))
+	(error "File %s stored in git annex but it is not available, and was not retrieved"
+	       path)))))
+
 (defun org-attach-commit ()
   "Commit changes to git if `org-attach-directory' is properly initialized.
 This checks for the existence of a \".git\" directory in that directory."
   (let* ((dir (expand-file-name org-attach-directory))
 	 (git-dir (vc-git-root dir))
+	 (use-annex (org-attach-use-annex))
 	 (changes 0))
     (when (and git-dir (executable-find "git"))
       (with-temp-buffer
 	(cd dir)
-	(let ((have-annex
-	       (and org-attach-git-annex-cutoff
-		    (or (file-exists-p (expand-file-name "annex" git-dir))
-			(file-exists-p (expand-file-name ".git/annex" git-dir))))))
-	  (dolist (new-or-modified
-		   (split-string
-		    (shell-command-to-string
-		     "git ls-files -zmo --exclude-standard") "\0" t))
-	    (if (and have-annex
-		     (>= (nth 7 (file-attributes new-or-modified))
-			 org-attach-git-annex-cutoff))
-		(call-process "git" nil nil nil "annex" "add" new-or-modified)
-	      (call-process "git" nil nil nil "add" new-or-modified))
-	    (incf changes)))
+        (dolist (new-or-modified
+                 (split-string
+                  (shell-command-to-string
+                   "git ls-files -zmo --exclude-standard") "\0" t))
+          (if (and use-annex
+                   (>= (nth 7 (file-attributes new-or-modified))
+                       org-attach-git-annex-cutoff))
+              (call-process "git" nil nil nil "annex" "add" new-or-modified)
+            (call-process "git" nil nil nil "add" new-or-modified))
+	    (incf changes))
 	(dolist (deleted
 		 (split-string
 		  (shell-command-to-string "git ls-files -z --deleted") "\0" t))
@@ -465,8 +500,10 @@ If IN-EMACS is non-nil, force opening in Emacs."
 	 (file (if (= (length files) 1)
 		   (car files)
 		 (completing-read "Open attachment: "
-				  (mapcar #'list files) nil t))))
-    (org-open-file (expand-file-name file attach-dir) in-emacs)))
+				  (mapcar #'list files) nil t)))
+         (path (expand-file-name file attach-dir)))
+    (org-attach-annex-get-maybe path)
+    (org-open-file path in-emacs)))
 
 (defun org-attach-open-in-emacs ()
   "Open attachment, force opening in Emacs.
diff --git a/mk/targets.mk b/mk/targets.mk
index d390fdb..cab65cb 100644
--- a/mk/targets.mk
+++ b/mk/targets.mk
@@ -158,4 +158,6 @@ cleandocs:
 	-$(FIND) doc -name \*~ -exec $(RM) {} \;
 
 cleantest:
+# git annex makes files 444, change to user writable so we can delete them
+	if [ -d $(testdir) ] ; then chmod u+w -R $(testdir) ; fi
 	$(RMR) $(testdir)
diff --git a/testing/lisp/test-org-attach.el b/testing/lisp/test-org-attach.el
new file mode 100644
index 0000000..9772bd7
--- /dev/null
+++ b/testing/lisp/test-org-attach.el
@@ -0,0 +1,97 @@
+;;; test-org-attach.el --- Tests for Org Attach
+;;
+;; Copyright (c) 2016 Erik Hetzner
+;; Authors: Erik Hetzner
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+(require 'org-attach)
+(require 'cl-lib)
+
+(defmacro test-org-attach-annex/with-annex (&rest body)
+  `(let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (shell-command "git annex init")
+	   ,@body))))
+
+(ert-deftest test-org-attach/use-annex ()
+  (org-test-for-executable "git-annex")
+  (test-org-attach-annex/with-annex
+   (let ((org-attach-git-annex-cutoff 1))
+     (should (org-attach-use-annex)))
+
+   (let ((org-attach-git-annex-cutoff nil))
+     (should-not (org-attach-use-annex))))
+
+  ;; test with non annex directory
+  (let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (should-not (org-attach-use-annex)))
+       (delete-directory tmpdir 'recursive))))
+
+(ert-deftest test-org-attach/get-maybe ()
+  (org-test-for-executable "git-annex")
+  (test-org-attach-annex/with-annex
+   (let ((path (expand-file-name "test-file"))
+	 (annex-dup (make-temp-file "org-annex-test" t)))
+     (with-temp-buffer
+       (insert "hello world\n")
+       (write-file path))
+     (shell-command "git annex add test-file")
+     (shell-command "git annex sync")
+     ;; Set up remote & copy files there
+     (let ((annex-original default-directory)
+	   (default-directory annex-dup))
+       (shell-command (format "git clone %s ." (shell-quote-argument annex-original)))
+       (shell-command "git annex init dup")
+       (shell-command (format "git remote add original %s" (shell-quote-argument annex-original)))
+       (shell-command "git annex get test-file")
+       (shell-command "git annex sync"))
+     (shell-command (format "git remote add dup %s" (shell-quote-argument annex-dup)))
+     (shell-command "git annex sync")
+     (shell-command "git annex drop --force test-file")
+     ;; test getting the file from the dup when we should ALWAYS get
+     (should (not (file-exists-p (file-symlink-p (expand-file-name "test-file")))))
+     (let ((org-attach-annex-auto-get t))
+       (org-attach-annex-get-maybe (expand-file-name "test-file"))
+       ;; check that the file has the right contents
+       (with-temp-buffer
+	 (insert-file-contents path)
+	 (should (string-equal "hello world\n" (buffer-string)))))
+     ;; test getting the file from the dup when we should NEVER get
+     (shell-command "git annex drop --force test-file")
+     (let ((org-attach-annex-auto-get nil))
+       (should-error (org-attach-annex-get-maybe (expand-file-name "test-file"))))
+     (let ((org-attach-annex-auto-get 'ask)
+	   (called nil))
+       (flet ((y-or-n-p (prompt)
+			   (setq called 'was-called)
+			   t))
+	 (org-attach-annex-get-maybe (expand-file-name "test-file"))
+	 ;; check that the file has the right contents
+	 (with-temp-buffer
+	   (insert-file-contents path)
+	   (should (string-equal "hello world\n" (buffer-string))))
+	 (should (eq called 'was-called)))))))
+
+;;; test-org-attach.el ends here
-- 
2.5.0

Re: [PATCH] org-attach.el: Get attachments from git annex

[Kyle Meyer]

Erik Hetzner <egh@e6h.org> writes:

> From: Erik Hetzner <egh@e6h.org>
>
> * org-attach.el (org-attach-use-annex): New function to check if git
>   annex should be used.
>   (org-attach-annex-get-maybe): New function to get a file from git
>   annex if necessary.
>   (org-attach-annex-auto-get): New defcustom to determine behavior
>   of org-attach-annex-get-maybe.
>   (org-attach-open): Automatically get attached files from git annex when
>   opening if necessary.
> * testing/lisp/test-org-annex.el: New file for testing org-attach. Only
>   contains code for testing org-attach with git annex at the moment.
> * mk/targets.mk: Fix cleantest target so it can delete git annex repos.
> ---
>  lisp/org-attach.el              | 69 ++++++++++++++++++++++-------
>  mk/targets.mk                   |  2 +
>  testing/lisp/test-org-attach.el | 97 +++++++++++++++++++++++++++++++++++++++++

This looks good to me.  I'll leave applying the patch to Rasmus in case
he has any comments on this version.

Thank you.

-- 
Kyle

Re: [PATCH] org-attach.el: Get attachments from git annex

[Rasmus]

Hi Erik and Kyle,

Sorry for the delay, I was traveling this week.  I’m happy with 
the patch now and it has been pushed to master.

Thanks!
Rasmus

-- 
Don't slow down Johnny, leave the Cadillac runnin'

Re: [PATCH] org-attach.el: Get attachments from git annex

[Erik Hetzner]

Hi Rasmus and Kyle,

On Sat, 06 Feb 2016 04:18:55 -0800,
Rasmus <rasmus@gmx.us> wrote:
>
> Hi Erik and Kyle,
>
> Sorry for the delay, I was traveling this week.  I’m happy with the patch now
> and it has been pushed to master.

Thank you for all your feedback on this code! I’m really happy with how it
turned out, and glad it is in master now.

best, Erik

--
Sent from my free software system <http://fsf.org/>.

Re: [PATCH] org-attach.el: Get attachments from git annex

[Achim Gratz]

Erik Hetzner writes:
> Thank you for all your feedback on this code! I’m really happy with how it
> turned out, and glad it is in master now.

Please fix the tests to not fail (preferrably not run) if git-annex is
not installed.

Also, revert the patch to mk/targets.mk; $(RMR) already includes the
'--force' flag, so it can remove write protected files just fine.  If
it's write protecting directories then that would be stupid, but still
no reason to chmod all files.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for Waldorf Q V3.00R3 and Q+ V3.54R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: [PATCH] org-attach.el: Get attachments from git annex

[Erik Hetzner]

On Sun, 07 Feb 2016 12:48:20 -0800,
Achim Gratz <Stromeko@nexgo.de> wrote:
> 
> Erik Hetzner writes:
> > Thank you for all your feedback on this code! I’m really happy with how it
> > turned out, and glad it is in master now.
> 
> Please fix the tests to not fail (preferrably not run) if git-annex is
> not installed.
> 
> Also, revert the patch to mk/targets.mk; $(RMR) already includes the
> '--force' flag, so it can remove write protected files just fine.  If
> it's write protecting directories then that would be stupid, but still
> no reason to chmod all files.

Hi Achim,

Sorry - I misunderstood how `org-test-for-executable' works. I’ll fix the issue.

As for the rm -rf, it does not work in the way you suggest, at least on ubuntu
GNU/Linux. Here is the result I get locally without the chmod:

make cleantest
make[1]: Entering directory '/home/egh/c/org-mode'
rm -fr /tmp/tmp-orgtest
rm: cannot remove ‘/tmp/tmp-orgtest/org-annex-test18033fml/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447’: Permission denied
rm: cannot remove ‘/tmp/tmp-orgtest/org-annex-test18033Scf/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447’: Permission denied
rm: cannot remove ‘/tmp/tmp-orgtest/org-annex-test172167kl/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447’: Permission denied
rm: cannot remove ‘/tmp/tmp-orgtest/org-annex-test17216uaf/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447’: Permission denied
mk/targets.mk:163: recipe for target 'cleantest' failed
make[1]: *** [cleantest] Error 1
make[1]: Leaving directory '/home/egh/c/org-mode'
mk/targets.mk:103: recipe for target 'test' failed
make: *** [test] Error 2

If you have a better suggestion to this issue I’d love to know! I’m not sure
what the best fix is here.

best, Erik

Re: [PATCH] org-attach.el: Get attachments from git annex

[Achim Gratz]

Erik Hetzner writes:
> Sorry - I misunderstood how `org-test-for-executable' works. I’ll fix the issue.

Thanks.

> As for the rm -rf, it does not work in the way you suggest, at least on ubuntu
> GNU/Linux. Here is the result I get locally without the chmod:

For pete's sake, what are the permissions?  As I said, the only reason I
see that is happening is if the directory containing the files is mode
55x.  And that would either be spectactularly stupid or exceptionally
clever, although I tend toward the first interpretation.  In any case,
you'd need to make the directories writable to fix this, but not the
files.  There might be a config option that you could/should set to
avoid this bit of cleverness or nuisance by git-annex..


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: [PATCH] org-attach.el: Get attachments from git annex

[Erik Hetzner]

Hi Achim,

On Tue, 09 Feb 2016 11:40:54 -0800,
Achim Gratz <Stromeko@nexgo.de> wrote:
>
> Erik Hetzner writes:
> > Sorry - I misunderstood how `org-test-for-executable' works. I’ll fix the issue.
>
> Thanks.
>
> > As for the rm -rf, it does not work in the way you suggest, at least on ubuntu
> > GNU/Linux. Here is the result I get locally without the chmod:
>
> For pete's sake, what are the permissions?  As I said, the only reason I
> see that is happening is if the directory containing the files is mode
> 55x.  And that would either be spectactularly stupid or exceptionally
> clever, although I tend toward the first interpretation.  In any case,
> you'd need to make the directories writable to fix this, but not the
> files.  There might be a config option that you could/should set to
> avoid this bit of cleverness or nuisance by git-annex..

Here are the permissions:

0755 /tmp/tmp-orgtest/
0700 /tmp/tmp-orgtest/org-annex-test19785b6F/
0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/
0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/
0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/
0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/jm/
0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/jm/P2/
0555 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/
0444 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447

I presume this is to maintain the integrity of the file contents in the
repository.

It would be possible to drop these files after the test (using git annex drop
--force), but if something goes wrong there the rm -rf would not clean up the
test directory properly. If you think that is a better solution I can do that.

best, Erik

Re: [PATCH] org-attach.el: Get attachments from git annex

[Achim Gratz]

Erik Hetzner writes:
> Here are the permissions:
>
> 0755 /tmp/tmp-orgtest/
> 0700 /tmp/tmp-orgtest/org-annex-test19785b6F/
> 0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/
> 0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/
> 0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/
> 0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/jm/
> 0775 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/jm/P2/
> 0555 /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/
> 0444
> /tmp/tmp-orgtest/org-annex-test19785b6F/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447
>
> I presume this is to maintain the integrity of the file contents in the
> repository.

Oh my, they even think it's a feature.  :-(

https://git-annex.branchable.com/internals/lockdown/

Those shenanigans with symbolic links are a bit cumbersome.

> It would be possible to drop these files after the test (using git annex drop
> --force), but if something goes wrong there the rm -rf would not clean up the
> test directory properly. If you think that is a better solution I can do that.

Well, the best solution would be to have an option for git-annex to
switch off that lockdown thing.  Since that's not going to happen soon
or anytime, try something like:

        $(RMR) $(testdir) || { $(CHMOD) -R u+w $(testdir); $(RMR) $(testdir) }

which of course requires the addition of

CHMOD = chmod

in mk/defaults.mk.

But it seems that direct mode (however deprecated it is) would allow us
to do exactly the thing we want, so maybe if the tests would put that
repo in direct mode we wouldn't even need to do that.  But that doesn't
seem to work in a v6 repo unless annex.thin is set to true.  What version
are you testing?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: [PATCH] org-attach.el: Get attachments from git annex

[Erik Hetzner]

Hi Achim

On Tue, 09 Feb 2016 14:19:25 -0800,
Achim Gratz <Stromeko@nexgo.de> wrote:
>
>
> […]
>
> Well, the best solution would be to have an option for git-annex to
> switch off that lockdown thing.  Since that's not going to happen soon
> or anytime, try something like:
>
>         $(RMR) $(testdir) || { $(CHMOD) -R u+w $(testdir); $(RMR) $(testdir) }
>
> which of course requires the addition of
>
> CHMOD = chmod
>
> in mk/defaults.mk.

Can you explain how this improves on what I had? The time spent appears to be
the same on my system (tested after clearing the filesystem cache):

$ time /bin/sh -c "if [ -d /tmp/tmp-orgtest/ ] ; then chmod u+w -R /tmp/tmp-orgtest/ ; fi ; rm -rf /tmp/orbit-egh/"

real	0m0.219s
user	0m0.004s
sys	0m0.000s

$ time /bin/sh -c "rm -rf /tmp/tmp-orgtest || { chmod -R u+w /tmp/tmp-orgtest; rm -rf /tmp/tmp-orgtest ; }"
rm: cannot remove ‘/tmp/tmp-orgtest/org-annex-test11629xxf/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447’: Permission denied
rm: cannot remove ‘/tmp/tmp-orgtest/org-annex-test11629knZ/.git/annex/objects/jm/P2/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447/SHA256E-s12--a948904f2f0f479b8f8197694b30184b0d2ed1c1cd2a1ec0fb85d299a192a447’: Permission denied

real	0m0.240s
user	0m0.000s
sys	0m0.012s

> But it seems that direct mode (however deprecated it is) would allow us
> to do exactly the thing we want, so maybe if the tests would put that
> repo in direct mode we wouldn't even need to do that.  But that doesn't
> seem to work in a v6 repo unless annex.thin is set to true.  What version
> are you testing?

git-annex version: 5.20150731-1build1. In any case, direct mode is a rather
different use case, as I understand it.

best, Erik

Re: [PATCH] org-attach.el: Get attachments from git annex

[Achim Gratz]

Erik Hetzner writes:
> Can you explain how this improves on what I had? The time spent appears to be
> the same on my system (tested after clearing the filesystem cache):

For users that are not using git-annex, it's doing exactly the same
thing as before.  The rest kicks in as long as git-annex keeps its
"lockdown" feature.  Lastly, systems that might not have a chown can
substitute a replacement.  I don't care about the time at all.

> git-annex version: 5.20150731-1build1. In any case, direct mode is a rather
> different use case, as I understand it.

The question was if it would allow the tests to succeed without this
extra nuisance of having to chown the git repo afterwards.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Waldorf MIDI Implementation & additional documentation:
http://Synth.Stromeko.net/Downloads.html#WaldorfDocs

[PATCH] testing/lisp/test-org-attach-annex.el: New file

[Erik Hetzner]

* testing/lisp/test-org-attach-annex.el: Move all org-attach tests that
  use git-annex to this file, which can test for the presence of
  git-annex. Prevents tests failing on systems where git-annex is not
  installed.
---
 testing/lisp/test-org-attach-annex.el | 96 ++++++++++++++++++++++++++++++++++
 testing/lisp/test-org-attach.el       | 97 -----------------------------------
 2 files changed, 96 insertions(+), 97 deletions(-)
 create mode 100644 testing/lisp/test-org-attach-annex.el
 delete mode 100644 testing/lisp/test-org-attach.el

diff --git a/testing/lisp/test-org-attach-annex.el b/testing/lisp/test-org-attach-annex.el
new file mode 100644
index 0000000..44b4ad0
--- /dev/null
+++ b/testing/lisp/test-org-attach-annex.el
@@ -0,0 +1,96 @@
+;;; test-org-annex-attach.el --- Tests for Org Attach with git-annex
+;;
+;; Copyright (c) 2016 Erik Hetzner
+;; Authors: Erik Hetzner
+
+;; This file is not part of GNU Emacs.
+
+;; This program is free software; you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; This program is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Code:
+(org-test-for-executable "git-annex")
+(require 'org-attach)
+(require 'cl-lib)
+
+(defmacro test-org-attach-annex/with-annex (&rest body)
+  `(let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (shell-command "git annex init")
+	   ,@body))))
+
+(ert-deftest test-org-attach/use-annex ()
+  (test-org-attach-annex/with-annex
+   (let ((org-attach-git-annex-cutoff 1))
+     (should (org-attach-use-annex)))
+
+   (let ((org-attach-git-annex-cutoff nil))
+     (should-not (org-attach-use-annex))))
+
+  ;; test with non annex directory
+  (let ((tmpdir (make-temp-file "org-annex-test" t)))
+     (unwind-protect
+	 (let ((default-directory tmpdir)
+	       (org-attach-directory tmpdir))
+	   (shell-command "git init")
+	   (should-not (org-attach-use-annex)))
+       (delete-directory tmpdir 'recursive))))
+
+(ert-deftest test-org-attach/get-maybe ()
+  (test-org-attach-annex/with-annex
+   (let ((path (expand-file-name "test-file"))
+	 (annex-dup (make-temp-file "org-annex-test" t)))
+     (with-temp-buffer
+       (insert "hello world\n")
+       (write-file path))
+     (shell-command "git annex add test-file")
+     (shell-command "git annex sync")
+     ;; Set up remote & copy files there
+     (let ((annex-original default-directory)
+	   (default-directory annex-dup))
+       (shell-command (format "git clone %s ." (shell-quote-argument annex-original)))
+       (shell-command "git annex init dup")
+       (shell-command (format "git remote add original %s" (shell-quote-argument annex-original)))
+       (shell-command "git annex get test-file")
+       (shell-command "git annex sync"))
+     (shell-command (format "git remote add dup %s" (shell-quote-argument annex-dup)))
+     (shell-command "git annex sync")
+     (shell-command "git annex drop --force test-file")
+     ;; test getting the file from the dup when we should ALWAYS get
+     (should (not (file-exists-p (file-symlink-p (expand-file-name "test-file")))))
+     (let ((org-attach-annex-auto-get t))
+       (org-attach-annex-get-maybe (expand-file-name "test-file"))
+       ;; check that the file has the right contents
+       (with-temp-buffer
+	 (insert-file-contents path)
+	 (should (string-equal "hello world\n" (buffer-string)))))
+     ;; test getting the file from the dup when we should NEVER get
+     (shell-command "git annex drop --force test-file")
+     (let ((org-attach-annex-auto-get nil))
+       (should-error (org-attach-annex-get-maybe (expand-file-name "test-file"))))
+     (let ((org-attach-annex-auto-get 'ask)
+	   (called nil))
+       (flet ((y-or-n-p (prompt)
+			   (setq called 'was-called)
+			   t))
+	 (org-attach-annex-get-maybe (expand-file-name "test-file"))
+	 ;; check that the file has the right contents
+	 (with-temp-buffer
+	   (insert-file-contents path)
+	   (should (string-equal "hello world\n" (buffer-string))))
+	 (should (eq called 'was-called)))))))
+
+;;; test-org-attach-annex.el ends here
diff --git a/testing/lisp/test-org-attach.el b/testing/lisp/test-org-attach.el
deleted file mode 100644
index 9772bd7..0000000
--- a/testing/lisp/test-org-attach.el
+++ /dev/null
@@ -1,97 +0,0 @@
-;;; test-org-attach.el --- Tests for Org Attach
-;;
-;; Copyright (c) 2016 Erik Hetzner
-;; Authors: Erik Hetzner
-
-;; This file is not part of GNU Emacs.
-
-;; This program is free software; you can redistribute it and/or modify
-;; it under the terms of the GNU General Public License as published by
-;; the Free Software Foundation, either version 3 of the License, or
-;; (at your option) any later version.
-
-;; This program is distributed in the hope that it will be useful,
-;; but WITHOUT ANY WARRANTY; without even the implied warranty of
-;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-;; GNU General Public License for more details.
-
-;; You should have received a copy of the GNU General Public License
-;; along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-;;; Code:
-(require 'org-attach)
-(require 'cl-lib)
-
-(defmacro test-org-attach-annex/with-annex (&rest body)
-  `(let ((tmpdir (make-temp-file "org-annex-test" t)))
-     (unwind-protect
-	 (let ((default-directory tmpdir)
-	       (org-attach-directory tmpdir))
-	   (shell-command "git init")
-	   (shell-command "git annex init")
-	   ,@body))))
-
-(ert-deftest test-org-attach/use-annex ()
-  (org-test-for-executable "git-annex")
-  (test-org-attach-annex/with-annex
-   (let ((org-attach-git-annex-cutoff 1))
-     (should (org-attach-use-annex)))
-
-   (let ((org-attach-git-annex-cutoff nil))
-     (should-not (org-attach-use-annex))))
-
-  ;; test with non annex directory
-  (let ((tmpdir (make-temp-file "org-annex-test" t)))
-     (unwind-protect
-	 (let ((default-directory tmpdir)
-	       (org-attach-directory tmpdir))
-	   (shell-command "git init")
-	   (should-not (org-attach-use-annex)))
-       (delete-directory tmpdir 'recursive))))
-
-(ert-deftest test-org-attach/get-maybe ()
-  (org-test-for-executable "git-annex")
-  (test-org-attach-annex/with-annex
-   (let ((path (expand-file-name "test-file"))
-	 (annex-dup (make-temp-file "org-annex-test" t)))
-     (with-temp-buffer
-       (insert "hello world\n")
-       (write-file path))
-     (shell-command "git annex add test-file")
-     (shell-command "git annex sync")
-     ;; Set up remote & copy files there
-     (let ((annex-original default-directory)
-	   (default-directory annex-dup))
-       (shell-command (format "git clone %s ." (shell-quote-argument annex-original)))
-       (shell-command "git annex init dup")
-       (shell-command (format "git remote add original %s" (shell-quote-argument annex-original)))
-       (shell-command "git annex get test-file")
-       (shell-command "git annex sync"))
-     (shell-command (format "git remote add dup %s" (shell-quote-argument annex-dup)))
-     (shell-command "git annex sync")
-     (shell-command "git annex drop --force test-file")
-     ;; test getting the file from the dup when we should ALWAYS get
-     (should (not (file-exists-p (file-symlink-p (expand-file-name "test-file")))))
-     (let ((org-attach-annex-auto-get t))
-       (org-attach-annex-get-maybe (expand-file-name "test-file"))
-       ;; check that the file has the right contents
-       (with-temp-buffer
-	 (insert-file-contents path)
-	 (should (string-equal "hello world\n" (buffer-string)))))
-     ;; test getting the file from the dup when we should NEVER get
-     (shell-command "git annex drop --force test-file")
-     (let ((org-attach-annex-auto-get nil))
-       (should-error (org-attach-annex-get-maybe (expand-file-name "test-file"))))
-     (let ((org-attach-annex-auto-get 'ask)
-	   (called nil))
-       (flet ((y-or-n-p (prompt)
-			   (setq called 'was-called)
-			   t))
-	 (org-attach-annex-get-maybe (expand-file-name "test-file"))
-	 ;; check that the file has the right contents
-	 (with-temp-buffer
-	   (insert-file-contents path)
-	   (should (string-equal "hello world\n" (buffer-string))))
-	 (should (eq called 'was-called)))))))
-
-;;; test-org-attach.el ends here
-- 
2.5.0

Re: [PATCH] testing/lisp/test-org-attach-annex.el: New file

[Achim Gratz]

Erik Hetzner writes:
> * testing/lisp/test-org-attach-annex.el: Move all org-attach tests that
>   use git-annex to this file, which can test for the presence of
>   git-annex. Prevents tests failing on systems where git-annex is not
>   installed.

Thanks, pushed with a slight edit of the commit summary.  I've added the
Makefile enhancements in a separate commit.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Erik Hetzner]

Hi Rasmus,

Thanks for the thoughtful feedback.

On Mon, 25 Jan 2016 13:19:56 -0800,
Rasmus <rasmus@gmx.us> wrote:
>
> Hi Erik,
>
> Thanks for the patch.
>
> Note that between git-annex v6 and annex.largefiles most of the checking
> is unnecessary.  In my opinion it would be much more useful to start
> ripping out annex specific code, though the automatic fetching should be
> added.

I agree, but I think this should be a separate patch.

> Erik Hetzner <egh@e6h.org> writes:
>
> > +(defun org-attach-use-annex ()
> > +  "Return non-nil if git annex can be used."
> > +  (let ((git-dir (vc-git-root (expand-file-name org-attach-directory))))
> > +    (and org-attach-git-annex-cutoff
> > +         (or (file-exists-p (expand-file-name "annex" git-dir))
> > +             (file-exists-p (expand-file-name ".git/annex" git-dir))))))
>
> Seems fine, but I wonder if it wouldn’t be better to check the exit code
> of say an annex command and relies on its checking.  E.g. on my system
>
>     (zerop (shell-command "cd ~/annex/doc.annex/ && git annex info --fast" nil)) => t
>     (zerop (shell-command "cd ~/src/code/org-mode && git annex info --fast" nil )) => nil

This would be great, but it returns t for my non-annex git repos. I’m not sure
why the behavior is different for you.

> AFAIK annex will check if get should get anything.  If that’s correct, I’d
> prefer to just rely on however git annex get checks files.

That makes sense. I’ll change that code. The only disadvantage here is that it
is no longer clear to me how to tell if the content was fetched, if we want to
notify the user.

> > +        (dolist (new-or-modified
> > +                 (split-string
> > +                  (shell-command-to-string
> > +                   "git ls-files -zmo --exclude-standard") "\0" t))
>
> When I run this command in one of my annex repos I get:
>
>      fatal: This operation must be run in a work tree
>
> Maybe you are assuming indirect mode?

This code is unchanged from org-attach.el.
http://git-annex.branchable.com/direct_mode/#index5h2 says: “you cannot git
commit or git pull” in direct mode - so I’m curious if direct mode would work at
all with org-attach?

> > +          (if (and (org-attach-use-annex)
>
> wouldn’t it be better to bind the return value of (org-attach-use-annex)
> rather than call it once per file to be added?  Since there’s no dir
> argument, I guess it won’t change.

Good call.

> > +                   (>= (nth 7 (file-attributes new-or-modified))
> > +                       org-attach-git-annex-cutoff))
>
> If people want this they can use annex.largefiles.  Reimplementing
> annex.largefiles is not within Org’s domain.  It’s even more the case with
> annex v6.

I’m happy to make that change, but I feel it should be a separate patch.

> > +              (call-process "git" nil nil nil "annex" "add" new-or-modified)
> > +            (call-process "git" nil nil nil "add" new-or-modified))
>
> In git annex v6 you don’t need to call "git annex add" (but can).  In git
> annex v5 you don’t need to call "git add".
>
> To be compatible between with both v5 and v6 you can just call "git annex
> add", I guess.

I’ll leave it, then. Thanks for the information.

> > +++ b/testing/lisp/test-org-attach.el
> > @@ -0,0 +1,81 @@
> > +;;; test-org-attach.el --- Tests for Org Attach
> > +;;
> > +;; Copyright (c) 2016 Erik Hetzner
> > +;; Authors: Erik Hetzner
>
> I did not check this part.

Thank you, Rasmus, for the detailed review!

best, Erik

Re: [PATCH] org-attach.el: Fetch attachments from git annex

[Rasmus]

Erik Hetzner <egh@e6h.org> writes:

>> Seems fine, but I wonder if it wouldn’t be better to check the exit code
>> of say an annex command and relies on its checking.  E.g. on my system
>>
>>     (zerop (shell-command "cd ~/annex/doc.annex/ && git annex info --fast" nil)) => t
>>     (zerop (shell-command "cd ~/src/code/org-mode && git annex info --fast" nil )) => nil
>
> This would be great, but it returns t for my non-annex git repos. I’m not sure
> why the behavior is different for you.

No idea.  I have 6.20160114 and some recent version of Emacs-git (when
xwidgets was just merged)...  Weird.

>> AFAIK annex will check if get should get anything.  If that’s correct, I’d
>> prefer to just rely on however git annex get checks files.
>
> That makes sense. I’ll change that code. The only disadvantage here is that it
> is no longer clear to me how to tell if the content was fetched, if we want to
> notify the user.

I guess you could check the return code?  And maybe output...  You could
just start by stating (message "getting $FILE..."); get it; (message "got
$FILE").  It will instantaneous when the file is available.

> This code is unchanged from org-attach.el.
> http://git-annex.branchable.com/direct_mode/#index5h2 says: “you cannot git
> commit or git pull” in direct mode - so I’m curious if direct mode would work at
> all with org-attach?

Don’t know.  I use org-attach.  My dir is in a direct repo.  It never
complained.  Maybe there’s something I haven’t configured.  Maybe it’s
cause I use the assistant.  Don’t know...

> I’m happy to make that change, but I feel it should be a separate patch.

Indeed. 


Thanks for working on this.
Rasmus 

-- 
Got mashed potatoes. Ain't got no T-Bone. No T-Bone