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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 8, 2011, 18:19 UTC
Message-ID: <7vzkf3hqor.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0nNtvrLHxQv17jfrFQ=BcwLfgh7eE9X-nDCCYY0nsOskg@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 7 quoted lines

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