From mboxrd@z Thu Jan 1 00:00:00 1970 From: Erik Hetzner Subject: Re: [PATCH] org-attach.el: Get attachments from git annex Date: Sun, 31 Jan 2016 19:32:40 -0800 Message-ID: <56aed1e0.0c46620a.e568d.ffff82e2@mx.google.com> 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> <87r3h2ekac.fsf@gmx.us> Reply-To: Erik Hetzner Mime-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:57711) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ5ET-00026Z-Vx for emacs-orgmode@gnu.org; Sun, 31 Jan 2016 22:32:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQ5EQ-0003SO-NZ for emacs-orgmode@gnu.org; Sun, 31 Jan 2016 22:32:53 -0500 Received: from mail-pa0-x230.google.com ([2607:f8b0:400e:c03::230]:36200) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ5EQ-0003Qb-D3 for emacs-orgmode@gnu.org; Sun, 31 Jan 2016 22:32:50 -0500 Received: by mail-pa0-x230.google.com with SMTP id yy13so73888374pab.3 for ; Sun, 31 Jan 2016 19:32:50 -0800 (PST) In-Reply-To: <87r3h2ekac.fsf@gmx.us> 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: Rasmus Cc: emacs-orgmode@gnu.org Hi Rasmus, Thanks again for the feedback! On Wed, 27 Jan 2016 14:20:59 -0800, Rasmus wrote: >=20 > Hi Erik, >=20 > Thanks for the updated patch! >=20 > A couple of more comments follow. > I hope I=E2=80=99m not being too annoying!=20 Not at all, I appreciate it. > Erik Hetzner writes: >=20 > > +(defcustom org-attach-annex-confirm-get-function #'y-or-n-p > > + "Function to call to confirm if Org should call git annex get if nec= essary. > > +If t, always get, if nil, never get." >=20 >=20 > Please note that the function should accept one argument cf. your code > below. Also, I wonder if there=E2=80=99s really a point in having the in= creased > flexibility of a function over just: t, nil and =E2=80=99ask. This is a good point - I was following the pattern of `org-confirm-shell-link-function' but I don=E2=80=99t think that in this ca= se 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))) >=20 > 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=E2=80=99t 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=3Dfound --in=3Dhere %s" (shell-q= uote-argument path)))))) > > + (if (if (functionp org-attach-annex-confirm-get-function) > > + (funcall org-attach-annex-confirm-get-function (format "Run git a= nnex 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)))) >=20 > Can=E2=80=99t you factor out the inner "if", e.g. to an outer let? Shoul= dn=E2=80=99t 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=E2=80=99t 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 () >=20 > Looks fine.=20 >=20 > > 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) >=20 > I wonder if it would be better to directly target the files you use? I > don=E2=80=99t think there=E2=80=99s a case where changing the mod of the = testdir is a > problem though.... Since it is about to be removed via =3Drm -rf=3D it doesn=E2=80=99t seem wo= rth worrying about it :) best, Erik >=20 > > +;;; test-org-attach.el --- Tests for Org Attach >=20 > Skipped again... >=20 > --=20 > Slowly unravels in a ball of yarn and the devil collects it