unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Question about intended behavior of 'insert-for-yank-1'.
@ 2016-09-12  5:17 Karl Fogel
  2016-09-12 16:59 ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Karl Fogel @ 2016-09-12  5:17 UTC (permalink / raw)
  To: Emacs Devel

The doc string for 'insert-for-yank-1' says:

  [...]
  If STRING has a non-nil ‘yank-handler’ property on its first
  character, the normal insert behavior is altered.  The value of
  the ‘yank-handler’ property must be a list of one to four
  elements, of the form (FUNCTION PARAM NOEXCLUDE UNDO).
  FUNCTION, if non-nil, should be a function of one argument, an
   object to insert; it is called instead of ‘insert’.
  PARAM, if present and non-nil, replaces STRING as the argument to
   FUNCTION or ‘insert’; e.g. if FUNCTION is ‘yank-rectangle’, PARAM
   may be a list of strings to insert as a rectangle.
  [...]

This implies that when the `yank-handler' property is present, then when PARAM is nil, the string passed to FUNCTION should be STRING.  That is, the entire length of STRING, even if the `yank-handler' property is set only on STRING's first character.

However, the way it actually behaves is that if you set the property on (say) exactly the first character of STRING, then, assuming you rely on the implicit parameter passing, what gets passed to FUNCTION is a string consisting of just the first character of STRING :-).  Later, when you yank with C-y, that's all you'll get.

Now, this might be a reasonable behavior -- after all, if one wanted the entirety of STRING passed, then maybe one should set the property on all of STRING -- but it's not what's actually documented.

Should I update the documentation to clarify?  Or should the behavior change instead?

(By the way, I have not tested what happens if you set that property on multiple disjoint extents of STRING.  Does FUNCTION get passed a newly-created concatenation of all the stretches of string that had the property?  I have no idea.  If the recommendation here is just to fix the documentation, though, then I'll do that testing.)

Best regards,
-Karl



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-09-12  5:17 Question about intended behavior of 'insert-for-yank-1' Karl Fogel
@ 2016-09-12 16:59 ` Eli Zaretskii
  2016-09-12 17:15   ` Karl Fogel
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2016-09-12 16:59 UTC (permalink / raw)
  To: Karl Fogel; +Cc: emacs-devel

> From: Karl Fogel <kfogel@red-bean.com>
> Date: Mon, 12 Sep 2016 00:17:14 -0500
> 
> The doc string for 'insert-for-yank-1' says:
> 
>   [...]
>   If STRING has a non-nil ‘yank-handler’ property on its first
>   character, the normal insert behavior is altered.  The value of
>   the ‘yank-handler’ property must be a list of one to four
>   elements, of the form (FUNCTION PARAM NOEXCLUDE UNDO).
>   FUNCTION, if non-nil, should be a function of one argument, an
>    object to insert; it is called instead of ‘insert’.
>   PARAM, if present and non-nil, replaces STRING as the argument to
>    FUNCTION or ‘insert’; e.g. if FUNCTION is ‘yank-rectangle’, PARAM
>    may be a list of strings to insert as a rectangle.
>   [...]
> 
> This implies that when the `yank-handler' property is present, then when PARAM is nil, the string passed to FUNCTION should be STRING.  That is, the entire length of STRING, even if the `yank-handler' property is set only on STRING's first character.
> 
> However, the way it actually behaves is that if you set the property on (say) exactly the first character of STRING, then, assuming you rely on the implicit parameter passing, what gets passed to FUNCTION is a string consisting of just the first character of STRING :-).  Later, when you yank with C-y, that's all you'll get.

But for insert-for-yank-1, STRING is its argument, and it's that
argument that gets passed to FUNCTION.  So I don't see how the doc
string of this subroutine is inaccurate or misleading.  The part that
tricked you is in insert-for-yank:

  (defun insert-for-yank (string)
    "Call `insert-for-yank-1' repetitively for each `yank-handler' segment.

  See `insert-for-yank-1' for more details."
    (let (to)
      (while (setq to (next-single-property-change 0 'yank-handler string))
	(insert-for-yank-1 (substring string 0 to)) <<<<<<<<<<<
	(setq string (substring string to))))
    (insert-for-yank-1 string))

It passes to insert-for-yank-1 the substring of its argument STRING
that was actually "covered" by the text property.  See also the while
loop.

> Should I update the documentation to clarify?  Or should the behavior change instead?

I see no problem with how the functions behave.  As for documentation,
can you show the change you had in mind?

> (By the way, I have not tested what happens if you set that property on multiple disjoint extents of STRING.  Does FUNCTION get passed a newly-created concatenation of all the stretches of string that had the property?  I have no idea.  If the recommendation here is just to fix the documentation, though, then I'll do that testing.)

Isn't that clear from the loop in insert-for-yank?  Or am I missing
something?

Thanks.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-09-12 16:59 ` Eli Zaretskii
@ 2016-09-12 17:15   ` Karl Fogel
  2016-09-12 18:32     ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Karl Fogel @ 2016-09-12 17:15 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

Eli Zaretskii <eliz@gnu.org> writes:
>But for insert-for-yank-1, STRING is its argument, and it's that
>argument that gets passed to FUNCTION.  So I don't see how the doc
>string of this subroutine is inaccurate or misleading.  The part that
>tricked you is in insert-for-yank:
>
>  (defun insert-for-yank (string)
>    "Call `insert-for-yank-1' repetitively for each `yank-handler' segment.
>
>  See `insert-for-yank-1' for more details."
>    (let (to)
>      (while (setq to (next-single-property-change 0 'yank-handler string))
>	(insert-for-yank-1 (substring string 0 to)) <<<<<<<<<<<
>	(setq string (substring string to))))
>    (insert-for-yank-1 string))
>
>It passes to insert-for-yank-1 the substring of its argument STRING
>that was actually "covered" by the text property.  See also the while
>loop.

Thanks, Eli.  Yes, that's true, but note that the doc string for `insert-for-yank' just refers the reader to `insert-for-yank-1' for details.  The only doc string where the STRING-passing behavior is discussed is the doc string of `insert-for-yank-1', and that doc string indicates, or strongly implies, that the entirety of STRING is passed (which it isn't).

>> (By the way, I have not tested what happens if you set that property
>> on multiple disjoint extents of STRING.  Does FUNCTION get passed a
>> newly-created concatenation of all the stretches of string that had
>> the property?  I have no idea.  If the recommendation here is just
>> to fix the documentation, though, then I'll do that testing.)
>
>Isn't that clear from the loop in insert-for-yank?  Or am I missing
>something?

Sure.  But I'm concerned with the behavior that can be figured out just from reading the documentation, since that's what the documentation is for.

Many things become clear from reading the code and from trying experiments, but I hadn't gone that far yet.  My assertion is just that the documentation alone does not give the user enough information to predict how these functions will behave.  As per above, that still seems true to me.

>I see no problem with how the functions behave.  As for documentation,
>can you show the change you had in mind?

Something about how the string passed is actually the first extent of string covered by the property assuming the property starts at character 0.  I haven't worded it yet; that takes time and I first wanted to make sure there was agreement that there is a *documentation* problem here rather than a code-behavior problem.

Are you saying that in your view there is no documentation deficiency here?  

Best regards,
-Karl



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-09-12 17:15   ` Karl Fogel
@ 2016-09-12 18:32     ` Eli Zaretskii
  2016-09-12 22:36       ` Karl Fogel
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2016-09-12 18:32 UTC (permalink / raw)
  To: Karl Fogel; +Cc: emacs-devel

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: emacs-devel@gnu.org
> Date: Mon, 12 Sep 2016 12:15:22 -0500
> 
> Thanks, Eli.  Yes, that's true, but note that the doc string for `insert-for-yank' just refers the reader to `insert-for-yank-1' for details.  The only doc string where the STRING-passing behavior is discussed is the doc string of `insert-for-yank-1', and that doc string indicates, or strongly implies, that the entirety of STRING is passed (which it isn't).

Ah, so this is about the doc string of insert-for-yank, not its
subroutine.

> Are you saying that in your view there is no documentation deficiency here?  

I agree that the doc string of insert-for-yank should describe what it
does.  What it says now hardly qualifies as documentation, and
referring to an internal subroutine for that is, shall we say,
suboptimal ;-)

Feel free to improve the doc string of insert-for-yank.

Thanks.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-09-12 18:32     ` Eli Zaretskii
@ 2016-09-12 22:36       ` Karl Fogel
  2016-09-12 22:54         ` Drew Adams
  0 siblings, 1 reply; 18+ messages in thread
From: Karl Fogel @ 2016-09-12 22:36 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel

Eli Zaretskii <eliz@gnu.org> writes:
>> Thanks, Eli.  Yes, that's true, but note that the doc string for
>> `insert-for-yank' just refers the reader to `insert-for-yank-1' for
>> details.  The only doc string where the STRING-passing behavior is
>> discussed is the doc string of `insert-for-yank-1', and that doc
>> string indicates, or strongly implies, that the entirety of STRING
>> is passed (which it isn't).
>
>Ah, so this is about the doc string of insert-for-yank, not its
>subroutine.

I think that's fair, yes.  It's about the combination of the two doc strings: right now, the `insert-for-yank' doc string just refers the reader to `insert-for-yank-1' for all the interesting stuff.  If your point is that solving this documentation bug involves changing the documentation of `insert-for-yank' more than that of `insert-for-yank-1' (and that the latter might not changing at all), that makes sense, and I thank you for pointing out the real source of the problem.

>I agree that the doc string of insert-for-yank should describe what it
>does.  What it says now hardly qualifies as documentation, and
>referring to an internal subroutine for that is, shall we say,
>suboptimal ;-)

Really, stepping back from the trees to see the forest, that should have been my first reaction :-).

>Feel free to improve the doc string of insert-for-yank.

Will do.

While I don't see any outright errors in the doc string of `insert-for-yank-1', IMHO it should more clearly document that STRING is the default argument to FUNCTION, so I may also fix that.

Thanks, Eli.

-Karl



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

* RE: Question about intended behavior of 'insert-for-yank-1'.
  2016-09-12 22:36       ` Karl Fogel
@ 2016-09-12 22:54         ` Drew Adams
  2016-09-12 23:08           ` Karl Fogel
  2016-10-03  0:53           ` Karl Fogel
  0 siblings, 2 replies; 18+ messages in thread
From: Drew Adams @ 2016-09-12 22:54 UTC (permalink / raw)
  To: Karl Fogel, Eli Zaretskii; +Cc: emacs-devel

> >> Thanks, Eli.  Yes, that's true, but note that the doc string for
> >> `insert-for-yank' just refers the reader to `insert-for-yank-1' for
> >> details.  The only doc string where the STRING-passing behavior is
> >> discussed is the doc string of `insert-for-yank-1', and that doc
> >> string indicates, or strongly implies, that the entirety of STRING
> >> is passed (which it isn't).
> >
> >Ah, so this is about the doc string of insert-for-yank, not its
> >subroutine.
> 
> I think that's fair, yes.  It's about the combination of the two doc
> strings: right now, the `insert-for-yank' doc string just refers the
> reader to `insert-for-yank-1' for all the interesting stuff.  If your
> point is that solving this documentation bug involves changing the
> documentation of `insert-for-yank' more than that of `insert-for-yank-1'
> (and that the latter might not changing at all), that makes sense, and I
> thank you for pointing out the real source of the problem.
> 
> >I agree that the doc string of insert-for-yank should describe what it
> >does.  What it says now hardly qualifies as documentation, and
> >referring to an internal subroutine for that is, shall we say,
> >suboptimal ;-)
> 
> Really, stepping back from the trees to see the forest, that should have
> been my first reaction :-).
> 
> >Feel free to improve the doc string of insert-for-yank.
> 
> Will do.
> 
> While I don't see any outright errors in the doc string of `insert-for-
> yank-1', IMHO it should more clearly document that STRING is the default
> argument to FUNCTION, so I may also fix that.

Bell ringing...

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=286



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-09-12 22:54         ` Drew Adams
@ 2016-09-12 23:08           ` Karl Fogel
  2016-10-03  0:53           ` Karl Fogel
  1 sibling, 0 replies; 18+ messages in thread
From: Karl Fogel @ 2016-09-12 23:08 UTC (permalink / raw)
  To: Drew Adams; +Cc: Eli Zaretskii, emacs-devel

Drew Adams <drew.adams@oracle.com> writes:
>Bell ringing...
>http://debbugs.gnu.org/cgi/bugreport.cgi?bug=286

Nice catch!  Will look at fixing that at the same time.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-09-12 22:54         ` Drew Adams
  2016-09-12 23:08           ` Karl Fogel
@ 2016-10-03  0:53           ` Karl Fogel
  2016-10-03  3:17             ` Noam Postavsky
  2016-10-03  6:44             ` Eli Zaretskii
  1 sibling, 2 replies; 18+ messages in thread
From: Karl Fogel @ 2016-10-03  0:53 UTC (permalink / raw)
  To: Drew Adams; +Cc: Eli Zaretskii, emacs-devel

This is done now (commit 8cd975ceb).  Drew, thanks for pointing out bug #286; I took care of that doc issue in the same commit.

I can't (re)close the bug, because it was closed earlier as "wontfix" and it now says "Bug is archived. No further changes may be made."  However, I agree with your reasoning in the bug report, and infer similar agreement from others' responses in this mailing list thread.

Best regards,
-Karl

Drew Adams <drew.adams@oracle.com> writes:
>> >> Thanks, Eli.  Yes, that's true, but note that the doc string for
>> >> `insert-for-yank' just refers the reader to `insert-for-yank-1' for
>> >> details.  The only doc string where the STRING-passing behavior is
>> >> discussed is the doc string of `insert-for-yank-1', and that doc
>> >> string indicates, or strongly implies, that the entirety of STRING
>> >> is passed (which it isn't).
>> >
>> >Ah, so this is about the doc string of insert-for-yank, not its
>> >subroutine.
>> 
>> I think that's fair, yes.  It's about the combination of the two doc
>> strings: right now, the `insert-for-yank' doc string just refers the
>> reader to `insert-for-yank-1' for all the interesting stuff.  If your
>> point is that solving this documentation bug involves changing the
>> documentation of `insert-for-yank' more than that of `insert-for-yank-1'
>> (and that the latter might not changing at all), that makes sense, and I
>> thank you for pointing out the real source of the problem.
>> 
>> >I agree that the doc string of insert-for-yank should describe what it
>> >does.  What it says now hardly qualifies as documentation, and
>> >referring to an internal subroutine for that is, shall we say,
>> >suboptimal ;-)
>> 
>> Really, stepping back from the trees to see the forest, that should have
>> been my first reaction :-).
>> 
>> >Feel free to improve the doc string of insert-for-yank.
>> 
>> Will do.
>> 
>> While I don't see any outright errors in the doc string of `insert-for-
>> yank-1', IMHO it should more clearly document that STRING is the default
>> argument to FUNCTION, so I may also fix that.
>
>Bell ringing...
>
>http://debbugs.gnu.org/cgi/bugreport.cgi?bug=286



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-03  0:53           ` Karl Fogel
@ 2016-10-03  3:17             ` Noam Postavsky
  2016-10-03  6:53               ` Eli Zaretskii
  2016-10-03 19:21               ` Karl Fogel
  2016-10-03  6:44             ` Eli Zaretskii
  1 sibling, 2 replies; 18+ messages in thread
From: Noam Postavsky @ 2016-10-03  3:17 UTC (permalink / raw)
  To: Karl Fogel; +Cc: Eli Zaretskii, Drew Adams, Emacs developers

On Sun, Oct 2, 2016 at 8:53 PM, Karl Fogel <kfogel@red-bean.com> wrote:
>
> I can't (re)close the bug, because it was closed earlier as "wontfix" and it now says "Bug is archived. No further changes may be made."

It looks like the "wontfix" tag wasn't actually applied. You can send
a message to <control@debbugs.gnu.org> with contents

unarchive 286
tag 286 fixed
quit

To mark it as "fixed". See https://debbugs.gnu.org/server-control.html
for more options/details.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-03  0:53           ` Karl Fogel
  2016-10-03  3:17             ` Noam Postavsky
@ 2016-10-03  6:44             ` Eli Zaretskii
  2016-10-03 19:17               ` Karl Fogel
  1 sibling, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2016-10-03  6:44 UTC (permalink / raw)
  To: Karl Fogel; +Cc: drew.adams, emacs-devel

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Sun, 02 Oct 2016 19:53:45 -0500
> 
> This is done now (commit 8cd975ceb).

Thanks, but you pushed this to the wrong branch, it should have been
pushed to emacs-25, as all doc fixes relevant to Emacs 25 are.

Please cherry-pick the commit from master to emacs-25.

Thanks.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-03  3:17             ` Noam Postavsky
@ 2016-10-03  6:53               ` Eli Zaretskii
  2016-10-03 19:21               ` Karl Fogel
  1 sibling, 0 replies; 18+ messages in thread
From: Eli Zaretskii @ 2016-10-03  6:53 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: kfogel, drew.adams, emacs-devel

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Sun, 2 Oct 2016 23:17:30 -0400
> Cc: Eli Zaretskii <eliz@gnu.org>, Drew Adams <drew.adams@oracle.com>,
> 	Emacs developers <emacs-devel@gnu.org>
> 
> On Sun, Oct 2, 2016 at 8:53 PM, Karl Fogel <kfogel@red-bean.com> wrote:
> >
> > I can't (re)close the bug, because it was closed earlier as "wontfix" and it now says "Bug is archived. No further changes may be made."
> 
> It looks like the "wontfix" tag wasn't actually applied. You can send
> a message to <control@debbugs.gnu.org> with contents
> 
> unarchive 286
> tag 286 fixed
> quit
> 
> To mark it as "fixed". See https://debbugs.gnu.org/server-control.html
> for more options/details.

I frequently wish for debbugs to be more helpful and not require such
subtle techniques.  It looks like it always takes the dumbest possible
interpretation of my commands, when there's a slight problem with it.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-03  6:44             ` Eli Zaretskii
@ 2016-10-03 19:17               ` Karl Fogel
  2016-10-04  6:03                 ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Karl Fogel @ 2016-10-03 19:17 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: drew.adams, emacs-devel

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
>> Date: Sun, 02 Oct 2016 19:53:45 -0500
>> 
>> This is done now (commit 8cd975ceb).
>
>Thanks, but you pushed this to the wrong branch, it should have been
>pushed to emacs-25, as all doc fixes relevant to Emacs 25 are.
>
>Please cherry-pick the commit from master to emacs-25.

Done (commit e1b2918c7c in emacs-25).

The "Branches" section in CONTRIBUTE makes a distinction between "development" and "bug fixes".  I think of this change as more of a background improvement -- of the sort that just needs to get into Emacs eventually -- than as a bug fix of the sort meant by CONTRIBUTE.  However, your mail implies that CONTRIBUTE means to include this sort of thing in the category "bug fix" too.  You probably have a better handle on the intentions behind the language in CONTRIBUTE than I do, so I'll re-interpret that section accordingly.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-03  3:17             ` Noam Postavsky
  2016-10-03  6:53               ` Eli Zaretskii
@ 2016-10-03 19:21               ` Karl Fogel
  1 sibling, 0 replies; 18+ messages in thread
From: Karl Fogel @ 2016-10-03 19:21 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: Eli Zaretskii, Drew Adams, Emacs developers

Noam Postavsky <npostavs@users.sourceforge.net> writes:
>> I can't (re)close the bug, because it was closed earlier as
>> "wontfix" and it now says "Bug is archived. No further changes may
>> be made."
>
>It looks like the "wontfix" tag wasn't actually applied. You can send
>a message to <control@debbugs.gnu.org> with contents
>
>unarchive 286
>tag 286 fixed
>quit
>
>To mark it as "fixed". See https://debbugs.gnu.org/server-control.html
>for more options/details.

Thank you.  I did exactly as you suggested, by rote.  (The debbugs email interface is not something I plan to devote much mental real estate too, but if you've given me the exact commands, I'd feel churlish not to send them :-) .)

It seems to have worked:  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=286



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-03 19:17               ` Karl Fogel
@ 2016-10-04  6:03                 ` Eli Zaretskii
  2016-10-04 17:40                   ` Karl Fogel
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2016-10-04  6:03 UTC (permalink / raw)
  To: Karl Fogel; +Cc: drew.adams, emacs-devel

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: drew.adams@oracle.com,  emacs-devel@gnu.org
> Date: Mon, 03 Oct 2016 14:17:41 -0500
> 
> >Please cherry-pick the commit from master to emacs-25.
> 
> Done (commit e1b2918c7c in emacs-25).

Thanks.

> The "Branches" section in CONTRIBUTE makes a distinction between "development" and "bug fixes".  I think of this change as more of a background improvement -- of the sort that just needs to get into Emacs eventually -- than as a bug fix of the sort meant by CONTRIBUTE.  However, your mail implies that CONTRIBUTE means to include this sort of thing in the category "bug fix" too.  You probably have a better handle on the intentions behind the language in CONTRIBUTE than I do, so I'll re-interpret that section accordingly.

You need to understand the logic behind the instructions in
CONTRIBUTE: the release branch should only get fixes that are either
necessary, or are safe, i.e. cannot possibly break the release.  And
doc changes obviously belong to this latter class.

I added a sentence about doc fixes to CONTRIBUTE.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-04  6:03                 ` Eli Zaretskii
@ 2016-10-04 17:40                   ` Karl Fogel
  2016-10-04 18:19                     ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Karl Fogel @ 2016-10-04 17:40 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: drew.adams, emacs-devel

Eli Zaretskii <eliz@gnu.org> writes:
>> The "Branches" section in CONTRIBUTE makes a distinction between
>> "development" and "bug fixes".  I think of this change as more of a
>> background improvement -- of the sort that just needs to get into
>> Emacs eventually -- than as a bug fix of the sort meant by
>> CONTRIBUTE.  However, your mail implies that CONTRIBUTE means to
>> include this sort of thing in the category "bug fix" too.  You
>> probably have a better handle on the intentions behind the language
>> in CONTRIBUTE than I do, so I'll re-interpret that section
>> accordingly.
>
>You need to understand the logic behind the instructions in
>CONTRIBUTE: the release branch should only get fixes that are either
>necessary, or are safe, i.e. cannot possibly break the release.  And
>doc changes obviously belong to this latter class.

I understand that logic.  However, do you mean s/only/always/ above?

The question is whether it is wrong to commit it to master instead, not whether it would have been right to commit it to emacs-25.  A non-urgent doc fix that lands on master will eventually "get into Emacs", when new release lines are created.  This was good enough for me.

You seem to be saying that, once the safe doc fix is available, it should always be committed to the current release branch.  For the committer, this would add the mental overhead of figuring out whether or not a particular release branch is in a freeze state or not.  This is actually non-trivial to figure out, since it's not maintained as a visible flag in some designated place, but rather as a thread on the mailing list, which I then have to go search for.

So for non-urgent stuff like this, I find it easier to just commit to master, because then I don't have to worry about the state of the current release branch.  I can start worrying about it, if you think it's important.  I just wanted to clarify why I aimed this change at master, and to see if my explanation affects the course of action you recommend for situations like this.  (When I want a fix to go into the current release, then I expend the extra effort to determine whether I can put it on the release branch.)

Now, *maybe* it is the case that a super-safe change like this is allowed onto a release branch even when that branch is frozen.  But I don't know that for sure, and CONTRIBUTE doesn't say.  Maybe this is just a doc bug in CONTRIBUTE and we should fix it.

IOW, committing to master wasn't an accident; it resulted from a balancing of priorities.

>I added a sentence about doc fixes to CONTRIBUTE.

That will help either way.  Thank you.

Best regards,
-Karl



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-04 17:40                   ` Karl Fogel
@ 2016-10-04 18:19                     ` Eli Zaretskii
  2016-10-04 20:16                       ` Davis Herring
  2016-10-04 21:04                       ` Karl Fogel
  0 siblings, 2 replies; 18+ messages in thread
From: Eli Zaretskii @ 2016-10-04 18:19 UTC (permalink / raw)
  To: Karl Fogel; +Cc: drew.adams, emacs-devel

> From: Karl Fogel <kfogel@red-bean.com>
> Cc: drew.adams@oracle.com,  emacs-devel@gnu.org
> Date: Tue, 04 Oct 2016 12:40:49 -0500
> 
> >You need to understand the logic behind the instructions in
> >CONTRIBUTE: the release branch should only get fixes that are either
> >necessary, or are safe, i.e. cannot possibly break the release.  And
> >doc changes obviously belong to this latter class.
> 
> I understand that logic.  However, do you mean s/only/always/ above?

I don't see the difference.  "Only" means "always" in this case.

> The question is whether it is wrong to commit it to master instead, not whether it would have been right to commit it to emacs-25.

It is "wrong" in the sense that committing to master will prevent the
next release from having the fix, for no good reason, but committing
to the release branch will never prevent master from having it.

> A non-urgent doc fix that lands on master will eventually "get into Emacs", when new release lines are created.  This was good enough for me.

Doc fixes are always "urgent", in the sense that there's no reason to
delay them.  They can never do any harm.

> You seem to be saying that, once the safe doc fix is available, it should always be committed to the current release branch.

Yes.  And not just "safe" doc fix: _any_ doc fix.  Doc fixes are by
definition safe.

> For the committer, this would add the mental overhead of figuring out whether or not a particular release branch is in a freeze state or not.

The freeze state is never relevant to doc fixes.  It is only relevant
to code changes.  The purpose of a freeze is to prevent unsafe
changes, and doc changes are never unsafe.

> This is actually non-trivial to figure out, since it's not maintained as a visible flag in some designated place, but rather as a thread on the mailing list, which I then have to go search for.

If you have trouble with that, just ask before pushing.  I think the
decision is very simple, but if it isn't, John and myself are here to
help.

> So for non-urgent stuff like this, I find it easier to just commit to master, because then I don't have to worry about the state of the current release branch.

Once again, doc fixes are always "urgent".  It may be easier for you
to always commit to master, but that's not our process here, so it
means someone else will have to watch your commits, detect problems
like this, and then cherry-pick for you, or ask you to do that.  It's
a burden we'd like to avoid, and I think the rules are simple enough
to allow anyone to follow them seamlessly.  And if they are not simple
enough, please just ask when in doubt.

> I can start worrying about it, if you think it's important.

It's important, yes.

> (When I want a fix to go into the current release, then I expend the extra effort to determine whether I can put it on the release branch.)

No such considerations are necessary for documentation changes.  the
only reason not to commit a doc fix to the release branch is if the
fix is relevant to behavior that's only available on master.

> Now, *maybe* it is the case that a super-safe change like this is allowed onto a release branch even when that branch is frozen.

Doc fixes are _always_ safe.

> But I don't know that for sure, and CONTRIBUTE doesn't say.  Maybe this is just a doc bug in CONTRIBUTE and we should fix it.

CONTRIBUTE already does say that, I made the change there today.

> IOW, committing to master wasn't an accident; it resulted from a balancing of priorities.

I didn't think it was an accident, I just explained why your decision
was incorrect.  I understand that it was a good-faith mistake, so no
sweat.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-04 18:19                     ` Eli Zaretskii
@ 2016-10-04 20:16                       ` Davis Herring
  2016-10-04 21:04                       ` Karl Fogel
  1 sibling, 0 replies; 18+ messages in thread
From: Davis Herring @ 2016-10-04 20:16 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Karl Fogel, Drew Adams, emacs-devel

>>> You need to understand the logic behind the instructions in
>>> CONTRIBUTE: the release branch should only get fixes that are either
>>> necessary, or are safe, i.e. cannot possibly break the release.  And
>>> doc changes obviously belong to this latter class.
>>
>> I understand that logic.  However, do you mean s/only/always/ above?
>
> I don't see the difference.  "Only" means "always" in this case.

The two possible meanings are "the release branch should get only 
fixes..." or "only the release branch should get fixes" (which is 
equivalent to "the release branch should get all fixes").

Of course, both can be true (iff).

Davis

-- 
This product is sold by volume, not by mass.  If it appears too dense or 
too sparse, it is because mass-energy conversion has occurred during 
shipping.



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

* Re: Question about intended behavior of 'insert-for-yank-1'.
  2016-10-04 18:19                     ` Eli Zaretskii
  2016-10-04 20:16                       ` Davis Herring
@ 2016-10-04 21:04                       ` Karl Fogel
  1 sibling, 0 replies; 18+ messages in thread
From: Karl Fogel @ 2016-10-04 21:04 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: drew.adams, emacs-devel

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Karl Fogel <kfogel@red-bean.com>
>> Cc: drew.adams@oracle.com,  emacs-devel@gnu.org
>> Date: Tue, 04 Oct 2016 12:40:49 -0500
>> 
>> >You need to understand the logic behind the instructions in
>> >CONTRIBUTE: the release branch should only get fixes that are either
>> >necessary, or are safe, i.e. cannot possibly break the release.  And
>> >doc changes obviously belong to this latter class.
>> 
>> I understand that logic.  However, do you mean s/only/always/ above?
>
>I don't see the difference.  "Only" means "always" in this case.

Ah, I see what happened, I think.  This is just an English usage issue.

In this context, a native speaker would (usually) interpret "only" as meaning: "the only circumstances under which you should commit a fix to a release branch are XYZ".  But you meant it like this: "in circumstances XYZ, you should commit only to the release branch, and not to the master branch".

If you take your original sentence and substitute "always" for "only", it will convey what you intended it to mean.

>> But I don't know that for sure, and CONTRIBUTE doesn't say.  Maybe
>> this is just a doc bug in CONTRIBUTE and we should fix it.
>
>CONTRIBUTE already does say that, I made the change there today.

I clarified in commit 4b347fe5 (on emacs-25) that this applies even when the release branch is in feature freeze.  Now there can be no misunderstanding.

Thanks,
-Karl



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

end of thread, other threads:[~2016-10-04 21:04 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-12  5:17 Question about intended behavior of 'insert-for-yank-1' Karl Fogel
2016-09-12 16:59 ` Eli Zaretskii
2016-09-12 17:15   ` Karl Fogel
2016-09-12 18:32     ` Eli Zaretskii
2016-09-12 22:36       ` Karl Fogel
2016-09-12 22:54         ` Drew Adams
2016-09-12 23:08           ` Karl Fogel
2016-10-03  0:53           ` Karl Fogel
2016-10-03  3:17             ` Noam Postavsky
2016-10-03  6:53               ` Eli Zaretskii
2016-10-03 19:21               ` Karl Fogel
2016-10-03  6:44             ` Eli Zaretskii
2016-10-03 19:17               ` Karl Fogel
2016-10-04  6:03                 ` Eli Zaretskii
2016-10-04 17:40                   ` Karl Fogel
2016-10-04 18:19                     ` Eli Zaretskii
2016-10-04 20:16                       ` Davis Herring
2016-10-04 21:04                       ` Karl Fogel

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.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).