From mboxrd@z Thu Jan 1 00:00:00 1970 From: Erik Hetzner Subject: Re: [PATCH] org-attach.el: Fetch attachments from git annex Date: Mon, 25 Jan 2016 21:31:20 -0800 Message-ID: <56a704a9.c18e420a.96e47.5774@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> 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]:34754) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNwDv-0003DN-AR for emacs-orgmode@gnu.org; Tue, 26 Jan 2016 00:31:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNwDs-0000oH-2Z for emacs-orgmode@gnu.org; Tue, 26 Jan 2016 00:31:27 -0500 Received: from mail-pf0-x229.google.com ([2607:f8b0:400e:c00::229]:34820) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNwDr-0000o7-NQ for emacs-orgmode@gnu.org; Tue, 26 Jan 2016 00:31:23 -0500 Received: by mail-pf0-x229.google.com with SMTP id 65so6260452pfd.2 for ; Mon, 25 Jan 2016 21:31:23 -0800 (PST) In-Reply-To: <87lh7dz79f.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 for the thoughtful feedback. On Mon, 25 Jan 2016 13:19:56 -0800, Rasmus 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 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=E2=80=99t be better to check the ex= it 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)) =3D> t > (zerop (shell-command "cd ~/src/code/org-mode && git annex info --fas= t" nil )) =3D> nil This would be great, but it returns t for my non-annex git repos. I=E2=80= =99m not sure why the behavior is different for you. > AFAIK annex will check if get should get anything. If that=E2=80=99s cor= rect, I=E2=80=99d > prefer to just rely on however git annex get checks files. That makes sense. I=E2=80=99ll 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: =E2=80=9Cyou ca= nnot git commit or git pull=E2=80=9D in direct mode - so I=E2=80=99m curious if dire= ct mode would work at all with org-attach? > > + (if (and (org-attach-use-annex) > > wouldn=E2=80=99t 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=E2=80=99s no = dir > argument, I guess it won=E2=80=99t change. Good call. > > + (>=3D (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=E2=80=99s domain. It=E2=80=99s even m= ore the case with > annex v6. I=E2=80=99m happy to make that change, but I feel it should be a separate p= atch. > > + (call-process "git" nil nil nil "annex" "add" new-or-mod= ified) > > + (call-process "git" nil nil nil "add" new-or-modified)) > > In git annex v6 you don=E2=80=99t need to call "git annex add" (but can).= In git > annex v5 you don=E2=80=99t need to call "git add". > > To be compatible between with both v5 and v6 you can just call "git annex > add", I guess. I=E2=80=99ll 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