bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline inviisble

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline inviisble

[Paul Rankin]

When attempting to use `outline' to collapse a heading that begins with a character with any invisible text property, the heading is mistakenly treated as invisible, when only a heading with the `outline' invisible text property should be considered invisible (i.e. collapsed).

To reproduce:

1. emacs -Q
2. insert ";;; heading"
3. M-: (outline-on-heading-p)
    => t
4. C-a
5. M-: (put-text-property (point) (1+ (point)) 'invisible 'foo)
6. M-; (outline-on-heading-p)
    => nil

Expected results:

(outline-on-heading-p)
    => t

Actual results:

(outline-on-heading-p)
    => nil

Solution:

The function outline-on-heading-p checks bolp for an invisible property, but not specifically the `outline' invisible property:

    (defun outline-on-heading-p (&optional invisible-ok)
      "Return t if point is on a (visible) heading line.
    If INVISIBLE-OK is non-nil, an invisible heading line is ok too."
      (save-excursion
        (beginning-of-line)
        (and (bolp) (or invisible-ok (not (outline-invisible-p)))
               (looking-at outline-regexp))))

Instead, it should check for only the `outline' invisible text property:

    (defun outline-on-heading-p (&optional invisible-ok)
        "Return t if point is on a (visible) heading line.
    If INVISIBLE-OK is non-nil, an invisible heading line is ok too."
        (save-excursion
          (beginning-of-line)
          (and (bolp) (or invisible-ok (not (eq (outline-invisible-p) 'outline)))
               (looking-at outline-regexp))))

-- 
Paul W. Rankin
www.paulwrankin.com

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline inviisble

[Paul Rankin]

A better solution is to make `outline-invisible-p' return t only if invisible text property is `outline'

    (defsubst outline-invisible-p (&optional pos)
      "Non-nil if the character after point is invisible."
      (eq (get-char-property (or pos (point)) 'invisible)
          'outline))

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline inviisble

[Eli Zaretskii]

> From: Paul Rankin <hello@paulwrankin.com>
> Date: Tue, 26 Jul 2016 19:29:18 +1000
> 
> A better solution is to make `outline-invisible-p' return t only if invisible text property is `outline'
> 
>     (defsubst outline-invisible-p (&optional pos)
>       "Non-nil if the character after point is invisible."
>       (eq (get-char-property (or pos (point)) 'invisible)
>           'outline))

For outline's own use, sure.  But this function is also used by Org,
so I think we need to coordinate with Org developers about such a
change, to make sure we don't break Org in the process.

Btw, can you describe the real-life use case where you bumped into
this issue?

Thanks.

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline inviisble

[Paul Rankin]

Eli Zaretskii <eliz@gnu.org> on Tue, 26 Jul 2016 18:14 +0300:
> For outline's own use, sure.  But this function is also used by Org,
> so I think we need to coordinate with Org developers about such a
> change, to make sure we don't break Org in the process.

I'll leave this in your hands.

> Btw, can you describe the real-life use case where you bumped into
> this issue?

I author a major mode for a format wherein some outline headings are prefixed with a non-printing "forced" character.

I've given the user the option to hide these non-printing "forced" characters by adding an invisible text property and keyword to the regexp group (the "forced" character), then using `add-to-invisibility-spec' to add that keyword.

The invisible property is managed by Font Lock.

It will probably not shed any further light, but here is the real-life code:
https://github.com/rnkn/fountain-mode/blob/0707b4753fabdc07cbe04030b1559671d87c57f2/fountain-mode.el#L3691-L3699

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Paul Rankin]

I've tried to create an advice patch for the benefit of users affected by the bug to make outline-invisible-p return t for either 'outline or t, nil otherwise...

    (advice-add 'outline-invisible-p :filter-return
                (lambda (return) (eq return (or 'outline t)))
                '((name . "fountain-mode-patch")))

However, it appears that other compiled functions in outline that call outline-invisible-p do not respect this advice, e.g.

1. navigate to heading affected by above bug
2. M-: (outline-on-heading-p)
    => nil
3. navigate to (defun outline-on-heading-p ...) in outline.el
4. C-M-x
5. repeat 1. and 2.
    => t

Do compiled functions not respect advice added to functions that they call?

-- 
Paul W. Rankin
www.paulwrankin.com

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Noam Postavsky]

On Thu, Jul 28, 2016 at 12:25 AM, Paul Rankin <hello@paulwrankin.com> wrote:
> I've tried to create an advice patch for the benefit of users affected by the bug to make outline-invisible-p return t for either 'outline or t, nil otherwise...
>
>     (advice-add 'outline-invisible-p :filter-return
>                 (lambda (return) (eq return (or 'outline t)))
>                 '((name . "fountain-mode-patch")))
>
> However, it appears that other compiled functions in outline that call outline-invisible-p do not respect this advice, e.g.
>
> 1. navigate to heading affected by above bug
> 2. M-: (outline-on-heading-p)
>     => nil
> 3. navigate to (defun outline-on-heading-p ...) in outline.el
> 4. C-M-x
> 5. repeat 1. and 2.
>     => t
>
> Do compiled functions not respect advice added to functions that they call?

I think it's because outline-invisible-p is a defsubst, so when
compiled, callers don't actually reference the symbol
`outline-invisible-p' at all.

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Paul Rankin]

Noam Postavsky <npostavs@users.sourceforge.net> on Thu, 28 Jul 2016 22:07 -0400:
> I think it's because outline-invisible-p is a defsubst, so when
> compiled, callers don't actually reference the symbol
> `outline-invisible-p' at all.

Ah yes. Thanks. A predicate function that doesn't return a t or nil and misuses defsubst! For shame outline... for shame....

Is there any way for a package to work around this with outline in its present state?

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[npostavs]

Paul Rankin <hello@paulwrankin.com> writes:

> Noam Postavsky <npostavs@users.sourceforge.net> on Thu, 28 Jul 2016 22:07 -0400:
>> I think it's because outline-invisible-p is a defsubst, so when
>> compiled, callers don't actually reference the symbol
>> `outline-invisible-p' at all.
>
> Ah yes. Thanks. A predicate function that doesn't return a t or nil
> and misuses defsubst!

I don't think it's necessarily a misuse of defsubst, that just happens
to be one of the limitations.

>
> Is there any way for a package to work around this with outline in its present state?

Well, as you saw, re`eval'uating outline-on-heading-p (so that it
becomes uncompiled) seems to work, you could try doing that from lisp:

    (pcase (find-function-noselect 'outline-on-heading-p)
      (`(,buffer . ,position)
       (with-current-buffer buffer
         (goto-char position)
         (eval (read (current-buffer)))))
      (_ (error "Couldn't find `outline-on-heading-p'")))

Or advise :override outline-on-heading-p instead of outline-invisible-p.

>     (advice-add 'outline-invisible-p :filter-return
>                 (lambda (return) (eq return (or 'outline t)))

By the way, shouldn't that be

                  (lambda (return) (or (eq return 'outline) return))

>                 '((name . "fountain-mode-patch")))

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Paul Rankin]

npostavs@users.sourceforge.net on Mon, 01 Aug 2016 10:16 -0400:
> I don't think it's necessarily a misuse of defsubst, that just happens
> to be one of the limitations.

Jks ;)

> Well, as you saw, re`eval'uating outline-on-heading-p (so that it
> becomes uncompiled) seems to work, you could try doing that from lisp:
> 
>     (pcase (find-function-noselect 'outline-on-heading-p)
>       (`(,buffer . ,position)
>        (with-current-buffer buffer
>          (goto-char position)
>          (eval (read (current-buffer)))))
>       (_ (error "Couldn't find `outline-on-heading-p'")))

Whoa this is terrifying...

Would something like the following work? Maybe with a
(condition-case ...) ?

    (let ((source (find-function-noselect 'outline-on-heading-p)))
      (with-current-buffer (car source)
        (goto-char (cdr source))
        (eval-defun nil)))
 
> Or advise :override outline-on-heading-p instead of outline-invisible-p.

But I want to act with a light touch...

> By the way, shouldn't that be
> 
>                   (lambda (return) (or (eq return 'outline) return))

I think this will fail when outline-invisible-p returns foo in the
initial example. We want to only return t when outline-invisible-p
returns outline, not foo or otherwise.

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Noam Postavsky]

On Mon, Aug 1, 2016 at 11:27 PM, Paul Rankin <hello@paulwrankin.com> wrote:
> Whoa this is terrifying...

:) Don't like pcase?

>
> Would something like the following work? Maybe with a
> (condition-case ...) ?
>
>     (let ((source (find-function-noselect 'outline-on-heading-p)))
>       (with-current-buffer (car source)
>         (goto-char (cdr source))
>         (eval-defun nil)))

Yes, that's about the same, though I would still suggest (eval (read
...)) over eval-defun for non-interactive code.

>> By the way, shouldn't that be
>>
>>                   (lambda (return) (or (eq return 'outline) return))
>
> I think this will fail when outline-invisible-p returns foo in the
> initial example. We want to only return t when outline-invisible-p
> returns outline, not foo or otherwise.

Oh, then your original was okay, it just had a bit of dead code.
Simply (lambda (return) (eq return 'outline)) should suffice.

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Paul Rankin]

Noam Postavsky <npostavs@users.sourceforge.net> on Mon, 01 Aug 2016 23:47 -0400:
> :) Don't like pcase?

I fear what I do not understand.

> > Would something like the following work? Maybe with a
> > (condition-case ...) ?
> >
> >     (let ((source (find-function-noselect 'outline-on-heading-p)))
> >       (with-current-buffer (car source)
> >         (goto-char (cdr source))
> >         (eval-defun nil)))
> 
> Yes, that's about the same, though I would still suggest (eval (read
> ...)) over eval-defun for non-interactive code.

Ah yes, I should have read the docs: "Read one Lisp expression as text
from STREAM" - I assumed it would eval the whole buffer... Thanks

I'm gonna assume if the package has (require 'outline) then it's safe to
assume the existence of outline-on-heading-p too... It appears that
(find-function-noselect 'missingfunction) will just hang Emacs anyway.

> >> By the way, shouldn't that be
> >>
> >>                   (lambda (return) (or (eq return 'outline) return))
> >
> > I think this will fail when outline-invisible-p returns foo in the
> > initial example. We want to only return t when outline-invisible-p
> > returns outline, not foo or otherwise.
> 
> Oh, then your original was okay, it just had a bit of dead code.
> Simply (lambda (return) (eq return 'outline)) should suffice.

Then this will fail if/when outline-invisible-p is fixed to return t
when I wanna future-proof this, so users don't notice the transition.
Sorry my previous reply neglected to mention returning t.

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Paul Rankin]

I've added the following into my major mode to patch outline until the
bug is addressed...

    (with-eval-after-load 'outline
      (advice-add 'outline-invisible-p :filter-return
                  (lambda (return) (eq return (or 'outline t)))
                  '((name . "fountain-mode-patch")))
      (dolist (fun '(outline-back-to-heading
                     outline-on-heading-p
                     outline-next-visible-heading))
        (let ((source (find-function-noselect fun)))
          (with-current-buffer (car source)
            (goto-char (cdr source))
            (eval (read (current-buffer))))))
      (message "Function `outline-invisible-p' has been patched"))

But I'm a bit concerned if users install Emacs without uncompiled Lisp
source, will this result in an error that Emacs simply can't find the
library outline?

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Noam Postavsky]

On Tue, Aug 2, 2016 at 12:22 AM, Paul Rankin <hello@paulwrankin.com> wrote:
> It appears that
> (find-function-noselect 'missingfunction) will just hang Emacs anyway.

That's a bug, but it seems to be fixed in Emacs-25, it signals void-function.

>
>> >> By the way, shouldn't that be
>> >>
>> >>                   (lambda (return) (or (eq return 'outline) return))
>> >
>> > I think this will fail when outline-invisible-p returns foo in the
>> > initial example. We want to only return t when outline-invisible-p
>> > returns outline, not foo or otherwise.
>>
>> Oh, then your original was okay, it just had a bit of dead code.
>> Simply (lambda (return) (eq return 'outline)) should suffice.
>
> Then this will fail if/when outline-invisible-p is fixed to return t
> when I wanna future-proof this, so users don't notice the transition.
> Sorry my previous reply neglected to mention returning t.

Oh, then you need (lambda (return) (or (eq return 'outline) (eq return t)))
Alternatively maybe I can tempt you into learning pcase ;)
 (lambda (return) (pcase return ((or `outline `t) t)))

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Noam Postavsky]

On Tue, Aug 2, 2016 at 3:20 AM, Paul Rankin <hello@paulwrankin.com> wrote:
> I've added the following into my major mode to patch outline until the
> bug is addressed...
>
>     (with-eval-after-load 'outline
>       (advice-add 'outline-invisible-p :filter-return
>                   (lambda (return) (eq return (or 'outline t)))
>                   '((name . "fountain-mode-patch")))
>       (dolist (fun '(outline-back-to-heading
>                      outline-on-heading-p
>                      outline-next-visible-heading))
>         (let ((source (find-function-noselect fun)))
>           (with-current-buffer (car source)
>             (goto-char (cdr source))
>             (eval (read (current-buffer))))))
>       (message "Function `outline-invisible-p' has been patched"))
>
> But I'm a bit concerned if users install Emacs without uncompiled Lisp
> source, will this result in an error that Emacs simply can't find the
> library outline?

Yes, perhaps you want to wrap the loop body in with-demoted-errors, or similar.

bug#24073: 24.5; outline-on-heading-p sees any invisible text property as outline invisible

[Paul Rankin]

Noam Postavsky <npostavs@users.sourceforge.net> on Tue, 02 Aug 2016 10:27 -0400:
> >> Oh, then your original was okay, it just had a bit of dead code.
> >> Simply (lambda (return) (eq return 'outline)) should suffice.
> >
> > Then this will fail if/when outline-invisible-p is fixed to
> > return t when I wanna future-proof this, so users don't notice
> > the transition. Sorry my previous reply neglected to mention
> > returning t.
>
> Oh, then you need (lambda (return) (or (eq return 'outline) (eq return
> t))) Alternatively maybe I can tempt you into learning pcase ;)
> (lambda (return) (pcase return ((or `outline `t) t)))

What a rookie error on my part! Yes, of course.

I'll look into pcase, but I don't like the aesthetic of the backtick or
underscore....


Noam Postavsky <npostavs@users.sourceforge.net> on Tue, 02 Aug 2016 10:31 -0400:
> > But I'm a bit concerned if users install Emacs without uncompiled
> > Lisp source, will this result in an error that Emacs simply can't
> > find the library outline?
>
> Yes, perhaps you want to wrap the loop body in with-demoted-errors, or
> similar.

Will do. Thanks for your help!

bug#24073: 25.1-rc2

[Paul Rankin]

I've just seen this bug is still in outline.el in 25.1-rc2.

The fix seems trivial to me so I'm wondering if there is anything holding it up from being included in 25?

bug#24073: 25.1-rc2

[Eli Zaretskii]

> From: Paul Rankin <hello@paulwrankin.com>
> Date: Wed, 31 Aug 2016 11:12:36 +1000
> 
> I've just seen this bug is still in outline.el in 25.1-rc2.
> 
> The fix seems trivial to me so I'm wondering if there is anything holding it up from being included in 25?

Yes, the fact that Emacs 25 is for all practical purposes already
released.

bug#24073: 25.1-rc2

[Paul Rankin]

On 31 Aug 2016, at 12:41 PM, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Paul Rankin <hello@paulwrankin.com>
>> Date: Wed, 31 Aug 2016 11:12:36 +1000
>> 
>> I've just seen this bug is still in outline.el in 25.1-rc2.
>> 
>> The fix seems trivial to me so I'm wondering if there is anything holding it up from being included in 25?
> 
> Yes, the fact that Emacs 25 is for all practical purposes already
> released.

Okay. 25.2?

bug#24073: 25.1-rc2

[John Wiegley]

>>>>> "PR" == Paul Rankin <hello@paulwrankin.com> writes:

>> Yes, the fact that Emacs 25 is for all practical purposes already
>> released.

PR> Okay. 25.2?

Sure, 25.2 will be open for commitments on the emacs-25 branch in a couple of
weeks (fingers crossed).

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#24073: 25.1-rc2

[Nicolas Petton]

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

Paul Rankin <hello@paulwrankin.com> writes:

> On 31 Aug 2016, at 12:41 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>
>>> From: Paul Rankin <hello@paulwrankin.com>
>>> Date: Wed, 31 Aug 2016 11:12:36 +1000
>>> 
>>> I've just seen this bug is still in outline.el in 25.1-rc2.
>>> 
>>> The fix seems trivial to me so I'm wondering if there is anything holding it up from being included in 25?
>> 
>> Yes, the fact that Emacs 25 is for all practical purposes already
>> released.
>
> Okay. 25.2?

You can push a fix to master.

Cheers,
Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

bug#24073: 25.1-rc2

[Eli Zaretskii]

> From: Paul Rankin <hello@paulwrankin.com>
> Date: Wed, 31 Aug 2016 12:56:13 +1000
> Cc: 24073@debbugs.gnu.org
> 
> >> The fix seems trivial to me so I'm wondering if there is anything holding it 
> >> up from being included in 25?
> > 
> > Yes, the fact that Emacs 25 is for all practical purposes already
> > released.
> 
> Okay. 25.2?

I don't see why not, provided that Org developers give us their
blessing.  Please bring this to their attention on the Org list, or
ask them to speak up here.  I don't want us to make any changes that
could adversely affect Org without consulting them first.

Thanks.

bug#24073: 25.1-rc2

[Paul Rankin]

Eli Zaretskii <eliz@gnu.org> on Wed, 31 Aug 2016 17:25 +0300:
> > From: Paul Rankin <hello@paulwrankin.com>
> > Date: Wed, 31 Aug 2016 12:56:13 +1000
> > Cc: 24073@debbugs.gnu.org
> > 
> > >> The fix seems trivial to me so I'm wondering if there is anything holding it 
> > >> up from being included in 25?
> > > 
> > > Yes, the fact that Emacs 25 is for all practical purposes already
> > > released.
> > 
> > Okay. 25.2?
> 
> I don't see why not, provided that Org developers give us their
> blessing.  Please bring this to their attention on the Org list, or
> ask them to speak up here.  I don't want us to make any changes that
> could adversely affect Org without consulting them first.

Dear Org Mode maintainers,

Looping you in on a proposed bug fix for `outline-invisible-p'.

Briefly, the problem is the function returns non-nil for any invisible
text property when it should only do so for the outline property. As a
defsubst, it is difficult to patch for other affected programs.

Diff pasted:

--- /usr/local/Cellar/emacs/25.1-rc2/share/emacs/25.1/lisp/outline.el.gz
+++ #<buffer outline.el.gz>
@@ -388,9 +388,9 @@
 		      nil 'move))
 
 (defsubst outline-invisible-p (&optional pos)
-  "Non-nil if the character after POS is invisible.
+  "Non-nil if the character after POS has outline invisible property.
 If POS is nil, use `point' instead."
-  (get-char-property (or pos (point)) 'invisible))
+  (eq (get-char-property (or pos (point)) 'invisible) 'outline))
 
 (defun outline-back-to-heading (&optional invisible-ok)
   "Move to previous heading line, or beg of this line if it's a heading.

Diff finished.  Sat Sep  3 14:35:22 2016

bug#24073: 25.1-rc2

[Eli Zaretskii]

Ping!

Bastien, could you or someone else please look into this and provide
your comments?  TIA.

> From: Paul Rankin <hello@paulwrankin.com>
> Cc: 24073@debbugs.gnu.org, emacs-orgmode@gnu.org
> Date: Sat, 03 Sep 2016 14:38:55 +1000
> 
> Eli Zaretskii <eliz@gnu.org> on Wed, 31 Aug 2016 17:25 +0300:
> > > From: Paul Rankin <hello@paulwrankin.com>
> > > Date: Wed, 31 Aug 2016 12:56:13 +1000
> > > Cc: 24073@debbugs.gnu.org
> > > 
> > > >> The fix seems trivial to me so I'm wondering if there is anything holding it 
> > > >> up from being included in 25?
> > > > 
> > > > Yes, the fact that Emacs 25 is for all practical purposes already
> > > > released.
> > > 
> > > Okay. 25.2?
> > 
> > I don't see why not, provided that Org developers give us their
> > blessing.  Please bring this to their attention on the Org list, or
> > ask them to speak up here.  I don't want us to make any changes that
> > could adversely affect Org without consulting them first.
> 
> Dear Org Mode maintainers,
> 
> Looping you in on a proposed bug fix for `outline-invisible-p'.
> 
> Briefly, the problem is the function returns non-nil for any invisible
> text property when it should only do so for the outline property. As a
> defsubst, it is difficult to patch for other affected programs.
> 
> Diff pasted:
> 
> --- /usr/local/Cellar/emacs/25.1-rc2/share/emacs/25.1/lisp/outline.el.gz
> +++ #<buffer outline.el.gz>
> @@ -388,9 +388,9 @@
>  		      nil 'move))
>  
>  (defsubst outline-invisible-p (&optional pos)
> -  "Non-nil if the character after POS is invisible.
> +  "Non-nil if the character after POS has outline invisible property.
>  If POS is nil, use `point' instead."
> -  (get-char-property (or pos (point)) 'invisible))
> +  (eq (get-char-property (or pos (point)) 'invisible) 'outline))
>  
>  (defun outline-back-to-heading (&optional invisible-ok)
>    "Move to previous heading line, or beg of this line if it's a heading.
> 
> Diff finished.  Sat Sep  3 14:35:22 2016
> 

bug#24073: 25.1-rc2

[Nicolas Goaziou]

Hello,

Eli Zaretskii <eliz@gnu.org> writes:

> Bastien, could you or someone else please look into this and provide
> your comments?  TIA.

This fix looks fine, obviously. You can go ahead as far as Org is
concerned.

Thank you for letting us know.

Regards,

-- 
Nicolas Goaziou

bug#24073: 25.1-rc2

[Eli Zaretskii]

> From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
> Cc: Paul Rankin <hello@paulwrankin.com>,  Bastien Guerry <bzg@gnu.org>,  24073@debbugs.gnu.org,  emacs-orgmode@gnu.org
> Date: Sun, 18 Sep 2016 16:45:34 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Bastien, could you or someone else please look into this and provide
> > your comments?  TIA.
> 
> This fix looks fine, obviously. You can go ahead as far as Org is
> concerned.

Thanks.

Paul, you have a go.  Thank you for your patience and perseverance.

bug#24073: 25.1-rc2

[Bastien Guerry]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> This fix looks fine, obviously. You can go ahead as far as Org is
> concerned.

+1

-- 
 Bastien

Re: bug#24073: 25.1-rc2

[Bastien Guerry]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> This fix looks fine, obviously. You can go ahead as far as Org is
> concerned.

+1

-- 
 Bastien

bug#24073: 25.1-rc2

[Paul Rankin]

Eli Zaretskii <eliz@gnu.org> on Sun, 18 Sep 2016 17:53 +0300:
> > From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
> > Cc: Paul Rankin <hello@paulwrankin.com>,  Bastien Guerry <bzg@gnu.org>,  24073@debbugs.gnu.org,  emacs-orgmode@gnu.org
> > Date: Sun, 18 Sep 2016 16:45:34 +0200
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > Bastien, could you or someone else please look into this and provide
> > > your comments?  TIA.
> > 
> > This fix looks fine, obviously. You can go ahead as far as Org is
> > concerned.
> 
> Thanks.
> 
> Paul, you have a go.  Thank you for your patience and perseverance.

Thanks Eli. So the Emacs git repo accepts anyone pushing to?

bug#24073: 25.1-rc2

[Eli Zaretskii]

> From: Paul Rankin <hello@paulwrankin.com>
> Cc: Nicolas Goaziou <mail@nicolasgoaziou.fr>, bzg@gnu.org,
>  24073@debbugs.gnu.org
> Date: Mon, 19 Sep 2016 19:40:05 +1000
> 
> > Paul, you have a go.  Thank you for your patience and perseverance.
> 
> Thanks Eli. So the Emacs git repo accepts anyone pushing to?

No, you need to get write access.  If you don't have it, someone else
will have to push your commit n your name.

bug#24073: 25.1-rc2

[Bastien Guerry]

Hi Eli and Paul,

Eli Zaretskii <eliz@gnu.org> writes:

> No, you need to get write access.  If you don't have it, someone else
> will have to push your commit n your name.

I can commit this when I'm back on a good internet connection,
sometimes next week.

Thanks,

-- 
 Bastien

bug#24073: 25.1-rc2

[Bastien Guerry]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Paul Rankin <hello@paulwrankin.com>
>> Cc: Nicolas Goaziou <mail@nicolasgoaziou.fr>, bzg@gnu.org,
>>  24073@debbugs.gnu.org
>> Date: Mon, 19 Sep 2016 19:40:05 +1000
>> 
>> > Paul, you have a go.  Thank you for your patience and perseverance.
>> 
>> Thanks Eli. So the Emacs git repo accepts anyone pushing to?
>
> No, you need to get write access.  If you don't have it, someone else
> will have to push your commit n your name.

I've just pushed the fix in master.

Thanks Paul.

-- 
 Bastien

bug#24073: 25.1-rc2

[Paul Rankin]

On Fri, 30 Sep 2016, at 06:06 PM, Bastien Guerry wrote:
> 
> I've just pushed the fix in master.
> 

I can see this fix in master [1] but I'm on 25.2-rc2 [2] and outline.el.gz still shows the old outline-invisible-p function.

Sorry, can anyone let me know where I've gone wrong?

[1]: https://github.com/emacs-mirror/emacs/blob/080a425db51e0b26b03f0f4bd06c814fc2b38578/lisp/outline.el#L390-L393
[2]: M-x version: GNU Emacs 25.2.1 (x86_64-apple-darwin16.4.0, NS appkit-1504.81 Version 10.12.3 (Build 16D32)) of 2017-02-26

bug#24073: 25.1-rc2

[npostavs]

Paul Rankin <hello@paulwrankin.com> writes:

> On Fri, 30 Sep 2016, at 06:06 PM, Bastien Guerry wrote:
>> 
>> I've just pushed the fix in master.
>> 
>
> I can see this fix in master [1] but I'm on 25.2-rc2 [2] and outline.el.gz still shows the old outline-invisible-p function.
>
> Sorry, can anyone let me know where I've gone wrong?

Essentially we kind of missed the boat, and we're back in the same
situation as [53] this time for 25.2: it's about to be released so it's
a bit late to make non-critical code changes.  The code in master will
become 26.1.

[53]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24073#53

bug#24073: 25.1-rc2

[Paul Rankin]

On Fri, 31 Mar 2017, at 10:16 AM, npostavs@users.sourceforge.net wrote:
> Paul Rankin <hello@paulwrankin.com> writes:
>
> > I can see this fix in master [1] but I'm on 25.2-rc2 [2] and
> > outline.el.gz still shows the old outline-invisible-p function.
> >
> > Sorry, can anyone let me know where I've gone wrong?
>
> Essentially we kind of missed the boat, and we're back in the same
> situation as [53] this time for 25.2: it's about to be released so
> it's a bit late to make non-critical code changes.  The code in master
> will become 26.1.
>
> [53]: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24073#53

Hmm this just raises more questions for me...

If I'm following correctly, the patch to outline.el was applied to master on a commit 9cc59ffbbb2f20fbbf1c72d2e0c9dc47c7906a99 on 30 September 2016. Then the "emacs-25.2-rc2" tag was applied to commit fe91ff27c54cc10b70a5c5d5bac4017922866717 on master on 22 February.[1]

If I browse the master tree at fe91ff27c54cc10b70a5c5d5bac4017922866717 the patch to outline.el is *not* there.[2]

But if I browse the current master tree, the patch is there.[3]

So the patch was on master, then it was not, and now it is (but not in release candidate). Hence I'm super confused about the Emacs dev team's git workflow....?

[1]: https://github.com/emacs-mirror/emacs/blob/9cc59ffbbb2f20fbbf1c72d2e0c9dc47c7906a99/lisp/outline.el#L393
[2]: https://github.com/emacs-mirror/emacs/blob/fe91ff27c54cc10b70a5c5d5bac4017922866717/lisp/outline.el#L393
[3]: https://github.com/emacs-mirror/emacs/blob/master/lisp/outline.el#L393

bug#24073: 25.1-rc2

[Andreas Schwab]

On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:

> If I'm following correctly, the patch to outline.el was applied to master on a commit 9cc59ffbbb2f20fbbf1c72d2e0c9dc47c7906a99 on 30 September 2016. Then the "emacs-25.2-rc2" tag was applied to commit fe91ff27c54cc10b70a5c5d5bac4017922866717 on master on 22 February.[1]

fe91ff27c5 isn't in master, only in the emacs-25 branch.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#24073: 25.1-rc2

[Paul Rankin]

On Sat, 1 Apr 2017, at 05:20 PM, Andreas Schwab wrote:
> On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:
> 
> > If I'm following correctly, the patch to outline.el was applied to master on a commit 9cc59ffbbb2f20fbbf1c72d2e0c9dc47c7906a99 on 30 September 2016. Then the "emacs-25.2-rc2" tag was applied to commit fe91ff27c54cc10b70a5c5d5bac4017922866717 on master on 22 February.[1]
> 
> fe91ff27c5 isn't in master, only in the emacs-25 branch.
> 
> Andreas.
> 

I thought maybe GitHub was deceiving me, but nope, here is the commit on savannah in master:
http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fe91ff27c54cc10b70a5c5d5bac4017922866717&h=master

#confused

bug#24073: 25.1-rc2

[Andreas Schwab]

On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:

> I thought maybe GitHub was deceiving me, but nope, here is the commit on savannah in master:
> http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fe91ff27c54cc10b70a5c5d5bac4017922866717&h=master

Where does it say that?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#24073: 25.1-rc2

[Paul Rankin]

On Sat, 1 Apr 2017, at 06:20 PM, Andreas Schwab wrote:
> On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:
> 
> > I thought maybe GitHub was deceiving me, but nope, here is the commit on savannah in master:
> > http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fe91ff27c54cc10b70a5c5d5bac4017922866717&h=master
> 
> Where does it say that?
> 
> Andreas.

Top right corner. And on GitHub it's just under the commit message. But if you trust neither of these, you can verify for yourself with:

$ git branch -a --contains fe91ff27c54cc10b70a5c5d5bac4017922866717
* master
  remotes/origin/HEAD -> origin/master
  remotes/origin/emacs-25
  remotes/origin/feature/mhtml-mode
  remotes/origin/fix-bug-21072
  remotes/origin/master
  remotes/origin/scratch/record
  remotes/origin/scratch/tzz/nettle

bug#24073: 25.1-rc2

[Andreas Schwab]

On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:

> On Sat, 1 Apr 2017, at 06:20 PM, Andreas Schwab wrote:
>> On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:
>> 
>> > I thought maybe GitHub was deceiving me, but nope, here is the commit on savannah in master:
>> > http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=fe91ff27c54cc10b70a5c5d5bac4017922866717&h=master
>> 
>> Where does it say that?
>> 
>> Andreas.
>
> Top right corner.

It doesn't say that.  You can switch to any branch you like.

> And on GitHub it's just under the commit message. But if you trust neither of these, you can verify for yourself with:
>
> $ git branch -a --contains fe91ff27c54cc10b70a5c5d5bac4017922866717
> * master
>   remotes/origin/HEAD -> origin/master
>   remotes/origin/emacs-25
>   remotes/origin/feature/mhtml-mode
>   remotes/origin/fix-bug-21072
>   remotes/origin/master
>   remotes/origin/scratch/record
>   remotes/origin/scratch/tzz/nettle

That's only after emacs-25 has been merged into master.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#24073: 25.1-rc2

[Paul Rankin]

On Sat, 1 Apr 2017, at 05:20 PM, Andreas Schwab wrote:
> On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:
> 
> > If I'm following correctly, the patch to outline.el was applied to master on a commit 9cc59ffbbb2f20fbbf1c72d2e0c9dc47c7906a99 on 30 September 2016. Then the "emacs-25.2-rc2" tag was applied to commit fe91ff27c54cc10b70a5c5d5bac4017922866717 on master on 22 February.[1]
> 
> fe91ff27c5 isn't in master, only in the emacs-25 branch.


On Sat, 1 Apr 2017, at 07:37 PM, Andreas Schwab wrote:
> On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:
>
> > And on GitHub it's just under the commit message. But if you trust neither of these, you can verify for yourself with:
> >
> > $ git branch -a --contains fe91ff27c54cc10b70a5c5d5bac4017922866717
> > * master
> >   remotes/origin/HEAD -> origin/master
> >   remotes/origin/emacs-25
> >   remotes/origin/feature/mhtml-mode
> >   remotes/origin/fix-bug-21072
> >   remotes/origin/master
> >   remotes/origin/scratch/record
> >   remotes/origin/scratch/tzz/nettle
> 
> That's only after emacs-25 has been merged into master.
> 

Okay so we've established that the commit is in master after all 👍

Emacs development appears to go along in a kind of unorthodox way. As someone familiar with git but unfamiliar with the Emacs dev workflow, my assumption was that anything in master is ready to ship out the door, with the bulk of commits happening on feature or hotfix branches. But it appears to work in the reverse, with everything going into master, and stable releases branching off, which seems like a good recipe for perpetual missing-of-boatsness. I remember when Emacs dev switched over to git there was a lot of confusion about how it works, so I think this is maybe the remnants of that.

Anyway, let's hope we can catch the boat next time round!

bug#24073: 25.1-rc2

[Eli Zaretskii]

> From: Paul Rankin <hello@paulwrankin.com>
> Date: Sat, 01 Apr 2017 20:18:37 +1000
> Cc: Bastien Guerry <bzg@gnu.org>, 24073@debbugs.gnu.org, mail@nicolasgoaziou.fr,
> 	npostavs@users.sourceforge.net
> 
> > > $ git branch -a --contains fe91ff27c54cc10b70a5c5d5bac4017922866717
> > > * master
> > >   remotes/origin/HEAD -> origin/master
> > >   remotes/origin/emacs-25
> > >   remotes/origin/feature/mhtml-mode
> > >   remotes/origin/fix-bug-21072
> > >   remotes/origin/master
> > >   remotes/origin/scratch/record
> > >   remotes/origin/scratch/tzz/nettle
> > 
> > That's only after emacs-25 has been merged into master.
> > 
> 
> Okay so we've established that the commit is in master after all 👍

That wasn't controversial to begin with.  The issue was with the
emacs-25.2-rc2 tag, not with the commit which fixed the bug in
question.

The commit is on master, whereas the tag was applied to the emacs-25
branch, which was later merged to master, as part of periodic merges
we do.

> Emacs development appears to go along in a kind of unorthodox way.

It's a merge-based workflow, one of widely used Git workflows.  We
didn't invent it.  The details are described in the file CONTRIBUTE in
the tree, under "Branches".

> As someone familiar with git but unfamiliar with the Emacs dev workflow, my assumption was that anything in master is ready to ship out the door, with the bulk of commits happening on feature or hotfix branches. But it appears to work in the reverse, with everything going into master, and stable releases branching off, which seems like a good recipe for perpetual missing-of-boatsness.

When the branch from which the next official release is about to be
shipped is close to a release, we don't allow unsafe changes to be
committed to that branch, because any such change could potentially
destabilize the entire branch.  There's nothing new here; if you were
ever involved in releasing very large and complex packages, you should
be familiar with this paradigm.

bug#24073: 25.1-rc2

[Andreas Schwab]

On Apr 01 2017, Paul Rankin <hello@paulwrankin.com> wrote:

> Emacs development appears to go along in a kind of unorthodox way. As someone familiar with git but unfamiliar with the Emacs dev workflow, my assumption was that anything in master is ready to ship out the door, with the bulk of commits happening on feature or hotfix branches. But it appears to work in the reverse, with everything going into master, and stable releases branching off, which seems like a good recipe for perpetual missing-of-boatsness. I remember when Emacs dev switched over to git there was a lot of confusion about how it works, so I think this is maybe the remnants of that.

It's not unusual that releases are created from a branch.  A lot of
projects work like that.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#24073: 25.1-rc2

[Paul Rankin]

On Sat, 1 Apr 2017, at 09:44 PM, Eli Zaretskii wrote:
> > From: Paul Rankin <hello@paulwrankin.com>
> > Date: Sat, 01 Apr 2017 20:18:37 +1000
> > Cc: Bastien Guerry <bzg@gnu.org>, 24073@debbugs.gnu.org, mail@nicolasgoaziou.fr,
> > 	npostavs@users.sourceforge.net
> > 
> > > > $ git branch -a --contains fe91ff27c54cc10b70a5c5d5bac4017922866717
> > > > * master
> > > >   remotes/origin/HEAD -> origin/master
> > > >   remotes/origin/emacs-25
> > > >   remotes/origin/feature/mhtml-mode
> > > >   remotes/origin/fix-bug-21072
> > > >   remotes/origin/master
> > > >   remotes/origin/scratch/record
> > > >   remotes/origin/scratch/tzz/nettle
> > > 
> > > That's only after emacs-25 has been merged into master.
> > > 
> > 
> > Okay so we've established that the commit is in master after all 👍
> 
> That wasn't controversial to begin with.  The issue was with the
> emacs-25.2-rc2 tag, not with the commit which fixed the bug in
> question.

Question was about this tagged commit in master in Feb after the bug fix commit last Sep.

> The commit is on master, whereas the tag was applied to the emacs-25
> branch, which was later merged to master, as part of periodic merges
> we do.
> 
> > Emacs development appears to go along in a kind of unorthodox way.
> 
> It's a merge-based workflow, one of widely used Git workflows.  We
> didn't invent it.  The details are described in the file CONTRIBUTE in
> the tree, under "Branches".
> 
> > As someone familiar with git but unfamiliar with the Emacs dev workflow, my assumption was that anything in master is ready to ship out the door, with the bulk of commits happening on feature or hotfix branches. But it appears to work in the reverse, with everything going into master, and stable releases branching off, which seems like a good recipe for perpetual missing-of-boatsness.
> 
> When the branch from which the next official release is about to be
> shipped is close to a release, we don't allow unsafe changes to be
> committed to that branch, because any such change could potentially
> destabilize the entire branch.  There's nothing new here; if you were
> ever involved in releasing very large and complex packages, you should
> be familiar with this paradigm.

Yep, the original question was about workflow, which this answers. Thanks ;)