Re: [O] [PATCH] org-attach.el: Get attachments from git annex

[Erik Hetzner]

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