Thread: psql: small patch to correct filename formatting error in '\s FILE' output
psql: small patch to correct filename formatting error in '\s FILE' output
From
Ian Lawrence Barwick
Date:
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
Attachment
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
Re: psql: small patch to correct filename formatting error in '\s FILE' output
From
"Dickson S. Guedes"
Date:
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
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
Re: psql: small patch to correct filename formatting error in '\s FILE' output
From
Ian Lawrence Barwick
Date:
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
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
Re: psql: small patch to correct filename formatting error in '\s FILE' output
From
Bruce Momjian
Date:
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. +
Re: psql: small patch to correct filename formatting error in '\s FILE' output
From
Ian Lawrence Barwick
Date:
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
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