Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser

Jonathan Nieder wrote:

My "alternatively" was bogus --- lookup_commit_reference takes a (raw)
full commit name as its argument.

I dunno.  The question was not actually rhetorical --- I just meant
that it's worth thinking about these cases.  There are a few cases:

 - missing object
 - object is present but corrupt
 - object is a blob, not a commit

In the second case, there's an error message printed describing the
problem, but in the other two there isn't.  The other callers tend to
say "not a valid <foo>" or "could not lookup commit <foo>, so I guess

       error: .git/sequencer/todo:5: not a valid commit: 78a89f493

would be fine.

Except that this focusses on the .git/sequencer/todo filename which
would leave the person debugging astray.  It is not that
.git/sequencer/todo contains a typo (that would have been caught by
get_sha1), but that it referred to a bad object or non-commit.  Maybe
something in the direction of

       error: cannot pick 78a89f493 because it is not a valid commit

would be more helpful.

Is this the right moment to report that error?  Will the operator be
happy that we errored out right away before cherry-picking anything
and wasting the human's time in assisting with that process, or will
she be unhappy that inability to do something later that she might
have been planning on skipping anyway prevented making progress right
away?  (I'm not sure what the best thing to do is --- I guess some
advice like

       hint: to abort, use cherry-pick --abort
       hint: to skip or replace that commit, use cherry-pick --edit

would help.)

Ignoring this in the re-roll: I'd be inclined to put these changes in
a separate series that begins with bolting on some advice
configuration to the sequencer.

Thanks for thinking about these things.

-- Ram