Hi Rasmus, Thanks again for the feedback!
On Wed, 27 Jan 2016 14:20:59 -0800, Rasmus <ras...@gmx.us> 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 <e...@e6h.org> 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