You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by br...@apache.org on 2012/12/16 23:43:26 UTC
svn commit: r1422706 - in /subversion/trunk/subversion: libsvn_subr/opt.c
tests/cmdline/svntest/main.py
Author: brane
Date: Sun Dec 16 22:43:25 2012
New Revision: 1422706
URL: http://svn.apache.org/viewvc?rev=1422706&view=rev
Log:
Print a warning in "svn --version" if plaintext password storage is enabled.
* subversion/libsvn_subr/opt.c: Inlcude unistd.h on *nix.
(svn_opt__print_version_info): If plaintext password storage is
enabled, print a warning about that after the copyright notice.
* subversion/tests/cmdline/svntest/main.py
(is_plaintext_password_storage_disabled): Instead of looking for
and scanning svn_private_config.h, look for the warning emitted
by "svn --version".
Modified:
subversion/trunk/subversion/libsvn_subr/opt.c
subversion/trunk/subversion/tests/cmdline/svntest/main.py
Modified: subversion/trunk/subversion/libsvn_subr/opt.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/opt.c?rev=1422706&r1=1422705&r2=1422706&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/opt.c (original)
+++ subversion/trunk/subversion/libsvn_subr/opt.c Sun Dec 16 22:43:25 2012
@@ -34,6 +34,10 @@
#include <apr_lib.h>
#include <apr_file_info.h>
+#ifndef WIN32
+#include <unistd.h>
+#endif
+
#include "svn_cmdline.h"
#include "svn_version.h"
#include "svn_types.h"
@@ -1121,6 +1125,24 @@ svn_opt__print_version_info(const char *
svn_version_ext_build_host(info)));
SVN_ERR(svn_cmdline_printf(pool, "%s\n", svn_version_ext_copyright(info)));
+#ifndef SVN_DISABLE_PLAINTEXT_PASSWORD_STORAGE
+ {
+ const char *warnstart = "";
+ const char *warnend = "";
+#ifndef WIN32
+ if (isatty(fileno(stdout)))
+ {
+ warnstart = "\033[1;31m";
+ warnend = "\033[0;m";
+ }
+#endif /* WIN32 */
+ SVN_ERR(svn_cmdline_printf(
+ pool,
+ _("%sWARNING: Plaintext password storage is enabled!%s\n\n"),
+ warnstart, warnend));
+ }
+#endif /* SVN_DISABLE_PLAINTEXT_PASSWORD_STORAGE */
+
if (footer)
{
SVN_ERR(svn_cmdline_printf(pool, "%s\n", footer));
Modified: subversion/trunk/subversion/tests/cmdline/svntest/main.py
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/main.py?rev=1422706&r1=1422705&r2=1422706&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/cmdline/svntest/main.py (original)
+++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Sun Dec 16 22:43:25 2012
@@ -1234,27 +1234,16 @@ def server_enforces_date_syntax():
def server_has_atomic_revprop():
return options.server_minor_version >= 7
-# FIXME: The following should use information retreived from "svn --version"
-# However, at the time of writing this predicate, that piece of
-# information was not available from a running Subversion client.
def is_plaintext_password_storage_disabled():
- confighandle = None
try:
- predicate = re.compile(r"^\s*#\s*define\s+"
- r"SVN_DISABLE_PLAINTEXT_PASSWORD_STORAGE")
- configfile = os.path.abspath(general_wc_dir)
- for i in range(4):
- configfile = os.path.dirname(configfile)
- configfile = os.path.join(configfile, "svn_private_config.h")
- confighandle = open(configfile, 'rt')
- for line in confighandle.readlines():
+ predicate = re.compile("^[^W]*WARNING: Plaintext password storage is enabled!")
+ code, out, err = run_svn(False, "--version")
+ for line in out:
if predicate.match(line):
- return True
+ return False
except:
- pass
- if confighandle:
- confighandle.close()
- return False
+ return False
+ return True
######################################################################
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Tue, Dec 18, 2012 at 11:27:25 +0100:
> On Tue, Dec 18, 2012 at 12:19:02PM +0200, Daniel Shahaf wrote:
> > Stefan Sperling wrote on Tue, Dec 18, 2012 at 11:03:56 +0100:
> > > Why would you *not* want to do this? Don't say "because the bug isn't
> > > in svn" since that is clear. We're not fixing a bug in 'svn'. We're
> > > making it work properly by default in more environments.
> >
> > I didn't say you should not apply the patch --- I was just raising
> > a point you seemed to have glossed on. You see that point and still
> > think the patch is an improvement, so I don't have a reason you
> > shouldn't apply the patch...
>
> OK, thanks!
>
> > > Or can you name a serious problem with this behaviour change?
> > > I cannot think of one but perhaps I'm overlooking something.
> >
> > Well, since you ask, does it break
> > echo p | svn up
> > ?
>
> That will postpone all conflicts since with my patch it's the same as
> svn up --non-interactive
>
Actually I was thinking of the "(p)ermanently" of the SSL cert prompt.
That doesn't affect your answer, though --- people who needed to not
pass --non-interactive in 1.7 will need to pass --force-interactive in
1.8.
> And if it was interactive it would only answer one of possibly several
> conflict prompts in the first place. You probably meant the equivalent of
> yes p | svn up
>
> With the patch applied you can either use
> yes p | svn up --force-interactive
> or, as usual,
> svn up --accept p
> to postpone all conflicts. Or svn up --accept ':-P' of course.
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Dec 18, 2012 at 12:19:02PM +0200, Daniel Shahaf wrote:
> Stefan Sperling wrote on Tue, Dec 18, 2012 at 11:03:56 +0100:
> > Why would you *not* want to do this? Don't say "because the bug isn't
> > in svn" since that is clear. We're not fixing a bug in 'svn'. We're
> > making it work properly by default in more environments.
>
> I didn't say you should not apply the patch --- I was just raising
> a point you seemed to have glossed on. You see that point and still
> think the patch is an improvement, so I don't have a reason you
> shouldn't apply the patch...
OK, thanks!
> > Or can you name a serious problem with this behaviour change?
> > I cannot think of one but perhaps I'm overlooking something.
>
> Well, since you ask, does it break
> echo p | svn up
> ?
That will postpone all conflicts since with my patch it's the same as
svn up --non-interactive
And if it was interactive it would only answer one of possibly several
conflict prompts in the first place. You probably meant the equivalent of
yes p | svn up
With the patch applied you can either use
yes p | svn up --force-interactive
or, as usual,
svn up --accept p
to postpone all conflicts. Or svn up --accept ':-P' of course.
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Tue, Dec 18, 2012 at 11:03:56 +0100:
> Why would you *not* want to do this? Don't say "because the bug isn't
> in svn" since that is clear. We're not fixing a bug in 'svn'. We're
> making it work properly by default in more environments.
I didn't say you should not apply the patch --- I was just raising
a point you seemed to have glossed on. You see that point and still
think the patch is an improvement, so I don't have a reason you
shouldn't apply the patch...
> Or can you name a serious problem with this behaviour change?
> I cannot think of one but perhaps I'm overlooking something.
Well, since you ask, does it break
echo p | svn up
?
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Dec 18, 2012 at 11:48:02AM +0200, Daniel Shahaf wrote:
> Stefan Sperling wrote on Tue, Dec 18, 2012 at 09:54:08 +0100:
> > On Tue, Dec 18, 2012 at 01:55:55AM +0200, Daniel Shahaf wrote:
> > > Not sure. For unattended scripts, the only difference between passing
> > > --non-interactive or not passing it is the error message they get, right?
> > >
> > > I mean, if they pass it they will get "Cannot prompt because we aren't
> > > interactive" (or is it "libsvn_auth has run out of username
> > > providers"?), and if they don't pass it they will on stderr an "Should
> > > I accept this SSL certificate?" prompt followed by an "End of file found on
> > > stdin" error.
> >
> > Depends. Sometimes the command just hangs and gives no output.
> > It may not always get an EOF from whatever file descriptor it is
> > blocking on. stderr might show the prompt but stderr might not
>
> Why? Shouldn't the automated test have either answered the prompt or
> closed stdin?
If it did, the problem wouldn't happen.
You can certainly blame the controlling process for misbehaving, no doubt.
The point is for 'svn' to try to by default keep working properly even if a
controlling process misbehaves like this, so that people don't have to worry
about it (adding a --non-interactive option isn't the first thing people
have in mind when they're writing a script that uses 'svn').
As it is now, 'svn' leaves it up to the user to work around this problem,
while it could be easily done automatically for them without harm.
Why would you *not* want to do this? Don't say "because the bug isn't
in svn" since that is clear. We're not fixing a bug in 'svn'. We're
making it work properly by default in more environments.
Or can you name a serious problem with this behaviour change?
I cannot think of one but perhaps I'm overlooking something.
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Tue, Dec 18, 2012 at 09:54:08 +0100:
> On Tue, Dec 18, 2012 at 01:55:55AM +0200, Daniel Shahaf wrote:
> > Not sure. For unattended scripts, the only difference between passing
> > --non-interactive or not passing it is the error message they get, right?
> >
> > I mean, if they pass it they will get "Cannot prompt because we aren't
> > interactive" (or is it "libsvn_auth has run out of username
> > providers"?), and if they don't pass it they will on stderr an "Should
> > I accept this SSL certificate?" prompt followed by an "End of file found on
> > stdin" error.
>
> Depends. Sometimes the command just hangs and gives no output.
> It may not always get an EOF from whatever file descriptor it is
> blocking on. stderr might show the prompt but stderr might not
Why? Shouldn't the automated test have either answered the prompt or
closed stdin?
> always be visible e.g. if it is being piped back to a controlling
So don't discard stderr when you investigate why a build fails.
> process which is also blocking, waiting for svn to finish before
> it will show the result.
>
> I've seen this happen in Jenkins builds that run 'svn export' and
> adding --non-interactive fixed it.
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Dec 18, 2012 at 09:54:08AM +0100, Stefan Sperling wrote:
> I've seen this happen in Jenkins builds that run 'svn export' and
> adding --non-interactive fixed it.
Correction: I misrembered. This wasn't happening in Jenkins, but in an
automated test environment ("SQS Test") which executes batch scripts on
remote Windows boxes without giving users an interactive shell.
But really, any program that runs 'svn' and collects stdout/stderr data
to generate a report based on that is affected by this problem, if it
waits until the 'svn' process is done before process collected data.
The controlling process cannot tell whether 'svn' is taking a long
time to produce another line on stdout, or whether 'svn' hangs.
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Dec 18, 2012 at 01:55:55AM +0200, Daniel Shahaf wrote:
> Not sure. For unattended scripts, the only difference between passing
> --non-interactive or not passing it is the error message they get, right?
>
> I mean, if they pass it they will get "Cannot prompt because we aren't
> interactive" (or is it "libsvn_auth has run out of username
> providers"?), and if they don't pass it they will on stderr an "Should
> I accept this SSL certificate?" prompt followed by an "End of file found on
> stdin" error.
Depends. Sometimes the command just hangs and gives no output.
It may not always get an EOF from whatever file descriptor it is
blocking on. stderr might show the prompt but stderr might not
always be visible e.g. if it is being piped back to a controlling
process which is also blocking, waiting for svn to finish before
it will show the result.
I've seen this happen in Jenkins builds that run 'svn export' and
adding --non-interactive fixed it.
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Mon, Dec 17, 2012 at 21:32:24 +0100:
> On Mon, Dec 17, 2012 at 09:14:42PM +0100, Branko Čibej wrote:
> > That "blatant abuse" happens at least in two places that I'm aware of:
> > our tests, and Emacs vc-mode.
>
> There is a difference between scraping the cli output and having
> a program type answers into prompts. Do our tests really do the latter?
>
> Anyway, we can always add a --force-interactive option for such use.
>
> > And we'd hardly go to the trouble of keeping the command-line output
> > backwards-compatible if we didn't /expect/ people to drive svn this way.
>
> I'm generally opposed to favouring scriptability of the CLI over
> interactive use of the CLI.
>
> In this case, however, I'd like to make simple scripting easier by
> preventing the need to add --non-interactive to every invocation of
> 'svn' in a script. You can surely see the advantage of that, no?
Not sure. For unattended scripts, the only difference between passing
--non-interactive or not passing it is the error message they get, right?
I mean, if they pass it they will get "Cannot prompt because we aren't
interactive" (or is it "libsvn_auth has run out of username
providers"?), and if they don't pass it they will on stderr an "Should
I accept this SSL certificate?" prompt followed by an "End of file found on
stdin" error.
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Dec 17, 2012 at 09:14:42PM +0100, Branko Čibej wrote:
> That "blatant abuse" happens at least in two places that I'm aware of:
> our tests, and Emacs vc-mode.
There is a difference between scraping the cli output and having
a program type answers into prompts. Do our tests really do the latter?
Anyway, we can always add a --force-interactive option for such use.
> And we'd hardly go to the trouble of keeping the command-line output
> backwards-compatible if we didn't /expect/ people to drive svn this way.
I'm generally opposed to favouring scriptability of the CLI over
interactive use of the CLI.
In this case, however, I'd like to make simple scripting easier by
preventing the need to add --non-interactive to every invocation of
'svn' in a script. You can surely see the advantage of that, no?
FWIW, every release since 1.6 has broken scripts that parsed CLI output in
a naive way. All these difference were documented in the release notes.
And we offer --xml mode and bindings with stable interfaces.
Re: svn commit: r1422706 - in /subversion/trunk/subversion: libsvn_subr/opt.c
tests/cmdline/svntest/main.py
Posted by Branko Čibej <br...@wandisco.com>.
On 17.12.2012 20:44, Peter Samuelson wrote:
> [Stefan Sperling]
>> We could use iatty() to enable --non-interactive if output is not
>> going to a terminal, for instance.
> I floated this idea some time ago and I'm still in favor of it. But I
> think a simple isatty(STDERR_FILENO) would be wrong. What you want is
> to detect that there is a controlling terminal at all - something like:
>
> #if WINDOWS
> #define CON "CON:"
> #else
> #define CON "/dev/tty"
> #endif
> fd = open(CON, O_RDWR);
> if (fd >= 0) {
> close(fd);
> ...
> }
>
> (Of course I have no idea if "CON:" behaves that way. But there must
> be _some_ way to determine, on Windows, whether you have a terminal
> window available.)
>
> Someone - was it Mark, perhaps? - objected to this idea on the basis
> that some wrapper programs out there may try to "scrape" the prompts,
> in interactive mode, and supply the correct input from, e.g., GUI
> dialog boxes.
>
> I feel like that's a blatant abuse of our interfaces and we shouldn't
> support it, and if anyone really needs to do that they should use
> 'expect', but that's just an opinion and it isn't (yet) backed with any
> code.
That "blatant abuse" happens at least in two places that I'm aware of:
our tests, and Emacs vc-mode.
And we'd hardly go to the trouble of keeping the command-line output
backwards-compatible if we didn't /expect/ people to drive svn this way.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Sperling wrote on Mon, Dec 17, 2012 at 21:39:49 +0100:
> On Mon, Dec 17, 2012 at 01:44:43PM -0600, Peter Samuelson wrote:
> > [Stefan Sperling]
> > > We could use iatty() to enable --non-interactive if output is not
> > > going to a terminal, for instance.
> >
> > I floated this idea some time ago and I'm still in favor of it. But I
> > think a simple isatty(STDERR_FILENO) would be wrong.
>
> Why? Not all ttys are interactive of course. But I don't really care if
> people piping their line printers or modems into svn's stdin get interactive
> mode by default. Is there any other downside to using isatty()?
>
> And, actually, don't we want to check stdin, not stderr or stdout?
>
> > What you want is
> > to detect that there is a controlling terminal at all - something like:
> >
> > #if WINDOWS
> > #define CON "CON:"
> > #else
> > #define CON "/dev/tty"
> > #endif
> > fd = open(CON, O_RDWR);
> > if (fd >= 0) {
> > close(fd);
> > ...
> > }
> >
> > (Of course I have no idea if "CON:" behaves that way. But there must
> > be _some_ way to determine, on Windows, whether you have a terminal
> > window available.)
>
> The simple patch below implements it with the isatty() on *nix and
> _isatty() on Windows (untested -- thanks for the hint Bert!).
>
> We can still change this implementation to use your open() /dev/tty idea later.
>
> However, this is enough to suppress prompts if stdin is piped into 'svn'
> and if 'svn' is run from cron. I believe that's an improvement.
>
> > Someone - was it Mark, perhaps? - objected to this idea on the basis
> > that some wrapper programs out there may try to "scrape" the prompts,
> > in interactive mode, and supply the correct input from, e.g., GUI
> > dialog boxes.
>
> Well, I would guess there are more people who are bitten by forgetting
> to pass --non-interactive in normal scripts than there are people writing
> scrapers that type answers into the prompts. I've met one person who
> made this mistake just last week, who couldn't figure out why 'svn' was
> hanging up their automated build jobs (it was asking for credentials).
Was the automated build job not running svn with an empty (or closed) stdin?
RE: svn commit: r1422706 - in /subversion/trunk/subversion: libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Stefan Sperling [mailto:stsp@elego.de]
> Sent: dinsdag 18 december 2012 10:46
> To: Peter Samuelson; dev@subversion.apache.org
> Subject: Re: svn commit: r1422706 - in /subversion/trunk/subversion:
> libsvn_subr/opt.c tests/cmdline/svntest/main.py
>
> On Mon, Dec 17, 2012 at 09:39:49PM +0100, Stefan Sperling wrote:
> > The simple patch below implements it with the isatty() on *nix and
> > _isatty() on Windows (untested -- thanks for the hint Bert!).
>
> Updated version that adds --force-interactive for users who require
> the previous behaviour for whatever purposes.
>
> [[[
> Add a new function to the cmdline library to determine whether standard
> input is connected to a terminal device. Set the --non-interactive flag
> by default if standard input is not a terminal device.
> Add a new --force-interactive option to force previous behaviour.
>
> * subversion/include/svn_cmdline.h
> (svn_cmdline__stdin_isatty): Declare.
>
> * subversion/libsvn_subr/cmdline.c: Include io.h on Windows.
> (svn_cmdline__stdin_isatty): New.
>
> * subversion/svn/cl.h
> (svn_cl__opt_state_t): Add force_interactive option.
>
> * subversion/svn/svn.c
> (svn_cl__longopt_t): Add opt_force_interactive.
> (sub_main): Set the --non-interactive option based on whether stdin is a
> tty,
> unless interactive mode has been forced with --force-interactive.
> Enforce mutual exclusion of --non-interactive and --force-interactive.
> ]]]
>
> Index: subversion/include/svn_cmdline.h
> ==========================================================
> =========
> --- subversion/include/svn_cmdline.h (revision 1423113)
> +++ subversion/include/svn_cmdline.h (working copy)
> @@ -381,6 +381,11 @@ svn_cmdline__getopt_init(apr_getopt_t **os,
> const char *argv[],
> apr_pool_t *pool);
>
> +/* Determine whether standard input is associated with a terminal.
> + * @since New in 1.8. */
> +svn_boolean_t
> +svn_cmdline__stdin_isatty(void);
> +
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
> Index: subversion/libsvn_subr/cmdline.c
> ==========================================================
> =========
> --- subversion/libsvn_subr/cmdline.c (revision 1423113)
> +++ subversion/libsvn_subr/cmdline.c (working copy)
> @@ -33,6 +33,7 @@
> #include <unistd.h>
> #else
> #include <crtdbg.h>
> +#include <io.h>
> #endif
>
> #include <apr_errno.h> /* for apr_strerror */
> @@ -923,3 +924,13 @@
> svn_cmdline__print_xml_prop_hash(svn_stringbuf_t *
>
> return SVN_NO_ERROR;
> }
> +
> +svn_boolean_t
> +svn_cmdline__stdin_isatty(void)
> +{
> +#ifdef WIN32
> + return (_isatty(0) != 0);
> +#else
> + return (isatty(STDIN_FILENO) != 0);
> +#endif
apr.h defines STDIN_FILENO (and friends) on Windows, so you can just use
that macro on Windows.
Bert
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Dec 17, 2012 at 09:39:49PM +0100, Stefan Sperling wrote:
> The simple patch below implements it with the isatty() on *nix and
> _isatty() on Windows (untested -- thanks for the hint Bert!).
Updated version that adds --force-interactive for users who require
the previous behaviour for whatever purposes.
[[[
Add a new function to the cmdline library to determine whether standard
input is connected to a terminal device. Set the --non-interactive flag
by default if standard input is not a terminal device.
Add a new --force-interactive option to force previous behaviour.
* subversion/include/svn_cmdline.h
(svn_cmdline__stdin_isatty): Declare.
* subversion/libsvn_subr/cmdline.c: Include io.h on Windows.
(svn_cmdline__stdin_isatty): New.
* subversion/svn/cl.h
(svn_cl__opt_state_t): Add force_interactive option.
* subversion/svn/svn.c
(svn_cl__longopt_t): Add opt_force_interactive.
(sub_main): Set the --non-interactive option based on whether stdin is a tty,
unless interactive mode has been forced with --force-interactive.
Enforce mutual exclusion of --non-interactive and --force-interactive.
]]]
Index: subversion/include/svn_cmdline.h
===================================================================
--- subversion/include/svn_cmdline.h (revision 1423113)
+++ subversion/include/svn_cmdline.h (working copy)
@@ -381,6 +381,11 @@ svn_cmdline__getopt_init(apr_getopt_t **os,
const char *argv[],
apr_pool_t *pool);
+/* Determine whether standard input is associated with a terminal.
+ * @since New in 1.8. */
+svn_boolean_t
+svn_cmdline__stdin_isatty(void);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/libsvn_subr/cmdline.c
===================================================================
--- subversion/libsvn_subr/cmdline.c (revision 1423113)
+++ subversion/libsvn_subr/cmdline.c (working copy)
@@ -33,6 +33,7 @@
#include <unistd.h>
#else
#include <crtdbg.h>
+#include <io.h>
#endif
#include <apr_errno.h> /* for apr_strerror */
@@ -923,3 +924,13 @@ svn_cmdline__print_xml_prop_hash(svn_stringbuf_t *
return SVN_NO_ERROR;
}
+
+svn_boolean_t
+svn_cmdline__stdin_isatty(void)
+{
+#ifdef WIN32
+ return (_isatty(0) != 0);
+#else
+ return (isatty(STDIN_FILENO) != 0);
+#endif
+}
Index: subversion/svn/cl.h
===================================================================
--- subversion/svn/cl.h (revision 1423113)
+++ subversion/svn/cl.h (working copy)
@@ -238,6 +238,7 @@ typedef struct svn_cl__opt_state_t
svn_boolean_t include_externals; /* Recurses (in)to file & dir externals */
svn_boolean_t show_inherited_props; /* get inherited properties */
apr_array_header_t* search_patterns; /* pattern arguments for --search */
+ svn_boolean_t force_interactive; /* force interactive prompting */
} svn_cl__opt_state_t;
Index: subversion/svn/svn.c
===================================================================
--- subversion/svn/svn.c (revision 1423113)
+++ subversion/svn/svn.c (working copy)
@@ -101,6 +101,7 @@ typedef enum svn_cl__longopt_t {
opt_no_ignore,
opt_no_unlock,
opt_non_interactive,
+ opt_force_interactive,
opt_old_cmd,
opt_record_only,
opt_relocate,
@@ -230,6 +231,10 @@ const apr_getopt_option_t svn_cl__options[] =
"with '--non-interactive')") },
{"non-interactive", opt_non_interactive, 0,
N_("do no interactive prompting")},
+ {"force-interactive", opt_force_interactive, 0,
+ N_("do interactive prompting even if standard input\n"
+ " "
+ "is not a terminal device")},
{"dry-run", opt_dry_run, 0,
N_("try operation but make no changes")},
{"ignore-ancestry", opt_ignore_ancestry, 0,
@@ -401,7 +406,8 @@ const apr_getopt_option_t svn_cl__options[] =
willy-nilly to every invocation of 'svn') . */
const int svn_cl__global_options[] =
{ opt_auth_username, opt_auth_password, opt_no_auth_cache, opt_non_interactive,
- opt_trust_server_cert, opt_config_dir, opt_config_options, 0
+ opt_force_interactive, opt_trust_server_cert, opt_config_dir,
+ opt_config_options, 0
};
/* Options for giving a log message. (Some of these also have other uses.)
@@ -1982,6 +1988,9 @@ sub_main(int argc, const char *argv[], apr_pool_t
case opt_non_interactive:
opt_state.non_interactive = TRUE;
break;
+ case opt_force_interactive:
+ opt_state.force_interactive = TRUE;
+ break;
case opt_trust_server_cert:
opt_state.trust_server_cert = TRUE;
break;
@@ -2191,6 +2200,21 @@ sub_main(int argc, const char *argv[], apr_pool_t
}
}
+ /* The --non-interactive and --force-interactive options are mutually
+ * exclusive. */
+ if (opt_state.non_interactive && opt_state.force_interactive)
+ {
+ err = svn_error_create(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
+ _("--non-interactive and --force-interactive "
+ "are mutually exclusive"));
+ return EXIT_ERROR(err);
+ }
+
+ /* If stdin is not a terminal and --force-interactive was not passed,
+ * set --non-interactive. */
+ if (!opt_state.force_interactive)
+ opt_state.non_interactive = !svn_cmdline__stdin_isatty();
+
/* Turn our hash of changelists into an array of unique ones. */
SVN_INT_ERR(svn_hash_keys(&(opt_state.changelists), changelists, pool));
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Peter Samuelson <pe...@p12n.org>.
> On Mon, Dec 17, 2012 at 01:44:43PM -0600, Peter Samuelson wrote:
> > [Stefan Sperling]
> > > We could use iatty() to enable --non-interactive if output is not
> > > going to a terminal, for instance.
> >
> > I floated this idea some time ago and I'm still in favor of it. But I
> > think a simple isatty(STDERR_FILENO) would be wrong.
[Stefan Sperling]
> Why? Not all ttys are interactive of course. But I don't really care if
> people piping their line printers or modems into svn's stdin get interactive
> mode by default. Is there any other downside to using isatty()?
>
> And, actually, don't we want to check stdin, not stderr or stdout?
I wasn't objecting to isatty(), but to the idea that
isatty(STDERR_FILENO) would be sufficient. As you say, stdin is more
appropriate than stderr. But you _can_ have a situation where you are
on a tty but stdin is not. For example:
# revert part of an import of a dirty working tree
find . -name '*.obj' | xargs svn rm
(Perhaps 'svn rm' will not cause interactive prompting - for now - but
I hope you get the idea.)
Of course, so long as interactive prompts use stdin, isatty(STDIN_FILENO)
makes sense. But this sort of usage is why I think prompting should
instead use /dev/tty, for both input and output. And then if /dev/tty
cannot be opened, assume --non-interactive.
Peter
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Dec 17, 2012 at 01:44:43PM -0600, Peter Samuelson wrote:
> [Stefan Sperling]
> > We could use iatty() to enable --non-interactive if output is not
> > going to a terminal, for instance.
>
> I floated this idea some time ago and I'm still in favor of it. But I
> think a simple isatty(STDERR_FILENO) would be wrong.
Why? Not all ttys are interactive of course. But I don't really care if
people piping their line printers or modems into svn's stdin get interactive
mode by default. Is there any other downside to using isatty()?
And, actually, don't we want to check stdin, not stderr or stdout?
> What you want is
> to detect that there is a controlling terminal at all - something like:
>
> #if WINDOWS
> #define CON "CON:"
> #else
> #define CON "/dev/tty"
> #endif
> fd = open(CON, O_RDWR);
> if (fd >= 0) {
> close(fd);
> ...
> }
>
> (Of course I have no idea if "CON:" behaves that way. But there must
> be _some_ way to determine, on Windows, whether you have a terminal
> window available.)
The simple patch below implements it with the isatty() on *nix and
_isatty() on Windows (untested -- thanks for the hint Bert!).
We can still change this implementation to use your open() /dev/tty idea later.
However, this is enough to suppress prompts if stdin is piped into 'svn'
and if 'svn' is run from cron. I believe that's an improvement.
> Someone - was it Mark, perhaps? - objected to this idea on the basis
> that some wrapper programs out there may try to "scrape" the prompts,
> in interactive mode, and supply the correct input from, e.g., GUI
> dialog boxes.
Well, I would guess there are more people who are bitten by forgetting
to pass --non-interactive in normal scripts than there are people writing
scrapers that type answers into the prompts. I've met one person who
made this mistake just last week, who couldn't figure out why 'svn' was
hanging up their automated build jobs (it was asking for credentials).
[[[
Add a new function to the cmdline library to determine whether standard
input is connected to a terminal device. Set the --non-interactive flag
by default if standard input is not a termina device.
* subversion/include/svn_cmdline.h
(svn_cmdline__stdin_isatty): Declare.
* subversion/libsvn_subr/cmdline.c: Include io.h on Windows.
(svn_cmdline__stdin_isatty): New.
* subversion/svn/svn.c
(sub_main): Right after parsing command line options, set the
--non-interactive option based on whether stdin is a tty.
]]]
Index: subversion/include/svn_cmdline.h
===================================================================
--- subversion/include/svn_cmdline.h (revision 1423113)
+++ subversion/include/svn_cmdline.h (working copy)
@@ -381,6 +381,11 @@ svn_cmdline__getopt_init(apr_getopt_t **os,
const char *argv[],
apr_pool_t *pool);
+/* Determine whether standard input is associated with a terminal.
+ * @since New in 1.8. */
+svn_boolean_t
+svn_cmdline__stdin_isatty(void);
+
#ifdef __cplusplus
}
#endif /* __cplusplus */
Index: subversion/libsvn_subr/cmdline.c
===================================================================
--- subversion/libsvn_subr/cmdline.c (revision 1423113)
+++ subversion/libsvn_subr/cmdline.c (working copy)
@@ -33,6 +33,7 @@
#include <unistd.h>
#else
#include <crtdbg.h>
+#include <io.h>
#endif
#include <apr_errno.h> /* for apr_strerror */
@@ -923,3 +924,13 @@ svn_cmdline__print_xml_prop_hash(svn_stringbuf_t *
return SVN_NO_ERROR;
}
+
+svn_boolean_t
+svn_cmdline__stdin_isatty(void)
+{
+#ifdef WIN32
+ return (_isatty(0) != 0);
+#else
+ return (isatty(STDIN_FILENO) != 0);
+#endif
+}
Index: subversion/svn/svn.c
===================================================================
--- subversion/svn/svn.c (revision 1423113)
+++ subversion/svn/svn.c (working copy)
@@ -2191,6 +2191,10 @@ sub_main(int argc, const char *argv[], apr_pool_t
}
}
+ /* If stdin is not a terminal, set --non-interactive. */
+ if (!opt_state.non_interactive)
+ opt_state.non_interactive = !svn_cmdline__stdin_isatty();
+
/* Turn our hash of changelists into an array of unique ones. */
SVN_INT_ERR(svn_hash_keys(&(opt_state.changelists), changelists, pool));
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Peter Samuelson <pe...@p12n.org>.
[Stefan Sperling]
> We could use iatty() to enable --non-interactive if output is not
> going to a terminal, for instance.
I floated this idea some time ago and I'm still in favor of it. But I
think a simple isatty(STDERR_FILENO) would be wrong. What you want is
to detect that there is a controlling terminal at all - something like:
#if WINDOWS
#define CON "CON:"
#else
#define CON "/dev/tty"
#endif
fd = open(CON, O_RDWR);
if (fd >= 0) {
close(fd);
...
}
(Of course I have no idea if "CON:" behaves that way. But there must
be _some_ way to determine, on Windows, whether you have a terminal
window available.)
Someone - was it Mark, perhaps? - objected to this idea on the basis
that some wrapper programs out there may try to "scrape" the prompts,
in interactive mode, and supply the correct input from, e.g., GUI
dialog boxes.
I feel like that's a blatant abuse of our interfaces and we shouldn't
support it, and if anyone really needs to do that they should use
'expect', but that's just an opinion and it isn't (yet) backed with any
code.
Re: svn commit: r1422706 - in /subversion/trunk/subversion:
libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Dec 16, 2012 at 10:43:26PM -0000, brane@apache.org wrote:
> Author: brane
> Date: Sun Dec 16 22:43:25 2012
> New Revision: 1422706
>
> URL: http://svn.apache.org/viewvc?rev=1422706&view=rev
> Log:
> Print a warning in "svn --version" if plaintext password storage is enabled.
One more remark on this:
> +#ifndef SVN_DISABLE_PLAINTEXT_PASSWORD_STORAGE
> + {
> + const char *warnstart = "";
> + const char *warnend = "";
> +#ifndef WIN32
> + if (isatty(fileno(stdout)))
In the past we didn't use isatty() since there is no APR wrapper for it.
If we're going to call isatty(), I think it would be best to implement a
portable wrapper first that works on all platforms we support, so that it
can also be used for other purposes.
We could use iatty() to enable --non-interactive if output is not going
to a terminal, for instance. This would prevent password or server cert
prompts from hanging scripts where the auther of the script forgets to
pass --non-interactive, which in my experience is a rather common problem.
Apparently, Perl has an isatty() implementation that works on *nix and
Windows that might serve as an example.
Are you willing to look into that? That would be great!
Re: svn commit: r1422706 - in /subversion/trunk/subversion: libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> -----Original Message-----
>> From: brane@apache.org [mailto:brane@apache.org]
>> Sent: zondag 16 december 2012 23:43
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1422706 - in /subversion/trunk/subversion:
>> libsvn_subr/opt.c tests/cmdline/svntest/main.py
>>
>> Author: brane
>> Date: Sun Dec 16 22:43:25 2012
>> New Revision: 1422706
>>
>> URL: http://svn.apache.org/viewvc?rev=1422706&view=rev
>> Log:
>> Print a warning in "svn --version" if plaintext password storage is enabled.
>
> I tried to fix the getopt tests that this change broke, but the extra
> newline is impossible to parse in the current test setup.
before I read your main I changed the test to delete all blank lines.
The coloured warning doesn't work very well in my red xterm.
--
Certified & Supported Apache Subversion Downloads:
http://www.wandisco.com/subversion/download
RE: svn commit: r1422706 - in /subversion/trunk/subversion: libsvn_subr/opt.c tests/cmdline/svntest/main.py
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: brane@apache.org [mailto:brane@apache.org]
> Sent: zondag 16 december 2012 23:43
> To: commits@subversion.apache.org
> Subject: svn commit: r1422706 - in /subversion/trunk/subversion:
> libsvn_subr/opt.c tests/cmdline/svntest/main.py
>
> Author: brane
> Date: Sun Dec 16 22:43:25 2012
> New Revision: 1422706
>
> URL: http://svn.apache.org/viewvc?rev=1422706&view=rev
> Log:
> Print a warning in "svn --version" if plaintext password storage is enabled.
I tried to fix the getopt tests that this change broke, but the extra newline is impossible to parse in the current test setup.
Given the amount of user responses on the introduction of the earlier warnings for plain text password, I would recommend changing the output to show all the enabled password storage backends. That would also help in diagnosing problems with optional stores, while the current warning just produces a warning when the plaintext store is the last remaining option.
We have already revved most of the functions needed, so adding an optional config argument shouldn't be a problem
Bert