Hi Rasmus, Thanks for the thoughtful feedback.
On Mon, 25 Jan 2016 13:19:56 -0800, Rasmus <ras...@gmx.us> 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 <e...@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 This would be great, but it returns t for my non-annex git repos. I’m not sure why the behavior is different for you. > 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. That makes sense. I’ll 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: “you cannot git commit or git pull” in direct mode - so I’m curious if direct mode would work at all with org-attach? > > + (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. Good call. > > + (>= (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. I’m happy to make that change, but I feel it should be a separate patch. > > + (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. I’ll 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
