Re: What's cooking in git.git (Dec 2011, #02; Mon, 5)

[Junio C Hamano <gitster@pobox.com>]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>>  * On "revert: make commit subjects in insn sheet optional"
>>
>>   After finding the verb and advancing the pointer "bol" at the beginning of
>>   the object name, end_of_object_name variable points at the first SP or LF
>>   using strcspn(bol, " \n"), but I wonder why we are not grabbing a run of
>>   hexdigits instead, i.e. strspn(bol, "0123456789abcdef"). Is this so that
>>   we can allow something like "pick rr/revert-cherry-pick~3"?
>
> Yes, it is exactly for that reason :)

That is not explained in the commit message, which in itself is a problem
that needs to be addressed, but ...

>>   I also wonder if this should be (sorry for pcre) "(pick|revert)\s+(\S+)\s"
>>   instead, i.e. allow users with fat fingers to use one or more SP or even HT
>>   to separate the verb and the operand.
>
> Hm, I'm not too enthusiastic about this change, because we don't
> advertise hand-editing the instruction sheet yet:...

... is inconsistent with the above.

If you plan to later allow editing and if you know what kind of editing
you are going to allow, it would be better to prepare the parser to accept
input with a reasonable slack, especially because you are already touching
it in this series anyway.

On the other hand, if you do not want to think about (or do not want us to
get distracted by thinking about) allowing editing in this series, which
is also a sensible stance to take, the parser should be made to accept
only what the mechanism in the series would produce and error out
otherwise, leaving the entire "the user now can edit and here are the rules"
part for a separate series that comes after this series is perfected.

>>  * On "revert: allow mixed pick and revert instructions"
>>
>>   Reporting what we did not understand from parse_insn_line() is a good
>>   idea, but I think the line number should be reported at the beginning
>>   of the same line.
>
> Makes sense.  Do you like this?

Not particularly.

> -		return error(_("Unrecognized action: %.*s"), (int)len, bol);
> +		return error(_("Line %d: Unrecognized action: %.*s"),

It may be just me, but personally I prefer to see which file of what line
had the problem, i.e. "%s:%d: Unrecognized action:%s".

But if there is not much point in reporting the filename because it always
is the same, "Unrecognized action (line %d): %s" would also be fine.