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

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:

>>>       /* Eat up extra spaces/ tabs before object name */
>>> -     padding = strspn(bol, " \t");
>>> -     if (!padding)
>>> -             return -1;
>>> -     bol += padding;
>>> +     bol += strspn(bol, " \t");
[...]
> Not a bugfix: since I have to report sensible error messages now, I
> changed the "pick" and "revert" checks to "pick " || "pick\t" and
> "revert " || "revert\t" -- then, I can report "invalid action" if it
> doesn't match instead of a useless "missing space after action".

Ah, I forgot that the "if (!padding)" check noticed errors like

	picking foo

before.  So this is just a code cleanup, with no functional effect.

However, you can still report "invalid action" with the old code
structure --- it would just mean duplicating an error message in the
code, since you reach the same conclusion by two code paths.  So it's
a relevant cleanup, but I'd still suggest lifting it into a patch that
comes before so future readers can assure themselves that it introduces
no functional change instead of being confused.

[...]
>>> +             return error(_("%s:%d: Not a valid commit: %.*s"),
>>> +                     todo_file, lineno, (int)error_len, bol);
>>> +     }
>>
>> Hmm, this one can be emitted even when there was no corruption or
>> internal error, because the user removed a commit she was
>> cherry-picking and the gc-ed before a "git cherry-pick --continue".
>> Alternatively, it can happen because the repository has grown very
>> crowded and what used to be an unambiguous commit name no longer is
>> one (not enough digits).  Will the error message be intuitive in these
>> situations?
>
> Something like "Unable to look up commit: %s" perhaps?

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.)

Thanks for some food for thought.
Jonathan