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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 8, 2012, 21:33 UTC
Message-ID: <20120108213318.GQ1942@burratino>
In-Reply-To: <CALkWK0mStgcb4EBB+ni9fisDJY=13cJZWCTEcgfyOUyAXbc=tA@mail.gmail.com>

Ramkumar Ramachandra wrote:

> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:

Show 6 quoted lines

>>>       /* 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.

[...]

Show 13 quoted lines

>>> +             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