ilya.bobyr@xxxxxxxxx writes:
> From: Ilya Bobyr <ilya.bobyr@xxxxxxxxx>
>
> It is not very likely that any of the "*=?!" Characters would be useful
> in the argument short or long names. On the other hand, there are
> already argument hints that contain the "=" sign. It used to be
> impossible to include any of the "*=?!" signs in the arguments hints
> before.
After reading this three times (without looking at the code), it is
unclear to me what the change wants to achieve. A few points that
confuse me:
- "It is not very likely..."; so what does this change do to such
an unlikely case? Does it just forbid? Or does it have escape
hatches?
- "... there are already ..."; so an unlikely case already exists?
- "It used to be impossible..."; hmmmm, it earlier said there are
already cases there---how they have been working?
Perhaps it would clarify the paragraph if you said upfront that a
parseopt option specification is <opt-spec> (i.e. short and long
names) optionally followed by <flags> (i.e. one or more of these
"*=?!" characters) and the <arg-hint> string to remind the readers
and reviewers, and phrase what you wrote to make the differences
between them stand out?
A line in the input to "rev-parse --parseopt" describes an
option by listing a short and/or long name, optional flags
[*=?!], argument hint, and then whitespace and help string.
The code first finds the help string and scans backwards to find
the flags, which would imply that [*=?!] is allowed in the
option names but not in argument hint string.
That is backwards; you do not want these special characters in
option names, but you may want to be able to include them
(especially '=', as in 'key=value') in the argument hint string.
Change the parsing to go from the beginning to find the first
occurrence of [*=?!] to find the flags and use the remainder as
argument hint.
or something, perhaps.
> Added test case with equals sign in the argument hint and updated the
"Add a test case with ... and update the test ...". Write your log
message as if you are giving somebody an order with your commit to
do such and such.
> test to perform all the operations in test_expect_success matching the
> t\README requirements and allowing commands like
t/README.
>
> ./t1502-rev-parse-parseopt.sh --run=1-2
>
> to stop at the test case 2 without any further modification of the test
> state area.
>
> Signed-off-by: Ilya Bobyr <ilya.bobyr@xxxxxxxxx>
> ---
> builtin/rev-parse.c | 36 ++++++++--------
> t/t1502-rev-parse-parseopt.sh | 97 ++++++++++++++++++++++++++-----------------
> 2 files changed, 77 insertions(+), 56 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index b623239..205ea67 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -423,17 +423,25 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
> o->flags = PARSE_OPT_NOARG;
> o->callback = &parseopt_dump;
>
> - /* Possible argument name hint */
> + /* parse names, type and the hint */
> end = s;
> - while (s > sb.buf && strchr("*=?!", s[-1]) == NULL)
> - --s;
I must have overlooked this one long time ago when a patch added
this; it is a horrible way to parse a thing from the tail. Good to
see the code go ;-)
> - if (s != sb.buf && s != end)
> - o->argh = xmemdupz(s, end - s);
> - if (s == sb.buf)
> - s = end;
> + s = sb.buf;
> +
> + /* name(s) */
> + while (s < end && strchr("*=?!", *s) == NULL)
> + ++s;
In C, we usually pre-decrement and post-increment, unless the value
is used.
More importantly, can't we write this more concisely by using
strcspn(3)?
const char *flags_chars = "*=?!";
size_t leading = strcspn(s, flags_chars);
if (s + leading < end)
... /* s + leading is the beginning of flags */
else
... /* there was no flags before end */
> +
> + if (s - sb.buf == 1) /* short option only */
> + o->short_name = *sb.buf;
> + else if (sb.buf[1] != ',') /* long option only */
> + o->long_name = xmemdupz(sb.buf, s - sb.buf);
> + else {
> + o->short_name = *sb.buf;
> + o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
> + }
>
> - while (s > sb.buf && strchr("*=?!", s[-1])) {
> - switch (*--s) {
> + while (s < end && strchr("*=?!", *s)) {
> + switch (*s++) {
> case '=':
No need for the strchr() dance, I think, because you will do
different things depending on *s inside the loop anyway. Just
while (s < end) {
switch (*s++) {
case '=':
do the "equal" thing;
continue;
case '*':
do the "asterisk" thing;
continue;
...
}
break;
}
or something.
Yes, I agree that the original is coded very incompetently, but
there is no reason to inherit that to your fixed version ;-).
Thanks.
--
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