From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rasmus Subject: Re: [PATCH] org-attach.el: Get attachments from git annex Date: Wed, 27 Jan 2016 23:20:59 +0100 Message-ID: <87r3h2ekac.fsf@gmx.us> References: <568b532e.d111620a.b25a8.ffffbb7c@mx.google.com> <87poxg8s22.fsf@kyleam.com> <568c6aaa.c345620a.7f4da.6359@mx.google.com> <56a5b193.ca77420a.1551e.667c@mx.google.com> <87lh7dz79f.fsf@gmx.us> <56a70513.6861420a.33633.5843@mx.google.com> <87egd4u6tq.fsf@kyleam.com> <56a7a139.885d620a.6b777.576d@mx.google.com> <87io2gb5xh.fsf@kyleam.com> <87oac8hu9p.fsf@gmx.us> <56a87251.0e2a620a.4811f.fffff1c6@mx.google.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:40721) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOYSg-0006ow-0h for emacs-orgmode@gnu.org; Wed, 27 Jan 2016 17:21:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOYSb-0008Sy-1a for emacs-orgmode@gnu.org; Wed, 27 Jan 2016 17:21:13 -0500 Received: from plane.gmane.org ([80.91.229.3]:41855) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOYSa-0008Ss-R9 for emacs-orgmode@gnu.org; Wed, 27 Jan 2016 17:21:08 -0500 Received: from list by plane.gmane.org with local (Exim 4.69) (envelope-from ) id 1aOYSW-0005Dp-U3 for emacs-orgmode@gnu.org; Wed, 27 Jan 2016 23:21:04 +0100 Received: from x590d8338.dyn.telefonica.de ([89.13.131.56]) by main.gmane.org with esmtp (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 27 Jan 2016 23:21:04 +0100 Received: from rasmus by x590d8338.dyn.telefonica.de with local (Gmexim 0.1 (Debian)) id 1AlnuQ-0007hv-00 for ; Wed, 27 Jan 2016 23:21:04 +0100 List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: emacs-orgmode@gnu.org Hi Erik, Thanks for the updated patch! A couple of more comments follow. I hope I’m not being too annoying! Erik Hetzner 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