Horton

Thread

Commits

  1. This patch shows the full path name when doing a \s in psql,

  1. psql: small patch to correct filename formatting error in '\s FILE' output

    Ian Lawrence Barwick <barwick@gmail.com> — 2013-01-21T07:58:13Z

    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

  2. Re: psql: small patch to correct filename formatting error in '\s FILE' output

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-22T21:36:34Z

    Ian Lawrence Barwick <barwick@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

  3. Re: psql: small patch to correct filename formatting error in '\s FILE' output

    Dickson S. Guedes <listas@guedesoft.net> — 2013-01-22T21:53:35Z

    2013/1/22 Tom Lane <tgl@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: guedes@guedesoft.net - skype: guediz
http://github.com/guedes - http://guedesoft.net
http://www.postgresql.org.br

  4. Re: psql: small patch to correct filename formatting error in '\s FILE' output

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-22T22:12:07Z

    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

  5. Re: psql: small patch to correct filename formatting error in '\s FILE' output

    Ian Lawrence Barwick <barwick@gmail.com> — 2013-01-22T23:05:03Z

    2013/1/23 Tom Lane <tgl@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/37ed240d0611200645l5b70c8ddw5fb735e0d35a7b22@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

  6. Re: psql: small patch to correct filename formatting error in '\s FILE' output

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-23T00:30:59Z

    Ian Lawrence Barwick <barwick@gmail.com> writes:
> Related email from the archives on this subject:
> http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7b22@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

  7. Re: psql: small patch to correct filename formatting error in '\s FILE' output

    Bruce Momjian <bruce@momjian.us> — 2013-09-10T01:25:00Z

    On Tue, Jan 22, 2013 at 07:30:59PM -0500, Tom Lane wrote:
> Ian Lawrence Barwick <barwick@gmail.com> writes:
> > Related email from the archives on this subject:
> > http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7b22@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  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

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

  8. Re: psql: small patch to correct filename formatting error in '\s FILE' output

    Ian Lawrence Barwick <barwick@gmail.com> — 2013-09-10T04:22:15Z

    2013/9/10 Bruce Momjian <bruce@momjian.us>:
> On Tue, Jan 22, 2013 at 07:30:59PM -0500, Tom Lane wrote:
>> Ian Lawrence Barwick <barwick@gmail.com> writes:
>> > Related email from the archives on this subject:
>> > http://www.postgresql.org/message-id/37ed240d0611200645l5b70c8ddw5fb735e0d35a7b22@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

  9. Re: psql: small patch to correct filename formatting error in '\s FILE' output

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-11-05T22:54:32Z

    Ian Lawrence Barwick <barwick@gmail.com> writes:
> 2013/9/10 Bruce Momjian <bruce@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