From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sun, 08 Jan 2012 20:16:07 GMT
Subject: Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser
Message-ID: <CALkWK0mStgcb4EBB+ni9fisDJY=13cJZWCTEcgfyOUyAXbc=tA@mail.gmail.com>
In-Reply-To: <20120108200748.GJ1942@burratino>

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