Re: [HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-11-05 Thread Tom Lane
Ian Lawrence Barwick barw...@gmail.com writes:
 2013/9/10 Bruce Momjian br...@momjian.us:
 I still see that weird behavior in git head:
 
 pgdevel=# \s history.txt
 Wrote history to file ./history.txt.
 pgdevel=# \s /tmp/history.txt
 Wrote history to file .//tmp/history.txt.
 pgdevel=# \cd /tmp
 pgdevel=# \s /tmp/history.txt
 Wrote history to file /tmp//tmp/history.txt.
 
 Should I revert the suggested patch?

 IIRC the patch was never applied, the reversion candidate is the existing
 commit 0725065b.

I reverted 0725065b.  AFAICT there is no interest in making \s produce
a reliable full path name.  There was some interest in making \cd tell
you where it'd chdir'd to, but that would be a separate patch.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-09-09 Thread Bruce Momjian
On Tue, Jan 22, 2013 at 07:30:59PM -0500, Tom Lane wrote:
 Ian Lawrence Barwick barw...@gmail.com writes:
  Related email from the archives on this subject:
  http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7...@mail.gmail.com
 
 I agree with the opinion stated there that \cd with no argument really
 ought to do what cd with no argument usually does on the platform.
 So if we're going to fix \cd to print the resulting current directory,
 wouldn't it work to just set dir to . rather than / for Windows?
 
  Does commit 0725065b just need to be reverted, or is an additional
  patch required to remove the prefixed working directory from \s output?
 
 Offhand it looked like reverting the commit would be enough, but I
 didn't look hard to see if there had been any subsequent related
 changes.  [ pokes around... ]  Well, at least there are still no other
 uses of pset.dirname.

I still see that weird behavior in git head:

  pgdevel=# \s history.txt
  Wrote history to file ./history.txt.
  pgdevel=# \s /tmp/history.txt
  Wrote history to file .//tmp/history.txt.
  pgdevel=# \cd /tmp
  pgdevel=# \s /tmp/history.txt
  Wrote history to file /tmp//tmp/history.txt.

Should I revert the suggested patch?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + It's impossible for everything to be true. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-09-09 Thread Ian Lawrence Barwick
2013/9/10 Bruce Momjian br...@momjian.us:
 On Tue, Jan 22, 2013 at 07:30:59PM -0500, Tom Lane wrote:
 Ian Lawrence Barwick barw...@gmail.com writes:
  Related email from the archives on this subject:
  http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7...@mail.gmail.com

 I agree with the opinion stated there that \cd with no argument really
 ought to do what cd with no argument usually does on the platform.
 So if we're going to fix \cd to print the resulting current directory,
 wouldn't it work to just set dir to . rather than / for Windows?

  Does commit 0725065b just need to be reverted, or is an additional
  patch required to remove the prefixed working directory from \s output?

 Offhand it looked like reverting the commit would be enough, but I
 didn't look hard to see if there had been any subsequent related
 changes.  [ pokes around... ]  Well, at least there are still no other
 uses of pset.dirname.

 I still see that weird behavior in git head:

   pgdevel=# \s history.txt
   Wrote history to file ./history.txt.
   pgdevel=# \s /tmp/history.txt
   Wrote history to file .//tmp/history.txt.
   pgdevel=# \cd /tmp
   pgdevel=# \s /tmp/history.txt
   Wrote history to file /tmp//tmp/history.txt.

 Should I revert the suggested patch?

IIRC the patch was never applied, the reversion candidate is the existing
commit 0725065b.

(Sorry for not following up earlier, this one dropped off my radar).

Regards

Ian Barwick


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-01-22 Thread Tom Lane
Ian Lawrence Barwick barw...@gmail.com writes:
 I've noticed a filename error in feedback messages from psql's '\s' command
 when saving the command line history to a file specified by an absolute
 filepath:

   psql (9.2.2)
   Type help for help.

   pgdevel=# \s history.txt
   Wrote history to file ./history.txt.
   pgdevel=# \s /tmp/history.txt
   Wrote history to file .//tmp/history.txt.
   pgdevel=# \cd /tmp
   pgdevel=# \s /tmp/history.txt
   Wrote history to file /tmp//tmp/history.txt.

 The second and third '\s' commands display incorrect filepaths in the feedback
 message, despite writing correctly to the specified file.

I wonder why we don't just revert commit 0725065b --- this complaint
shows that it was quite poorly thought out, and I don't really see the
need for it anyway.  Aside from the complained-of bug, the code added to
the \cd command is entirely incapable of tracking through multiple \cd
operations properly.

If we did think that this specific backslash command needed to be able
to print something other than the filename as-entered, I'd be inclined
to just apply make_absolute_path() to the name, instead of relying on
inadequate dead-reckoning.  However, that would require making
make_absolute_path available in src/port/ or someplace, which seems
a bit more than this feature is worth.  Why should \s, and \s alone,
need to remind you where you're cd'd to?

Thoughts?

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-01-22 Thread Dickson S. Guedes
2013/1/22 Tom Lane t...@sss.pgh.pa.us:
 Why should \s, and \s alone,
 need to remind you where you're cd'd to?

Why not just get rid of that prefixed cd'd path in \s?

[]s
-- 
Dickson S. Guedes
mail/xmpp: gue...@guedesoft.net - skype: guediz
http://github.com/guedes - http://guedesoft.net
http://www.postgresql.org.br


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-01-22 Thread Tom Lane
I wrote:
 If we did think that this specific backslash command needed to be able
 to print something other than the filename as-entered, I'd be inclined
 to just apply make_absolute_path() to the name, instead of relying on
 inadequate dead-reckoning.  However, that would require making
 make_absolute_path available in src/port/ or someplace, which seems
 a bit more than this feature is worth.  Why should \s, and \s alone,
 need to remind you where you're cd'd to?

It strikes me that a more useful reminder feature could be implemented
by having \cd itself print the new current directory, which it could do
with a simple call to getcwd(), thus not requiring refactoring of
make_absolute_path.  Then for instance if you'd forgotten where you
were, \cd . would tell you.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-01-22 Thread Ian Lawrence Barwick
2013/1/23 Tom Lane t...@sss.pgh.pa.us:
 I wrote:
 If we did think that this specific backslash command needed to be able
 to print something other than the filename as-entered, I'd be inclined
 to just apply make_absolute_path() to the name, instead of relying on
 inadequate dead-reckoning.  However, that would require making
 make_absolute_path available in src/port/ or someplace, which seems
 a bit more than this feature is worth.  Why should \s, and \s alone,
 need to remind you where you're cd'd to?

 It strikes me that a more useful reminder feature could be implemented
 by having \cd itself print the new current directory, which it could do
 with a simple call to getcwd(), thus not requiring refactoring of
 make_absolute_path.  Then for instance if you'd forgotten where you
 were, \cd . would tell you.

 \! pwd does the trick as well.

However personally I prefer to get some kind of feedback from an
action, so having
\cd confirm the directory would be nice. I'll submit a patch.

Related email from the archives on this subject:
http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7...@mail.gmail.com

Does commit 0725065b just need to be reverted, or is an additional
patch required to remove the prefixed working directory from \s output?


Ian Barwick


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-01-22 Thread Tom Lane
Ian Lawrence Barwick barw...@gmail.com writes:
 Related email from the archives on this subject:
 http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7...@mail.gmail.com

I agree with the opinion stated there that \cd with no argument really
ought to do what cd with no argument usually does on the platform.
So if we're going to fix \cd to print the resulting current directory,
wouldn't it work to just set dir to . rather than / for Windows?

 Does commit 0725065b just need to be reverted, or is an additional
 patch required to remove the prefixed working directory from \s output?

Offhand it looked like reverting the commit would be enough, but I
didn't look hard to see if there had been any subsequent related
changes.  [ pokes around... ]  Well, at least there are still no other
uses of pset.dirname.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


[HACKERS] psql: small patch to correct filename formatting error in '\s FILE' output

2013-01-20 Thread Ian Lawrence Barwick
I've noticed a filename error in feedback messages from psql's '\s' command
when saving the command line history to a file specified by an absolute
filepath:

  psql (9.2.2)
  Type help for help.

  pgdevel=# \s history.txt
  Wrote history to file ./history.txt.
  pgdevel=# \s /tmp/history.txt
  Wrote history to file .//tmp/history.txt.
  pgdevel=# \cd /tmp
  pgdevel=# \s /tmp/history.txt
  Wrote history to file /tmp//tmp/history.txt.

The second and third '\s' commands display incorrect filepaths in the feedback
message, despite writing correctly to the specified file.

Also, if the specified file could not be written to, the error message displayed
formats the filepath differently (i.e. it does not prepend the current working
directory), which is potentially confusing, and certainly visually inconsistent:

  pgdevel=# \cd /tmp
  pgdevel=# \s foo/history.txt
  could not save history to file foo/history.txt: No such file or directory
  pgdevel=# \! mkdir foo
  pgdevel=# \s foo/history.txt
  Wrote history to file /tmp/foo/history.txt.


The attached patch rectifies these issues by adding a small function
'format_fname()' to psql/stringutils.c which formats the filepath
appropriately, depending on whether an absolute filepath was supplied
or psql's cwd is set.

  pgdevel_head=# \s history.txt
  Wrote history to file ./history.txt.
  pgdevel_head=# \s /tmp/history.txt
  Wrote history to file /tmp/history.txt.
  pgdevel_head=# \cd /tmp
  pgdevel_head=# \s /tmp/history.txt
  Wrote history to file /tmp/history.txt.

  pgdevel_head=# \cd /tmp
  pgdevel_head=# \s bar/history.txt
  could not save history to file /tmp/bar/history.txt: No such file
or directory
  pgdevel_head=# \! mkdir bar
  pgdevel_head=# \s bar/history.txt
  Wrote history to file /tmp/bar/history.txt.


Notes/caveats
- The function 'format_fname()' deterimines whether the supplied filepath is
  absolute by checking for the presence of a '/' as the first character. This
  strikes me as a bit hacky but I can't think of an alternative.
- As far as I can tell, Windows does not support the '\s' command, so there is
  presumably no need to worry about supporting Windows-style file paths
- As far as I can tell, this is the only psql slash command which, after saving
  data to a file, provides a feedback message containing the filename/path.



Regards

Ian Lawrence Barwick


psql-save-history-2013-01-21.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers