From: Erik Hetzner <egh@e6h.org>
To: Rasmus <rasmus@gmx.us>
Cc: emacs-orgmode@gnu.org
Subject: Re: [PATCH] org-attach.el: Fetch attachments from git annex
Date: Mon, 25 Jan 2016 21:31:20 -0800 [thread overview]
Message-ID: <56a704a9.c18e420a.96e47.5774@mx.google.com> (raw)
In-Reply-To: <87lh7dz79f.fsf@gmx.us>
Hi Rasmus,
Thanks for the thoughtful feedback.
On Mon, 25 Jan 2016 13:19:56 -0800,
Rasmus <rasmus@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 <egh@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
next prev parent reply other threads:[~2016-01-26 5:31 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-05 5:09 [PATCH] org-attach.el: Fetch attachments from git annex Erik Hetzner
2016-01-05 5:30 ` Eric Abrahamsen
2016-01-05 6:11 ` Erik Hetzner
2016-01-05 6:36 ` Kyle Meyer
2016-01-05 9:56 ` Rasmus
2016-01-05 17:14 ` Kyle Meyer
2016-01-05 18:16 ` Rasmus
2016-01-05 19:30 ` Kyle Meyer
2016-01-05 21:55 ` Rasmus
2016-01-06 5:43 ` Kyle Meyer
2016-01-06 1:27 ` Erik Hetzner
2016-01-06 9:37 ` Rasmus
2016-01-05 6:21 ` Kyle Meyer
2016-01-06 1:15 ` Erik Hetzner
2016-01-25 5:24 ` Erik Hetzner
2016-01-25 21:19 ` Rasmus
2016-01-25 4:34 ` Erik Hetzner
2016-01-26 7:40 ` Kyle Meyer
2016-01-26 16:39 ` Erik Hetzner
2016-01-26 17:34 ` Kyle Meyer
2016-01-26 22:04 ` Rasmus
2016-01-25 4:34 ` [PATCH] org-attach.el: Get " Erik Hetzner
2016-01-27 22:20 ` Rasmus
2016-02-01 3:32 ` Erik Hetzner
2016-01-29 5:39 ` Kyle Meyer
2016-01-25 4:34 ` Erik Hetzner
2016-02-05 2:41 ` Kyle Meyer
2016-02-06 12:18 ` Rasmus
2016-02-07 17:15 ` Erik Hetzner
2016-02-07 20:48 ` Achim Gratz
2016-02-09 5:25 ` Erik Hetzner
2016-02-09 19:40 ` Achim Gratz
2016-02-09 21:12 ` Erik Hetzner
2016-02-09 22:19 ` Achim Gratz
2016-02-11 2:24 ` Erik Hetzner
2016-02-11 18:57 ` Achim Gratz
2016-02-09 5:40 ` [PATCH] testing/lisp/test-org-attach-annex.el: New file Erik Hetzner
2016-02-14 11:50 ` Achim Gratz
2016-01-26 5:31 ` Erik Hetzner [this message]
2016-01-26 22:10 ` [PATCH] org-attach.el: Fetch attachments from git annex Rasmus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56a704a9.c18e420a.96e47.5774@mx.google.com \
--to=egh@e6h.org \
--cc=emacs-orgmode@gnu.org \
--cc=rasmus@gmx.us \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).