Moving and resetting attachments

Moving and resetting attachments

[Florian Lindner]

Hello,

two questions about moving attachments to org files:

C-c C-a a attaches a file and stores it under ./data/ID/...

Using C-c C-a s I can set another directory a attachment directory. Can I make org-mode move the content of the previous
directory to the new directory?

Can I "reset" the attachment directory, i.e. like C-c C-a s but :ATTACH_DIR: is deleted and the contents of the previous
directory are moved to ./data/ID?

Rationale:

I use org mode as a document management system. Create an entry Papers -> Interpolation -> ECCOMAS. I know create an
custom attachment directory ECCOMAS, next to the org file as long as I am working on that paper. When it's finished, I
want to move the contents of the ECCOMAS attachment directory to ./data/ID/.

Thanks,
Florian

Re: Moving and resetting attachments

[Eric Abrahamsen]

Florian Lindner <mailinglists@xgm.de> writes:

> Hello,
>
> two questions about moving attachments to org files:
>
> C-c C-a a attaches a file and stores it under ./data/ID/...
>
> Using C-c C-a s I can set another directory a attachment directory.
> Can I make org-mode move the content of the previous
> directory to the new directory?
>
> Can I "reset" the attachment directory, i.e. like C-c C-a s but
> :ATTACH_DIR: is deleted and the contents of the previous
> directory are moved to ./data/ID?
>
> Rationale:
>
> I use org mode as a document management system. Create an entry Papers -> Interpolation -> ECCOMAS. I know create an
> custom attachment directory ECCOMAS, next to the org file as long as I
> am working on that paper. When it's finished, I
> want to move the contents of the ECCOMAS attachment directory to ./data/ID/.

It doesn't work this way now, but I think it makes sense, and I would
also find that helpful. `org-attach-set-directory' could be changed to
check for existing files, and offer to move them. There's no
`org-attach-unset-directory', but I suppose there could be.

Eric

Re: Moving and resetting attachments

[Florian Lindner]

Am 01.06.2017 um 06:39 schrieb Eric Abrahamsen:
> Florian Lindner <mailinglists@xgm.de> writes:
> 
>> Hello,
>>
>> two questions about moving attachments to org files:
>>
>> C-c C-a a attaches a file and stores it under ./data/ID/...
>>
>> Using C-c C-a s I can set another directory a attachment directory.
>> Can I make org-mode move the content of the previous
>> directory to the new directory?
>>
>> Can I "reset" the attachment directory, i.e. like C-c C-a s but
>> :ATTACH_DIR: is deleted and the contents of the previous
>> directory are moved to ./data/ID?
>>
>> Rationale:
>>
>> I use org mode as a document management system. Create an entry Papers -> Interpolation -> ECCOMAS. I know create an
>> custom attachment directory ECCOMAS, next to the org file as long as I
>> am working on that paper. When it's finished, I
>> want to move the contents of the ECCOMAS attachment directory to ./data/ID/.
> 
> It doesn't work this way now, but I think it makes sense, and I would
> also find that helpful. `org-attach-set-directory' could be changed to
> check for existing files, and offer to move them. There's no
> `org-attach-unset-directory', but I suppose there could be.

Hey,

I hacked together some lines of lisp that should achieve that. It's my first non-trivial (from my point of trivialness)
piece of code. I'm open for any suggestions:

(defun flo/org-attach-move ()
  (interactive)
  (when (org-attach-dir)
    (let ((target (read-string "Move attachments to: ")) ; read-directory-name here?
          (attach-dir (org-attach-dir)))

      (if (string= target "")
          (progn
            (org-entry-delete nil "ATTACH_DIR")
            (setq target (org-attach-dir t)))
        (progn
          (org-entry-put nil "ATTACH_DIR" target)
          (make-directory target t))
        )

      (unless (string= target attach-dir)
        (copy-directory attach-dir target t nil t)
        (message "Deleting %s" attach-dir)
        ;; (shell-command "rm -rf %s" attach-dir)
        )
      )
    )
  )


Best,
Florian

Re: Moving and resetting attachments

[Eric Abrahamsen]

Florian Lindner <mailinglists@xgm.de> writes:

> Am 01.06.2017 um 06:39 schrieb Eric Abrahamsen:
>> Florian Lindner <mailinglists@xgm.de> writes:
>> 
>>> Hello,
>>>
>>> two questions about moving attachments to org files:
>>>
>>> C-c C-a a attaches a file and stores it under ./data/ID/...
>>>
>>> Using C-c C-a s I can set another directory a attachment directory.
>>> Can I make org-mode move the content of the previous
>>> directory to the new directory?
>>>
>>> Can I "reset" the attachment directory, i.e. like C-c C-a s but
>>> :ATTACH_DIR: is deleted and the contents of the previous
>>> directory are moved to ./data/ID?

> I hacked together some lines of lisp that should achieve that. It's my first non-trivial (from my point of trivialness)
> piece of code. I'm open for any suggestions:
>
> (defun flo/org-attach-move ()
>   (interactive)
>   (when (org-attach-dir)
>     (let ((target (read-string "Move attachments to: ")) ; read-directory-name here?
>           (attach-dir (org-attach-dir)))
>
>       (if (string= target "")
>           (progn
>             (org-entry-delete nil "ATTACH_DIR")
>             (setq target (org-attach-dir t)))
>         (progn
>           (org-entry-put nil "ATTACH_DIR" target)
>           (make-directory target t))
>         )
>
>       (unless (string= target attach-dir)
>         (copy-directory attach-dir target t nil t)
>         (message "Deleting %s" attach-dir)
>         ;; (shell-command "rm -rf %s" attach-dir)
>         )
>       )
>     )
>   )

Looks like a good start! My first comment is, this should definitely be
written as a patch to `org-attach-set-directory'. It's useful
functionality, and fits well into the whole system -- so long as you
give users a chance to say no, I don't see why it shouldn't be part of
the library.

Various comments:

1. Use the prefix arg to differentiate between setting and unsetting a
   directory. Ie, no prefix arg means set (and an empty string for
   directory name aborts),
   prefix arg means unset. The `org-attach' dispatch mechanism will pass
   the prefix arg through to this function.
2. Definitely use `read-directory-name'!
3. This is a good use of `copy-directory' with the COPY-CONTENTS flag,
   but I'd still recommend using `directory-files' and then looping over
   all the files with a `map-y-or-n-p'. That will give users a chance to
   selectively choose files to move. This is a matter of taste. If you
   stick with `copy-directory', at least ask the user first.
4. I think you're right not to delete the directory afterwards. Best not
   to assume too much.
5. The "else" branch of an `if' statement has an implicit `progn', you
   don't need to add it.
6. Convention is to not put closing parentheses on their own line. Just
   pile them up at the end of the last form.
7. Personally I'd rework things so you only call `org-attach-dir' once.
   How to handle this depends a bit on when when-let was introduced into
   Emacs, and whether Org is okay to support it. Probably safest to use
   when-let*. so:

(when-let* ((attach-dir (org-attach-dir))
            (target (read-directory-name "Move attachments to: ")))

That way everything will bail if there's no attach dir.

HTH,
Eric

Re: Moving and resetting attachments

[Nicolas Goaziou]

Hello,

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Looks like a good start! My first comment is, this should definitely be
> written as a patch to `org-attach-set-directory'. It's useful
> functionality, and fits well into the whole system -- so long as you
> give users a chance to say no, I don't see why it shouldn't be part of
> the library.

FWIW, I agree.

> Various comments:

[...]

> 3. This is a good use of `copy-directory' with the COPY-CONTENTS flag,
>    but I'd still recommend using `directory-files' and then looping over
>    all the files with a `map-y-or-n-p'. That will give users a chance to
>    selectively choose files to move. This is a matter of taste. If you
>    stick with `copy-directory', at least ask the user first.
> 4. I think you're right not to delete the directory afterwards. Best not
>    to assume too much.

What about using `rename-file' so as to move the whole directory to the
new location?

Maybe a defcustom could let the user choose between moving and copying
the attachment directory.

> 7. Personally I'd rework things so you only call `org-attach-dir' once.
>    How to handle this depends a bit on when when-let was introduced into
>    Emacs, and whether Org is okay to support it. Probably safest to use
>    when-let*. so:
>
> (when-let* ((attach-dir (org-attach-dir))
>             (target (read-directory-name "Move attachments to: ")))

We cannot use `when-let*'. Besides,

  (let ((attch-dir (org-attach-dir)))
    (when attach-dir
      (let ((target (read-directory-name "Move attachments to: ")))
        ...)))

is fine, too, or even

  (let ((attch-dir (or (org-attach-dir) (error "No attachment directory")))
        (target (read-directory-name "Move attachments to: ")))
    ...)


Regards,

-- 
Nicolas Goaziou

Re: Moving and resetting attachments

[Eric Abrahamsen]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Looks like a good start! My first comment is, this should definitely be
>> written as a patch to `org-attach-set-directory'. It's useful
>> functionality, and fits well into the whole system -- so long as you
>> give users a chance to say no, I don't see why it shouldn't be part of
>> the library.
>
> FWIW, I agree.

I should have mentioned it wasn't my decision :)

>> Various comments:
>
> [...]
>
>> 3. This is a good use of `copy-directory' with the COPY-CONTENTS flag,
>>    but I'd still recommend using `directory-files' and then looping over
>>    all the files with a `map-y-or-n-p'. That will give users a chance to
>>    selectively choose files to move. This is a matter of taste. If you
>>    stick with `copy-directory', at least ask the user first.
>> 4. I think you're right not to delete the directory afterwards. Best not
>>    to assume too much.
>
> What about using `rename-file' so as to move the whole directory to the
> new location?
>
> Maybe a defcustom could let the user choose between moving and copying
> the attachment directory.

Mostly I was thinking of two things: 1) giving the user the chance to
say yea or nay on a file-by-file basis, and 2) the possibility of
clobbering an existing directory. Even if the usual warnings kick in, it
still makes me nervous to think of what you could lose. Plus,
potentially merging directories might be useful.

>> 7. Personally I'd rework things so you only call `org-attach-dir' once.
>>    How to handle this depends a bit on when when-let was introduced into
>>    Emacs, and whether Org is okay to support it. Probably safest to use
>>    when-let*. so:
>>
>> (when-let* ((attach-dir (org-attach-dir))
>>             (target (read-directory-name "Move attachments to: ")))
>
> We cannot use `when-let*'. Besides,
>
>   (let ((attch-dir (org-attach-dir)))
>     (when attach-dir
>       (let ((target (read-directory-name "Move attachments to: ")))
>         ...)))
>
> is fine, too, or even
>
>   (let ((attch-dir (or (org-attach-dir) (error "No attachment directory")))
>         (target (read-directory-name "Move attachments to: ")))
>     ...)

Good to know about when-let, etc. There are indeed many ways to skin
that cat.

Eric

Re: Moving and resetting attachments

[Nicolas Goaziou]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Mostly I was thinking of two things: 1) giving the user the chance to
> say yea or nay on a file-by-file basis,

I cannot see why this could be a good thing. IIRC, attachment directory
naming scheme can be somewhat opaque, when using ID, and by changing the
name, you lose the way to find back the files in the old directory. IOW,
you most certainly want to move all files.

Regards,

Re: Moving and resetting attachments

[Eric Abrahamsen]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> Mostly I was thinking of two things: 1) giving the user the chance to
>> say yea or nay on a file-by-file basis,
>
> I cannot see why this could be a good thing. IIRC, attachment directory
> naming scheme can be somewhat opaque, when using ID, and by changing the
> name, you lose the way to find back the files in the old directory. IOW,
> you most certainly want to move all files.

What I meant was, first ask the user for a directory, then cycle through
the files and ask whether to move each file to that directory. The user
wouldn't specify the directory itself for each file. And with
`map-y-or-n-p', the user can just hit "!" to take care of all files.

Am I misunderstanding your concern? The only time the ID attachment
directory scheme (which is indeed opaque) would come into play is when
the command was called with a prefix argument -- ie, when removing a
custom directory, and reverting to the automatic directory. In that
case, the directory would be automatically derived, and the user would
simply be presented with the choice to move each file there, or not.

Hopefully this is a misunderstanding,
E

Re: Moving and resetting attachments

[Nicolas Goaziou]

Hello,

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> What I meant was, first ask the user for a directory, then cycle through
> the files and ask whether to move each file to that directory. The user
> wouldn't specify the directory itself for each file. And with
> `map-y-or-n-p', the user can just hit "!" to take care of all files.
>
> Am I misunderstanding your concern? The only time the ID attachment
> directory scheme (which is indeed opaque) would come into play is when
> the command was called with a prefix argument -- ie, when removing a
> custom directory, and reverting to the automatic directory. In that
> case, the directory would be automatically derived, and the user would
> simply be presented with the choice to move each file there, or not.

When asking for each file if it should be moved or not, you can end up
with files in both the old and the new directory. My concern is that,
when the old directory is a default ID directory, there is no simple way
to get the files back. So, I think we should automatically move files in
this case.

I also agree with your special case described above.

Regards,

-- 
Nicolas Goaziou

Re: Moving and resetting attachments

[Eric Abrahamsen]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> What I meant was, first ask the user for a directory, then cycle through
>> the files and ask whether to move each file to that directory. The user
>> wouldn't specify the directory itself for each file. And with
>> `map-y-or-n-p', the user can just hit "!" to take care of all files.
>>
>> Am I misunderstanding your concern? The only time the ID attachment
>> directory scheme (which is indeed opaque) would come into play is when
>> the command was called with a prefix argument -- ie, when removing a
>> custom directory, and reverting to the automatic directory. In that
>> case, the directory would be automatically derived, and the user would
>> simply be presented with the choice to move each file there, or not.
>
> When asking for each file if it should be moved or not, you can end up
> with files in both the old and the new directory. My concern is that,
> when the old directory is a default ID directory, there is no simple way
> to get the files back. So, I think we should automatically move files in
> this case.
>
> I also agree with your special case described above.

Okay, makes sense. I hope we haven't totally scared off Florian...

Florian, are you interested in doing another version of the patch?

Eric

Re: Moving and resetting attachments

[Florian Lindner]

Am 05.06.2017 um 01:25 schrieb Eric Abrahamsen:
> Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:
> 
>> Hello,
>>
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> What I meant was, first ask the user for a directory, then cycle through
>>> the files and ask whether to move each file to that directory. The user
>>> wouldn't specify the directory itself for each file. And with
>>> `map-y-or-n-p', the user can just hit "!" to take care of all files.
>>>
>>> Am I misunderstanding your concern? The only time the ID attachment
>>> directory scheme (which is indeed opaque) would come into play is when
>>> the command was called with a prefix argument -- ie, when removing a
>>> custom directory, and reverting to the automatic directory. In that
>>> case, the directory would be automatically derived, and the user would
>>> simply be presented with the choice to move each file there, or not.
>>
>> When asking for each file if it should be moved or not, you can end up
>> with files in both the old and the new directory. My concern is that,
>> when the old directory is a default ID directory, there is no simple way
>> to get the files back. So, I think we should automatically move files in
>> this case.
>>
>> I also agree with your special case described above.
> 
> Okay, makes sense. I hope we haven't totally scared off Florian...
> 
> Florian, are you interested in doing another version of the patch?

Haha, I couldn't follow everything in your discussion, but I'm happy to hear, that you two deem that a worthy feature to
integrate.

I will try to modify my code accordingly and count on your assistance!

Best,
Florian

Re: Moving and resetting attachments

[Florian Lindner]

Am 02.06.2017 um 11:19 schrieb Eric Abrahamsen:> Florian Lindner <mailinglists@xgm.de> writes:
>
>> Am 01.06.2017 um 06:39 schrieb Eric Abrahamsen:
>>> Florian Lindner <mailinglists@xgm.de> writes:
>>>
>>>> Hello,
>>>>
>>>> two questions about moving attachments to org files:
>>>>
>>>> C-c C-a a attaches a file and stores it under ./data/ID/...
>>>>
>>>> Using C-c C-a s I can set another directory a attachment directory.
>>>> Can I make org-mode move the content of the previous
>>>> directory to the new directory?
>>>>
>>>> Can I "reset" the attachment directory, i.e. like C-c C-a s but
>>>> :ATTACH_DIR: is deleted and the contents of the previous
>>>> directory are moved to ./data/ID?
>
>> I hacked together some lines of lisp that should achieve that. It's my first non-trivial (from my point of trivialness)
>> piece of code. I'm open for any suggestions:
>>
[...]
>
> Looks like a good start! My first comment is, this should definitely be
> written as a patch to `org-attach-set-directory'. It's useful
> functionality, and fits well into the whole system -- so long as you
> give users a chance to say no, I don't see why it shouldn't be part of
> the library.
>
> Various comments:
>
> 1. Use the prefix arg to differentiate between setting and unsetting a
>    directory. Ie, no prefix arg means set (and an empty string for
>    directory name aborts),
>    prefix arg means unset. The `org-attach' dispatch mechanism will pass
>    the prefix arg through to this function.
> 2. Definitely use `read-directory-name'!
> 3. This is a good use of `copy-directory' with the COPY-CONTENTS flag,
>    but I'd still recommend using `directory-files' and then looping over
>    all the files with a `map-y-or-n-p'. That will give users a chance to
>    selectively choose files to move. This is a matter of taste. If you
>    stick with `copy-directory', at least ask the user first.
> 4. I think you're right not to delete the directory afterwards. Best not
>    to assume too much.
> 5. The "else" branch of an `if' statement has an implicit `progn', you
>    don't need to add it.
> 6. Convention is to not put closing parentheses on their own line. Just
>    pile them up at the end of the last form.
> 7. Personally I'd rework things so you only call `org-attach-dir' once.
>    How to handle this depends a bit on when when-let was introduced into
>    Emacs, and whether Org is okay to support it. Probably safest to use
>    when-let*. so:
>
> (when-let* ((attach-dir (org-attach-dir))
>             (target (read-directory-name "Move attachments to: ")))
>
> That way everything will bail if there's no attach dir.

Ok, my new version is here. It should be able to replace org-attach-set-directory

(defun flo/org-attach-move (prefix)
  "If PREFIX arg, reset attach directory, else set target directory."
  (interactive "p") ; Correct check for boolean prefix?

  (let ((old-attach-dir (org-attach-dir))
        (new-attach-dir (if (eq prefix 1)
                            (let ((dir (org-entry-get nil "ATTACH_DIR")))
                              (setq dir (read-directory-name "Attachment directory: " dir))
                              (org-entry-put nil "ATTACH_DIR" dir)
                              (org-attach-dir t)) ; Changes semantics
                          (org-entry-delete nil "ATTACH_DIR")
                          (org-attach-dir t))))

    (message "old-attach-dir = %s" old-attach-dir)
    (message "new-attach-dir = %s" new-attach-dir)
    (unless (or (string= old-attach-dir new-attach-dir)
                (not old-attach-dir))
      (when (y-or-n-p "Copy over attachments from old directory? ")
        (copy-directory old-attach-dir new-attach-dir t nil t))
      (when (y-or-n-p (concat "Delete " old-attach-dir))
        (shell-command (format "rm -fr %s" old-attach-dir))
        ))))

Some questions about the code

* Is that the correct way to deal with a boolean prefix arg? I'm not interested in the value of the prefix arg, only if
it's given or not.

* The code changes the semantics of org-attach-set-directory, because it creates the newly set attach dir. IMHO this
makes more sense.

* It deletes only the first part of the dir, e.g. data/83/1234567, it only deletes the 1234567 dir, even if 83 is empty
afterwards. But I think that's ok.

> 1. Use the prefix arg to differentiate between setting and unsetting a
>    directory. Ie, no prefix arg means set (and an empty string for
>    directory name aborts),
>    prefix arg means unset. The `org-attach' dispatch mechanism will pass
>    the prefix arg through to this function.

Tried to take that into account.

> 3. This is a good use of `copy-directory' with the COPY-CONTENTS flag,
>    but I'd still recommend using `directory-files' and then looping over
>    all the files with a `map-y-or-n-p'. That will give users a chance to
>    selectively choose files to move. This is a matter of taste. If you
>    stick with `copy-directory', at least ask the user first.
> 4. I think you're right not to delete the directory afterwards. Best not
>    to assume too much.

I think this it is a good compromise now. IMHO the default workflow when changing an attachment directory is to move
contents.

> 5. The "else" branch of an `if' statement has an implicit `progn', you
>    don't need to add it.

Yes, I know, but I like it for reasons of consistency with the "then" branch.

Thanks for your comments!

Best,
Florian

Re: Moving and resetting attachments

[Nicolas Goaziou]

Hello,

Florian Lindner <mailinglists@xgm.de> writes:

> Ok, my new version is here. It should be able to replace
> org-attach-set-directory

Thank you. Comments follow.

> Some questions about the code
>
> * Is that the correct way to deal with a boolean prefix arg? I'm not interested in the value of the prefix arg, only if
> it's given or not.

No, it should be (interactive "P") so PREFIX, or more commonly, ARG, is
nil when not provided.

> * The code changes the semantics of org-attach-set-directory, because it creates the newly set attach dir. IMHO this
> makes more sense.

OK.

> * It deletes only the first part of the dir, e.g. data/83/1234567, it only deletes the 1234567 dir, even if 83 is empty
> afterwards. But I think that's ok.

OK.

Here is an update of your function, with comments and FIXME. The
docstring could certainly be improved, but you get the idea.

  (defun flo/org-attach-move (&optional arg)
    "Move current attachements to another directory.
  When ARG is non-nil, reset attach directory.  Create directory if
  needed."
    (interactive "P")
    (let ((old (org-attach-dir))
          (new
           (progn
             (if arg (org-entry-delete nil "ATTACH_DIR")
               (let ((dir (read-directory-name
                           "Attachment directory: "
                           (org-entry-get nil
                                          "ATTACH_DIR"
                                          (and org-attach-allow-inheritance t)))))
                 (org-entry-put nil "ATTACH_DIR" dir)))
             (org-attach-dir t))))
      (message "old-attach-dir = %S" old) ;FIXME: remove?
      (message "new-attach-dir = %S" new) ;FIXME: remove?
      (unless (or (string= old new)
                  (not old))
        ;; FIXME: Need a special case for directory reset (non-nil ARG).
        ;; FIXME: Maybe `yes-or-no-p' is safer when moving data around?
        (when (y-or-n-p "Copy over attachments from old directory? ")
          (copy-directory old-attach-dir new t nil t))
        (when (y-or-n-p (concat "Delete " old))
          ;; FIXME: Why not `delete-directory'?
          (shell-command (format "rm -fr %s" old))))))

Regards,

-- 
Nicolas Goaziou

Re: Moving and resetting attachments

[Florian Lindner]

Am 10.06.2017 um 09:36 schrieb Nicolas Goaziou:
> Hello,
> 
> Florian Lindner <mailinglists@xgm.de> writes:
> 
>> Ok, my new version is here. It should be able to replace
>> org-attach-set-directory
> 
> Thank you. Comments follow.
> 
>> Some questions about the code
>>
>> * Is that the correct way to deal with a boolean prefix arg? I'm not interested in the value of the prefix arg, only if
>> it's given or not.
> 
> No, it should be (interactive "P") so PREFIX, or more commonly, ARG, is
> nil when not provided.

Thanks!.

>> * The code changes the semantics of org-attach-set-directory, because it creates the newly set attach dir. IMHO this
>> makes more sense.
> 
> OK.
> 
>> * It deletes only the first part of the dir, e.g. data/83/1234567, it only deletes the 1234567 dir, even if 83 is empty
>> afterwards. But I think that's ok.
> 
> OK.
> 
> Here is an update of your function, with comments and FIXME. The
> docstring could certainly be improved, but you get the idea.

Yeah, docstring is usually the last I add, since I should at least know what the function is supposed to do ;-)

>   (defun flo/org-attach-move (&optional arg)
>     "Move current attachements to another directory.
>   When ARG is non-nil, reset attach directory.  Create directory if
>   needed."
>     (interactive "P")
>     (let ((old (org-attach-dir))
>           (new
>            (progn
>              (if arg (org-entry-delete nil "ATTACH_DIR")
>                (let ((dir (read-directory-name
>                            "Attachment directory: "
>                            (org-entry-get nil
>                                           "ATTACH_DIR"
>                                           (and org-attach-allow-inheritance t)))))

What is the use of (and org-attach-allow-inheritance t)? Doesn't it always returns org-attach-allow-inheritance?
Anyways, I'm not really sure if I understand the doc of org-entry-get correctly. Does org-entry-get not automatically
take inheritance into account, based on the the per-entry or global setting?

>                  (org-entry-put nil "ATTACH_DIR" dir)))
>              (org-attach-dir t))))
>       (message "old-attach-dir = %S" old) ;FIXME: remove?
>       (message "new-attach-dir = %S" new) ;FIXME: remove?

Yes, of course.

>       (unless (or (string= old new)
>                   (not old))
>         ;; FIXME: Need a special case for directory reset (non-nil ARG).

Why that? Aren't old and new holding the appropriate dirs in that case and copy over / delete as they should?

>         ;; FIXME: Maybe `yes-or-no-p' is safer when moving data around?

Ok. I wasn't aware of the difference, since I have (fset 'yes-or-no-p 'y-or-n-p) in my .emacs.

>         (when (y-or-n-p "Copy over attachments from old directory? ")
>           (copy-directory old-attach-dir new t nil t))
>         (when (y-or-n-p (concat "Delete " old))
>           ;; FIXME: Why not `delete-directory'?
>           (shell-command (format "rm -fr %s" old))))))

I took it from org-attach-delete-all. But you're delete-directory is probably better than a shell-command.

Latest version:

(defun flo/org-attach-move (&optional arg)
  "Move current attachements to another directory.
  When ARG is non-nil, reset attach directory.  Create directory if
  needed."
  (interactive "P")
  (let ((old (org-attach-dir))
        (new
         (progn
           (if arg (org-entry-delete nil "ATTACH_DIR")
             (let ((dir (read-directory-name
                         "Attachment directory: "
                         (org-entry-get nil
                                        "ATTACH_DIR"
                                        (and org-attach-allow-inheritance t)))))
               (org-entry-put nil "ATTACH_DIR" dir)))
           (org-attach-dir t))))
    (unless (or (string= old new)
                (not old))
      ;; FIXME: Need a special case for directory reset (non-nil ARG).
      (when (yes-or-no-p "Copy over attachments from old directory? ")
        (copy-directory old new t nil t))
      (when (yes-or-no-p (concat "Delete " old))
        (delete-directory old t)))))



Best,
Florian

Re: Moving and resetting attachments

[Nicolas Goaziou]

Hello,

Florian Lindner <mailinglists@xgm.de> writes:

> What is the use of (and org-attach-allow-inheritance t)? Doesn't it always returns org-attach-allow-inheritance?

It return nil if `org-attach-allow-inheritance' is nil, t otherwise. In
particular, if `org-attach-allow-inheritance' is set to `selective', the
S-exp returns t.

> Anyways, I'm not really sure if I understand the doc of org-entry-get correctly. Does org-entry-get not automatically
> take inheritance into account, based on the the per-entry or global
> setting?

No it doesn't. The caller choose if it should ignore inheritance (the
default), use it unconditionally (a non-nil INHERIT argument), or let
the user decide (`selective' INHERIT argument).
>>         ;; FIXME: Need a special case for directory reset (non-nil ARG).
>
> Why that? Aren't old and new holding the appropriate dirs in that case
> and copy over / delete as they should?

Probably. I was thinking to some special case that may not exist, after
all. Never mind then.

> Latest version:
>
> (defun flo/org-attach-move (&optional arg)
>   "Move current attachements to another directory.
>   When ARG is non-nil, reset attach directory.  Create directory if
>   needed."
>   (interactive "P")
>   (let ((old (org-attach-dir))
>         (new
>          (progn
>            (if arg (org-entry-delete nil "ATTACH_DIR")
>              (let ((dir (read-directory-name
>                          "Attachment directory: "
>                          (org-entry-get nil
>                                         "ATTACH_DIR"
>                                         (and org-attach-allow-inheritance t)))))
>                (org-entry-put nil "ATTACH_DIR" dir)))
>            (org-attach-dir t))))
>     (unless (or (string= old new)
>                 (not old))
>       ;; FIXME: Need a special case for directory reset (non-nil ARG).
>       (when (yes-or-no-p "Copy over attachments from old directory? ")
>         (copy-directory old new t nil t))
>       (when (yes-or-no-p (concat "Delete " old))
>         (delete-directory old t)))))

It looks good.

Could you provide a patch for that, and an entry in ORG-NEWS? Also, it
would be nice to provide test for the feature.

Thank you !

Regards,

-- 
Nicolas Goaziou

Re: Moving and resetting attachments

[Florian Lindner]

Am 13.06.2017 um 23:41 schrieb Nicolas Goaziou:
> Hello,
> 
> Florian Lindner <mailinglists@xgm.de> writes:
> 
>> What is the use of (and org-attach-allow-inheritance t)? Doesn't it always returns org-attach-allow-inheritance?
> 
> It return nil if `org-attach-allow-inheritance' is nil, t otherwise. In
> particular, if `org-attach-allow-inheritance' is set to `selective', the
> S-exp returns t.

Ok, so it's basically a conversion to bool.

>> Anyways, I'm not really sure if I understand the doc of org-entry-get correctly. Does org-entry-get not automatically
>> take inheritance into account, based on the the per-entry or global
>> setting?
> 
> No it doesn't. The caller choose if it should ignore inheritance (the
> default), use it unconditionally (a non-nil INHERIT argument), or let
> the user decide (`selective' INHERIT argument).
>>>         ;; FIXME: Need a special case for directory reset (non-nil ARG).
>>
>> Why that? Aren't old and new holding the appropriate dirs in that case
>> and copy over / delete as they should?
> 
> Probably. I was thinking to some special case that may not exist, after
> all. Never mind then.
> 
>> Latest version:
>>
>> (defun flo/org-attach-move (&optional arg)
>>   "Move current attachements to another directory.
>>   When ARG is non-nil, reset attach directory.  Create directory if
>>   needed."
>>   (interactive "P")
>>   (let ((old (org-attach-dir))
>>         (new
>>          (progn
>>            (if arg (org-entry-delete nil "ATTACH_DIR")
>>              (let ((dir (read-directory-name
>>                          "Attachment directory: "
>>                          (org-entry-get nil
>>                                         "ATTACH_DIR"
>>                                         (and org-attach-allow-inheritance t)))))
>>                (org-entry-put nil "ATTACH_DIR" dir)))
>>            (org-attach-dir t))))
>>     (unless (or (string= old new)
>>                 (not old))
>>       ;; FIXME: Need a special case for directory reset (non-nil ARG).
>>       (when (yes-or-no-p "Copy over attachments from old directory? ")
>>         (copy-directory old new t nil t))
>>       (when (yes-or-no-p (concat "Delete " old))
>>         (delete-directory old t)))))
> 
> It looks good.
> 
> Could you provide a patch for that, and an entry in ORG-NEWS? Also, it

Sure, I will do that.

> would be nice to provide test for the feature.

That I'm not so sure of. I try to get myself aquainted to ert, but my elisp knowledge, you surely know, is very limited.

Do I understand correctly, that there is no test for any org-attach stuff so far? I've found lisp/test-org-attach-annex.el, but that's for git-annex
stuff.

Best,
Florian

Re: Moving and resetting attachments

[Nicolas Goaziou]

Hello,

Florian Lindner <mailinglists@xgm.de> writes:

> Ok, so it's basically a conversion to bool.

Correct.

> That I'm not so sure of. I try to get myself aquainted to ert, but my elisp knowledge, you surely know, is very limited.

I think you're doing well so far.

> Do I understand correctly, that there is no test for any org-attach stuff so far? I've found lisp/test-org-attach-annex.el, but that's for git-annex
> stuff.

That's correct. Anyway, tests can be postponed to a later patch. It can
be cumbersome to play with temporary files without clobbering the
repository.

However, I think tests are important in this case, considering we're
moving user's data around.


Regards,

-- 
Nicolas Goaziou

Re: Moving and resetting attachments

[Florian Lindner]

Am 24.06.2017 um 10:53 schrieb Nicolas Goaziou:> Hello,
>
> Florian Lindner <mailinglists@xgm.de> writes:
>
>> Ok, so it's basically a conversion to bool.
>
> Correct.
>
>> That I'm not so sure of. I try to get myself aquainted to ert, but my elisp knowledge, you surely know, is very limited.
>
> I think you're doing well so far.
>
>> Do I understand correctly, that there is no test for any org-attach stuff so far? I've found
lisp/test-org-attach-annex.el, but that's for git-annex
>> stuff.
>
> That's correct. Anyway, tests can be postponed to a later patch. It can
> be cumbersome to play with temporary files without clobbering the
> repository.
>
> However, I think tests are important in this case, considering we're
> moving user's data around.

Hey,

I have attached a patch, based on the current git master. I'm not sure how the contribution process works. Do I need to
sign stuff such as copyright assignement? Do you use a Git webinterface, GitHub or Lab?

Writing a test for it is on my todo list.

Best,
Florian

Re: Moving and resetting attachments

[Florian Lindner]

Am 10.06.2017 um 09:36 schrieb Nicolas Goaziou:
> Hello,
> 
> Florian Lindner <mailinglists@xgm.de> writes:
> 
>> Ok, my new version is here. It should be able to replace
>> org-attach-set-directory
> 
> Thank you. Comments follow.
> 
>> Some questions about the code
>>
>> * Is that the correct way to deal with a boolean prefix arg? I'm not interested in the value of the prefix arg, only if
>> it's given or not.
> 
> No, it should be (interactive "P") so PREFIX, or more commonly, ARG, is
> nil when not provided.

Thanks!.

>> * The code changes the semantics of org-attach-set-directory, because it creates the newly set attach dir. IMHO this
>> makes more sense.
> 
> OK.
> 
>> * It deletes only the first part of the dir, e.g. data/83/1234567, it only deletes the 1234567 dir, even if 83 is empty
>> afterwards. But I think that's ok.
> 
> OK.
> 
> Here is an update of your function, with comments and FIXME. The
> docstring could certainly be improved, but you get the idea.

Yeah, docstring is usually the last I add, since I should at least know what the function is supposed to do ;-)

>   (defun flo/org-attach-move (&optional arg)
>     "Move current attachements to another directory.
>   When ARG is non-nil, reset attach directory.  Create directory if
>   needed."
>     (interactive "P")
>     (let ((old (org-attach-dir))
>           (new
>            (progn
>              (if arg (org-entry-delete nil "ATTACH_DIR")
>                (let ((dir (read-directory-name
>                            "Attachment directory: "
>                            (org-entry-get nil
>                                           "ATTACH_DIR"
>                                           (and org-attach-allow-inheritance t)))))

What is the use of (and org-attach-allow-inheritance t)? Doesn't it always returns org-attach-allow-inheritance?
Anyways, I'm not really sure if I understand the doc of org-entry-get correctly. Does org-entry-get not automatically
take inheritance into account, based on the the per-entry or global setting?

>                  (org-entry-put nil "ATTACH_DIR" dir)))
>              (org-attach-dir t))))
>       (message "old-attach-dir = %S" old) ;FIXME: remove?
>       (message "new-attach-dir = %S" new) ;FIXME: remove?

Yes, of course.

>       (unless (or (string= old new)
>                   (not old))
>         ;; FIXME: Need a special case for directory reset (non-nil ARG).

Why that? Aren't old and new holding the appropriate dirs in that case and copy over / delete as they should?

>         ;; FIXME: Maybe `yes-or-no-p' is safer when moving data around?

Ok. I wasn't aware of the difference, since I have (fset 'yes-or-no-p 'y-or-n-p) in my .emacs.

>         (when (y-or-n-p "Copy over attachments from old directory? ")
>           (copy-directory old-attach-dir new t nil t))
>         (when (y-or-n-p (concat "Delete " old))
>           ;; FIXME: Why not `delete-directory'?
>           (shell-command (format "rm -fr %s" old))))))

I took it from org-attach-delete-all. But you're delete-directory is probably better than a shell-command.

Latest version:

(defun flo/org-attach-move (&optional arg)
  "Move current attachements to another directory.
  When ARG is non-nil, reset attach directory.  Create directory if
  needed."
  (interactive "P")
  (let ((old (org-attach-dir))
        (new
         (progn
           (if arg (org-entry-delete nil "ATTACH_DIR")
             (let ((dir (read-directory-name
                         "Attachment directory: "
                         (org-entry-get nil
                                        "ATTACH_DIR"
                                        (and org-attach-allow-inheritance t)))))
               (org-entry-put nil "ATTACH_DIR" dir)))
           (org-attach-dir t))))
    (unless (or (string= old new)
                (not old))
      ;; FIXME: Need a special case for directory reset (non-nil ARG).
      (when (yes-or-no-p "Copy over attachments from old directory? ")
        (copy-directory old new t nil t))
      (when (yes-or-no-p (concat "Delete " old))
        (delete-directory old t)))))



Best,
Florian