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

[Rasmus <rasmus@gmx.us>]

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