bug#31446: 26.1; sql-interactive-mode buffer naming is broken

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Phil Sainty]

Depending on whether/how many C-u prefix arguments I supply to
sql-postgres, the resulting buffer name is bad.

M-x sql-postgres: "*SQL: *"
C-u M-x sql-postgres: "*SQL: ^D*"
C-u C-u M-x sql-postgres: "*SQL: ^P*"

I've converted them for the bug report, but the ^D and ^P are actual
control characters, which I observe are characters 4 and 16
respectively, and therefore are a match for the numeric value of the
supplied prefix argument.  This is surely not what was intended.

In 25.3 the buffer name would be "*SQL*" with no prefix argument, and if
if a prefix argument was given I would be prompted for the buffer name,
with "*SQL: <user>@<database>*" as the default suggestion.


-Phil





In GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, X toolkit, Xaw scroll 
bars)
  of 2018-04-11 built on mts-devtools
Windowing system distributor 'The X.Org Foundation', version 
11.0.11501000
System Description:	Ubuntu 14.04.5 LTS

Recent messages:
funcall-interactively: End of buffer [3 times]
Login...done
funcall-interactively: End of buffer
Entering debugger...
Continuing.
custom-initialize-reset: Symbol’s value as variable is void: buffer-name
"*SQL: \x10*"
kill-region: Buffer is read-only: #<buffer *Messages*>
Scanning for dabbrevs...done
user-error: No dynamic expansion for ‘sql-int’ found

Configured using:
  'configure --prefix=/usr/local/src/emacs/26.1/usr/local
  --with-x-toolkit=lucid --without-sound'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK DBUS GSETTINGS NOTIFY GNUTLS
LIBXML2 FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11 THREADS LCMS2

Important settings:
   value of $LANG: en_NZ.UTF-8
   locale-coding-system: utf-8-unix

Major mode: Messages

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   buffer-read-only: t
   line-number-mode: t
   transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr dabbrev emacsbug message rmc puny seq dired
dired-loaddefs format-spec rfc822 mml mml-sec password-cache epa derived
epg epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils help-mode
cl-print byte-opt gv bytecomp byte-compile cconv debug sql easymenu view
thingatpt comint ansi-color ring cl-loaddefs cl-lib elec-pair time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 108297 11486)
  (symbols 48 21234 1)
  (miscs 40 119 276)
  (strings 32 31999 1264)
  (string-bytes 1 893910)
  (vectors 16 15325)
  (vector-slots 8 506954 12212)
  (floats 8 63 171)
  (intervals 56 329 0)
  (buffers 992 16)
  (heap 1024 45845 3308))

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Mon, 14 May 2018 17:54:52 +1200
> From: Phil Sainty <psainty@orcon.net.nz>
> 
> Depending on whether/how many C-u prefix arguments I supply to
> sql-postgres, the resulting buffer name is bad.
> 
> M-x sql-postgres: "*SQL: *"
> C-u M-x sql-postgres: "*SQL: ^D*"
> C-u C-u M-x sql-postgres: "*SQL: ^P*"
> 
> I've converted them for the bug report, but the ^D and ^P are actual
> control characters, which I observe are characters 4 and 16
> respectively, and therefore are a match for the numeric value of the
> supplied prefix argument.  This is surely not what was intended.

I cannot use SQL here; does the patch below give good results?

diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index e4db6cc..dd96810 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -4263,9 +4263,17 @@ sql-product-interactive
                 (funcall (sql-get-product-feature product :sqli-comint-func)
                          product
                          (sql-get-product-feature product :sqli-options)
-                         (if (and new-name (string-prefix-p "SQL" new-name t))
-                             new-name
-                           (concat "SQL: " new-name))))
+                         (when new-name
+                           (cond
+                            ((stringp new-name)
+                             (if (string-prefix-p "SQL" new-name t)
+                                 new-name
+                               (concat "SQL: " new-name)))
+                            ((eq new-name '(4))
+                             (sql-rename-buffer new-name)
+                             sql-alternate-buffer-name)
+                            (t
+                             (format "SQL: %s" new-name))))))
 
               ;; Set SQLi mode.
               (let ((sql-interactive-product product))

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Fri, 18 May 2018 12:49:28 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 31446@debbugs.gnu.org
> 
> does the patch below give good results?

Sorry, that patch left out an important use case.  Please try the one
below.

(Is it correct to use "*SQL*" when no prefix arg is given?)

diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index e4db6cc..2ced15c 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -4263,9 +4263,18 @@ sql-product-interactive
                 (funcall (sql-get-product-feature product :sqli-comint-func)
                          product
                          (sql-get-product-feature product :sqli-options)
-                         (if (and new-name (string-prefix-p "SQL" new-name t))
-                             new-name
-                           (concat "SQL: " new-name))))
+                         (cond
+                          ((zerop new-name)
+                           "*SQL*")
+                          ((stringp new-name)
+                           (if (string-prefix-p "SQL" new-name t)
+                               new-name
+                             (concat "*SQL: " new-name "*")))
+                          ((eq new-name '(4))
+                           (sql-rename-buffer new-name)
+                           sql-alternate-buffer-name)
+                          (t
+                           (format "*SQL: %s*" new-name)))))
 
               ;; Set SQLi mode.
               (let ((sql-interactive-product product))

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Filipp Gunbin]

On 18/05/2018 15:19 +0300, Eli Zaretskii wrote:
...
>                  (funcall (sql-get-product-feature product :sqli-comint-func)
>                           product
>                           (sql-get-product-feature product :sqli-options)
> -                         (if (and new-name (string-prefix-p "SQL" new-name t))
> -                             new-name
> -                           (concat "SQL: " new-name))))
> +                         (cond
> +                          ((zerop new-name)
> +                           "*SQL*")
> +                          ((stringp new-name)
> +                           (if (string-prefix-p "SQL" new-name t)
> +                               new-name
> +                             (concat "*SQL: " new-name "*")))
> +                          ((eq new-name '(4))
> +                           (sql-rename-buffer new-name)
> +                           sql-alternate-buffer-name)
> +                          (t
> +                           (format "*SQL: %s*" new-name)))))
...

But `sql-comint' (called from `sql-comint-postgres' and the like) has
logic which prepares buffer name (adding those asterisks etc.)

In `sql-product-interactive', `new-name', if set to '(4) from prefix, or
to some other value from argument, controls this:

          (if (and (not new-name) buf)
              (pop-to-buffer buf)

So if `new-name' is set, we shouldn't switch to existing buffer.

Maybe just pass `new-name' to comint func like this (undoing possible
setting of `new-name' to '(4) above), letting it do all the
name-choosing work?

                (funcall (sql-get-product-feature product :sqli-comint-func)
                         product
                         (sql-get-product-feature product :sqli-options)
                         (if (equal '(4) new-name)
                            nil
                          new-name))

Filipp

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> From: Filipp Gunbin <fgunbin@fastmail.fm>
> Cc: psainty@orcon.net.nz,  31446@debbugs.gnu.org
> Date: Fri, 18 May 2018 18:44:38 +0300
> 
> But `sql-comint' (called from `sql-comint-postgres' and the like) has
> logic which prepares buffer name (adding those asterisks etc.)
> 
> In `sql-product-interactive', `new-name', if set to '(4) from prefix, or
> to some other value from argument, controls this:
> 
>           (if (and (not new-name) buf)
>               (pop-to-buffer buf)
> 
> So if `new-name' is set, we shouldn't switch to existing buffer.

I'm sorry, but I don't understand the issue you are alluding to.  The
bug I tried to fix was that the buffer got named incorrectly.  Is that
fixed by the patch?  If it isn't, please tell why.

If the patch does fix the bug, then the issues you mention are
separate problems, because I don't think I changed the logic of which
buffer is going to become the current.

What am I missing?

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Filipp Gunbin]

On 18/05/2018 19:04 +0300, Eli Zaretskii wrote:

>> From: Filipp Gunbin <fgunbin@fastmail.fm>
>> Cc: psainty@orcon.net.nz,  31446@debbugs.gnu.org
>> Date: Fri, 18 May 2018 18:44:38 +0300
>>
>> But `sql-comint' (called from `sql-comint-postgres' and the like) has
>> logic which prepares buffer name (adding those asterisks etc.)
>>
>> In `sql-product-interactive', `new-name', if set to '(4) from prefix, or
>> to some other value from argument, controls this:
>>
>>           (if (and (not new-name) buf)
>>               (pop-to-buffer buf)
>>
>> So if `new-name' is set, we shouldn't switch to existing buffer.
>
> I'm sorry, but I don't understand the issue you are alluding to.  The
> bug I tried to fix was that the buffer got named incorrectly.  Is that
> fixed by the patch?  If it isn't, please tell why.
>
> If the patch does fix the bug, then the issues you mention are
> separate problems, because I don't think I changed the logic of which
> buffer is going to become the current.
>
> What am I missing?

I was trying to say that probably this function should not mess with the
buffer name at all.

Your patch, while it may have fixed the bug, introduces some logic on
setting buffer name - so we could get undesired behaviour change.

While a simpler approach would be just to pass `new-name' like I
proposed.

I'm not the person who filed this bug, but I also suffer from sqli-mode
buffers naming problem :-)

Sorry if it's actually me who's missing something.

Filipp

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> From: Filipp Gunbin <fgunbin@fastmail.fm>
> Cc: psainty@orcon.net.nz,  31446@debbugs.gnu.org
> Date: Fri, 18 May 2018 21:03:00 +0300
> 
> Your patch, while it may have fixed the bug, introduces some logic on
> setting buffer name - so we could get undesired behaviour change.

AFAIK, the logic was already there, I just fixed it to behave more
reasonably.

The original code was

                         (if (and new-name (string-prefix-p "SQL" new-name t))
                             new-name
                           (concat "SQL: " new-name))))

This is now
                         (cond
                          ((zerop new-name)
                           "*SQL*")
                          ((stringp new-name)
                           (if (string-prefix-p "SQL" new-name t)
                               new-name
                             (concat "*SQL: " new-name "*")))
                          ((eq new-name '(4))
                           (sql-rename-buffer new-name)
                           sql-alternate-buffer-name)
                          (t
                           (format "*SQL: %s*" new-name)))))

which (a) avoids concatenating a string and a list '(4); (b) avoids
calling string-prefix-p with 2nd arg not a string, something that
works only by sheer luck; (c) calls sql-rename-buffer to choose the
buffer name, because that function implements the logic of naming the
buffer, and duplicating that sounds redundant; (d) does something
reasonable when new-name is neither a string nor a prefix arg nor nil.

And that is the only thing that I changed.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Phil Sainty]

Hi Eli,

Apologies for lack of testing; I was away while the fix was
being discussed.

I'll need to test more thoroughly later, but I can tell you
that the current code isn't working, as the (zerop new-name)
condition produces code like (= 0 nil) or (= 0 '(4)) which
results in, e.g., (wrong-type-argument number-or-marker-p (4))

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Mon, 21 May 2018 10:17:08 +1200
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: fgunbin@fastmail.fm, 31446@debbugs.gnu.org
> 
> I'll need to test more thoroughly later, but I can tell you
> that the current code isn't working, as the (zerop new-name)
> condition produces code like (= 0 nil) or (= 0 '(4)) which
> results in, e.g., (wrong-type-argument number-or-marker-p (4))

Sorry, that was a stupid typo, now fixed.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Phil Sainty]

On 2018-05-21 14:35, Eli Zaretskii wrote:
> Sorry, that was a stupid typo, now fixed.

No error now, but there's still a bug.

(eq new-name '(4)) is never true, so it falls through to
(format "*SQL: %s*" new-name) when a prefix arg is supplied.

I *think* we'd want (consp new-name) for that test (especially as
it ought to cover multiple uses of C-u), *however* doing this then
triggers error "Current buffer is not a SQL interactive buffer" in
`sql-rename-buffer', as this is happening *before* the call to the
:sqli-comint-func function which creates the buffer.

I've briefly tested Filipp's suggestion, but that seemed to result in
just *SQL* as a buffer name regardless of the prefix argument, rather
than causing it to prompt for the name (however that might turn out to
be sane in conjunction with the additional changes I've made below?)

The following is working from initial/cursory testing, but it needs
more testing/confirmation at minimum.


diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index 1b2cdaf5f6..45ce9154b0 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -4264,14 +4264,13 @@ sql-product-interactive
                           product
                           (sql-get-product-feature product 
:sqli-options)
                           (cond
-                          ((null new-name)
+                          ((or (null new-name)
+                               (consp new-name))
                             "*SQL*")
                            ((stringp new-name)
                             (if (string-prefix-p "*SQL: " new-name t)
                                 new-name
                               (concat "*SQL: " new-name "*")))
-                          ((eq new-name '(4))
-                           (sql-rename-buffer new-name))
                            (t
                             (format "*SQL: %s*" new-name)))))

@@ -4279,6 +4278,10 @@ sql-product-interactive
                (let ((sql-interactive-product product))
                  (sql-interactive-mode))

+              ;; Prompt for the buffer name if a prefix argument was 
given.
+              (when (consp new-name)
+                (sql-rename-buffer new-name))
+
                ;; Set the new buffer name
                (setq new-sqli-buffer (current-buffer))
                (set (make-local-variable 'sql-buffer)


Setting just "*SQL*" initially in the case of (consp new-name) is to
prevent the comint process name from ending up as, e.g., "SQL: (4)",
which is particularly noticeable when exiting the process, to the
message: "Process SQL: (4) finished"

Comparing with Emacs 25.3, this would simply say "Process SQL
finished", and using "*SQL*" in the new code appears to give this
result.


This bug could probably use some attention from whoever made the
changes to how this was in Emacs 25, which I think was this commit:

commit c5a31f8292c94d19b80a3dbe0b3026693cc1090e
Author: Michael R. Mauger <michael@mauger.com>
Date:   Mon Mar 20 23:26:53 2017 -0400

Ccing Michael -- See
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31446


-Phil

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Tue, 22 May 2018 00:09:36 +1200
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: 31446@debbugs.gnu.org, fgunbin@fastmail.fm, bug-gnu-emacs
>  <bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org>, "Michael R. Mauger"
>  <michael@mauger.com>
> 
> (eq new-name '(4)) is never true, so it falls through to
> (format "*SQL: %s*" new-name) when a prefix arg is supplied.

Right, thanks.

> I *think* we'd want (consp new-name) for that test (especially as
> it ought to cover multiple uses of C-u), *however* doing this then
> triggers error "Current buffer is not a SQL interactive buffer" in
> `sql-rename-buffer', as this is happening *before* the call to the
> :sqli-comint-func function which creates the buffer.

Right, so we need to pull the relevant stuff out of that function.

> I've briefly tested Filipp's suggestion, but that seemed to result in
> just *SQL* as a buffer name regardless of the prefix argument, rather
> than causing it to prompt for the name (however that might turn out to
> be sane in conjunction with the additional changes I've made below?)
> 
> The following is working from initial/cursory testing, but it needs
> more testing/confirmation at minimum.

Thanks, I tried fixing it in a slightly different way, please take a
look.  I'd like to avoid releasing Emacs 26.1 with sql.el that was
broken by latest changes.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Phil Sainty]

On 2018-05-22 03:45, Eli Zaretskii wrote:
> Thanks, I tried fixing it in a slightly different way, please take a
> look.  I'd like to avoid releasing Emacs 26.1 with sql.el that was
> broken by latest changes.

That seems to work, except for regressing this bit:

>> I *think* we'd want (consp new-name) for that test (especially as
>> it ought to cover multiple uses of C-u)

With C-u C-u M-x sql-postgres I end up with buffer name "*SQL: (16)*"
and "Process SQL: (16) finished" etc.

Another difference from Emacs 25 is that using a prefix arg in Emacs 25
was guaranteed to create a new buffer/process (if the chosen buffer name
conflicted then it was uniquified), whereas the new code is switching to
the pre-existing buffer in the situation.

Calling `sql-rename-buffer' handles uniquification, and my suggested
code seems to be consistent with the old behaviour.

Hopefully Filipp (or others) can do some testing as well -- I've not 
been
as thorough as I would like, so it would be good to have others 
verifying
the changes too.


-Phil

As a side-note, I've just observed that killing SQLi buffers from the
`list-buffers' buffer triggers "error in process sentinel: Selecting
deleted buffer" after confirming that the process should be killed
(which prevents marking and killing *multiple* such buffers).  I see
this also happens in Emacs 25, however.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Tue, 22 May 2018 10:08:05 +1200
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: 31446@debbugs.gnu.org, fgunbin@fastmail.fm,
>  bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org, michael@mauger.com
> 
> On 2018-05-22 03:45, Eli Zaretskii wrote:
> > Thanks, I tried fixing it in a slightly different way, please take a
> > look.  I'd like to avoid releasing Emacs 26.1 with sql.el that was
> > broken by latest changes.
> 
> That seems to work, except for regressing this bit:
> 
> >> I *think* we'd want (consp new-name) for that test (especially as
> >> it ought to cover multiple uses of C-u)
> 
> With C-u C-u M-x sql-postgres I end up with buffer name "*SQL: (16)*"
> and "Process SQL: (16) finished" etc.

What is "C-u C-u" sup[posed to achieve?  It isn't called out in the
doc strings, AFAICS, is it?

> Another difference from Emacs 25 is that using a prefix arg in Emacs 25
> was guaranteed to create a new buffer/process (if the chosen buffer name
> conflicted then it was uniquified), whereas the new code is switching to
> the pre-existing buffer in the situation.

This will have to be dealt with after Emacs 26.1 release.

Thanks.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Phil Sainty]

On 2018-05-22 14:37, Eli Zaretskii wrote:
> What is "C-u C-u" supposed to achieve?  It isn't called out in the
> doc strings, AFAICS, is it?

Well that's an excellent question.  `sql-product-interactive' does 
support
C-u C-u, but I realise now that that `product' is its prefix arg and in 
the
case of C-u C-u it sets a value of '(4) for `new-name'; so you're right 
--
there's no particular need to support other values.  Personally I would 
still
be inclined to use `consp' just so that additional C-u presses didn't 
result
in the odd names that you currently get, but that's obviously not a 
major
concern.


>> Another difference from Emacs 25 is that using a prefix arg in Emacs 
>> 25
>> was guaranteed to create a new buffer/process (if the chosen buffer 
>> name
>> conflicted then it was uniquified), whereas the new code is switching 
>> to
>> the pre-existing buffer in the situation.
> 
> This will have to be dealt with after Emacs 26.1 release.

Ok.


-Phil

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Filipp Gunbin]

On 18/05/2018 23:24 +0300, Eli Zaretskii wrote:

>> From: Filipp Gunbin <fgunbin@fastmail.fm>
>> Cc: psainty@orcon.net.nz,  31446@debbugs.gnu.org
>> Date: Fri, 18 May 2018 21:03:00 +0300
>>
>> Your patch, while it may have fixed the bug, introduces some logic on
>> setting buffer name - so we could get undesired behaviour change.
>
> AFAIK, the logic was already there, I just fixed it to behave more
> reasonably.
>
> The original code was
>
>                          (if (and new-name (string-prefix-p "SQL" new-name t))
>                              new-name
>                            (concat "SQL: " new-name))))
>
> This is now
>                          (cond
>                           ((zerop new-name)
>                            "*SQL*")
>                           ((stringp new-name)
>                            (if (string-prefix-p "SQL" new-name t)
>                                new-name
>                              (concat "*SQL: " new-name "*")))
>                           ((eq new-name '(4))
>                            (sql-rename-buffer new-name)
>                            sql-alternate-buffer-name)
>                           (t
>                            (format "*SQL: %s*" new-name)))))
>
> which (a) avoids concatenating a string and a list '(4); (b) avoids
> calling string-prefix-p with 2nd arg not a string, something that
> works only by sheer luck; (c) calls sql-rename-buffer to choose the
> buffer name, because that function implements the logic of naming the
> buffer, and duplicating that sounds redundant; (d) does something
> reasonable when new-name is neither a string nor a prefix arg nor nil.
>
> And that is the only thing that I changed.

That looks reasonable, but, as I wrote earlier, why implement
name-choosing logic here, if it's in `sql-comint' already.  Even if some
of such logic was here before the fix, it doesn't necessarily mean that
was right.

>                           ((eq new-name '(4))
>                            (sql-rename-buffer new-name)
>                            sql-alternate-buffer-name)

`sql-alternate-buffer-name' not always matches the effective name set by
`sql-rename-buffer'..

I'll test my suggestion tomorrow and will write about the result, if you
don't mind.

Filipp

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> From: Filipp Gunbin <fgunbin@fastmail.fm>
> Cc: psainty@orcon.net.nz,  31446@debbugs.gnu.org
> Date: Mon, 21 May 2018 02:53:08 +0300
> 
> That looks reasonable, but, as I wrote earlier, why implement
> name-choosing logic here, if it's in `sql-comint' already.  Even if some
> of such logic was here before the fix, it doesn't necessarily mean that
> was right.

Maybe I'm missing something, but it looks like sql-comint and
sql-product-interactive belong to 2 different families of entry points
in the package.  Some call sql-product-interactive, while the others
call sql-comint.  Isn't that so?

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Filipp Gunbin]

On 21/05/2018 17:55 +0300, Eli Zaretskii wrote:

>> From: Filipp Gunbin <fgunbin@fastmail.fm>
>> Cc: psainty@orcon.net.nz,  31446@debbugs.gnu.org
>> Date: Mon, 21 May 2018 02:53:08 +0300
>>
>> That looks reasonable, but, as I wrote earlier, why implement
>> name-choosing logic here, if it's in `sql-comint' already.  Even if some
>> of such logic was here before the fix, it doesn't necessarily mean that
>> was right.
>
> Maybe I'm missing something, but it looks like sql-comint and
> sql-product-interactive belong to 2 different families of entry points
> in the package.  Some call sql-product-interactive, while the others
> call sql-comint.  Isn't that so?

sql-product-interactive is one of the entry points (a generic one).
Other entry points are sql-postgres and the like, which specify product.

Well, this patch works for me.

If I call `C-u C-u sql-product-interactive' multiple times, then first I
get *SQL* buffer, then *SQL-postgres* buffer, then *SQL-postgres1* and
so on.  With just C-u I am switched to existing buffer.

Filipp


diff --git a/lisp/progmodes/sql.el b/lisp/progmodes/sql.el
index 64651aff11..856250c194 100644
--- a/lisp/progmodes/sql.el
+++ b/lisp/progmodes/sql.el
@@ -4196,10 +4196,8 @@ sql-product-interactive
 If buffer `*SQL*' exists but no process is running, make a new process.
 If buffer exists and a process is running, just switch to buffer `*SQL*'.

-To specify the SQL product, prefix the call with
-\\[universal-argument].  To set the buffer name as well, prefix
-the call to \\[sql-product-interactive] with
-\\[universal-argument] \\[universal-argument].
+When called interactively, \\[universal-argument] enables product selection.
+\\[universal-argument] \\[universal-argument], in addition to that, forces new buffer to be created.

 \(Type \\[describe-mode] in the SQL buffer for a list of commands.)"
   (interactive "P")
@@ -4254,9 +4252,7 @@ sql-product-interactive
                 (funcall (sql-get-product-feature product :sqli-comint-func)
                          product
                          (sql-get-product-feature product :sqli-options)
-                         (if (and new-name (string-prefix-p "SQL" new-name t))
-                             new-name
-                           (concat "SQL: " new-name))))
+                         (if (consp new-name) nil new-name)))

               ;; Set SQLi mode.
               (let ((sql-interactive-product product))

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> From: Filipp Gunbin <fgunbin@fastmail.fm>
> Cc: psainty@orcon.net.nz,  31446@debbugs.gnu.org
> Date: Tue, 22 May 2018 14:27:27 +0300
> 
> > Maybe I'm missing something, but it looks like sql-comint and
> > sql-product-interactive belong to 2 different families of entry points
> > in the package.  Some call sql-product-interactive, while the others
> > call sql-comint.  Isn't that so?
> 
> sql-product-interactive is one of the entry points (a generic one).
> Other entry points are sql-postgres and the like, which specify product.

I said "families" of entry points.  I could be missing something, but
it looked to me that interactive sql.el commands that call sql-comint
don't call sql-product-interactive, and vice versa.  If I'm right,
this problem cannot be solved by using sql-comint.

> Well, this patch works for me.

Thanks, I hope Micheal (CC'ed) will look into this when he has time.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Filipp Gunbin]

On 22/05/2018 19:42 +0300, Eli Zaretskii wrote:

>> From: Filipp Gunbin <fgunbin@fastmail.fm>
>> Cc: psainty@orcon.net.nz,  31446@debbugs.gnu.org
>> Date: Tue, 22 May 2018 14:27:27 +0300
>> 
>> > Maybe I'm missing something, but it looks like sql-comint and
>> > sql-product-interactive belong to 2 different families of entry points
>> > in the package.  Some call sql-product-interactive, while the others
>> > call sql-comint.  Isn't that so?
>> 
>> sql-product-interactive is one of the entry points (a generic one).
>> Other entry points are sql-postgres and the like, which specify product.
>
> I said "families" of entry points.  I could be missing something, but
> it looked to me that interactive sql.el commands that call sql-comint
> don't call sql-product-interactive, and vice versa.  If I'm right,
> this problem cannot be solved by using sql-comint.

Here's a quote from sql.el:

;; sql-interactive-mode is used to interact with a SQL interpreter
;; process in a SQLi buffer (usually called `*SQL*').  The SQLi buffer
;; is created by calling a SQL interpreter-specific entry function or
;; sql-product-interactive.  Do *not* call sql-interactive-mode by
;; itself.

That's what I wrote about previously.  If there are some other ways of
entering interactive sql mode, then I'm not aware of them, sorry.

First entry point, sql-product-interactive, calls specific comint
function, say sql-comint-postgres.  These product-specific functions
call sql-comint.

Second entry point(s), sql-postgres and the like, just call
sql-product-interactive.

So it all boils down to sql-comint in the end.

Sorry if I'm wrong somewhere.

>> Well, this patch works for me.
>
> Thanks, I hope Micheal (CC'ed) will look into this when he has time.

Yes, thanks.

Filipp

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Michael Mauger]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On May 22, 2018 3:15 PM, Filipp Gunbin <fgunbin@fastmail.fm> wrote:

> > 
> > Thanks, I hope Micheal (CC'ed) will look into this when he has time.
> 
> Yes, thanks.
> 
> Filipp

There is some confusion here and I will take a look. I won't get to it until this weekend however.

The original package had buffer naming scattered in several places. I've tried to consolidate it. It works much better for my workflow but there are obviously still some problems.

​--

MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer​

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Filipp Gunbin]

On 23/05/2018 09:48 -0400, Michael Mauger wrote:

> There is some confusion here and I will take a look. I won't get to it
> until this weekend however.

Michael, ping?

Thanks.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Michael Mauger]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

On May 29, 2018 3:52 AM, Filipp Gunbin <fgunbin@fastmail.fm> wrote:

> ​​
> 
> On 23/05/2018 09:48 -0400, Michael Mauger wrote:
> 
> > There is some confusion here and I will take a look. I won't get to it
> > 
> > until this weekend however.
> 
> Michael, ping?
> 
> Thanks.

Sorry, got home late last nite from the long weekend (US Memorial Day).  I was away but spent several hours researching this over the weekend and trying to figure out how to address it.

I was confused because I had addressed this bug months ago. My work journal indicates that I had committed a significant reworking of the buffer naming code last Fall. However, it appears that I messed up with git and the commit never made it to savannah. I was using a copy of the changes on an older distribution I use in the office where I interact with databases daily. 

I realize that 26.1 has been released, so my changes obviously won't make it. I have a version of the package with vastly improved and consistent buffer naming but I want to now test it more heavily. My plan is to update the DOCSTRINGs, update the NEWS file, and add to the test suite so that behavior is better defined and consistent. Longer-term, I want to introduce a Info document to describe configuration and workflows related to SQL development.

I am very sorry that I messed this up and hadn't properly followed-up to make sure things were stable. I have been battling with my employer over my contributions which are not done on their time. Despite their interference, I am refocusing my energy on the sql.el code and making sure that its quality meets the high standards of the rest of Emacs.

​-- 
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer​

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Tue, 29 May 2018 10:36:13 -0400
> From: Michael Mauger <mmauger@protonmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, "psainty\\@orcon.net.nz" <psainty@orcon.net.nz>, "31446\\@debbugs.gnu.org" <31446@debbugs.gnu.org>
> 
> I realize that 26.1 has been released, so my changes obviously won't make it.

We could put them in Emacs 26.2, if the changes are relatively safe
bugfixes.

Thanks.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Filipp Gunbin]

On 29/05/2018 10:36 -0400, Michael Mauger wrote:

> I realize that 26.1 has been released, so my changes obviously won't
> make it. I have a version of the package with vastly improved and
> consistent buffer naming but I want to now test it more heavily. My
> plan is to update the DOCSTRINGs, update the NEWS file, and add to the
> test suite so that behavior is better defined and
> consistent. Longer-term, I want to introduce a Info document to
> describe configuration and workflows related to SQL development.

I was just confused a bit about the latest changes.  While they may have
fixed the original bug (and got into 26.1), I find them strange in that
they AFAICT duplicate (in incomplete way) what is done in sql-comint.

May I ask you to look at past messages in this thread and clarify the
matters, if you have some time?  Currently we're talking about few lines
of changes.  If it all looks bad to you, then we just forget this until
your improved version.

Filipp

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Phil Sainty]

On 2018-05-30 02:36, Michael Mauger wrote:
> I realize that 26.1 has been released, so my changes obviously won't
> make it. I have a version of the package with vastly improved and
> consistent buffer naming but I want to now test it more heavily.

If it's in a state where some help with testing would be useful,
perhaps you could push the new code to a separate branch?  (My own
availability is rather patchy for the next 2-3 weeks, but I should be
able to give it a decent whirl over the next few days, and maybe
others can too?)

cheers,
-Phil

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Michael Mauger]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On May 29, 2018 7:32 PM, Phil Sainty <psainty@orcon.net.nz> wrote:
> 
> On 2018-05-30 02:36, Michael Mauger wrote:
> 
> > I realize that 26.1 has been released, so my changes obviously won't
> > make it. I have a version of the package with vastly improved and
> > consistent buffer naming but I want to now test it more heavily.
> 
> If it's in a state where some help with testing would be useful,
> perhaps you could push the new code to a separate branch? (My own
> availability is rather patchy for the next 2-3 weeks, but I should be
> able to give it a decent whirl over the next few days, and maybe
> others can too?)

I expect to make an initial commit to master in a couple of days of sql.el. 
That will be followed up with a commit of improved DOCSTRINGs, tests,
and possibly an Info file.

I'll leave it to Eli et al. to decide whether the sql.el on master gets merged 
into the emacs-26 branch later.

​--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer​

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Tue, 29 May 2018 22:51:35 -0400
> From: Michael Mauger <mmauger@protonmail.com>
> Cc: "31446@debbugs.gnu.org" <31446@debbugs.gnu.org>,
> 	Filipp Gunbin <fgunbin@fastmail.fm>,
> 	bug-gnu-emacs <bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org>
> 
> I expect to make an initial commit to master in a couple of days of sql.el. 
> That will be followed up with a commit of improved DOCSTRINGs, tests,
> and possibly an Info file.
> 
> I'll leave it to Eli et al. to decide whether the sql.el on master gets merged 
> into the emacs-26 branch later.

Thanks.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Michael Mauger]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On May 30, 2018 12:43 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> > Date: Tue, 29 May 2018 22:51:35 -0400
> > From: Michael Mauger mmauger@protonmail.com
> > 
> > I expect to make an initial commit to master in a couple of days of sql.el.
> >
> > That will be followed up with a commit of improved DOCSTRINGs, tests,
> > and possibly an Info file.
> > 
> 
> Thanks.

I have made a commit that fixes the buffer name handling. This code was 
intended for 26.1 but obviously missed that. Please review it and let me
know what you find.

Outstanding work:
* There is a bug around locating sql-interactive buffers from a sql buffer; I 
   know how to fix it, just need to further test the code.
* Rewrite the DOCSTRINGs to reflect the improved logic
* Add code tests to limit regressions like this in the future
* Add a Info file documenting sql-mode and the intended behavior.

Thanks and sorry for the bug.
​--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer​

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Sat, 02 Jun 2018 19:47:17 -0400
> From: Michael Mauger <mmauger@protonmail.com>
> 
> I have made a commit that fixes the buffer name handling. This code was 
> intended for 26.1 but obviously missed that. Please review it and let me
> know what you find.

Thanks.  Please in the future include in the log message a
ChangeLog-style list of specific changes you made.

> Outstanding work:
> * There is a bug around locating sql-interactive buffers from a sql buffer; I 
>    know how to fix it, just need to further test the code.
> * Rewrite the DOCSTRINGs to reflect the improved logic
> * Add code tests to limit regressions like this in the future
> * Add a Info file documenting sql-mode and the intended behavior.

Great, thanks in advance.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Michael Mauger]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On June 3, 2018 12:30 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > From: Michael Mauger mmauger@protonmail.com
> > 
> > I have made a commit that fixes the buffer name handling. This code was
> > intended for 26.1 but obviously missed that. Please review it and let me
> > know what you find.
> 
> Thanks. Please in the future include in the log message a
> ChangeLog-style list of specific changes you made.

Damn, I spent an afternoon writing up the ChangeLog out of habit 
and then forgot it!

It doesn't appear that I can amend the commit at this point, should 
I submit an essentially empty commit with the ChangeLog (and an
appropriate cross reference to the original commit), attach it to my 
next commit, or just forget the whole thing and leave a note for 
myself to be more cautious when I commit?

​-- 
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer​

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Eli Zaretskii]

> Date: Sun, 03 Jun 2018 16:47:09 -0400
> From: Michael Mauger <mmauger@protonmail.com>
> Cc: "psainty@orcon.net.nz" <psainty@orcon.net.nz>, "31446@debbugs.gnu.org" <31446@debbugs.gnu.org>, "fgunbin@fastmail.fm" <fgunbin@fastmail.fm>
> 
> Damn, I spent an afternoon writing up the ChangeLog out of habit 
> and then forgot it!
> 
> It doesn't appear that I can amend the commit at this point, should 
> I submit an essentially empty commit with the ChangeLog (and an
> appropriate cross reference to the original commit), attach it to my 
> next commit, or just forget the whole thing and leave a note for 
> myself to be more cautious when I commit?

The latter, I think.  It isn't a catastrophe, anyway.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Phil Sainty]

On 2018-06-03 11:47, Michael Mauger wrote:
> I have made a commit that fixes the buffer name handling. This code
> was intended for 26.1 but obviously missed that. Please review it
> and let me know what you find.

Buffer naming behaviour seems good to me at first look.

Running sql-postgres (with no prefix argument) when there is an
existing SQLi buffer now displays the buffer but does not select
the window, which seems like a bug?


-Phil



> Outstanding work:

> * There is a bug around locating sql-interactive buffers from a sql
>   buffer; I know how to fix it, just need to further test the code.
> * Rewrite the DOCSTRINGs to reflect the improved logic
> * Add code tests to limit regressions like this in the future
> * Add a Info file documenting sql-mode and the intended behavior.

bug#31446: 26.1; sql-interactive-mode buffer naming is broken

[Filipp Gunbin]

On 02/06/2018 19:47 -0400, Michael Mauger wrote:

> I have made a commit that fixes the buffer name handling. This code was
> intended for 26.1 but obviously missed that. Please review it and let me
> know what you find.

I've tested it a bit, LGTM.

Thanks.

bug#31446: sql-interactive-mode buffer naming is broken

[Michael Mauger]

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

Based on a previous commit and this commit, buffer naming should be more consistent and correct.
Added tests to insure I don't mess them up again!

--
MICHAEL@MAUGER.COM // FSF and EFF member // GNU Emacs sql.el maintainer

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