Re: [PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Fri, Jun 2, 2017 at 10:11 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jun 02, 2017 at 09:10:09PM +0200, SZEDER Gábor wrote:
>
>> @@ -1785,15 +1785,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>>       } else if (!strcmp(arg, "--author-date-order")) {
>>               revs->sort_order = REV_SORT_BY_AUTHOR_DATE;
>>               revs->topo_order = 1;
>> -     } else if (starts_with(arg, "--early-output")) {
>> +     } else if (skip_prefix(arg, "--early-output", &optarg)) {
>>               int count = 100;
>> -             switch (arg[14]) {
>> +             switch (*optarg) {
>>               case '=':
>> -                     count = atoi(arg+15);
>> +                     count = atoi(optarg + 1);
>>                       /* Fallthrough */
>>               case 0:
>>                       revs->topo_order = 1;
>> -                    revs->early_output = count;
>> +                     revs->early_output = count;
>>               }
>
> What happens if I say "--early-output-foobar"? There should probably be
> a "default" here that rejects it. Though we'd probably to goto to get to
> the unknown block, yuck.

I don't even know what should happen when I say '--early--output', as
it's not mentioned anywhere in 'git help log' :)
And it's broken anyway...  see the first patch in v2.

As it is, 'git log' would act as if '--early-output-foobar' wasn't
specified at all: the switch statement only looks for '=' and '\0',
that '-' it gets is neither and there is no default:, so it takes no
action and the control flow resumes after the switch statement.

Anyway, this option should be rejected, of course.

Embarrassingly, the code handling '--show-linear-break' did the right
thing and refused '--show-linear-break-foo' until I came along and
with the switch to skip_prefix() I modelled its control flow after
that of '--early-output'.  So with this patch it too would ignore such
a bogus option.

> Alternatively, a helper like:
>
>   int match_opt(const char *have, const char *want, const char **argout)
>   {
>         const char *arg;
>         if (!skip_prefix(have, want, &arg))
>                 return 0;
>         if (!*arg)
>                 *argout = NULL;
>         else if (*arg == '=')
>                 *argout = arg + 1;
>         else
>                 return 0;
>         return 1;
>   }
>
> would let us do:
>
>   if (match_opt(arg, "--early-output"), &optarg)) {
>         int count = optarg ? atoi(optarg) : 100;
>         ...
>   }
>
> which is a little nicer and could maybe help other options (I didn't see
> any, though).

Besides '--show-linear-break' and '--pretty', other options that could
benefit from this, i.e. long options with an optional argument, are
'--expand-tabs', '--abbrev' and '--no-walk'.  These are handled
differently than '--early--output' and '--show-linear-break': each is
covered by two if branches, one with and one without the optional 
argument, i.e.:

  } else if (!strcmp(arg, "--option")) {
    ...
  } else if (starts_with(arg, "--option=")) {
    ...
  } else ...

'--pretty=' couldn't benefit, though, because it is special in that
it's equivalent with '--format=', and the two are handled in the same
branch.

So inherently there are a few repeated option names and variable
assignments, and that's not so good.  However, refactoring these to
use match_opt() adds 40% more lines than it removes and, more
importantly, increases the number of nested conditions.  Subjectively
I don't think it's better, so I went with the "follow the conventions
of the surrounding code" rule for the update.

> If we're going to go that route, though, I suspect there
> may be some helpers we already have. Looks like parse_long_opt() is
> almost there, but doesn't handle options. I wonder if we could reuse
> bits of parse-options here (or even better, just parse-optify many of
> these).

Well, parse-optifying this many options would be a tad too much for
something that started out as a little while-at-it when I tried to
make sense of --format='%p', --parents/--children, -L:func:file and
their various combinations :)

As far as I can tell, parse-options doesn't handle options with an
optional argument by itself, but only with callback functions, so it
is no help here as it is.


So, here comes v2.  The interdiff is below, the changes since v1 are:

 - Patch 1/5 is new to fix a more fundamental problem with
   '--early-output'.
 - Patch 3/5 is new to fix this '--early-output-foo' issue and also
   to tighten up the parsing of its integer argument, while at it.
 - A fix for '--show-linear-break-foo' in v1.
 - A little cleanup in the handling of '--show-notes/--notes'.


SZEDER Gábor (5):
  revision.h: turn rev_info.early_output back into an unsigned int
  revision.c: stricter parsing of '--no-{min,max}-parents'
  revision.c: stricter parsing of '--early-output'
  revision.c: use skip_prefix() in handle_revision_opt()
  revision.c: use skip_prefix() in handle_revision_pseudo_opt()

 revision.c | 87 +++++++++++++++++++++++++++++---------------------------------
 revision.h |  5 ++--
 2 files changed, 44 insertions(+), 48 deletions(-)

-- 
2.13.0.420.g54001f015

diff --git a/revision.c b/revision.c
index ab0279572..12a44189e 100644
--- a/revision.c
+++ b/revision.c
@@ -1785,16 +1785,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--author-date-order")) {
 		revs->sort_order = REV_SORT_BY_AUTHOR_DATE;
 		revs->topo_order = 1;
-	} else if (skip_prefix(arg, "--early-output", &optarg)) {
-		int count = 100;
-		switch (*optarg) {
-		case '=':
-			count = atoi(optarg + 1);
-			/* Fallthrough */
-		case 0:
-			revs->topo_order = 1;
-			revs->early_output = count;
-		}
+	} else if (!strcmp(arg, "--early-output")) {
+		revs->early_output = 100;
+		revs->topo_order = 1;
+	} else if (skip_prefix(arg, "--early-output=", &optarg)) {
+		if (strtoul_ui(optarg, 10, &revs->early_output) < 0)
+			die("'%s': not a non-negative integer", optarg);
+		revs->topo_order = 1;
 	} else if (!strcmp(arg, "--parents")) {
 		revs->rewrite_parents = 1;
 		revs->print_parents = 1;
@@ -1923,14 +1920,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->show_signature = 1;
 	} else if (!strcmp(arg, "--no-show-signature")) {
 		revs->show_signature = 0;
-	} else if (skip_prefix(arg, "--show-linear-break", &optarg)) {
-		switch (*optarg) {
-		case '=':
-			revs->break_bar = xstrdup(optarg + 1);
-			break;
-		case 0:
-			revs->break_bar = "                    ..........";
-		}
+	} else if (!strcmp(arg, "--show-linear-break")) {
+		revs->break_bar = "                    ..........";
+		revs->track_linear = 1;
+		revs->track_first_time = 1;
+	} else if (skip_prefix(arg, "--show-linear-break=", &optarg)) {
+		revs->break_bar = xstrdup(optarg);
 		revs->track_linear = 1;
 		revs->track_first_time = 1;
 	} else if (skip_prefix(arg, "--show-notes=", &optarg) ||
@@ -1938,12 +1933,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		struct strbuf buf = STRBUF_INIT;
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
-		if (starts_with(arg, "--show-notes=")) {
-			if (revs->notes_opt.use_default_notes < 0)
-				revs->notes_opt.use_default_notes = 1;
-			strbuf_addstr(&buf, optarg);
-		} else
-			strbuf_addstr(&buf, optarg);
+		if (starts_with(arg, "--show-notes=") &&
+		    revs->notes_opt.use_default_notes < 0)
+			revs->notes_opt.use_default_notes = 1;
+		strbuf_addstr(&buf, optarg);
 		expand_notes_ref(&buf);
 		string_list_append(&revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));
diff --git a/revision.h b/revision.h
index a91dd3d5d..f96e7f7f4 100644
--- a/revision.h
+++ b/revision.h
@@ -74,8 +74,9 @@ struct rev_info {
 	/* topo-sort */
 	enum rev_sort_order sort_order;
 
-	unsigned int	early_output:1,
-			ignore_missing:1,
+	unsigned int early_output;
+
+	unsigned int	ignore_missing:1,
 			ignore_missing_links:1;
 
 	/* Traversal flags */