[MERGE] Improve error handling in msgeditor._run_editor.

[James Westby]

On Thu, 2008-12-18 at 10:53 +1100, Andrew Bennetts wrote:
> A user on IRC just reported trouble using “bzr commit” without -m.  It
> turned out that somehow they had this line in their bazaar.conf:
> 
>     editor = ""
> 
> There are two issues here.  First is that executing "" causes EACCES, and
> _run_editor wasn't catching that.  The second is that the error reporting
> was very unhelpful in this case; it didn't report which command bzr was
> trying to run, or why.

> I'm interested in feedback about emitting a warning in this situation.  On
> one hand, it would have made this problem easier to diagnose — the user on
> IRC today probably wouldn't have needed to come to #bzr for help if we
> emitted it.  But maybe it's too chatty to complain if a user's environment
> is slightly broken?  I think probably this change is fine (it's usually not
> hard to set $EDITOR correctly), but I know extra output can irritate people,
> so let me know.

I don't think this is a problem.

I do have a couple of small comments about the message though.

> +            if e.errno in (errno.ENOENT, errno.EACCES):
> +                if candidate_source is not None:
> +                    # We tried this editor because some user
> configuration (an
> +                    # environment variable or config file) said to
> try it.  Let
> +                    # the user know their configuration is broken.
> +                    trace.warning(
> +                        "Could not start editor %s: %s (specified by
> %s)\n"
> +                        % (candidate, str(e), candidate_source))

If the editor is specified as "" then this message becomes unclear:

   Could not start editor :

Should the value be quoted to indicate that there should be something
there.

The second is that "$BZR_EDITOR" may not be clear enough for some 
people, would "$BZR_EDITOR in the environment" or similar be clearer?

Thanks,

James