Provide contextual error messages, not “Internal exception occurred”

[bignose-debian]

In many failure scenarios (e.g. issue #153, issue #162, and others) the app presents a raw message from the jgit library.

The jgit library messages are very low-level, and the message presented to the user is lacking important context from the Password Store app about what it was doing when the jgit library raised an exception.

Please catch jgit library exceptions, at each point where useful contextual information from Password Store can be presented in a more useful diagnostic error message.

[zeapo]

Can we list all the different cases that need to be handled? This will be transferred into a PR once we start working on it.

• The given path is not a bare repository Invalid remote: origin #238
• None of the keys match -> fall back to password auth Internal exception: jgit - length of null array #153
• HEAD is detached (happens in case of conflict mostly) Internal exception occured. Message from jgit: HEAD is detached #162

[bignose-debian]

On 01-Nov-2016, Mohamed Zenadi wrote:

Can we list all the different cases that need to be handled?

I think the comprehensive approach is not to enumerate failure cases,
but to enumerate the places in the code where the jgit library is
used.

Any use of jgit should catch jgit exceptions, and provide context at
that point about what was being done when the exception occurred.

So the list of tasks is IMO to be found by enumerating all the
distinct code locations that use jgit to do some action.

Catch any jgit exception that occurs at that location. Annotate the
resulting message with “I was doing this specific action when a jgit
exception occurred”.

The “this specific action” should be whatever the user needs to know
to tell developers exactly what caused a jgit exception.

This will be transferred into a PR once we start working on it.

Thanks.

\ “Consider the daffodil. And while you're doing that, I'll be |
`\ over here, looking through your stuff.” —Jack Handey |
o_) |
Ben Finney ben@benfinney.id.au

[zeapo]

Well, in fact there is a single place where jgit is used https://github.com/zeapo/Android-Password-Store/blob/master/app/src/main/java/com/zeapo/pwdstore/git/GitAsyncTask.java#L65-L85

That's the place where we ought to handle the different cases. The command being handled is known at https://github.com/zeapo/Android-Password-Store/blob/master/app/src/main/java/com/zeapo/pwdstore/git/GitAsyncTask.java#L42 and the result is returned in as a String. We have to change that to send back a combination of Cmd/jgit exception so that the post processing knows how to give a feedback.

The list I started to define earlier is only meant for a better description / documentation of the issue. We also have to check in eclipse git plugin how the implemented exceptions handling for jgit errors.

The issue here is that the project in its current state is not easily maintainable, a lot of refactoring has to be done, and I have not much time to do it on my own.

[bignose-debian]

On 06-Nov-2016, Mohamed Zenadi wrote:

Well, in fact there is a single place where jgit is used
https://github.com/zeapo/Android-Password-Store/blob/master/app/src/main/java/com/zeapo/pwdstore/git/GitAsyncTask.java#L65-L85

Thanks. If I understand correctly, that is a wrapper to execute the
command asynchronously, and has no context about why the command is
being executed.

So I think that location in the code does not have the context
needed to provide useful diagnostic information.

The command being handled is known at
https://github.com/zeapo/Android-Password-Store/blob/master/app/src/main/java/com/zeapo/pwdstore/git/GitAsyncTask.java#L42
and the result is returned in as a String. We have to change that
to send back a combination of Cmd/jgit exception so that the post
processing knows how to give a feedback.

I think the exception should not be caught in that function; as you
say, it knows only the command, and not the user operation which
caused that command.

Instead, the exception should propagate to a point where the code
does know the user action that led to that Git command.

The list I started to define earlier is only meant for a better
description / documentation of the issue.

As a rough guide – and, importantly, an easy criterion – I would say a
complete set of locations where useful contextual information about
the user action is known:

All the places in the code that invoke a GitAsyncTask. Catch the
exception and provide diagnostic information about what Password Store
was doing (i.e. why it needed a GitAsyncTask).

That can be refined further, but it would at least be complete :-) and
need no difficult choices at first.

The issue here is that the project in its current state is not
easily maintainable, a lot of refactoring has to be done, and I have
not much time to do it on my own.

I don't know Java at all well. Thank you for maintaining this project.

I'm making these suggestions to hopefully improve the diagnostic
information without a lot of redesign, which in turn should make the
bug reports easier to understand.

\ “For every complex problem, there is a solution that is simple, |
`\ neat, and wrong.” —Henry L. Mencken |
o_) |
Ben Finney ben@benfinney.id.au

[zeapo]

Thanks for your comments.

One way I could do that is by passing a callback to the GitAsyncTask to be called in onPostExecute with the exception. That way we could handle it from the calling activity.

[bignose-debian]

On 06-Nov-2016, Mohamed Zenadi wrote:

One way I could do that is by passing a callback to the
GitAsyncTask to be called in onPostExecute with the exception.
That way we could handle it from the calling activity.

Sounds fine to me, I'll trust you with knowing the techniques :-)

\ “If you can't beat them, arrange to have them beaten.” —George |
`\ Carlin |
o_) |
Ben Finney ben@benfinney.id.au

[zeapo]

This is being worked on :)

[hughdavenport]

What was left to work on for this one @zeapo ?