[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [O] [PATCH] org-attach.el: Get attachments from git annex
From: |
Erik Hetzner |
Subject: |
Re: [O] [PATCH] org-attach.el: Get attachments from git annex |
Date: |
Sun, 31 Jan 2016 19:32:40 -0800 |
User-agent: |
Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (Gojō) APEL/10.8 EasyPG/1.0.0 Emacs/25.1.50 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) |
Hi Rasmus,
Thanks again for the feedback!
On Wed, 27 Jan 2016 14:20:59 -0800,
Rasmus <address@hidden> wrote:
>
> Hi Erik,
>
> Thanks for the updated patch!
>
> A couple of more comments follow.
> I hope I’m not being too annoying!
Not at all, I appreciate it.
> Erik Hetzner <address@hidden> 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.
This is a good point - I was following the pattern of
`org-confirm-shell-link-function' but I don’t think that in this case
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)))
>
> 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’t 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=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).
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 ()
>
> 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....
Since it is about to be removed via =rm -rf= it doesn’t seem worth worrying
about it :)
best, Erik
>
> > +;;; test-org-attach.el --- Tests for Org Attach
>
> Skipped again...
>
> --
> Slowly unravels in a ball of yarn and the devil collects it
- Re: [O] [PATCH] org-attach.el: Fetch attachments from git annex, (continued)
- [O] [PATCH] org-attach.el: Fetch attachments from git annex, Erik Hetzner, 2016/01/26
- Re: [O] [PATCH] org-attach.el: Fetch attachments from git annex, Kyle Meyer, 2016/01/26
- Re: [O] [PATCH] org-attach.el: Fetch attachments from git annex, Erik Hetzner, 2016/01/26
- Re: [O] [PATCH] org-attach.el: Fetch attachments from git annex, Kyle Meyer, 2016/01/26
- Re: [O] [PATCH] org-attach.el: Fetch attachments from git annex, Rasmus, 2016/01/26
- [O] [PATCH] org-attach.el: Get attachments from git annex, Erik Hetzner, 2016/01/27
- Re: [O] [PATCH] org-attach.el: Get attachments from git annex, Rasmus, 2016/01/27
- Re: [O] [PATCH] org-attach.el: Get attachments from git annex,
Erik Hetzner <=
- Re: [O] [PATCH] org-attach.el: Get attachments from git annex, Kyle Meyer, 2016/01/29