Use skip_prefix() in handle_revision_{,pseudo_}opt()
cover Use skip_prefix() in handle_revision_{,pseudo_}opt()
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(-)
Signature · 1 line
2.13.0.420.g54001f015
PATCH 1/3 revision.c: stricter parsing of '--no-{min,max}-parents'
These two options are parsed using starts_with(), allowing things like 'git log --no-min-parents-foobarbaz' to succeed.
Use strcmp() instead.
--- revision.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
revision.c +2 -2
@@ -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 ;
Signature · 1 line
2.13.0.420.g54001f015
PATCH 2/3 revision.c: use skip_prefix() in handle_revision_opt()
In reply to SZEDER GáborInstead 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'.
--- revision.c | 54 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 26 deletions(-)
revision.c +28 -26
@@ -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 )
Signature · 1 line
2.13.0.420.g54001f015
Re: [PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()
Quoted 8 lines · SZEDER Gábor
revision.c +1
-2@@ -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 ]) {
revision.c static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg · line 1797
Quoted 10 lines
revision.c +3
-2+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()
Quoted 7 lines · Jeff King
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()
In reply to Jeff KingQuoted 16 lines · Jeff King
Quoted 14 lines · SZEDER Gábor
(quoted diff) +3
-3@@ -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:
Quoted 8 lines
Review static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg · line 1797
Quoted 4 lines
(quoted diff) +1
-1revs->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.
Quoted 18 lines
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:
Quoted 7 lines
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.
Quoted 5 lines
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(-)
Signature · 1 line
2.13.0.420.g54001f015
revision.c +17 -24
@@ -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 **argstruct 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 ));
revision.h +3 -2
@@ -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 */
PATCH v2 1/5 revision.h: turn rev_info.early_output back into an unsigned int
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.
--- revision.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
revision.h +3 -2
@@ -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 */
Signature · 1 line
2.13.0.420.g54001f015
Re: [PATCHv2 1/5] revision.h: turn rev_info.early_output back into an unsigned int
Quoted 9 lines · 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 incc243c3ce(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
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.
---
Quoted 3 lines
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(-)
revision.h +3 -2
@@ -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 */
Signature · 1 line
2.13.0.420.g54001f015
Re: [PATCHv2.1] revision.h: turn rev_info.early_output back into an unsigned int
Quoted 22 lines · 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 incc243c3ce(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 <[email protected]>---
Quoted 3 lines
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...
Quoted 6 lines
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
In reply to Jeff KingQuoted 21 lines · Jeff King
Quoted 9 lines · 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 incc243c3ce(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.
Quoted 6 lines
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
Quoted 11 lines · Junio C Hamano
Quoted 6 lines
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/[email protected]/
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
Quoted 4 lines · Jeff King
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
PATCH v2 2/5 revision.c: stricter parsing of '--no-{min,max}-parents'
In reply to SZEDER GáborThese two options are parsed using starts_with(), allowing things like 'git log --no-min-parents-foobarbaz' to succeed.
Use strcmp() instead.
--- revision.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
revision.c +2 -2
@@ -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 ;
Signature · 1 line
2.13.0.420.g54001f015
PATCH v2 3/5 revision.c: stricter parsing of '--early-output'
In reply to SZEDER GáborThe 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.
--- revision.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
revision.c +7 -10
@@ -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 ;
Signature · 1 line
2.13.0.420.g54001f015
Re: [PATCHv2 3/5] revision.c: stricter parsing of '--early-output'
Quoted 24 lines · 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 ofstarts_with()and magic numbers.Signed-off-by SZEDER Gábor <[email protected]>--- revision.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)revision.c +0
-0@@ -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 ;
revision.c static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg · line 1797
Quoted 10 lines
revision.c +0
-10- }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.
Quoted 10 lines
revision.c +7
-0+ }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 ;
PATCH v2 4/5 revision.c: use skip_prefix() in handle_revision_opt()
In reply to SZEDER GáborInstead of starts_with() and a bunch of magic numbers.
--- revision.c | 48 +++++++++++++++++++++++------------------------- 1 file changed, 23 insertions(+), 25 deletions(-)
revision.c +23 -25
@@ -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 )
Signature · 1 line
2.13.0.420.g54001f015
PATCH v2 5/5 revision.c: use skip_prefix() in handle_revision_pseudo_opt()
In reply to SZEDER GáborInstead of starts_with() and a bunch of magic numbers.
--- revision.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
revision.c +9 -9
@@ -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" );
Signature · 1 line
2.13.0.420.g54001f015
Re: [PATCH 2/3] revision.c: use skip_prefix() in handle_revision_opt()
In reply to SZEDER GáborQuoted 17 lines · SZEDER Gábor
Quoted 9 lines
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.:
Quoted 5 lines
} 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(...);}
Quoted 3 lines
'--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);
}Quoted 6 lines
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. ;)
Quoted 3 lines
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()
In reply to SZEDER GáborQuoted 17 lines · SZEDER Gábor
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 intrevision.c: stricter parsing of '--no-{min,max}-parents'revision.c: stricter parsing of '--early-output'revision.c: useskip_prefix()inhandle_revision_opt()revision.c: useskip_prefix()inhandle_revision_pseudo_opt()
Quoted 3 lines
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()
In reply to SZEDER GáborInstead of starts_with() and a bunch of magic numbers.
--- revision.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
revision.c +9 -9
@@ -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" );
Signature · 1 line
2.13.0.420.g54001f015
Quoted 5 lines · 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.
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