Re: [PATCH] gpg-interface: Add some output from gpg when it errors out.

From: Mike Hommey <mh@glandium.org>
Date: Jan 25, 2017, 23:54 UTC
Message-ID: <20170125235410.byxwmo7o7zdszzot@glandium.org>
In-Reply-To: <xmqqtw8m7ncp.fsf@gitster.mtv.corp.google.com>

On Wed, Jan 25, 2017 at 03:04:38PM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> Mike Hommey <mh@glandium.org> writes:
> 
> > For instance, after changing my laptop for a new one, I copied my
> > configs, but had some environment differences that broke gpg.
> > With this change applied, the output becomes, on this new machine:
> >   gpg: keyblock resource '/usr/share/keyrings/debian-keyring.gpg': No
> > such file or directory
> >   error: gpg failed to sign the data
> >   error: unable to sign the tag
> >
> > which makes it clearer what's wrong.
> 
> Overall I think this is a good thing to do.  Instead of eating the
> status output, showing what we got, especially when we know the
> command failed, would make the bug-hunting of user's environment
> easier, like you showed above.
> 
> The only thing in the design that makes me wonder is the filtering
> out based on "[GNUPG:]" prefix.  Why do we need to filter them out?

The [GNUPG:] lines are part of the status-fd protocol. They show details that don't really seem interesting to the user. In fact, they even contain the signed message (yes, in my case, it turns out gpg was actually still signing, but returned an error code...).

For instance, in that failed run above, the entire output looks like:

gpg: keyblock resource '/usr/share/keyrings/debian-keyring.gpg': No such file or directory [GNUPG:] ERROR add_keyblock_resource ... [GNUPG:] KEY_CONSIDERED ... [GNUPG:] BEGIN_SIGNING H2 [GNUPG:] SIG_CREATED ...

All the [GNUPG:] lines are implementation details that don't seem particularly useful. A case could be made for the ERROR one, though, although it's also implementation detail-y.

> Implementation-wise, I'd be happier if we do not add any new
> callsites of strbuf_split(), which is a horrible interface.  But
> that is a minor detail.

What would you suggest otherwise?

Mike