This commit has two issues.
The first is that it prints warnings for each bad argument, and then
prints a confusing redundant error at the end for last one only.
Perhaps this should be fixed by adding a new error code
('svn'-specific?) which means "just clear the error without printing
it, but exit false"?
The second is that there's an error leak (ie, maint-mode segv)
somewhere. Namely:
On Tue, May 20, 2008 at 2:11 PM, <kfogel_at_tigris.org> wrote:
> Author: kfogel
> Date: Tue May 20 14:11:26 2008
> New Revision: 31320
>
> Log:
> Fix bug whereby 'svn info' would return success (zero) even if it
> printed warnings about some of the targets. Now if any target
> encounters an error, return non-zero after handling all targets.
>
> * subversion/svn/info-cmd.c
> (svn_cl__info): If any targets errored, return the last error at
> the end, so svn will exit with a non-zero exit code. Add a test for
> SVN_ERR_ENTRY_NOT_FOUND, which is the actual code returned for a
> non-existent or existent-but-unversioned file (as indicated by
> manual testing, and thank goodness for tools/dev/which-error.py).
> Finally, rework the conditional structure a bit, making this
> change appear larger than it really is.
>
> Suggested by: Mark E. Hamilton <mhamilt_at_sandia.gov>
> epg
>
> See this message and the thread around it:
>
> http://subversion.tigris.org/servlets/ReadMsg?list=users&msgNo=77852
> From: "Mark E. Hamilton" <mhamilt_at_sandia.gov>
> To: users_at_subversion.tigris.org
> Subject: Re: svn info not setting exit status.
> Message-ID: <483324FA.3040404_at_sandia.gov>
> Date: Tue, 20 May 2008 13:22:34 -0600
>
> Modified:
> trunk/subversion/svn/info-cmd.c
>
> Modified: trunk/subversion/svn/info-cmd.c
> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/svn/info-cmd.c?pathrev=31320&r1=31319&r2=31320
> ==============================================================================
> --- trunk/subversion/svn/info-cmd.c Tue May 20 13:04:22 2008 (r31319)
> +++ trunk/subversion/svn/info-cmd.c Tue May 20 14:11:26 2008 (r31320)
> @@ -446,7 +446,8 @@ svn_cl__info(apr_getopt_t *os,
> apr_array_header_t *targets = NULL;
> apr_pool_t *subpool = svn_pool_create(pool);
> int i;
> - svn_error_t *err;
> + svn_error_t *err = NULL;
> + svn_error_t *prev_err = NULL; /* save last error, if multiple targets */
> svn_opt_revision_t peg_revision;
> svn_info_receiver_t receiver;
>
> @@ -485,6 +486,16 @@ svn_cl__info(apr_getopt_t *os,
> const char *truepath;
> const char *target = APR_ARRAY_IDX(targets, i, const char *);
>
> + /* We loop over multiple targets, so save previous error (if any).
> + Do this at loop top because some failures 'continue'. */
> + if (err)
> + {
> + if (prev_err)
> + svn_error_clear(prev_err);
> + prev_err = err;
> + err = NULL; /* not strictly necessary, but good form */
> + }
> +
> svn_pool_clear(subpool);
> SVN_ERR(svn_cl__check_cancel(ctx->cancel_baton));
>
> @@ -501,34 +512,40 @@ svn_cl__info(apr_getopt_t *os,
> receiver, NULL, opt_state->depth,
> opt_state->changelists, ctx, subpool);
>
> - /* If one of the targets is a non-existent URL or wc-entry,
> - don't bail out. Just warn and move on to the next target. */
> - if (err && err->apr_err == SVN_ERR_UNVERSIONED_RESOURCE)
> + if (err)
> {
> - svn_error_clear(err);
> - SVN_ERR(svn_cmdline_fprintf
> - (stderr, subpool,
> - _("%s: (Not a versioned resource)\n\n"),
> - svn_path_local_style(target, pool)));
> - continue;
> + /* If one of the targets is a non-existent URL or wc-entry,
> + don't bail out. Just warn and move on to the next target. */
> + if (err->apr_err == SVN_ERR_UNVERSIONED_RESOURCE
> + || err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
> + {
> + SVN_ERR(svn_cmdline_fprintf
> + (stderr, subpool,
> + _("%s: (Not a versioned resource)\n\n"),
> + svn_path_local_style(target, pool)));
> + continue;
> + }
> + else if (err->apr_err == SVN_ERR_RA_ILLEGAL_URL)
> + {
> + SVN_ERR(svn_cmdline_fprintf
> + (stderr, subpool,
> + _("%s: (Not a valid URL)\n\n"),
> + svn_path_local_style(target, pool)));
> + continue;
> + }
> + else
> + {
[ This isn't the actual "namely", but you might need to clear prev_err
here too. ]
> + return err;
> + }
> }
> - else if (err && err->apr_err == SVN_ERR_RA_ILLEGAL_URL)
> - {
> - svn_error_clear(err);
> - SVN_ERR(svn_cmdline_fprintf
> - (stderr, subpool,
> - _("%s: (Not a valid URL)\n\n"),
> - svn_path_local_style(target, pool)));
> - continue;
> - }
> - else if (err)
> - return err;
> -
> }
> svn_pool_destroy(subpool);
>
> if (opt_state->xml && (! opt_state->incremental))
> SVN_ERR(svn_cl__xml_print_footer("info", pool));
>
> - return SVN_NO_ERROR;
> + if (err)
Right here, you need to clear prev_err.
> + return err;
> + else
> + return prev_err;
> }
However, you can simplify all the prev_err code away if you think my
suggestion for the first problem is reasonable, since then you just
have to maintain a printed_some_warning boolean.
--dave
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-21 01:37:29 CEST