Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Three kinds of errors can arise from parsing '.git/sequencer/todo':
>> 1. Unrecognized action
>> 2. Malformed object name
>> 3. Object name does not refer to a valid commit
>>
>> Since we would like to make the instruction sheet user-editable in the
>> future (much like the 'rebase -i' sheet), report more fine-grained
>> parse errors prefixed with the filename and line number.
>
> Seems like a sensible idea. In other words,
> [...]
Thanks for the new wording.
> [...]
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
>> return 0;
>> }
>>
>> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
>> +static int parse_insn_line(char *bol, char *eol,
>> + struct replay_insn_list *item, int lineno)
>> {
>> + const char *todo_file = git_path(SEQ_TODO_FILE);
>
> This idiom is _still_ scary. I don't want to have to shout about
> this, but why didn't the commit message at least acknowledge it to put
> the reader's mind at ease when this has come up again and again?
Carried over from the previous re-roll; sorry I didn't pay enough attention.
> [...]
>> + bol += strlen("revert ");
>> + } else {
>> + error_len = eol - bol > 255 ? 255 : eol - bol;
>> + return error(_("%s:%d: Unrecognized action: %.*s"),
>> + todo_file, lineno, (int)error_len, bol);
>
> Ah, so my example above was wrong: the actual message is
>
> error: .git/sequencer/todo:5: Unrecognized action: the quick bro
> wn fox jumps over the lazy dog
>
> I guess that's fine. Is it intended?
>
>> + }
>>
>> /* Eat up extra spaces/ tabs before object name */
>> - padding = strspn(bol, " \t");
>> - if (!padding)
>> - return -1;
>> - bol += padding;
>> + bol += strspn(bol, " \t");
>
> What does this have to do with the stated goal of the patch? It seems
> to me that an unrelated and unadvertised bugfix snuck in.
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".
> [...]
> By the way, this is gross. Probably get_sha1 should grow a variant
> that takes a buffer and a length.
Yes; will do.
> [...]
>> item->operand = lookup_commit_reference(commit_sha1);
>> - if (!item->operand)
>> - return -1;
>> + if (!item->operand) {
>> + error_len = eol - bol > 255 ? 255 : eol - bol;
>> + 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?
> [...]
>> @@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>>
>> for (i = 1; *p; i++) {
>> char *eol = strchrnul(p, '\n');
>> - if (parse_insn_line(p, eol, &item) < 0)
>> - return error(_("Could not parse line %d."), i);
>> + if (parse_insn_line(p, eol, &item, i) < 0)
>> + return -1;
>
> Not related to this patch, but why the "< 0" test? It makes this
> reader worry that there is some unusual return value convention that
> he should be taking into account.
Right. Will fix.
Thanks.
-- Ram
--
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