[-- 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 --]
[-- 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
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?
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.
[-- 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
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.
[-- 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 --]