* 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; 7+ 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] 7+ 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 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/)] 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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 2021-05-25 17:52 ` [PATCH] org-refile: Fix double slashes in the refile targets Bhavin Gandhi 0 siblings, 1 reply; 7+ 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] 7+ messages in thread
* [PATCH] org-refile: Fix double slashes in the refile targets 2021-05-24 16:58 ` Gustavo Barros @ 2021-05-25 17:52 ` Bhavin Gandhi 0 siblings, 0 replies; 7+ messages in thread From: Bhavin Gandhi @ 2021-05-25 17:52 UTC (permalink / raw) Cc: emacs-orgmode [-- Attachment #1: Type: text/plain, Size: 73 bytes --] Adding [PATCH] to the subject so that it appears on updates.orgmode.org. [-- Attachment #2: Type: text/html, Size: 140 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-05-25 17:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-05-10 18:04 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/)] 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 2021-05-25 17:52 ` [PATCH] org-refile: Fix double slashes in the refile targets Bhavin Gandhi
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).