[PATCH 0/3] Use skip_prefix() in handle_revision_{,pseudo_}opt()

[PATCH 0/3] Use skip_prefix() in handle_revision_{,pseudo_}opt()

[SZEDER Gábor]

While at it, the first one fixes a minor bug, which allowed e.g. 'git
log --no-min-parents-foobarbaz' to succeed.

The other two are fairly straightforward starts_with() ->
skip_prefix() conversions.

SZEDER Gábor (3):
  revision.c: stricter parsing of '--no-{min,max}-parents'
  revision.c: use skip_prefix() in handle_revision_opt()
  revision.c: use skip_prefix() in handle_revision_pseudo_opt()

 revision.c | 76 ++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 39 insertions(+), 37 deletions(-)

-- 
2.13.0.420.g54001f015

[PATCH 1/3] revision.c: stricter parsing of '--no-{min,max}-parents'

[SZEDER Gábor]

These two options are parsed using starts_with(), allowing things like
'git log --no-min-parents-foobarbaz' to succeed.

Use strcmp() instead.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index f88c14bab..2f37e1e3a 100644
--- a/revision.c
+++ b/revision.c
@@ -1812,11 +1812,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->max_parents = 1;
 	} else if (starts_with(arg, "--min-parents=")) {
 		revs->min_parents = atoi(arg+14);
-	} else if (starts_with(arg, "--no-min-parents")) {
+	} else if (!strcmp(arg, "--no-min-parents")) {
 		revs->min_parents = 0;
 	} else if (starts_with(arg, "--max-parents=")) {
 		revs->max_parents = atoi(arg+14);
-	} else if (starts_with(arg, "--no-max-parents")) {
+	} else if (!strcmp(arg, "--no-max-parents")) {
 		revs->max_parents = -1;
 	} else if (!strcmp(arg, "--boundary")) {
 		revs->boundary = 1;
-- 
2.13.0.420.g54001f015

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

[SZEDER Gábor]

Instead of starts_with() and a bunch of magic numbers.

While at it, there is an indentation fix where processing
'--early-output', and a coding style fix where processing
'--show-notes'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 revision.c | 54 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/revision.c b/revision.c
index 2f37e1e3a..2b64b7e0e 100644
--- a/revision.c
+++ b/revision.c
@@ -1725,8 +1725,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->max_count = atoi(argv[1]);
 		revs->no_walk = 0;
 		return 2;
-	} else if (starts_with(arg, "-n")) {
-		revs->max_count = atoi(arg + 2);
+	} else if (skip_prefix(arg, "-n", &optarg)) {
+		revs->max_count = atoi(optarg);
 		revs->no_walk = 0;
 	} else if ((argcount = parse_long_opt("max-age", argv, &optarg))) {
 		revs->max_age = atoi(optarg);
@@ -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;
 		}
 	} else if (!strcmp(arg, "--parents")) {
 		revs->rewrite_parents = 1;
@@ -1810,12 +1810,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->min_parents = 2;
 	} else if (!strcmp(arg, "--no-merges")) {
 		revs->max_parents = 1;
-	} else if (starts_with(arg, "--min-parents=")) {
-		revs->min_parents = atoi(arg+14);
+	} else if (skip_prefix(arg, "--min-parents=", &optarg)) {
+		revs->min_parents = atoi(optarg);
 	} else if (!strcmp(arg, "--no-min-parents")) {
 		revs->min_parents = 0;
-	} else if (starts_with(arg, "--max-parents=")) {
-		revs->max_parents = atoi(arg+14);
+	} else if (skip_prefix(arg, "--max-parents=", &optarg)) {
+		revs->max_parents = atoi(optarg);
 	} else if (!strcmp(arg, "--no-max-parents")) {
 		revs->max_parents = -1;
 	} else if (!strcmp(arg, "--boundary")) {
@@ -1897,14 +1897,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(NULL, revs);
-	} else if (starts_with(arg, "--pretty=") || starts_with(arg, "--format=")) {
+	} else if (skip_prefix(arg, "--pretty=", &optarg) ||
+		   skip_prefix(arg, "--format=", &optarg)) {
 		/*
 		 * Detached form ("--pretty X" as opposed to "--pretty=X")
 		 * not allowed, since the argument is optional.
 		 */
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
-		get_commit_format(arg+9, revs);
+		get_commit_format(optarg, revs);
 	} else if (!strcmp(arg, "--expand-tabs")) {
 		revs->expand_tabs_in_log = 8;
 	} else if (!strcmp(arg, "--no-expand-tabs")) {
@@ -1922,26 +1923,27 @@ 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 (!strcmp(arg, "--show-linear-break") ||
-		   starts_with(arg, "--show-linear-break=")) {
-		if (starts_with(arg, "--show-linear-break="))
-			revs->break_bar = xstrdup(arg + 20);
-		else
+	} else if (skip_prefix(arg, "--show-linear-break", &optarg)) {
+		switch (*optarg) {
+		case '=':
+			revs->break_bar = xstrdup(optarg + 1);
+			break;
+		case 0:
 			revs->break_bar = "                    ..........";
+		}
 		revs->track_linear = 1;
 		revs->track_first_time = 1;
-	} else if (starts_with(arg, "--show-notes=") ||
-		   starts_with(arg, "--notes=")) {
+	} else if (skip_prefix(arg, "--show-notes=", &optarg) ||
+		   skip_prefix(arg, "--notes=", &optarg)) {
 		struct strbuf buf = STRBUF_INIT;
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
-		if (starts_with(arg, "--show-notes")) {
+		if (starts_with(arg, "--show-notes=")) {
 			if (revs->notes_opt.use_default_notes < 0)
 				revs->notes_opt.use_default_notes = 1;
-			strbuf_addstr(&buf, arg+13);
-		}
-		else
-			strbuf_addstr(&buf, arg+8);
+			strbuf_addstr(&buf, optarg);
+		} else
+			strbuf_addstr(&buf, optarg);
 		expand_notes_ref(&buf);
 		string_list_append(&revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));
@@ -1978,8 +1980,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->abbrev = 0;
 	} else if (!strcmp(arg, "--abbrev")) {
 		revs->abbrev = DEFAULT_ABBREV;
-	} else if (starts_with(arg, "--abbrev=")) {
-		revs->abbrev = strtoul(arg + 9, NULL, 10);
+	} else if (skip_prefix(arg, "--abbrev=", &optarg)) {
+		revs->abbrev = strtoul(optarg, NULL, 10);
 		if (revs->abbrev < MINIMUM_ABBREV)
 			revs->abbrev = MINIMUM_ABBREV;
 		else if (revs->abbrev > 40)
-- 
2.13.0.420.g54001f015

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

[Jeff King]

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.

Perhaps we could do:

  if (skip_prefix(arg, "--early-output", &optarg) &&
      (*optarg == '=' || !*optarg)) {
          int count = *optarg ? atoi(optarg + 1) : 100;
	  revs->topo_order = 1;
	  revs->early_output = count;
  }

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

Anyway, none of that is caused by your patch, but at least doing the
minimal fix (my first hunk) seems like it fits into your series.

-Peff

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

[Jeff King]

On Fri, Jun 02, 2017 at 04:11:43PM -0400, Jeff King wrote:

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

I take it back. This would help --show-linear-break that your patch also
touches. And maybe --pretty, too.

-Peff

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

[SZEDER Gábor]

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

[PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int

[SZEDER Gábor]

rev_info.early_output started out as an unsigned int in cdcefbc97 (Add
"--early-output" log flag for interactive GUI use, 2007-11-03), but
later it was turned into a single bit in a bit field in cc243c3ce
(show: --ignore-missing, 2011-05-18) without explanation, though its
users still expect it to be a regular integer type.  Consequently, any
even number given via '--early-output=<N>' effectively disabled the
feature.

Turn it back into an unsigned int, restoring its original data type.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 revision.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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 */
-- 
2.13.0.420.g54001f015

Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int

[Jeff King]

On Fri, Jun 09, 2017 at 08:17:29PM +0200, SZEDER Gábor wrote:

> rev_info.early_output started out as an unsigned int in cdcefbc97 (Add
> "--early-output" log flag for interactive GUI use, 2007-11-03), but
> later it was turned into a single bit in a bit field in cc243c3ce
> (show: --ignore-missing, 2011-05-18) without explanation, though its
> users still expect it to be a regular integer type.  Consequently, any
> even number given via '--early-output=<N>' effectively disabled the
> feature.
> 
> Turn it back into an unsigned int, restoring its original data type.

This confused me for a moment, as on my first read it seems like the
obvious solution is to normalize the input to a bit-field, like:

  revs->early_output = !!atoi(optarg);

But the "users still expect" bit was a bit subtle to me, as I thought
you meant users of Git. But you mean that the feature itself is not a
boolean, but rather an integer count of how much early output to show.

I'm not sure if I was just being thick or if that point (and the fact
that --early-output has basically been a noop since 2011!) should be
made more explicit.

Given that nobody noticed, I kind of wonder if we should consider
ripping the feature out entirely.

-Peff

[PATCHv2.1] revision.h: turn rev_info.early_output back into an unsigned int

[SZEDER Gábor]

rev_info.early_output started out as an unsigned int in cdcefbc97 (Add
"--early-output" log flag for interactive GUI use, 2007-11-03), but
later it was turned into a single bit in a bit field in cc243c3ce
(show: --ignore-missing, 2011-05-18) without explanation, though the
code using it still expects it to be a regular integer type and uses
it as a counter.  Consequently, any even number given via
'--early-output=<N>', or indeed a plain '--early-output' defaulting to
100 effectively disabled the feature.

Turn rev_info.early_output back into its origin unsigned int data
type, making '--early-output' work again.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

> But the "users still expect" bit was a bit subtle to me, as I thought
> you meant users of Git. But you mean that the feature itself is not a
> boolean, but rather an integer count of how much early output to show.

Yeah, I wrote "callsites" first, but then realized it's not a
function...

Here is the same patch with an updated commit message now saying "code
using it" and "used as a counter" to make it clearer.  It also
mentions that an argumentless '--early-output' turns off the feature,
too.

I won't resend the rest of the series.

 revision.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

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 */
-- 
2.13.0.420.g54001f015

Re: [PATCHv2.1] revision.h: turn rev_info.early_output back into an unsigned int

[Jeff King]

On Sat, Jun 10, 2017 at 01:41:01PM +0200, SZEDER Gábor wrote:

> rev_info.early_output started out as an unsigned int in cdcefbc97 (Add
> "--early-output" log flag for interactive GUI use, 2007-11-03), but
> later it was turned into a single bit in a bit field in cc243c3ce
> (show: --ignore-missing, 2011-05-18) without explanation, though the
> code using it still expects it to be a regular integer type and uses
> it as a counter.  Consequently, any even number given via
> '--early-output=<N>', or indeed a plain '--early-output' defaulting to
> 100 effectively disabled the feature.
> 
> Turn rev_info.early_output back into its origin unsigned int data
> type, making '--early-output' work again.
> 
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> 
> > But the "users still expect" bit was a bit subtle to me, as I thought
> > you meant users of Git. But you mean that the feature itself is not a
> > boolean, but rather an integer count of how much early output to show.
> 
> Yeah, I wrote "callsites" first, but then realized it's not a
> function...
> 
> Here is the same patch with an updated commit message now saying "code
> using it" and "used as a counter" to make it clearer.  It also
> mentions that an argumentless '--early-output' turns off the feature,
> too.
> 
> I won't resend the rest of the series.

Thanks, this version is much more clear.

-Peff

Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jun 09, 2017 at 08:17:29PM +0200, SZEDER Gábor wrote:
>
>> rev_info.early_output started out as an unsigned int in cdcefbc97 (Add
>> "--early-output" log flag for interactive GUI use, 2007-11-03), but
>> later it was turned into a single bit in a bit field in cc243c3ce
>> (show: --ignore-missing, 2011-05-18) without explanation, though its
>> users still expect it to be a regular integer type.  Consequently, any
>> even number given via '--early-output=<N>' effectively disabled the
>> feature.
>> 
>> Turn it back into an unsigned int, restoring its original data type.
>
> This confused me for a moment, as on my first read it seems like the
> obvious solution is to normalize the input to a bit-field, like:
>
>   revs->early_output = !!atoi(optarg);
>
> But the "users still expect" bit was a bit subtle to me, as I thought
> you meant users of Git. But you mean that the feature itself is not a
> boolean, but rather an integer count of how much early output to show.
>
> I'm not sure if I was just being thick or if that point (and the fact
> that --early-output has basically been a noop since 2011!) should be
> made more explicit.
>
> Given that nobody noticed, I kind of wonder if we should consider
> ripping the feature out entirely.

Yes, we may want to think about deprecating it, especially given
that it is not advertised anywhere.

In any case, the patch looks correct ;-)

Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int

[Jeff King]

On Mon, Jun 12, 2017 at 01:36:08PM -0700, Junio C Hamano wrote:

> > I'm not sure if I was just being thick or if that point (and the fact
> > that --early-output has basically been a noop since 2011!) should be
> > made more explicit.
> >
> > Given that nobody noticed, I kind of wonder if we should consider
> > ripping the feature out entirely.
> 
> Yes, we may want to think about deprecating it, especially given
> that it is not advertised anywhere.
> 
> In any case, the patch looks correct ;-)

Yeah, it's definitely orthogonal to Gábor's patches.

I also wondered who might be using the feature. I assumed it was written
with gitk in mind. Digging in the list archive that seems to be the
case, and there was even an RFC patch for gitk to use it:

  http://public-inbox.org/git/18221.2285.259487.655684@cargo.ozlabs.ibm.com/

but AFAICT it was never merged.

It looks like QGit had a patch around that time, too, but left
the line using "--early-output" commented out until Git merged the
patches. And then it never got uncommented. ;)

Tig doesn't seem to use it at all. I don't know about other GUIs. I'd
kind of doubt, given the obscurity of the feature (both gitk and qgit
authors were involved in the discussion way back in 2007).

The nice thing about deprecating it is that I think callers need to be
prepared to handle the case already that it does nothing. So if we just
ripped out the code and treated it as a silent noop, everything would
just work.

-Peff

Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int

[Jeff King]

On Mon, Jun 12, 2017 at 05:59:02PM -0400, Jeff King wrote:

> The nice thing about deprecating it is that I think callers need to be
> prepared to handle the case already that it does nothing. So if we just
> ripped out the code and treated it as a silent noop, everything would
> just work.

Actually, I take that back. We might or might not have early output, but
we always say "Final output". So in theory a caller would be confused if
we no longer print that.

(I do still suspect that there are no callers).

-Peff

[PATCHv2 2/5] revision.c: stricter parsing of '--no-{min,max}-parents'

[SZEDER Gábor]

These two options are parsed using starts_with(), allowing things like
'git log --no-min-parents-foobarbaz' to succeed.

Use strcmp() instead.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index f88c14bab..2f37e1e3a 100644
--- a/revision.c
+++ b/revision.c
@@ -1812,11 +1812,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->max_parents = 1;
 	} else if (starts_with(arg, "--min-parents=")) {
 		revs->min_parents = atoi(arg+14);
-	} else if (starts_with(arg, "--no-min-parents")) {
+	} else if (!strcmp(arg, "--no-min-parents")) {
 		revs->min_parents = 0;
 	} else if (starts_with(arg, "--max-parents=")) {
 		revs->max_parents = atoi(arg+14);
-	} else if (starts_with(arg, "--no-max-parents")) {
+	} else if (!strcmp(arg, "--no-max-parents")) {
 		revs->max_parents = -1;
 	} else if (!strcmp(arg, "--boundary")) {
 		revs->boundary = 1;
-- 
2.13.0.420.g54001f015

[PATCHv2 3/5] revision.c: stricter parsing of '--early-output'

[SZEDER Gábor]

The parsing of '--early-output' with or without its optional integer
argument allowed bogus options like '--early-output-foobarbaz' to slip
through and be ignored.

Fix it by parsing '--early-output' in the same way as other options
with an optional argument are parsed.  Furthermore, use strtoul_ui()
to parse the optional integer argument and to refuse negative numbers.

While at it, use skip_prefix() instead of starts_with() and magic
numbers.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 revision.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/revision.c b/revision.c
index 2f37e1e3a..68531ff5d 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 (starts_with(arg, "--early-output")) {
-		int count = 100;
-		switch (arg[14]) {
-		case '=':
-			count = atoi(arg+15);
-			/* 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;
-- 
2.13.0.420.g54001f015

Re: [PATCHv2 3/5] revision.c: stricter parsing of '--early-output'

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> The parsing of '--early-output' with or without its optional integer
> argument allowed bogus options like '--early-output-foobarbaz' to slip
> through and be ignored.
>
> Fix it by parsing '--early-output' in the same way as other options
> with an optional argument are parsed.  Furthermore, use strtoul_ui()
> to parse the optional integer argument and to refuse negative numbers.
>
> While at it, use skip_prefix() instead of starts_with() and magic
> numbers.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  revision.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 2f37e1e3a..68531ff5d 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 (starts_with(arg, "--early-output")) {
> -		int count = 100;
> -		switch (arg[14]) {
> -		case '=':
> -			count = atoi(arg+15);
> -			/* Fallthrough */
> -		case 0:
> -			revs->topo_order = 1;
> -		       revs->early_output = count;
> -		}

This shows how correct patch 1/5 is, huh?  It's kind of surprising
that nobody complained about the breakage since May 2011 which in
turn makes me suspect that early-output might not be missed if we
dropped it someday, but that is a separate issue.  This series makes
it work as the feature was envisioned to.

Thanks.


> +	} 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;

[PATCHv2 4/5] revision.c: use skip_prefix() in handle_revision_opt()

[SZEDER Gábor]

Instead of starts_with() and a bunch of magic numbers.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 revision.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/revision.c b/revision.c
index 68531ff5d..c99c47c50 100644
--- a/revision.c
+++ b/revision.c
@@ -1725,8 +1725,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->max_count = atoi(argv[1]);
 		revs->no_walk = 0;
 		return 2;
-	} else if (starts_with(arg, "-n")) {
-		revs->max_count = atoi(arg + 2);
+	} else if (skip_prefix(arg, "-n", &optarg)) {
+		revs->max_count = atoi(optarg);
 		revs->no_walk = 0;
 	} else if ((argcount = parse_long_opt("max-age", argv, &optarg))) {
 		revs->max_age = atoi(optarg);
@@ -1807,12 +1807,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->min_parents = 2;
 	} else if (!strcmp(arg, "--no-merges")) {
 		revs->max_parents = 1;
-	} else if (starts_with(arg, "--min-parents=")) {
-		revs->min_parents = atoi(arg+14);
+	} else if (skip_prefix(arg, "--min-parents=", &optarg)) {
+		revs->min_parents = atoi(optarg);
 	} else if (!strcmp(arg, "--no-min-parents")) {
 		revs->min_parents = 0;
-	} else if (starts_with(arg, "--max-parents=")) {
-		revs->max_parents = atoi(arg+14);
+	} else if (skip_prefix(arg, "--max-parents=", &optarg)) {
+		revs->max_parents = atoi(optarg);
 	} else if (!strcmp(arg, "--no-max-parents")) {
 		revs->max_parents = -1;
 	} else if (!strcmp(arg, "--boundary")) {
@@ -1894,14 +1894,15 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(NULL, revs);
-	} else if (starts_with(arg, "--pretty=") || starts_with(arg, "--format=")) {
+	} else if (skip_prefix(arg, "--pretty=", &optarg) ||
+		   skip_prefix(arg, "--format=", &optarg)) {
 		/*
 		 * Detached form ("--pretty X" as opposed to "--pretty=X")
 		 * not allowed, since the argument is optional.
 		 */
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
-		get_commit_format(arg+9, revs);
+		get_commit_format(optarg, revs);
 	} else if (!strcmp(arg, "--expand-tabs")) {
 		revs->expand_tabs_in_log = 8;
 	} else if (!strcmp(arg, "--no-expand-tabs")) {
@@ -1919,26 +1920,23 @@ 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 (!strcmp(arg, "--show-linear-break") ||
-		   starts_with(arg, "--show-linear-break=")) {
-		if (starts_with(arg, "--show-linear-break="))
-			revs->break_bar = xstrdup(arg + 20);
-		else
-			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 (starts_with(arg, "--show-notes=") ||
-		   starts_with(arg, "--notes=")) {
+	} else if (skip_prefix(arg, "--show-notes=", &optarg) ||
+		   skip_prefix(arg, "--notes=", &optarg)) {
 		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, arg+13);
-		}
-		else
-			strbuf_addstr(&buf, arg+8);
+		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));
@@ -1975,8 +1973,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->abbrev = 0;
 	} else if (!strcmp(arg, "--abbrev")) {
 		revs->abbrev = DEFAULT_ABBREV;
-	} else if (starts_with(arg, "--abbrev=")) {
-		revs->abbrev = strtoul(arg + 9, NULL, 10);
+	} else if (skip_prefix(arg, "--abbrev=", &optarg)) {
+		revs->abbrev = strtoul(optarg, NULL, 10);
 		if (revs->abbrev < MINIMUM_ABBREV)
 			revs->abbrev = MINIMUM_ABBREV;
 		else if (revs->abbrev > 40)
-- 
2.13.0.420.g54001f015

[PATCHv2 5/5] revision.c: use skip_prefix() in handle_revision_pseudo_opt()

[SZEDER Gábor]

Instead of starts_with() and a bunch of magic numbers.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 revision.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/revision.c b/revision.c
index c99c47c50..12a44189e 100644
--- a/revision.c
+++ b/revision.c
@@ -2135,20 +2135,20 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		return argcount;
-	} else if (starts_with(arg, "--branches=")) {
+	} else if (skip_prefix(arg, "--branches=", &optarg)) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (starts_with(arg, "--tags=")) {
+	} else if (skip_prefix(arg, "--tags=", &optarg)) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (starts_with(arg, "--remotes=")) {
+	} else if (skip_prefix(arg, "--remotes=", &optarg)) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
@@ -2158,14 +2158,14 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		*flags ^= UNINTERESTING | BOTTOM;
 	} else if (!strcmp(arg, "--no-walk")) {
 		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-	} else if (starts_with(arg, "--no-walk=")) {
+	} else if (skip_prefix(arg, "--no-walk=", &optarg)) {
 		/*
 		 * Detached form ("--no-walk X" as opposed to "--no-walk=X")
 		 * not allowed, since the argument is optional.
 		 */
-		if (!strcmp(arg + 10, "sorted"))
+		if (!strcmp(optarg, "sorted"))
 			revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-		else if (!strcmp(arg + 10, "unsorted"))
+		else if (!strcmp(optarg, "unsorted"))
 			revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
 		else
 			return error("invalid argument to --no-walk");
-- 
2.13.0.420.g54001f015

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

[Jeff King]

On Fri, Jun 09, 2017 at 08:17:28PM +0200, SZEDER Gábor wrote:

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

I think those multi-branch cases end up as an improvement with a helper:

  if (match_opt(arg, "--no-walk", &optarg)) {
	if (!optarg || !strcmp(optarg, "sorted"))
		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
	else if (!strcmp(optarg, "unsorted"))
		revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
	else
		return error(...);
  }

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

I think you could still handle them both in the same branch, like:

  if (match_opt(arg, "--pretty", &optarg) ||
      skip_prefix(arg, "--format=", &optarg)) {
       revs->verbose_header = 1;
       revs->pretty-given = 1;
       /* OK to pass NULL for --pretty case */
       get_commit_format(optarg, revs);
  }

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

I care less about lines of boilerplate code and more about repeated
logic. In the --pretty example above, the first two lines of the block
are common to both --pretty and --pretty=. If they ever need to change,
somebody has to update two spots.

Anyway. I certainly don't insist on you working on this, especially if
you don't agree with the aesthetics. Just fixing the actual bugs would
be sufficient for my review. ;)

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

There's a flag, PARSE_OPT_OPTARG, which would do what you want. But I
agree that converting the whole thing to parse-options would be a lot of
work (quite a few of these really aren't just "this is a string", but
would need independent callback functions.

-Peff

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

[Jeff King]

On Fri, Jun 09, 2017 at 08:17:28PM +0200, SZEDER Gábor wrote:

> 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(-)

I noted a minor nit in the first commit message, but otherwise these all
look good to me.

-Peff

[PATCH 3/3] revision.c: use skip_prefix() in handle_revision_pseudo_opt()

[SZEDER Gábor]

Instead of starts_with() and a bunch of magic numbers.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 revision.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/revision.c b/revision.c
index 2b64b7e0e..ab0279572 100644
--- a/revision.c
+++ b/revision.c
@@ -2142,20 +2142,20 @@ static int handle_revision_pseudo_opt(const char *submodule,
 	} else if ((argcount = parse_long_opt("exclude", argv, &optarg))) {
 		add_ref_exclusion(&revs->ref_excludes, optarg);
 		return argcount;
-	} else if (starts_with(arg, "--branches=")) {
+	} else if (skip_prefix(arg, "--branches=", &optarg)) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (starts_with(arg, "--tags=")) {
+	} else if (skip_prefix(arg, "--tags=", &optarg)) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
-	} else if (starts_with(arg, "--remotes=")) {
+	} else if (skip_prefix(arg, "--remotes=", &optarg)) {
 		struct all_refs_cb cb;
 		init_all_refs_cb(&cb, revs, *flags);
-		for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
+		for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
 		clear_ref_exclusion(&revs->ref_excludes);
 	} else if (!strcmp(arg, "--reflog")) {
 		add_reflogs_to_pending(revs, *flags);
@@ -2165,14 +2165,14 @@ static int handle_revision_pseudo_opt(const char *submodule,
 		*flags ^= UNINTERESTING | BOTTOM;
 	} else if (!strcmp(arg, "--no-walk")) {
 		revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-	} else if (starts_with(arg, "--no-walk=")) {
+	} else if (skip_prefix(arg, "--no-walk=", &optarg)) {
 		/*
 		 * Detached form ("--no-walk X" as opposed to "--no-walk=X")
 		 * not allowed, since the argument is optional.
 		 */
-		if (!strcmp(arg + 10, "sorted"))
+		if (!strcmp(optarg, "sorted"))
 			revs->no_walk = REVISION_WALK_NO_WALK_SORTED;
-		else if (!strcmp(arg + 10, "unsorted"))
+		else if (!strcmp(optarg, "unsorted"))
 			revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
 		else
 			return error("invalid argument to --no-walk");
-- 
2.13.0.420.g54001f015

Re: [PATCH 0/3] Use skip_prefix() in handle_revision_{,pseudo_}opt()

[Jeff King]

On Fri, Jun 02, 2017 at 09:10:07PM +0200, SZEDER Gábor wrote:

> While at it, the first one fixes a minor bug, which allowed e.g. 'git
> log --no-min-parents-foobarbaz' to succeed.
> 
> The other two are fairly straightforward starts_with() ->
> skip_prefix() conversions.

These all look fine to me. It would be nice to address the extra cases I
mentioned on top, but even without that this is a strict improvement.

-Peff