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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html