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

[Rasmus <rasmus@gmx.us>]

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