emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
@ 2020-09-21 18:34 Gustavo Barros
  2020-09-23 14:17 ` stardiviner
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Gustavo Barros @ 2020-09-21 18:34 UTC (permalink / raw)
  To: emacs-orgmode

Hi All,

some time ago, I've reported an issue regarding duplicity of the default 
candidate in `org-refile' 
(https://orgmode.org/list/87lftw1k2n.fsf@gmail.com/).  The problem was 
that, when using `org-refile-use-outline-path' an "extra" slash was 
appended at the end of every path, but candidates were stored in 
`org-refile-history' without that extra slash.  Bastien took care of 
that and indeed changed things so as to pass the elements to 
`org-refile-history' with the trailing slash as appropriate.

At the time, I reported a difference of behavior between 
`completing-read-default' and `ivy-completing-read' after the mentioned 
commit by Bastien.  But the issue did not appear for Bastien, which does 
not use Ivy, and I also was not able to diagnose the problem properly. 
I felt I didn't have enough to offer as to insist, so I resorted to an 
old hack of mine.  But the new release this week (thank you very much!, 
btw) has bitten me again on this, so I went back to some digging, and 
hopefully I can do a better job this time in diagnosing the problem and 
suggesting a fix.


An ECM to reproduce the issue as it currently stands is:

- Start 'emacs -Q'

- Do an initial setup:
  #+begin_src emacs-lisp
  (add-to-list 'load-path "~/.emacs.d/elpa/org-plus-contrib-20200921")
  (add-to-list 'load-path "~/.emacs.d/elpa/ivy-20200826.955")
  ;; Those are the latest Org weekly build (Org 9.4) and the current up 
  to date
  ;; Ivy at Melpa
  (setq org-refile-targets '(("~/org/test.org" :maxlevel . 2)))
  (setq org-refile-use-outline-path 'file)
  (setq org-outline-path-complete-in-steps nil)
  (require 'ivy)
  (ivy-mode)
  ;; Bear with me, the problem is not with Ivy, I'll demonstrate that.
  #+end_src

- Open file "~/org/test.org", with contents:
  #+begin_src org
  ,* Top heading 1
  ,* Top heading 2
  ,** Entry 1
  ,** Entry 2
  #+end_src

- Go to heading "Entry 1", refile it to "Top heading 1"

- Go to heading "Entry 2", and call `org-refile'

- Observe the available candidates, and notice "test.org/Top heading 1/" 
  is there twice, once as the default candidate, with a *double* 
  trailing slash, and also on the paths list, with a single trailing 
  slash.


I've tried to pin down what is going on here and my understanding is 
that Bastien's fix on that previous thread did indeed correct the 
problem of the missing trailing slash in `org-refile-history' and this 
indeed corresponds correctly to the state of the completion "collection" 
(the let bound `tbl' variable in `org-refile-get-location'), as it 
should.  But there remained a couple of instances in 
`org-refile-get-location' which added the trailing slash considering 
`org-refile-history' didn't have them, so that when this is done, we get 
a double trailing slash.

The two instances are: 1) when the completion function is actually 
called:

#+begin_src emacs-lisp
(setq answ (funcall cfunc prompt tbl nil (not new-nodes)
			    nil 'org-refile-history
			    (or cdef (concat (car org-refile-history) 
			    extra))))
#+end_src

2) And three lines bellow, on the let binding:

#+begin_src emacs-lisp
(let* ((last-refile-loc (car org-refile-history))
       (last-refile-loc-path (concat last-refile-loc extra)))
  ...)
#+end_src

In both instances, we are getting the `car' of `org-refile-history' 
which now already has `extra' (that is, the trailing slash) and adding 
it again.

My suggested fix is to remove these `extra's in duplicity, they are 
remnants of when `org-refile-history' didn't have them already.  That 
is:

#+begin_src emacs-lisp
(setq answ (funcall cfunc prompt tbl nil (not new-nodes)
			    nil 'org-refile-history
			    (or cdef (car org-refile-history))))
#+end_src

And:

#+begin_src emacs-lisp
(let* ((last-refile-loc (car org-refile-history))
       (last-refile-loc-path last-refile-loc))
  ...)
#+end_src

Of course, the second opens some opportunity to simplify the code that 
follows considering `last-refile-loc-path' and `last-refile-loc' are now 
identical.


Why I think this is the problem and the correct way to fix it:

1) If you add inspection points at the appropriate locations for the 
sexps =(concat (car org-refile-history) extra)= and =(concat 
last-refile-loc extra)= you will find the double trailing slash there, 
and it shouldn't be there.

2) The visual manifestation of this double trailing slash in the default 
candidate with `ivy-mode' is there therefore independently of 
`ivy-mode`.  Indeed, `ivy-mode' basically sets 
`completing-read-function' to `ivy-completing-read', which in turn calls 
its main API function `ivy-read'.  `ivy-completing-read' just passes 
along the `def' argument without manipulating it.  As far as I can see, 
`ivy-read' also does not change it before calling 
`read-from-minibuffer'.

Why `completing-read-default' does not show this second trailing slash 
of the `def' argument it received is another matter.  But it's there...

3) I've tested this suggested fix in the following scenarios (starting 
from the ECM above):

#+begin_src emacs-lisp
(setq org-refile-use-outline-path 'file)
(setq org-outline-path-complete-in-steps nil)
;; (ivy-mode)
#+end_src

#+begin_src emacs-lisp
(setq org-refile-use-outline-path 'file)
(setq org-outline-path-complete-in-steps nil)
(ivy-mode)
#+end_src

#+begin_src emacs-lisp
(setq org-refile-use-outline-path 'file)
(setq org-outline-path-complete-in-steps t)
;; `org-olpath-completing-read' does not play well with `ivy-mode' so no 
   need
;; to use it here
#+end_src

#+begin_src emacs-lisp
(setq org-refile-use-outline-path nil)
(ivy-mode)
#+end_src

#+begin_src emacs-lisp
(setq org-refile-use-outline-path nil)
;; (ivy-mode)
#+end_src

Other non-nil values of `org-refile-use-outline-path' should be 
equivalent to `file' with respect to the issue at hand, so they are 
implicitly covered.

Having tested these cases, to the best of my knowledge, they all work as 
expected.  Though I'm admittedly less acquainted with =(setq 
org-refile-use-outline-path nil)= and =(setq 
org-outline-path-complete-in-steps t)=.

All tests and ECM were ran with "Org mode version 9.4 
(9.4-7-g3eccc5-elpaplus @ 
/home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)" and "GNU Emacs 
27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 
1.16.0) of 2020-08-11".

I hope this is a more useful report than last time.


Best,
Gustavo.





Emacs  : GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 
3.24.20, cairo version 1.16.0)
 of 2020-08-11
Package: Org mode version 9.4 (9.4-7-g3eccc5-elpaplus @ 
/home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)

current state:
==============
(setq
 org-src-mode-hook '(org-src-babel-configure-edit-buffer
		     org-src-mode-configure-edit-buffer)
 org-link-shell-confirm-function 'yes-or-no-p
 org-metadown-hook '(org-babel-pop-to-session-maybe)
 org-clock-out-hook '(org-clock-remove-empty-clock-drawer)
 org-refile-targets '(("~/org/test.org" :maxlevel . 2))
 org-mode-hook '(#[0 "\300\301\302\303\304$\207"
		   [add-hook change-major-mode-hook org-show-all append 
		   local]
		   5]
		 #[0 "\300\301\302\303\304$\207"
		   [add-hook change-major-mode-hook 
		   org-babel-show-result-all
		    append local]
		   5]
		 org-babel-result-hide-spec org-babel-hide-all-hashes
		 org-eldoc-load)
 org-outline-path-complete-in-steps nil
 org-archive-hook '(org-attach-archive-delete-maybe)
 org-confirm-elisp-link-function 'yes-or-no-p
 org-agenda-before-write-hook '(org-agenda-add-entry-text)
 org-metaup-hook '(org-babel-load-in-session-maybe)
 org-bibtex-headline-format-function #[257 "\300\236A\207" [:title] 3 
 "\n\n(fn ENTRY)"]
 org-babel-pre-tangle-hook '(save-buffer)
 org-tab-first-hook '(org-babel-hide-result-toggle-maybe
		      org-babel-header-arg-expand)
 org-agenda-loop-over-headlines-in-active-region nil
 org-src-lang-modes '(("arduino" . arduino) ("redis" . redis) ("php" 
 . php)
		      ("C" . c) ("C++" . c++) ("asymptote" . asy)
		      ("bash" . sh) ("beamer" . latex) ("calc" 
		      . fundamental)
		      ("cpp" . c++) ("ditaa" . artist) ("dot" 
		      . fundamental)
		      ("elisp" . emacs-lisp) ("ocaml" . tuareg)
		      ("screen" . shell-script) ("shell" . sh)
		      ("sqlite" . sql))
 org-occur-hook '(org-first-headline-recenter)
 org-cycle-hook '(org-cycle-hide-archived-subtrees 
 org-cycle-hide-drawers
		  org-cycle-show-empty-lines
		  org-optimize-window-after-visibility-change)
 org-speed-command-hook '(org-speed-command-activate
			  org-babel-speed-command-activate)
 org-refile-use-outline-path 'file
 org-export-before-parsing-hook '(org-attach-expand-links)
 org-confirm-shell-link-function 'yes-or-no-p
 org-link-parameters '(("attachment" :follow org-attach-follow :complete
			org-attach-complete-link)
		       ("id" :follow org-id-open)
		       ("eww" :follow org-eww-open :store 
		       org-eww-store-link)
		       ("rmail" :follow org-rmail-open :store
			org-rmail-store-link)
		       ("mhe" :follow org-mhe-open :store 
		       org-mhe-store-link)
		       ("irc" :follow org-irc-visit :store 
		       org-irc-store-link
			:export org-irc-export)
		       ("info" :follow org-info-open :export 
		       org-info-export
			:store org-info-store-link)
		       ("gnus" :follow org-gnus-open :store
			org-gnus-store-link)
		       ("docview" :follow org-docview-open :export
			org-docview-export :store 
			org-docview-store-link)
		       ("bibtex" :follow org-bibtex-open :store
			org-bibtex-store-link)
		       ("bbdb" :follow org-bbdb-open :export 
		       org-bbdb-export
			:complete org-bbdb-complete-link :store
			org-bbdb-store-link)
		       ("w3m" :store org-w3m-store-link) ("file+sys")
		       ("file+emacs") ("shell" :follow 
		       org-link--open-shell)
		       ("news" :follow
			#[514 "\301\300\302Q\"\207"
			  ["news" browse-url ":"] 6 "\n\n(fn URL ARG)"]
			)
		       ("mailto" :follow
			#[514 "\301\300\302Q\"\207"
			  ["mailto" browse-url ":"] 6 "\n\n(fn URL 
			  ARG)"]
			)
		       ("https" :follow
			#[514 "\301\300\302Q\"\207"
			  ["https" browse-url ":"] 6 "\n\n(fn URL ARG)"]
			)
		       ("http" :follow
			#[514 "\301\300\302Q\"\207"
			  ["http" browse-url ":"] 6 "\n\n(fn URL ARG)"]
			)
		       ("ftp" :follow
			#[514 "\301\300\302Q\"\207" ["ftp" browse-url 
                         ":"]
			  6 "\n\n(fn URL ARG)"]
			)
		       ("help" :follow org-link--open-help)
		       ("file" :complete org-link-complete-file)
		       ("elisp" :follow org-link--open-elisp)
		       ("doi" :follow org-link--open-doi))
 org-link-elisp-confirm-function 'yes-or-no-p
 )


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2020-09-21 18:34 Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)] Gustavo Barros
@ 2020-09-23 14:17 ` stardiviner
  2020-09-23 14:49   ` Gustavo Barros
  2021-02-14 12:55 ` Gustavo Barros
  2021-05-04  6:27 ` Bastien
  2 siblings, 1 reply; 11+ messages in thread
From: stardiviner @ 2020-09-23 14:17 UTC (permalink / raw)
  To: Gustavo Barros; +Cc: emacs-orgmode


I have same issue when using Ivy. But can't reproduce this by disabled ivy-mode.
And only happened when I refiled once, then the target will has two slash like this:

#+begin_example
Tasks/kk//   (file.org)
Tasks/hello/ (file.org)
#+end_example

Gustavo Barros <gusbrs.2016@gmail.com> writes:

> Hi All,
>
> some time ago, I've reported an issue regarding duplicity of the default 
> candidate in `org-refile' 
> (https://orgmode.org/list/87lftw1k2n.fsf@gmail.com/).  The problem was 
> that, when using `org-refile-use-outline-path' an "extra" slash was 
> appended at the end of every path, but candidates were stored in 
> `org-refile-history' without that extra slash.  Bastien took care of 
> that and indeed changed things so as to pass the elements to 
> `org-refile-history' with the trailing slash as appropriate.
>
> At the time, I reported a difference of behavior between 
> `completing-read-default' and `ivy-completing-read' after the mentioned 
> commit by Bastien.  But the issue did not appear for Bastien, which does 
> not use Ivy, and I also was not able to diagnose the problem properly. 
> I felt I didn't have enough to offer as to insist, so I resorted to an 
> old hack of mine.  But the new release this week (thank you very much!, 
> btw) has bitten me again on this, so I went back to some digging, and 
> hopefully I can do a better job this time in diagnosing the problem and 
> suggesting a fix.
>
>
> An ECM to reproduce the issue as it currently stands is:
>
> - Start 'emacs -Q'
>
> - Do an initial setup:
>   #+begin_src emacs-lisp
>   (add-to-list 'load-path "~/.emacs.d/elpa/org-plus-contrib-20200921")
>   (add-to-list 'load-path "~/.emacs.d/elpa/ivy-20200826.955")
>   ;; Those are the latest Org weekly build (Org 9.4) and the current up 
>   to date
>   ;; Ivy at Melpa
>   (setq org-refile-targets '(("~/org/test.org" :maxlevel . 2)))
>   (setq org-refile-use-outline-path 'file)
>   (setq org-outline-path-complete-in-steps nil)
>   (require 'ivy)
>   (ivy-mode)
>   ;; Bear with me, the problem is not with Ivy, I'll demonstrate that.
>   #+end_src
>
> - Open file "~/org/test.org", with contents:
>   #+begin_src org
>   ,* Top heading 1
>   ,* Top heading 2
>   ,** Entry 1
>   ,** Entry 2
>   #+end_src
>
> - Go to heading "Entry 1", refile it to "Top heading 1"
>
> - Go to heading "Entry 2", and call `org-refile'
>
> - Observe the available candidates, and notice "test.org/Top heading 1/" 
>   is there twice, once as the default candidate, with a *double* 
>   trailing slash, and also on the paths list, with a single trailing 
>   slash.
>
>
> I've tried to pin down what is going on here and my understanding is 
> that Bastien's fix on that previous thread did indeed correct the 
> problem of the missing trailing slash in `org-refile-history' and this 
> indeed corresponds correctly to the state of the completion "collection" 
> (the let bound `tbl' variable in `org-refile-get-location'), as it 
> should.  But there remained a couple of instances in 
> `org-refile-get-location' which added the trailing slash considering 
> `org-refile-history' didn't have them, so that when this is done, we get 
> a double trailing slash.
>
> The two instances are: 1) when the completion function is actually 
> called:
>
> #+begin_src emacs-lisp
> (setq answ (funcall cfunc prompt tbl nil (not new-nodes)
> 			    nil 'org-refile-history
> 			    (or cdef (concat (car org-refile-history) 
> 			    extra))))
> #+end_src
>
> 2) And three lines bellow, on the let binding:
>
> #+begin_src emacs-lisp
> (let* ((last-refile-loc (car org-refile-history))
>        (last-refile-loc-path (concat last-refile-loc extra)))
>   ...)
> #+end_src
>
> In both instances, we are getting the `car' of `org-refile-history' 
> which now already has `extra' (that is, the trailing slash) and adding 
> it again.
>
> My suggested fix is to remove these `extra's in duplicity, they are 
> remnants of when `org-refile-history' didn't have them already.  That 
> is:
>
> #+begin_src emacs-lisp
> (setq answ (funcall cfunc prompt tbl nil (not new-nodes)
> 			    nil 'org-refile-history
> 			    (or cdef (car org-refile-history))))
> #+end_src
>
> And:
>
> #+begin_src emacs-lisp
> (let* ((last-refile-loc (car org-refile-history))
>        (last-refile-loc-path last-refile-loc))
>   ...)
> #+end_src
>
> Of course, the second opens some opportunity to simplify the code that 
> follows considering `last-refile-loc-path' and `last-refile-loc' are now 
> identical.
>
>
> Why I think this is the problem and the correct way to fix it:
>
> 1) If you add inspection points at the appropriate locations for the 
> sexps =(concat (car org-refile-history) extra)= and =(concat 
> last-refile-loc extra)= you will find the double trailing slash there, 
> and it shouldn't be there.
>
> 2) The visual manifestation of this double trailing slash in the default 
> candidate with `ivy-mode' is there therefore independently of 
> `ivy-mode`.  Indeed, `ivy-mode' basically sets 
> `completing-read-function' to `ivy-completing-read', which in turn calls 
> its main API function `ivy-read'.  `ivy-completing-read' just passes 
> along the `def' argument without manipulating it.  As far as I can see, 
> `ivy-read' also does not change it before calling 
> `read-from-minibuffer'.
>
> Why `completing-read-default' does not show this second trailing slash 
> of the `def' argument it received is another matter.  But it's there...
>
> 3) I've tested this suggested fix in the following scenarios (starting 
> from the ECM above):
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path 'file)
> (setq org-outline-path-complete-in-steps nil)
> ;; (ivy-mode)
> #+end_src
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path 'file)
> (setq org-outline-path-complete-in-steps nil)
> (ivy-mode)
> #+end_src
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path 'file)
> (setq org-outline-path-complete-in-steps t)
> ;; `org-olpath-completing-read' does not play well with `ivy-mode' so no 
>    need
> ;; to use it here
> #+end_src
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path nil)
> (ivy-mode)
> #+end_src
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path nil)
> ;; (ivy-mode)
> #+end_src
>
> Other non-nil values of `org-refile-use-outline-path' should be 
> equivalent to `file' with respect to the issue at hand, so they are 
> implicitly covered.
>
> Having tested these cases, to the best of my knowledge, they all work as 
> expected.  Though I'm admittedly less acquainted with =(setq 
> org-refile-use-outline-path nil)= and =(setq 
> org-outline-path-complete-in-steps t)=.
>
> All tests and ECM were ran with "Org mode version 9.4 
> (9.4-7-g3eccc5-elpaplus @ 
> /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)" and "GNU Emacs 
> 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 
> 1.16.0) of 2020-08-11".
>
> I hope this is a more useful report than last time.
>
>
> Best,
> Gustavo.
>
>
>
>
>
> Emacs  : GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 
> 3.24.20, cairo version 1.16.0)
>  of 2020-08-11
> Package: Org mode version 9.4 (9.4-7-g3eccc5-elpaplus @ 
> /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)
>
> current state:
> ==============
> (setq
>  org-src-mode-hook '(org-src-babel-configure-edit-buffer
> 		     org-src-mode-configure-edit-buffer)
>  org-link-shell-confirm-function 'yes-or-no-p
>  org-metadown-hook '(org-babel-pop-to-session-maybe)
>  org-clock-out-hook '(org-clock-remove-empty-clock-drawer)
>  org-refile-targets '(("~/org/test.org" :maxlevel . 2))
>  org-mode-hook '(#[0 "\300\301\302\303\304$\207"
> 		   [add-hook change-major-mode-hook org-show-all append 
> 		   local]
> 		   5]
> 		 #[0 "\300\301\302\303\304$\207"
> 		   [add-hook change-major-mode-hook 
> 		   org-babel-show-result-all
> 		    append local]
> 		   5]
> 		 org-babel-result-hide-spec org-babel-hide-all-hashes
> 		 org-eldoc-load)
>  org-outline-path-complete-in-steps nil
>  org-archive-hook '(org-attach-archive-delete-maybe)
>  org-confirm-elisp-link-function 'yes-or-no-p
>  org-agenda-before-write-hook '(org-agenda-add-entry-text)
>  org-metaup-hook '(org-babel-load-in-session-maybe)
>  org-bibtex-headline-format-function #[257 "\300\236A\207" [:title] 3 
>  "\n\n(fn ENTRY)"]
>  org-babel-pre-tangle-hook '(save-buffer)
>  org-tab-first-hook '(org-babel-hide-result-toggle-maybe
> 		      org-babel-header-arg-expand)
>  org-agenda-loop-over-headlines-in-active-region nil
>  org-src-lang-modes '(("arduino" . arduino) ("redis" . redis) ("php" 
>  . php)
> 		      ("C" . c) ("C++" . c++) ("asymptote" . asy)
> 		      ("bash" . sh) ("beamer" . latex) ("calc" 
> 		      . fundamental)
> 		      ("cpp" . c++) ("ditaa" . artist) ("dot" 
> 		      . fundamental)
> 		      ("elisp" . emacs-lisp) ("ocaml" . tuareg)
> 		      ("screen" . shell-script) ("shell" . sh)
> 		      ("sqlite" . sql))
>  org-occur-hook '(org-first-headline-recenter)
>  org-cycle-hook '(org-cycle-hide-archived-subtrees 
>  org-cycle-hide-drawers
> 		  org-cycle-show-empty-lines
> 		  org-optimize-window-after-visibility-change)
>  org-speed-command-hook '(org-speed-command-activate
> 			  org-babel-speed-command-activate)
>  org-refile-use-outline-path 'file
>  org-export-before-parsing-hook '(org-attach-expand-links)
>  org-confirm-shell-link-function 'yes-or-no-p
>  org-link-parameters '(("attachment" :follow org-attach-follow :complete
> 			org-attach-complete-link)
> 		       ("id" :follow org-id-open)
> 		       ("eww" :follow org-eww-open :store 
> 		       org-eww-store-link)
> 		       ("rmail" :follow org-rmail-open :store
> 			org-rmail-store-link)
> 		       ("mhe" :follow org-mhe-open :store 
> 		       org-mhe-store-link)
> 		       ("irc" :follow org-irc-visit :store 
> 		       org-irc-store-link
> 			:export org-irc-export)
> 		       ("info" :follow org-info-open :export 
> 		       org-info-export
> 			:store org-info-store-link)
> 		       ("gnus" :follow org-gnus-open :store
> 			org-gnus-store-link)
> 		       ("docview" :follow org-docview-open :export
> 			org-docview-export :store 
> 			org-docview-store-link)
> 		       ("bibtex" :follow org-bibtex-open :store
> 			org-bibtex-store-link)
> 		       ("bbdb" :follow org-bbdb-open :export 
> 		       org-bbdb-export
> 			:complete org-bbdb-complete-link :store
> 			org-bbdb-store-link)
> 		       ("w3m" :store org-w3m-store-link) ("file+sys")
> 		       ("file+emacs") ("shell" :follow 
> 		       org-link--open-shell)
> 		       ("news" :follow
> 			#[514 "\301\300\302Q\"\207"
> 			  ["news" browse-url ":"] 6 "\n\n(fn URL ARG)"]
> 			)
> 		       ("mailto" :follow
> 			#[514 "\301\300\302Q\"\207"
> 			  ["mailto" browse-url ":"] 6 "\n\n(fn URL 
> 			  ARG)"]
> 			)
> 		       ("https" :follow
> 			#[514 "\301\300\302Q\"\207"
> 			  ["https" browse-url ":"] 6 "\n\n(fn URL ARG)"]
> 			)
> 		       ("http" :follow
> 			#[514 "\301\300\302Q\"\207"
> 			  ["http" browse-url ":"] 6 "\n\n(fn URL ARG)"]
> 			)
> 		       ("ftp" :follow
> 			#[514 "\301\300\302Q\"\207" ["ftp" browse-url 
>                          ":"]
> 			  6 "\n\n(fn URL ARG)"]
> 			)
> 		       ("help" :follow org-link--open-help)
> 		       ("file" :complete org-link-complete-file)
> 		       ("elisp" :follow org-link--open-elisp)
> 		       ("doi" :follow org-link--open-doi))
>  org-link-elisp-confirm-function 'yes-or-no-p
>  )


-- 
[ stardiviner ]
       I try to make every word tell the meaning that I want to express.

       Blog: https://stardiviner.github.io/
       IRC(freenode): stardiviner, Matrix: stardiviner
       GPG: F09F650D7D674819892591401B5DF1C95AE89AC3


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2020-09-23 14:17 ` stardiviner
@ 2020-09-23 14:49   ` Gustavo Barros
  0 siblings, 0 replies; 11+ messages in thread
From: Gustavo Barros @ 2020-09-23 14:49 UTC (permalink / raw)
  To: numbchild; +Cc: emacs-orgmode

Hi stardiviner,

On Wed, 23 Sep 2020 at 11:17, stardiviner <numbchild@gmail.com> wrote:

> I have same issue when using Ivy. But can't reproduce this by disabled 
> ivy-mode.
> And only happened when I refiled once, then the target will has two 
> slash like this:
>
> #+begin_example
> Tasks/kk//   (file.org)
> Tasks/hello/ (file.org)
> #+end_example
>

Thank you for checking this and for confirming you also observe this 
behavior with Ivy.

Regarding when ivy-mode is off, as I argued in the original report, the 
problem is indeed *not visible* when using `completing-read-default', 
the double trailing slash is nevertheless there if you inspect the value 
being passed as default value to the completing-read-function.  The 
tests included with ivy-mode turned off were meant to emphasize things 
are expected to work with the suggested fix also in this case, in other 
words, that this is not "just a fix for ivy-mode", which is not the 
problem here.

Best,
Gustavo.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2020-09-21 18:34 Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)] Gustavo Barros
  2020-09-23 14:17 ` stardiviner
@ 2021-02-14 12:55 ` Gustavo Barros
  2021-05-04  6:27 ` Bastien
  2 siblings, 0 replies; 11+ messages in thread
From: Gustavo Barros @ 2021-02-14 12:55 UTC (permalink / raw)
  To: emacs-orgmode

Hi All,

I'd like to kindly bump this report.
It is an issue which has been around for some time.  The report provides 
a clear reproduction recipe, which stardiviner was able to reproduce, 
and still affects current "org-plus-contrib-20210208".  The report also 
provides a hopefully convincing code analysis of the affected function 
`org-refile-get-location' and a suggested fix (I just don't send a 
patch, because I can't provide CA).

Best regards,
Gustavo.


On Mon, 21 Sep 2020 at 15:34, Gustavo Barros <gusbrs.2016@gmail.com> 
wrote:

> Hi All,
>
> some time ago, I've reported an issue regarding duplicity of the 
> default
> candidate in `org-refile' 
> (https://orgmode.org/list/87lftw1k2n.fsf@gmail.com/).  The problem was 
> that,
> when using `org-refile-use-outline-path' an "extra" slash was appended 
> at the
> end of every path, but candidates were stored in `org-refile-history' 
> without
> that extra slash.  Bastien took care of that and indeed changed things 
> so as
> to pass the elements to `org-refile-history' with the trailing slash 
> as
> appropriate.
>
> At the time, I reported a difference of behavior between
> `completing-read-default' and `ivy-completing-read' after the 
> mentioned 
> commit by Bastien.  But the issue did not appear for Bastien, which 
> does not
> use Ivy, and I also was not able to diagnose the problem properly. I 
> felt I
> didn't have enough to offer as to insist, so I resorted to an old hack 
> of
> mine.  But the new release this week (thank you very much!, btw) has 
> bitten me
> again on this, so I went back to some digging, and hopefully I can do 
> a better
> job this time in diagnosing the problem and suggesting a fix.
>
>
> An ECM to reproduce the issue as it currently stands is:
>
> - Start 'emacs -Q'
>
> - Do an initial setup:
>  #+begin_src emacs-lisp
>  (add-to-list 'load-path "~/.emacs.d/elpa/org-plus-contrib-20200921")
>  (add-to-list 'load-path "~/.emacs.d/elpa/ivy-20200826.955")
>  ;; Those are the latest Org weekly build (Org 9.4) and the current up 
>  to
>    date
>  ;; Ivy at Melpa
>  (setq org-refile-targets '(("~/org/test.org" :maxlevel . 2)))
>  (setq org-refile-use-outline-path 'file)
>  (setq org-outline-path-complete-in-steps nil)
>  (require 'ivy)
>  (ivy-mode)
>  ;; Bear with me, the problem is not with Ivy, I'll demonstrate that.
>  #+end_src
>
> - Open file "~/org/test.org", with contents:
>  #+begin_src org
>  ,* Top heading 1
>  ,* Top heading 2
>  ,** Entry 1
>  ,** Entry 2
>  #+end_src
>
> - Go to heading "Entry 1", refile it to "Top heading 1"
>
> - Go to heading "Entry 2", and call `org-refile'
>
> - Observe the available candidates, and notice "test.org/Top heading 
> 1/"   is
>  there twice, once as the default candidate, with a *double* 
>  trailing slash,
> and also on the paths list, with a single trailing   slash.
>
>
> I've tried to pin down what is going on here and my understanding is 
> that
> Bastien's fix on that previous thread did indeed correct the problem 
> of the
> missing trailing slash in `org-refile-history' and this indeed 
> corresponds
> correctly to the state of the completion "collection" (the let bound 
> `tbl'
> variable in `org-refile-get-location'), as it should.  But there 
> remained a
> couple of instances in `org-refile-get-location' which added the 
> trailing
> slash considering `org-refile-history' didn't have them, so that when 
> this is
> done, we get a double trailing slash.
>
> The two instances are: 1) when the completion function is actually 
> called:
>
> #+begin_src emacs-lisp
> (setq answ (funcall cfunc prompt tbl nil (not new-nodes)
> 			    nil 'org-refile-history
> 			    (or cdef (concat (car org-refile-history)
> 			    extra))))
> #+end_src
>
> 2) And three lines bellow, on the let binding:
>
> #+begin_src emacs-lisp
> (let* ((last-refile-loc (car org-refile-history))
>       (last-refile-loc-path (concat last-refile-loc extra)))
>  ...)
> #+end_src
>
> In both instances, we are getting the `car' of `org-refile-history' 
> which now
> already has `extra' (that is, the trailing slash) and adding it again.
>
> My suggested fix is to remove these `extra's in duplicity, they are 
> remnants
> of when `org-refile-history' didn't have them already.  That is:
>
> #+begin_src emacs-lisp
> (setq answ (funcall cfunc prompt tbl nil (not new-nodes)
> 			    nil 'org-refile-history
> 			    (or cdef (car org-refile-history))))
> #+end_src
>
> And:
>
> #+begin_src emacs-lisp
> (let* ((last-refile-loc (car org-refile-history))
>       (last-refile-loc-path last-refile-loc))
>  ...)
> #+end_src
>
> Of course, the second opens some opportunity to simplify the code that 
> follows
> considering `last-refile-loc-path' and `last-refile-loc' are now 
> identical.
>
>
> Why I think this is the problem and the correct way to fix it:
>
> 1) If you add inspection points at the appropriate locations for the 
> sexps
> =(concat (car org-refile-history) extra)= and =(concat last-refile-loc 
> extra)=
> you will find the double trailing slash there, and it shouldn't be 
> there.
>
> 2) The visual manifestation of this double trailing slash in the 
> default
> candidate with `ivy-mode' is there therefore independently of 
> `ivy-mode`.  Indeed, `ivy-mode' basically sets 
> `completing-read-function' to
> `ivy-completing-read', which in turn calls its main API function 
> `ivy-read'.
> `ivy-completing-read' just passes along the `def' argument without
> manipulating it.  As far as I can see, `ivy-read' also does not change 
> it
> before calling `read-from-minibuffer'.
>
> Why `completing-read-default' does not show this second trailing slash 
> of the
> `def' argument it received is another matter.  But it's there...
>
> 3) I've tested this suggested fix in the following scenarios (starting 
> from
> the ECM above):
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path 'file)
> (setq org-outline-path-complete-in-steps nil)
> ;; (ivy-mode)
> #+end_src
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path 'file)
> (setq org-outline-path-complete-in-steps nil)
> (ivy-mode)
> #+end_src
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path 'file)
> (setq org-outline-path-complete-in-steps t)
> ;; `org-olpath-completing-read' does not play well with `ivy-mode' so 
> no
>    need
> ;; to use it here
> #+end_src
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path nil)
> (ivy-mode)
> #+end_src
>
> #+begin_src emacs-lisp
> (setq org-refile-use-outline-path nil)
> ;; (ivy-mode)
> #+end_src
>
> Other non-nil values of `org-refile-use-outline-path' should be 
> equivalent to
> `file' with respect to the issue at hand, so they are implicitly 
> covered.
>
> Having tested these cases, to the best of my knowledge, they all work 
> as
> expected.  Though I'm admittedly less acquainted with =(setq 
> org-refile-use-outline-path nil)= and =(setq
> org-outline-path-complete-in-steps t)=.
>
> All tests and ECM were ran with "Org mode version 9.4 
> (9.4-7-g3eccc5-elpaplus
> @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)" and "GNU 
> Emacs 
> 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo 
> version
> 1.16.0) of 2020-08-11".
>
> I hope this is a more useful report than last time.
>
>
> Best,
> Gustavo.
>
>
>
>
>
> Emacs  : GNU Emacs 27.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 
> 3.24.20,
> cairo version 1.16.0)
> of 2020-08-11
> Package: Org mode version 9.4 (9.4-7-g3eccc5-elpaplus @
> /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)
>
> current state:
> ==============
> (setq
> org-src-mode-hook '(org-src-babel-configure-edit-buffer
> 		     org-src-mode-configure-edit-buffer)
> org-link-shell-confirm-function 'yes-or-no-p
> org-metadown-hook '(org-babel-pop-to-session-maybe)
> org-clock-out-hook '(org-clock-remove-empty-clock-drawer)
> org-refile-targets '(("~/org/test.org" :maxlevel . 2))
> org-mode-hook '(#[0 "\300\301\302\303\304$\207"
> 		   [add-hook change-major-mode-hook org-show-all append
> 		   local]
> 		   5]
> 		 #[0 "\300\301\302\303\304$\207"
> 		   [add-hook change-major-mode-hook
> 		   org-babel-show-result-all
> 		    append local]
> 		   5]
> 		 org-babel-result-hide-spec org-babel-hide-all-hashes
> 		 org-eldoc-load)
> org-outline-path-complete-in-steps nil
> org-archive-hook '(org-attach-archive-delete-maybe)
> org-confirm-elisp-link-function 'yes-or-no-p
> org-agenda-before-write-hook '(org-agenda-add-entry-text)
> org-metaup-hook '(org-babel-load-in-session-maybe)
> org-bibtex-headline-format-function #[257 "\300\236A\207" [:title] 3 
> "\n\n(fn
> ENTRY)"]
> org-babel-pre-tangle-hook '(save-buffer)
> org-tab-first-hook '(org-babel-hide-result-toggle-maybe
> 		      org-babel-header-arg-expand)
> org-agenda-loop-over-headlines-in-active-region nil
> org-src-lang-modes '(("arduino" . arduino) ("redis" . redis) ("php" 
> . php)
> 		      ("C" . c) ("C++" . c++) ("asymptote" . asy)
> 		      ("bash" . sh) ("beamer" . latex) ("calc"
> 		      . fundamental)
> 		      ("cpp" . c++) ("ditaa" . artist) ("dot"
> 		      . fundamental)
> 		      ("elisp" . emacs-lisp) ("ocaml" . tuareg)
> 		      ("screen" . shell-script) ("shell" . sh)
> 		      ("sqlite" . sql))
> org-occur-hook '(org-first-headline-recenter)
> org-cycle-hook '(org-cycle-hide-archived-subtrees 
> org-cycle-hide-drawers
> 		  org-cycle-show-empty-lines
> 		  org-optimize-window-after-visibility-change)
> org-speed-command-hook '(org-speed-command-activate
> 			  org-babel-speed-command-activate)
> org-refile-use-outline-path 'file
> org-export-before-parsing-hook '(org-attach-expand-links)
> org-confirm-shell-link-function 'yes-or-no-p
> org-link-parameters '(("attachment" :follow org-attach-follow 
> :complete
> 			org-attach-complete-link)
> 		       ("id" :follow org-id-open)
> 		       ("eww" :follow org-eww-open :store
> 		       org-eww-store-link)
> 		       ("rmail" :follow org-rmail-open :store
> 			org-rmail-store-link)
> 		       ("mhe" :follow org-mhe-open :store
> 		       org-mhe-store-link)
> 		       ("irc" :follow org-irc-visit :store
> 		       org-irc-store-link
> 			:export org-irc-export)
> 		       ("info" :follow org-info-open :export
> 		       org-info-export
> 			:store org-info-store-link)
> 		       ("gnus" :follow org-gnus-open :store
> 			org-gnus-store-link)
> 		       ("docview" :follow org-docview-open :export
> 			org-docview-export :store
> 			org-docview-store-link)
> 		       ("bibtex" :follow org-bibtex-open :store
> 			org-bibtex-store-link)
> 		       ("bbdb" :follow org-bbdb-open :export
> 		       org-bbdb-export
> 			:complete org-bbdb-complete-link :store
> 			org-bbdb-store-link)
> 		       ("w3m" :store org-w3m-store-link) ("file+sys")
> 		       ("file+emacs") ("shell" :follow
> 		       org-link--open-shell)
> 		       ("news" :follow
> 			#[514 "\301\300\302Q\"\207"
> 			  ["news" browse-url ":"] 6 "\n\n(fn URL ARG)"]
> 			)
> 		       ("mailto" :follow
> 			#[514 "\301\300\302Q\"\207"
> 			  ["mailto" browse-url ":"] 6 "\n\n(fn URL
> 			  ARG)"]
> 			)
> 		       ("https" :follow
> 			#[514 "\301\300\302Q\"\207"
> 			  ["https" browse-url ":"] 6 "\n\n(fn URL ARG)"]
> 			)
> 		       ("http" :follow
> 			#[514 "\301\300\302Q\"\207"
> 			  ["http" browse-url ":"] 6 "\n\n(fn URL ARG)"]
> 			)
> 		       ("ftp" :follow
> 			#[514 "\301\300\302Q\"\207" ["ftp" browse-url
>                          ":"]
> 			  6 "\n\n(fn URL ARG)"]
> 			)
> 		       ("help" :follow org-link--open-help)
> 		       ("file" :complete org-link-complete-file)
> 		       ("elisp" :follow org-link--open-elisp)
> 		       ("doi" :follow org-link--open-doi))
> org-link-elisp-confirm-function 'yes-or-no-p
> )



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2020-09-21 18:34 Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)] Gustavo Barros
  2020-09-23 14:17 ` stardiviner
  2021-02-14 12:55 ` Gustavo Barros
@ 2021-05-04  6:27 ` Bastien
  2 siblings, 0 replies; 11+ messages in thread
From: Bastien @ 2021-05-04  6:27 UTC (permalink / raw)
  To: Gustavo Barros; +Cc: emacs-orgmode

Hi Gustavo,

Gustavo Barros <gusbrs.2016@gmail.com> writes:

> some time ago, I've reported an issue regarding duplicity of the default 
> candidate in `org-refile' 
> (https://orgmode.org/list/87lftw1k2n.fsf@gmail.com/).  The problem was 
> that, when using `org-refile-use-outline-path' an "extra" slash was 
> appended at the end of every path, but candidates were stored in 
> `org-refile-history' without that extra slash.  Bastien took care of 
> that and indeed changed things so as to pass the elements to 
> `org-refile-history' with the trailing slash as appropriate.
>
> At the time, I reported a difference of behavior between 
> `completing-read-default' and `ivy-completing-read' after the mentioned 
> commit by Bastien.  But the issue did not appear for Bastien, which does 
> not use Ivy, and I also was not able to diagnose the problem properly. 
> I felt I didn't have enough to offer as to insist, so I resorted to an 
> old hack of mine.  But the new release this week (thank you very much!, 
> btw) has bitten me again on this, so I went back to some digging, and 
> hopefully I can do a better job this time in diagnosing the problem and 
> suggesting a fix.

Bump this thread so that it appears on updates.orgmode.org.

Thanks,

-- 
 Bastien


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
@ 2021-05-10 18:04 Bhavin Gandhi
  2021-05-23 18:05 ` Bhavin Gandhi
  0 siblings, 1 reply; 11+ messages in thread
From: Bhavin Gandhi @ 2021-05-10 18:04 UTC (permalink / raw)
  To: bzg; +Cc: emacs-orgmode

[-- Attachment #1: Type: text/plain, Size: 222 bytes --]

Hello Bastien

I'm new to Org mode codebase as well as Elisp. Is this something I can pick
up, I was not sure that's why I thought of asking here on the list.

--
Regards,
Bhavin Gandhi (bhavin192) | https://geeksocket.in

[-- Attachment #2: Type: text/html, Size: 346 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2021-05-10 18:04 Bhavin Gandhi
@ 2021-05-23 18:05 ` Bhavin Gandhi
  2021-05-23 18:23   ` Timothy
  2021-05-23 18:58   ` Gustavo Barros
  0 siblings, 2 replies; 11+ messages in thread
From: Bhavin Gandhi @ 2021-05-23 18:05 UTC (permalink / raw)
  To: gusbrs.2016; +Cc: emacs-orgmode


[-- Attachment #1.1: Type: text/plain, Size: 440 bytes --]

Finally after spending a couple of hours, I was able to understand the code
of org-refile-get-location \o/. The detailed bug report helped me to
understand the issue. I'm attaching a patch here which should fix the
problem, it has other details as well. I have tested a few basic scenarios
as mentioned in the report.

OTOH, I haven't signed the FSF Copyright assignment yet, should I do it
before this gets merged or it can be done later?

[-- Attachment #1.2: Type: text/html, Size: 534 bytes --]

[-- Attachment #2: 0001-org-refile-Fix-double-slashes-in-the-refile-targets.patch --]
[-- Type: text/x-patch, Size: 2126 bytes --]

From 234316ed49023362d116d884ba7f2859e5f04c1b Mon Sep 17 00:00:00 2001
From: Bhavin Gandhi <bhavin192@geeksocket.in>
Date: Sun, 23 May 2021 23:07:13 +0530
Subject: [PATCH] org-refile: Fix double slashes in the refile targets

* org-refile.el (org-refile-get-location): When we generate the `tbl'
variable, we add extra slash depending on the value of
`org-refile-use-outline-path'.  This patch updates some locations which
add another extra slash assuming the target did not have it.

`org-refile--get-location' does lookup for entries with and without
slash, so it was not causing any issues before.  It works as it is now
as well.

Thanks to Gustavo Barros for a very detailed bug report.

TINYCHANGE
---
 lisp/org-refile.el | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lisp/org-refile.el b/lisp/org-refile.el
index bffad0a81..c4ac1c108 100644
--- a/lisp/org-refile.el
+++ b/lisp/org-refile.el
@@ -649,20 +649,18 @@ this function appends the default value from
     (setq old-hist org-refile-history)
     (setq answ (funcall cfunc prompt tbl nil (not new-nodes)
 			nil 'org-refile-history
-			(or cdef (concat (car org-refile-history) extra))))
+			(or cdef (car org-refile-history))))
     (if (setq pa (org-refile--get-location answ tbl))
-	(let* ((last-refile-loc (car org-refile-history))
-	       (last-refile-loc-path (concat last-refile-loc extra)))
+	(let* ((last-refile-loc (car org-refile-history)))
 	  (org-refile-check-position pa)
 	  (when (or (not org-refile-history)
 		    (not (eq old-hist org-refile-history))
-		    (not (equal (car pa) last-refile-loc-path)))
+		    (not (equal (car pa) last-refile-loc)))
 	    (setq org-refile-history
 		  (cons (car pa) (if (assoc last-refile-loc tbl)
 				     org-refile-history
 				   (cdr org-refile-history))))
-	    (when (or (equal last-refile-loc-path (nth 1 org-refile-history))
-		      (equal last-refile-loc (nth 1 org-refile-history)))
+	    (when (equal last-refile-loc (nth 1 org-refile-history))
 	      (pop org-refile-history)))
 	  pa)
       (if (string-match "\\`\\(.*\\)/\\([^/]+\\)\\'" answ)
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2021-05-23 18:05 ` Bhavin Gandhi
@ 2021-05-23 18:23   ` Timothy
  2021-05-23 18:58   ` Gustavo Barros
  1 sibling, 0 replies; 11+ messages in thread
From: Timothy @ 2021-05-23 18:23 UTC (permalink / raw)
  To: Bhavin Gandhi; +Cc: emacs-orgmode, gusbrs.2016


Hi Bhavin! Great to see you on the mailing list :)

Thank you so much for looking at the bugs, investigating one, /and/
working out a fix. That's absolutely fantastic of you!

For cumulative contributions under 15 (non-trivial) lines from a
contributor (such as yourself) we can accept (i.e. merge without FSF
papers) the changes and with a "TINYCHANGE" line at the end of the
commit message --- but I see you've noticed this and have a nicely
formatted commit message, brilliant!

Thanks again, and I hope to see you around,

Timothy.

p.s. For merging hopefully someone like Nicolas or Bastien will be able
to take a look at the commit. It looks good to me though :)

Bhavin Gandhi <bhavin7392@gmail.com> writes:

> Finally after spending a couple of hours, I was able to understand the code
> of org-refile-get-location \o/. The detailed bug report helped me to
> understand the issue. I'm attaching a patch here which should fix the
> problem, it has other details as well. I have tested a few basic scenarios
> as mentioned in the report.
>
> OTOH, I haven't signed the FSF Copyright assignment yet, should I do it
> before this gets merged or it can be done later?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2021-05-23 18:05 ` Bhavin Gandhi
  2021-05-23 18:23   ` Timothy
@ 2021-05-23 18:58   ` Gustavo Barros
  2021-05-24 16:34     ` Bhavin Gandhi
  1 sibling, 1 reply; 11+ messages in thread
From: Gustavo Barros @ 2021-05-23 18:58 UTC (permalink / raw)
  To: Bhavin Gandhi; +Cc: emacs-orgmode

Hi Bhavin,

On Sun, 23 May 2021 at 15:05, Bhavin Gandhi <bhavin7392@gmail.com> 
wrote:

> Finally after spending a couple of hours, I was able to understand the 
> code
> of org-refile-get-location \o/. The detailed bug report helped me to
> understand the issue. I'm attaching a patch here which should fix the
> problem, it has other details as well. I have tested a few basic 
> scenarios
> as mentioned in the report.

Thank you very much for working on this patch.  I couldn't offer it 
myself (out of my legal bounds) and had gone as far as I was allowed to 
here, so I'm happy you took it from there.

The patch looks good to me, and corresponds to my analysis of the 
problem and suggested fix.  I have only one minor nitpick: you could go 
with a simple `let' there, instead of a `let*', since we only have one 
let-bound variable there anyway.

About:

> [...] it has other details as well.

As far as I could see, we are very much aligned on the problem and fix. 
But perhaps I'm missing something, could you elaborate on that?

Thanks again.

Best,
Gustavo.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2021-05-23 18:58   ` Gustavo Barros
@ 2021-05-24 16:34     ` Bhavin Gandhi
  2021-05-24 16:58       ` Gustavo Barros
  0 siblings, 1 reply; 11+ messages in thread
From: Bhavin Gandhi @ 2021-05-24 16:34 UTC (permalink / raw)
  To: Gustavo Barros, tecosaur; +Cc: emacs-orgmode


[-- Attachment #1.1: Type: text/plain, Size: 1015 bytes --]

On Sun, 23 May 2021 at 23:53, Timothy wrote:
>
> Hi Bhavin! Great to see you on the mailing list :)
>
> Thank you so much for looking at the bugs, investigating one, /and/
> working out a fix. That's absolutely fantastic of you!

Thank you for a welcoming message! :)


On Mon, 24 May 2021 at 00:28, Gustavo Barros wrote:
>
> The patch looks good to me, and corresponds to my analysis of the
> problem and suggested fix.  I have only one minor nitpick: you could go
> with a simple `let' there, instead of a `let*', since we only have one
> let-bound variable there anyway.

Yes, I have attached an updated patch.

> > [...] it has other details as well.
>
> As far as I could see, we are very much aligned on the problem and fix.
> But perhaps I'm missing something, could you elaborate on that?

We are indeed aligned. The only additional thing I discovered was
the reason `org-refile--get-location' works despite having double
slashes. That was new for me.

Just experimenting with Woof!
X-Woof-Patch: confirmed

[-- Attachment #1.2: Type: text/html, Size: 1190 bytes --]

[-- Attachment #2: 0001-org-refile-Fix-double-slashes-in-the-refile-targets.patch --]
[-- Type: text/x-patch, Size: 2125 bytes --]

From e069c35ff6011a7f9efe372e675a0bc43ba1fa80 Mon Sep 17 00:00:00 2001
From: Bhavin Gandhi <bhavin192@geeksocket.in>
Date: Sun, 23 May 2021 23:07:13 +0530
Subject: [PATCH] org-refile: Fix double slashes in the refile targets

* org-refile.el (org-refile-get-location): When we generate the `tbl'
variable, we add extra slash depending on the value of
`org-refile-use-outline-path'.  This patch updates some locations which
add another extra slash assuming the target did not have it.

`org-refile--get-location' does lookup for entries with and without
slash, so it was not causing any issues before.  It works as it is now
as well.

Thanks to Gustavo Barros for a very detailed bug report.

TINYCHANGE
---
 lisp/org-refile.el | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/lisp/org-refile.el b/lisp/org-refile.el
index bffad0a81..678759e10 100644
--- a/lisp/org-refile.el
+++ b/lisp/org-refile.el
@@ -649,20 +649,18 @@ this function appends the default value from
     (setq old-hist org-refile-history)
     (setq answ (funcall cfunc prompt tbl nil (not new-nodes)
 			nil 'org-refile-history
-			(or cdef (concat (car org-refile-history) extra))))
+			(or cdef (car org-refile-history))))
     (if (setq pa (org-refile--get-location answ tbl))
-	(let* ((last-refile-loc (car org-refile-history))
-	       (last-refile-loc-path (concat last-refile-loc extra)))
+	(let ((last-refile-loc (car org-refile-history)))
 	  (org-refile-check-position pa)
 	  (when (or (not org-refile-history)
 		    (not (eq old-hist org-refile-history))
-		    (not (equal (car pa) last-refile-loc-path)))
+		    (not (equal (car pa) last-refile-loc)))
 	    (setq org-refile-history
 		  (cons (car pa) (if (assoc last-refile-loc tbl)
 				     org-refile-history
 				   (cdr org-refile-history))))
-	    (when (or (equal last-refile-loc-path (nth 1 org-refile-history))
-		      (equal last-refile-loc (nth 1 org-refile-history)))
+	    (when (equal last-refile-loc (nth 1 org-refile-history))
 	      (pop org-refile-history)))
 	  pa)
       (if (string-match "\\`\\(.*\\)/\\([^/]+\\)\\'" answ)
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)]
  2021-05-24 16:34     ` Bhavin Gandhi
@ 2021-05-24 16:58       ` Gustavo Barros
  0 siblings, 0 replies; 11+ messages in thread
From: Gustavo Barros @ 2021-05-24 16:58 UTC (permalink / raw)
  To: Bhavin Gandhi; +Cc: emacs-orgmode, tecosaur


On Mon, 24 May 2021 at 13:34, Bhavin Gandhi <bhavin7392@gmail.com> 
wrote:

>
> Yes, I have attached an updated patch.
>

Looks good to me. Thank you!

>
> We are indeed aligned. The only additional thing I discovered was
> the reason `org-refile--get-location' works despite having double
> slashes. That was new for me.
>

Great!  Thanks for clearing that up too.


Best,
Gustavo.


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-05-24 17:28 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-21 18:34 Bug: Double trailing slash for default candidate in org-refile-get-target [9.4 (9.4-7-g3eccc5-elpaplus @ /home/gustavo/.emacs.d/elpa/org-plus-contrib-20200921/)] Gustavo Barros
2020-09-23 14:17 ` stardiviner
2020-09-23 14:49   ` Gustavo Barros
2021-02-14 12:55 ` Gustavo Barros
2021-05-04  6:27 ` Bastien
  -- strict thread matches above, loose matches on Subject: below --
2021-05-10 18:04 Bhavin Gandhi
2021-05-23 18:05 ` Bhavin Gandhi
2021-05-23 18:23   ` Timothy
2021-05-23 18:58   ` Gustavo Barros
2021-05-24 16:34     ` Bhavin Gandhi
2021-05-24 16:58       ` Gustavo Barros

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).