* Migration of builtin-blame to parse-option @ 2008-07-08 9:55 Pierre Habouzit 2008-07-08 9:55 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Pierre Habouzit 2008-07-08 10:02 ` Migration of builtin-blame to parse-option Pierre Habouzit 0 siblings, 2 replies; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 9:55 UTC (permalink / raw) To: git; +Cc: torvalds, gitster, peff This series is twofold. The first 4 patches refactor the revision parsing machine so that it's easier to deal with from a parse-opt based parser: [PATCH 1/6] revisions: refactor init_revisions and setup_revisions. 21 files changed, 70 insertions(+), 54 deletions(-) This is the patch I sent before, revisited. [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions. 2 files changed, 273 insertions(+), 308 deletions(-) This patch splits out the revisions _option_ parsing (as opposed to revisions arguments or pseudo arguments like --all, --not, ...) from the revision parser. The patch is huge, but quite straightforward. [PATCH 3/6] revisions: parse_revisions refactor. 1 files changed, 38 insertions(+), 25 deletions(-) This patch reworks parse_revisions so that it works internally as if it was a parse-opt parser. It's equivalent to the previous code, but changes are tricky, and a separate commit is really worth it. [PATCH 4/6] revisions: split handle_revision_args from parse_revisions. 2 files changed, 52 insertions(+), 46 deletions(-) This patch splits out the last bit of parse-revisions (from the previous commit) so that what parses revisions arguments (refs and pseudo arguments like --all/--not/...) can be called independently. The commit is straightforward but moves code around, hence the separate commit from patch 3 so that one isn't lost in the code moves while reviewing the tricky bits from patch 3. The second part is the git-blame migration for real: [PATCH 5/6] git-blame: migrate to incremental parse-option [1/2] 1 files changed, 118 insertions(+), 102 deletions(-) This bit is really alike the proof of concept I saw. Most of the code is stolen from Linus initial patch. It merely removes the git-blame own options and deal with them with a parse-opt parser, in an incremental way. All in all, it's a pretty straighforward patch. [PATCH 6/6] git-blame: migrate to incremental parse-option [2/2] 1 files changed, 40 insertions(+), 92 deletions(-) This is by far the trickiest patch of the series, though it passes the testsuite fine. This patch uses the function from patch 4 to process the revision arguments. We know the dashdash position from the incremental parse-opt, but we have to deal with the "old way" of passing arguments to git-blame. The new code is really less involved than before, because we deal with a filtered argv array where we only have the revisions arguments (no options anymore) and the <path>. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/6] revisions: refactor init_revisions and setup_revisions. 2008-07-08 9:55 Migration of builtin-blame to parse-option Pierre Habouzit @ 2008-07-08 9:55 ` Pierre Habouzit 2008-07-08 9:56 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Pierre Habouzit 2008-07-08 10:59 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Johannes Schindelin 2008-07-08 10:02 ` Migration of builtin-blame to parse-option Pierre Habouzit 1 sibling, 2 replies; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 9:55 UTC (permalink / raw) To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit struct rev_info gains two new field: * .def to store --default argument; * .show_merge 1-bit field. setup_revisions has been split in two: parse_revisions that does (almost) only argument parsing, to be more like what parse-options can do, and setup_revisions that does the rest. Many places had no arguments to pass to setup_revisions, and those don't use parse_revisions at all. Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- builtin-add.c | 2 +- builtin-blame.c | 3 +- builtin-commit.c | 4 +- builtin-diff-files.c | 3 +- builtin-diff-index.c | 3 +- builtin-diff-tree.c | 3 +- builtin-diff.c | 3 +- builtin-fast-export.c | 3 +- builtin-fmt-merge-msg.c | 2 +- builtin-log.c | 7 +++-- builtin-pack-objects.c | 3 +- builtin-rev-list.c | 3 +- builtin-revert.c | 2 +- builtin-shortlog.c | 3 +- bundle.c | 6 +++- http-push.c | 3 +- remote.c | 3 +- revision.c | 51 +++++++++++++++++++++++------------------------ revision.h | 7 ++++- upload-pack.c | 4 +- wt-status.c | 6 ++-- 21 files changed, 70 insertions(+), 54 deletions(-) diff --git a/builtin-add.c b/builtin-add.c index 9930cf5..8ba9604 100644 --- a/builtin-add.c +++ b/builtin-add.c @@ -121,7 +121,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags) struct update_callback_data data; struct rev_info rev; init_revisions(&rev, prefix); - setup_revisions(0, NULL, &rev, NULL); + setup_revisions(&rev, NULL); rev.prune_data = pathspec; rev.diffopt.output_format = DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = update_callback; diff --git a/builtin-blame.c b/builtin-blame.c index cf41511..1e26b88 100644 --- a/builtin-blame.c +++ b/builtin-blame.c @@ -2425,7 +2425,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix) argv[unk] = NULL; init_revisions(&revs, NULL); - setup_revisions(unk, argv, &revs, NULL); + parse_revisions(unk, argv, &revs); + setup_revisions(&revs, NULL); memset(&sb, 0, sizeof(sb)); sb.revs = &revs; diff --git a/builtin-commit.c b/builtin-commit.c index 745c11e..37b4e30 100644 --- a/builtin-commit.c +++ b/builtin-commit.c @@ -605,7 +605,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix) else { init_revisions(&rev, ""); rev.abbrev = 0; - setup_revisions(0, NULL, &rev, parent); + setup_revisions(&rev, parent); DIFF_OPT_SET(&rev.diffopt, QUIET); DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS); run_diff_index(&rev, 1 /* cached */); @@ -850,7 +850,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1) die("could not parse newly created commit"); init_revisions(&rev, prefix); - setup_revisions(0, NULL, &rev, NULL); + setup_revisions(&rev, NULL); rev.abbrev = 0; rev.diff = 1; diff --git a/builtin-diff-files.c b/builtin-diff-files.c index 384d871..e7e202a 100644 --- a/builtin-diff-files.c +++ b/builtin-diff-files.c @@ -23,7 +23,8 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix) git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; - argc = setup_revisions(argc, argv, &rev, NULL); + argc = parse_revisions(argc, argv, &rev); + setup_revisions(&rev, NULL); while (1 < argc && argv[1][0] == '-') { if (!strcmp(argv[1], "--base")) rev.max_count = 1; diff --git a/builtin-diff-index.c b/builtin-diff-index.c index 2f44ebf..286a9de 100644 --- a/builtin-diff-index.c +++ b/builtin-diff-index.c @@ -20,7 +20,8 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix) git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ rev.abbrev = 0; - argc = setup_revisions(argc, argv, &rev, NULL); + argc = parse_revisions(argc, argv, &rev); + setup_revisions(&rev, NULL); for (i = 1; i < argc; i++) { const char *arg = argv[i]; diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c index 9d2a48f..0900175 100644 --- a/builtin-diff-tree.c +++ b/builtin-diff-tree.c @@ -72,7 +72,8 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix) nr_sha1 = 0; opt->abbrev = 0; opt->diff = 1; - argc = setup_revisions(argc, argv, opt, NULL); + argc = parse_revisions(argc, argv, opt); + setup_revisions(opt, NULL); while (--argc > 0) { const char *arg = *++argv; diff --git a/builtin-diff.c b/builtin-diff.c index 4c289e7..a92dbd4 100644 --- a/builtin-diff.c +++ b/builtin-diff.c @@ -283,7 +283,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix) if (nongit) die("Not a git repository"); - argc = setup_revisions(argc, argv, &rev, NULL); + argc = parse_revisions(argc, argv, &rev); + setup_revisions(&rev, NULL); if (!rev.diffopt.output_format) { rev.diffopt.output_format = DIFF_FORMAT_PATCH; if (diff_setup_done(&rev.diffopt) < 0) diff --git a/builtin-fast-export.c b/builtin-fast-export.c index 75132ba..d6726a8 100644 --- a/builtin-fast-export.c +++ b/builtin-fast-export.c @@ -454,7 +454,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix) git_config(git_default_config, NULL); init_revisions(&revs, prefix); - argc = setup_revisions(argc, argv, &revs, NULL); + argc = parse_revisions(argc, argv, &revs); + setup_revisions(&revs, NULL); argc = parse_options(argc, argv, options, fast_export_usage, 0); if (argc > 1) usage_with_options (fast_export_usage, options); diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c index 69a8a92..3db10c4 100644 --- a/builtin-fmt-merge-msg.c +++ b/builtin-fmt-merge-msg.c @@ -183,7 +183,7 @@ static void shortlog(const char *name, unsigned char *sha1, if (!branch || branch->type != OBJ_COMMIT) return; - setup_revisions(0, NULL, rev, NULL); + setup_revisions(rev, NULL); rev->ignore_merges = 1; add_pending_object(rev, branch, name); add_pending_object(rev, &head->object, "^HEAD"); diff --git a/builtin-log.c b/builtin-log.c index 430d876..8be9a28 100644 --- a/builtin-log.c +++ b/builtin-log.c @@ -68,8 +68,8 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix, if (default_date_mode) rev->date_mode = parse_date_format(default_date_mode); - argc = setup_revisions(argc, argv, rev, "HEAD"); - + argc = parse_revisions(argc, argv, rev); + setup_revisions(rev, "HEAD"); if (rev->diffopt.pickaxe || rev->diffopt.filter) rev->always_show_header = 0; if (DIFF_OPT_TST(&rev->diffopt, FOLLOW_RENAMES)) { @@ -919,7 +919,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (numbered_files && use_stdout) die ("--numbered-files and --stdout are mutually exclusive."); - argc = setup_revisions(argc, argv, &rev, "HEAD"); + argc = parse_revisions(argc, argv, &rev); + setup_revisions(&rev, "HEAD"); if (argc > 1) die ("unrecognized argument: %s", argv[1]); diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 2dadec1..3d43050 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -1964,7 +1964,8 @@ static void get_object_list(int ac, const char **av) init_revisions(&revs, NULL); save_commit_buffer = 0; - setup_revisions(ac, av, &revs, NULL); + parse_revisions(ac, av, &revs); + setup_revisions(&revs, NULL); while (fgets(line, sizeof(line), stdin) != NULL) { int len = strlen(line); diff --git a/builtin-rev-list.c b/builtin-rev-list.c index b4a2c44..a9f5e41 100644 --- a/builtin-rev-list.c +++ b/builtin-rev-list.c @@ -588,7 +588,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) init_revisions(&revs, prefix); revs.abbrev = 0; revs.commit_format = CMIT_FMT_UNSPECIFIED; - argc = setup_revisions(argc, argv, &revs, NULL); + argc = parse_revisions(argc, argv, &revs); + setup_revisions(&revs, NULL); for (i = 1 ; i < argc; i++) { const char *arg = argv[i]; diff --git a/builtin-revert.c b/builtin-revert.c index 0270f9b..0adff14 100644 --- a/builtin-revert.c +++ b/builtin-revert.c @@ -253,7 +253,7 @@ static int index_is_dirty(void) { struct rev_info rev; init_revisions(&rev, NULL); - setup_revisions(0, NULL, &rev, "HEAD"); + setup_revisions(&rev, "HEAD"); DIFF_OPT_SET(&rev.diffopt, QUIET); DIFF_OPT_SET(&rev.diffopt, EXIT_WITH_STATUS); run_diff_index(&rev, 1); diff --git a/builtin-shortlog.c b/builtin-shortlog.c index e6a2865..3428bf6 100644 --- a/builtin-shortlog.c +++ b/builtin-shortlog.c @@ -256,7 +256,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) argc--; } init_revisions(&rev, prefix); - argc = setup_revisions(argc, argv, &rev, NULL); + argc = parse_revisions(argc, argv, &rev); + setup_revisions(&rev, NULL); if (argc > 1) die ("unrecognized argument: %s", argv[1]); diff --git a/bundle.c b/bundle.c index 0ba5df1..c4848d7 100644 --- a/bundle.c +++ b/bundle.c @@ -120,7 +120,8 @@ int verify_bundle(struct bundle_header *header, int verbose) if (revs.pending.nr != p->nr) return ret; req_nr = revs.pending.nr; - setup_revisions(2, argv, &revs, NULL); + parse_revisions(2, argv, &revs); + setup_revisions(&revs, NULL); memset(&refs, 0, sizeof(struct object_array)); for (i = 0; i < revs.pending.nr; i++) { @@ -226,7 +227,8 @@ int create_bundle(struct bundle_header *header, const char *path, return error("rev-list died"); /* write references */ - argc = setup_revisions(argc, argv, &revs, NULL); + argc = parse_revisions(argc, argv, &revs); + setup_revisions(&revs, NULL); if (argc > 1) return error("unrecognized argument: %s'", argv[1]); diff --git a/http-push.c b/http-push.c index 2cd068a..aa348aa 100644 --- a/http-push.c +++ b/http-push.c @@ -2383,7 +2383,8 @@ int main(int argc, char **argv) commit_argc++; } init_revisions(&revs, setup_git_directory()); - setup_revisions(commit_argc, commit_argv, &revs, NULL); + parse_revisions(commit_argc, commit_argv, &revs); + setup_revisions(&revs, NULL); revs.edge_hint = 0; /* just in case */ free(new_sha1_hex); if (old_sha1_hex) { diff --git a/remote.c b/remote.c index df8bd72..c95a510 100644 --- a/remote.c +++ b/remote.c @@ -1280,7 +1280,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs) strcpy(symmetric + 43, sha1_to_hex(theirs->object.sha1)); init_revisions(&revs, NULL); - setup_revisions(rev_argc, rev_argv, &revs, NULL); + parse_revisions(rev_argc, rev_argv, &revs); + setup_revisions(&revs, NULL); prepare_revision_walk(&revs); /* ... and count the commits on each side. */ diff --git a/revision.c b/revision.c index 0191160..3a46ed4 100644 --- a/revision.c +++ b/revision.c @@ -981,14 +981,11 @@ static void add_ignore_packed(struct rev_info *revs, const char *name) * Returns the number of arguments left that weren't recognized * (which are also moved to the head of the argument list) */ -int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def) +int parse_revisions(int argc, const char **argv, struct rev_info *revs) { - int i, flags, seen_dashdash, show_merge; + int i, flags, seen_dashdash; const char **unrecognized = argv + 1; int left = 1; - int all_match = 0; - int regflags = 0; - int fixed = 0; /* First, search for "--" */ seen_dashdash = 0; @@ -1004,7 +1001,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch break; } - flags = show_merge = 0; + flags = 0; for (i = 1; i < argc; i++) { const char *arg = argv[i]; if (*arg == '-') { @@ -1092,11 +1089,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch if (!strcmp(arg, "--default")) { if (++i >= argc) die("bad --default argument"); - def = argv[i]; + revs->def = argv[i]; continue; } if (!strcmp(arg, "--merge")) { - show_merge = 1; + revs->show_merge = 1; continue; } if (!strcmp(arg, "--topo-order")) { @@ -1302,21 +1299,25 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch } if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) { - regflags |= REG_EXTENDED; + if (revs->grep_filter) + revs->grep_filter->regflags |= REG_EXTENDED; continue; } if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) { - regflags |= REG_ICASE; + if (revs->grep_filter) + revs->grep_filter->regflags |= REG_ICASE; continue; } if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) { - fixed = 1; + if (revs->grep_filter) + revs->grep_filter->fixed = 1; continue; } if (!strcmp(arg, "--all-match")) { - all_match = 1; + if (revs->grep_filter) + revs->grep_filter->all_match = 1; continue; } if (!prefixcmp(arg, "--encoding=")) { @@ -1374,22 +1375,23 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch break; } } + return left; +} - if (revs->grep_filter) { - revs->grep_filter->regflags |= regflags; - revs->grep_filter->fixed = fixed; - } - - if (show_merge) +void setup_revisions(struct rev_info *revs, const char *def) +{ + if (revs->def == NULL) + revs->def = def; + if (revs->show_merge) prepare_show_merge(revs); - if (def && !revs->pending.nr) { + if (revs->def && !revs->pending.nr) { unsigned char sha1[20]; struct object *object; unsigned mode; - if (get_sha1_with_mode(def, sha1, &mode)) - die("bad default revision '%s'", def); - object = get_reference(revs, def, sha1, 0); - add_pending_object_with_mode(revs, object, def, mode); + if (get_sha1_with_mode(revs->def, sha1, &mode)) + die("bad default revision '%s'", revs->def); + object = get_reference(revs, revs->def, sha1, 0); + add_pending_object_with_mode(revs, object, revs->def, mode); } /* Did the user ask for any diff output? Run the diff! */ @@ -1423,7 +1425,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch die("diff_setup_done failed"); if (revs->grep_filter) { - revs->grep_filter->all_match = all_match; compile_grep_patterns(revs->grep_filter); } @@ -1440,8 +1441,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch if (revs->reflog_info && revs->graph) die("cannot combine --walk-reflogs with --graph"); - - return left; } static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) diff --git a/revision.h b/revision.h index 5b8c56b..e45b86e 100644 --- a/revision.h +++ b/revision.h @@ -26,6 +26,7 @@ struct rev_info { /* Basic information */ const char *prefix; + const char *def; void *prune_data; unsigned int early_output; @@ -66,8 +67,9 @@ struct rev_info { /* Format info */ unsigned int shown_one:1, - abbrev_commit:1, + show_merge:1, use_terminator:1, + abbrev_commit:1, missing_newline:1; enum date_mode date_mode; @@ -119,7 +121,8 @@ typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *); volatile show_early_output_fn_t show_early_output; extern void init_revisions(struct rev_info *revs, const char *prefix); -extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def); +extern int parse_revisions(int argc, const char **argv, struct rev_info *revs); +extern void setup_revisions(struct rev_info *revs, const char *def); extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename); extern int prepare_revision_walk(struct rev_info *revs); diff --git a/upload-pack.c b/upload-pack.c index 9f82941..a87a9a3 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -116,7 +116,7 @@ static int do_rev_list(int fd, void *create_full_pack) if (create_full_pack) { const char *args[] = {"rev-list", "--all", NULL}; - setup_revisions(2, args, &revs, NULL); + parse_revisions(2, args, &revs); } else { for (i = 0; i < want_obj.nr; i++) { struct object *o = want_obj.objects[i].item; @@ -129,8 +129,8 @@ static int do_rev_list(int fd, void *create_full_pack) o->flags |= UNINTERESTING; add_pending_object(&revs, o, NULL); } - setup_revisions(0, NULL, &revs, NULL); } + setup_revisions(&revs, NULL); if (prepare_revision_walk(&revs)) die("revision walk setup failed"); mark_edges_uninteresting(revs.commits, &revs, show_edge); diff --git a/wt-status.c b/wt-status.c index 889e50f..df1130c 100644 --- a/wt-status.c +++ b/wt-status.c @@ -206,7 +206,7 @@ static void wt_status_print_updated(struct wt_status *s) { struct rev_info rev; init_revisions(&rev, NULL); - setup_revisions(0, NULL, &rev, s->reference); + setup_revisions(&rev, s->reference); rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = wt_status_print_updated_cb; rev.diffopt.format_callback_data = s; @@ -220,7 +220,7 @@ static void wt_status_print_changed(struct wt_status *s) { struct rev_info rev; init_revisions(&rev, ""); - setup_revisions(0, NULL, &rev, NULL); + setup_revisions(&rev, NULL); rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK; rev.diffopt.format_callback = wt_status_print_changed_cb; rev.diffopt.format_callback_data = s; @@ -308,7 +308,7 @@ static void wt_status_print_verbose(struct wt_status *s) struct rev_info rev; init_revisions(&rev, NULL); - setup_revisions(0, NULL, &rev, s->reference); + setup_revisions(&rev, s->reference); rev.diffopt.output_format |= DIFF_FORMAT_PATCH; rev.diffopt.detect_rename = 1; rev.diffopt.file = s->fp; -- 1.5.6.2.352.g416a6 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions. 2008-07-08 9:55 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Pierre Habouzit @ 2008-07-08 9:56 ` Pierre Habouzit 2008-07-08 9:56 ` [PATCH 3/6] revisions: parse_revisions refactor Pierre Habouzit 2008-07-08 10:51 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Johannes Sixt 2008-07-08 10:59 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Johannes Schindelin 1 sibling, 2 replies; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 9:56 UTC (permalink / raw) To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- revision.c | 579 ++++++++++++++++++++++++++++-------------------------------- revision.h | 2 + 2 files changed, 273 insertions(+), 308 deletions(-) diff --git a/revision.c b/revision.c index 3a46ed4..a3792c9 100644 --- a/revision.c +++ b/revision.c @@ -974,6 +974,273 @@ static void add_ignore_packed(struct rev_info *revs, const char *name) revs->ignore_packed[num] = NULL; } +int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, + int *unkc, const char **unkv) +{ + const char *arg = argv[0]; + + /* pseudo revision arguments */ + if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") || + !strcmp(arg, "--tags") || !strcmp(arg, "--remotes") || + !strcmp(arg, "--reflog") || !strcmp(arg, "--not") || + !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) + { + unkv[*unkc++] = arg; + return 0; + } + + if (!prefixcmp(arg, "--max-count=")) { + revs->max_count = atoi(arg + 12); + } + else if (!prefixcmp(arg, "--skip=")) { + revs->skip_count = atoi(arg + 7); + } + /* accept -<digit>, like traditional "head" */ + else if ((*arg == '-') && isdigit(arg[1])) { + revs->max_count = atoi(arg + 1); + } + else if (!strcmp(arg, "-n")) { + if (argc <= 1) + return error("-n requires an argument"); + revs->max_count = atoi(argv[1]); + return 2; + } + else if (!prefixcmp(arg, "-n")) { + revs->max_count = atoi(arg + 2); + } + else if (!prefixcmp(arg, "--max-age=")) { + revs->max_age = atoi(arg + 10); + } + else if (!prefixcmp(arg, "--since=")) { + revs->max_age = approxidate(arg + 8); + } + else if (!prefixcmp(arg, "--after=")) { + revs->max_age = approxidate(arg + 8); + } + else if (!prefixcmp(arg, "--min-age=")) { + revs->min_age = atoi(arg + 10); + } + else if (!prefixcmp(arg, "--before=")) { + revs->min_age = approxidate(arg + 9); + } + else if (!prefixcmp(arg, "--until=")) { + revs->min_age = approxidate(arg + 8); + } + else if (!strcmp(arg, "--first-parent")) { + revs->first_parent_only = 1; + } + else if (!strcmp(arg, "-g") || + !strcmp(arg, "--walk-reflogs")) { + init_reflog_walk(&revs->reflog_info); + } + else if (!strcmp(arg, "--default")) { + if (argc <= 1) + return error("bad --default argument"); + revs->def = argv[1]; + return 2; + } + else if (!strcmp(arg, "--merge")) { + revs->show_merge = 1; + } + else if (!strcmp(arg, "--topo-order")) { + revs->lifo = 1; + revs->topo_order = 1; + } + else if (!strcmp(arg, "--date-order")) { + revs->lifo = 0; + revs->topo_order = 1; + } + else if (!prefixcmp(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, "--parents")) { + revs->rewrite_parents = 1; + revs->print_parents = 1; + } + else if (!strcmp(arg, "--dense")) { + revs->dense = 1; + } + else if (!strcmp(arg, "--sparse")) { + revs->dense = 0; + } + else if (!strcmp(arg, "--show-all")) { + revs->show_all = 1; + } + else if (!strcmp(arg, "--remove-empty")) { + revs->remove_empty_trees = 1; + } + else if (!strcmp(arg, "--no-merges")) { + revs->no_merges = 1; + } + else if (!strcmp(arg, "--boundary")) { + revs->boundary = 1; + } + else if (!strcmp(arg, "--left-right")) { + revs->left_right = 1; + } + else if (!strcmp(arg, "--cherry-pick")) { + revs->cherry_pick = 1; + revs->limited = 1; + } + else if (!strcmp(arg, "--objects")) { + revs->tag_objects = 1; + revs->tree_objects = 1; + revs->blob_objects = 1; + } + else if (!strcmp(arg, "--objects-edge")) { + revs->tag_objects = 1; + revs->tree_objects = 1; + revs->blob_objects = 1; + revs->edge_hint = 1; + } + else if (!strcmp(arg, "--unpacked")) { + revs->unpacked = 1; + free(revs->ignore_packed); + revs->ignore_packed = NULL; + revs->num_ignore_packed = 0; + } + else if (!prefixcmp(arg, "--unpacked=")) { + revs->unpacked = 1; + add_ignore_packed(revs, arg+11); + } + else if (!strcmp(arg, "-r")) { + revs->diff = 1; + DIFF_OPT_SET(&revs->diffopt, RECURSIVE); + } + else if (!strcmp(arg, "-t")) { + revs->diff = 1; + DIFF_OPT_SET(&revs->diffopt, RECURSIVE); + DIFF_OPT_SET(&revs->diffopt, TREE_IN_RECURSIVE); + } + else if (!strcmp(arg, "-m")) { + revs->ignore_merges = 0; + } + else if (!strcmp(arg, "-c")) { + revs->diff = 1; + revs->dense_combined_merges = 0; + revs->combine_merges = 1; + } + else if (!strcmp(arg, "--cc")) { + revs->diff = 1; + revs->dense_combined_merges = 1; + revs->combine_merges = 1; + } + else if (!strcmp(arg, "-v")) { + revs->verbose_header = 1; + } + else if (!strcmp(arg, "--pretty")) { + revs->verbose_header = 1; + get_commit_format(arg+8, revs); + } + else if (!prefixcmp(arg, "--pretty=")) { + revs->verbose_header = 1; + get_commit_format(arg+9, revs); + } + else if (!strcmp(arg, "--graph")) { + revs->topo_order = 1; + revs->rewrite_parents = 1; + revs->graph = graph_init(revs); + } + else if (!strcmp(arg, "--root")) { + revs->show_root_diff = 1; + } + else if (!strcmp(arg, "--no-commit-id")) { + revs->no_commit_id = 1; + } + else if (!strcmp(arg, "--always")) { + revs->always_show_header = 1; + } + else if (!strcmp(arg, "--no-abbrev")) { + revs->abbrev = 0; + } + else if (!strcmp(arg, "--abbrev")) { + revs->abbrev = DEFAULT_ABBREV; + } + else if (!prefixcmp(arg, "--abbrev=")) { + revs->abbrev = strtoul(arg + 9, NULL, 10); + if (revs->abbrev < MINIMUM_ABBREV) + revs->abbrev = MINIMUM_ABBREV; + else if (revs->abbrev > 40) + revs->abbrev = 40; + } + else if (!strcmp(arg, "--abbrev-commit")) { + revs->abbrev_commit = 1; + } + else if (!strcmp(arg, "--full-diff")) { + revs->diff = 1; + revs->full_diff = 1; + } + else if (!strcmp(arg, "--full-history")) { + revs->simplify_history = 0; + } + else if (!strcmp(arg, "--relative-date")) { + revs->date_mode = DATE_RELATIVE; + } + else if (!strncmp(arg, "--date=", 7)) { + revs->date_mode = parse_date_format(arg + 7); + } + else if (!strcmp(arg, "--log-size")) { + revs->show_log_size = 1; + } + /* + * Grepping the commit log + */ + else if (!prefixcmp(arg, "--author=")) { + add_header_grep(revs, "author", arg+9); + } + else if (!prefixcmp(arg, "--committer=")) { + add_header_grep(revs, "committer", arg+12); + } + else if (!prefixcmp(arg, "--grep=")) { + add_message_grep(revs, arg+7); + } + else if (!strcmp(arg, "--extended-regexp") || + !strcmp(arg, "-E")) { + if (revs->grep_filter) + revs->grep_filter->regflags |= REG_EXTENDED; + } + else if (!strcmp(arg, "--regexp-ignore-case") || + !strcmp(arg, "-i")) { + if (revs->grep_filter) + revs->grep_filter->regflags |= REG_ICASE; + } + else if (!strcmp(arg, "--fixed-strings") || + !strcmp(arg, "-F")) { + if (revs->grep_filter) + revs->grep_filter->fixed = 1; + } + else if (!strcmp(arg, "--all-match")) { + if (revs->grep_filter) + revs->grep_filter->all_match = 1; + } + else if (!prefixcmp(arg, "--encoding=")) { + arg += 11; + if (strcmp(arg, "none")) + git_log_output_encoding = xstrdup(arg); + else + git_log_output_encoding = ""; + } + else if (!strcmp(arg, "--reverse")) { + revs->reverse ^= 1; + } + else if (!strcmp(arg, "--children")) { + revs->children.name = "children"; + revs->limited = 1; + } + else + return diff_opt_parse(&revs->diffopt, argv, argc); + + return 1; +} + /* * Parse revision information, filling in the "rev_info" structure, * and removing the used arguments from the argument list. @@ -1006,53 +1273,7 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) const char *arg = argv[i]; if (*arg == '-') { int opts; - if (!prefixcmp(arg, "--max-count=")) { - revs->max_count = atoi(arg + 12); - continue; - } - if (!prefixcmp(arg, "--skip=")) { - revs->skip_count = atoi(arg + 7); - continue; - } - /* accept -<digit>, like traditional "head" */ - if ((*arg == '-') && isdigit(arg[1])) { - revs->max_count = atoi(arg + 1); - continue; - } - if (!strcmp(arg, "-n")) { - if (argc <= i + 1) - die("-n requires an argument"); - revs->max_count = atoi(argv[++i]); - continue; - } - if (!prefixcmp(arg, "-n")) { - revs->max_count = atoi(arg + 2); - continue; - } - if (!prefixcmp(arg, "--max-age=")) { - revs->max_age = atoi(arg + 10); - continue; - } - if (!prefixcmp(arg, "--since=")) { - revs->max_age = approxidate(arg + 8); - continue; - } - if (!prefixcmp(arg, "--after=")) { - revs->max_age = approxidate(arg + 8); - continue; - } - if (!prefixcmp(arg, "--min-age=")) { - revs->min_age = atoi(arg + 10); - continue; - } - if (!prefixcmp(arg, "--before=")) { - revs->min_age = approxidate(arg + 9); - continue; - } - if (!prefixcmp(arg, "--until=")) { - revs->min_age = approxidate(arg + 8); - continue; - } + if (!strcmp(arg, "--all")) { handle_refs(revs, flags, for_each_ref); continue; @@ -1069,269 +1290,14 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) handle_refs(revs, flags, for_each_remote_ref); continue; } - if (!strcmp(arg, "--first-parent")) { - revs->first_parent_only = 1; - continue; - } if (!strcmp(arg, "--reflog")) { handle_reflog(revs, flags); continue; } - if (!strcmp(arg, "-g") || - !strcmp(arg, "--walk-reflogs")) { - init_reflog_walk(&revs->reflog_info); - continue; - } if (!strcmp(arg, "--not")) { flags ^= UNINTERESTING; continue; } - if (!strcmp(arg, "--default")) { - if (++i >= argc) - die("bad --default argument"); - revs->def = argv[i]; - continue; - } - if (!strcmp(arg, "--merge")) { - revs->show_merge = 1; - continue; - } - if (!strcmp(arg, "--topo-order")) { - revs->lifo = 1; - revs->topo_order = 1; - continue; - } - if (!strcmp(arg, "--date-order")) { - revs->lifo = 0; - revs->topo_order = 1; - continue; - } - if (!prefixcmp(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; - continue; - } - } - if (!strcmp(arg, "--parents")) { - revs->rewrite_parents = 1; - revs->print_parents = 1; - continue; - } - if (!strcmp(arg, "--dense")) { - revs->dense = 1; - continue; - } - if (!strcmp(arg, "--sparse")) { - revs->dense = 0; - continue; - } - if (!strcmp(arg, "--show-all")) { - revs->show_all = 1; - continue; - } - if (!strcmp(arg, "--remove-empty")) { - revs->remove_empty_trees = 1; - continue; - } - if (!strcmp(arg, "--no-merges")) { - revs->no_merges = 1; - continue; - } - if (!strcmp(arg, "--boundary")) { - revs->boundary = 1; - continue; - } - if (!strcmp(arg, "--left-right")) { - revs->left_right = 1; - continue; - } - if (!strcmp(arg, "--cherry-pick")) { - revs->cherry_pick = 1; - revs->limited = 1; - continue; - } - if (!strcmp(arg, "--objects")) { - revs->tag_objects = 1; - revs->tree_objects = 1; - revs->blob_objects = 1; - continue; - } - if (!strcmp(arg, "--objects-edge")) { - revs->tag_objects = 1; - revs->tree_objects = 1; - revs->blob_objects = 1; - revs->edge_hint = 1; - continue; - } - if (!strcmp(arg, "--unpacked")) { - revs->unpacked = 1; - free(revs->ignore_packed); - revs->ignore_packed = NULL; - revs->num_ignore_packed = 0; - continue; - } - if (!prefixcmp(arg, "--unpacked=")) { - revs->unpacked = 1; - add_ignore_packed(revs, arg+11); - continue; - } - if (!strcmp(arg, "-r")) { - revs->diff = 1; - DIFF_OPT_SET(&revs->diffopt, RECURSIVE); - continue; - } - if (!strcmp(arg, "-t")) { - revs->diff = 1; - DIFF_OPT_SET(&revs->diffopt, RECURSIVE); - DIFF_OPT_SET(&revs->diffopt, TREE_IN_RECURSIVE); - continue; - } - if (!strcmp(arg, "-m")) { - revs->ignore_merges = 0; - continue; - } - if (!strcmp(arg, "-c")) { - revs->diff = 1; - revs->dense_combined_merges = 0; - revs->combine_merges = 1; - continue; - } - if (!strcmp(arg, "--cc")) { - revs->diff = 1; - revs->dense_combined_merges = 1; - revs->combine_merges = 1; - continue; - } - if (!strcmp(arg, "-v")) { - revs->verbose_header = 1; - continue; - } - if (!strcmp(arg, "--pretty")) { - revs->verbose_header = 1; - get_commit_format(arg+8, revs); - continue; - } - if (!prefixcmp(arg, "--pretty=")) { - revs->verbose_header = 1; - get_commit_format(arg+9, revs); - continue; - } - if (!strcmp(arg, "--graph")) { - revs->topo_order = 1; - revs->rewrite_parents = 1; - revs->graph = graph_init(revs); - continue; - } - if (!strcmp(arg, "--root")) { - revs->show_root_diff = 1; - continue; - } - if (!strcmp(arg, "--no-commit-id")) { - revs->no_commit_id = 1; - continue; - } - if (!strcmp(arg, "--always")) { - revs->always_show_header = 1; - continue; - } - if (!strcmp(arg, "--no-abbrev")) { - revs->abbrev = 0; - continue; - } - if (!strcmp(arg, "--abbrev")) { - revs->abbrev = DEFAULT_ABBREV; - continue; - } - if (!prefixcmp(arg, "--abbrev=")) { - revs->abbrev = strtoul(arg + 9, NULL, 10); - if (revs->abbrev < MINIMUM_ABBREV) - revs->abbrev = MINIMUM_ABBREV; - else if (revs->abbrev > 40) - revs->abbrev = 40; - continue; - } - if (!strcmp(arg, "--abbrev-commit")) { - revs->abbrev_commit = 1; - continue; - } - if (!strcmp(arg, "--full-diff")) { - revs->diff = 1; - revs->full_diff = 1; - continue; - } - if (!strcmp(arg, "--full-history")) { - revs->simplify_history = 0; - continue; - } - if (!strcmp(arg, "--relative-date")) { - revs->date_mode = DATE_RELATIVE; - continue; - } - if (!strncmp(arg, "--date=", 7)) { - revs->date_mode = parse_date_format(arg + 7); - continue; - } - if (!strcmp(arg, "--log-size")) { - revs->show_log_size = 1; - continue; - } - - /* - * Grepping the commit log - */ - if (!prefixcmp(arg, "--author=")) { - add_header_grep(revs, "author", arg+9); - continue; - } - if (!prefixcmp(arg, "--committer=")) { - add_header_grep(revs, "committer", arg+12); - continue; - } - if (!prefixcmp(arg, "--grep=")) { - add_message_grep(revs, arg+7); - continue; - } - if (!strcmp(arg, "--extended-regexp") || - !strcmp(arg, "-E")) { - if (revs->grep_filter) - revs->grep_filter->regflags |= REG_EXTENDED; - continue; - } - if (!strcmp(arg, "--regexp-ignore-case") || - !strcmp(arg, "-i")) { - if (revs->grep_filter) - revs->grep_filter->regflags |= REG_ICASE; - continue; - } - if (!strcmp(arg, "--fixed-strings") || - !strcmp(arg, "-F")) { - if (revs->grep_filter) - revs->grep_filter->fixed = 1; - continue; - } - if (!strcmp(arg, "--all-match")) { - if (revs->grep_filter) - revs->grep_filter->all_match = 1; - continue; - } - if (!prefixcmp(arg, "--encoding=")) { - arg += 11; - if (strcmp(arg, "none")) - git_log_output_encoding = xstrdup(arg); - else - git_log_output_encoding = ""; - continue; - } - if (!strcmp(arg, "--reverse")) { - revs->reverse ^= 1; - continue; - } if (!strcmp(arg, "--no-walk")) { revs->no_walk = 1; continue; @@ -1340,17 +1306,14 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) revs->no_walk = 0; continue; } - if (!strcmp(arg, "--children")) { - revs->children.name = "children"; - revs->limited = 1; - continue; - } - opts = diff_opt_parse(&revs->diffopt, argv+i, argc-i); + opts = handle_revision_opt(revs, argc - i, argv + i, NULL, NULL); if (opts > 0) { i += opts - 1; continue; } + if (opts < 0) + exit(128); *unrecognized++ = arg; left++; continue; diff --git a/revision.h b/revision.h index e45b86e..46ab713 100644 --- a/revision.h +++ b/revision.h @@ -123,6 +123,8 @@ volatile show_early_output_fn_t show_early_output; extern void init_revisions(struct rev_info *revs, const char *prefix); extern int parse_revisions(int argc, const char **argv, struct rev_info *revs); extern void setup_revisions(struct rev_info *revs, const char *def); +extern int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, + int *unkc, const char **unkv); extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename); extern int prepare_revision_walk(struct rev_info *revs); -- 1.5.6.2.352.g416a6 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/6] revisions: parse_revisions refactor. 2008-07-08 9:56 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Pierre Habouzit @ 2008-07-08 9:56 ` Pierre Habouzit 2008-07-08 9:56 ` [PATCH 4/6] revisions: split handle_revision_args from parse_revisions Pierre Habouzit 2008-07-08 10:51 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Johannes Sixt 1 sibling, 1 reply; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 9:56 UTC (permalink / raw) To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit Reorder parse_revisions so that it works more like a parse-opt based parser would: use two pass, the first filtering out options that the revision parser understands, and remember if we saw a "--", and then parse revision arguments. Once everyone is using a parse-opt parser, it will be possible to simplify parse_revisions further. The first pass is a NOOP for incremental parse-opt based parsers. Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- revision.c | 63 ++++++++++++++++++++++++++++++++++++----------------------- 1 files changed, 38 insertions(+), 25 deletions(-) diff --git a/revision.c b/revision.c index a3792c9..87f89a2 100644 --- a/revision.c +++ b/revision.c @@ -1250,30 +1250,44 @@ int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, */ int parse_revisions(int argc, const char **argv, struct rev_info *revs) { - int i, flags, seen_dashdash; - const char **unrecognized = argv + 1; - int left = 1; + int i, left, flags; + int seen_dashdash = 0; - /* First, search for "--" */ - seen_dashdash = 0; - for (i = 1; i < argc; i++) { + /* First, filter out revision options and look for "--" */ + for (left = i = 1; i < argc; i++) { const char *arg = argv[i]; - if (strcmp(arg, "--")) + int opts; + + if (arg[0] != '-') { + argv[left++] = arg; continue; - argv[i] = NULL; - argc = i; - if (argv[i + 1]) - revs->prune_data = get_pathspec(revs->prefix, argv + i + 1); - seen_dashdash = 1; - break; + } + + if (!strcmp(arg, "--")) { + seen_dashdash = left; + memcpy(argv + left, argv + i, sizeof(*argv) * (argc - i)); + left += argc - i; + break; + } + + opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv); + if (opts > 0) { + i += opts - 1; + continue; + } + if (opts < 0) + exit(128); + argv[left++] = arg; } + argv[left] = NULL; + argc = left; + /* Second, parse revisions arguments */ flags = 0; - for (i = 1; i < argc; i++) { + for (left = i = 1; i < argc; i++) { const char *arg = argv[i]; - if (*arg == '-') { - int opts; + if (*arg == '-') { if (!strcmp(arg, "--all")) { handle_refs(revs, flags, for_each_ref); continue; @@ -1306,16 +1320,15 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) revs->no_walk = 0; continue; } - - opts = handle_revision_opt(revs, argc - i, argv + i, NULL, NULL); - if (opts > 0) { - i += opts - 1; - continue; + if (i == seen_dashdash) { + argv[i] = NULL; + argc = i; + if (argv[i + 1]) + revs->prune_data = get_pathspec(revs->prefix, argv + i + 1); + break; } - if (opts < 0) - exit(128); - *unrecognized++ = arg; - left++; + + argv[left++] = arg; continue; } -- 1.5.6.2.352.g416a6 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 4/6] revisions: split handle_revision_args from parse_revisions. 2008-07-08 9:56 ` [PATCH 3/6] revisions: parse_revisions refactor Pierre Habouzit @ 2008-07-08 9:56 ` Pierre Habouzit 2008-07-08 9:56 ` [PATCH 5/6] git-blame: migrate to incremental parse-option [1/2] Pierre Habouzit 0 siblings, 1 reply; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 9:56 UTC (permalink / raw) To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit This new function is meant to only parse non option revision arguments in a row. IOW it's meant to parse what remains from a parse-opt filtering of the argument list, with the knowledge of the "--" position (0 means none). Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- revision.c | 97 +++++++++++++++++++++++++++++++---------------------------- revision.h | 1 + 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/revision.c b/revision.c index 87f89a2..c7de30f 100644 --- a/revision.c +++ b/revision.c @@ -1241,49 +1241,11 @@ int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, return 1; } -/* - * Parse revision information, filling in the "rev_info" structure, - * and removing the used arguments from the argument list. - * - * Returns the number of arguments left that weren't recognized - * (which are also moved to the head of the argument list) - */ -int parse_revisions(int argc, const char **argv, struct rev_info *revs) +int handle_revision_args(struct rev_info *revs, + int argc, const char **argv, int dashdash_pos) { - int i, left, flags; - int seen_dashdash = 0; - - /* First, filter out revision options and look for "--" */ - for (left = i = 1; i < argc; i++) { - const char *arg = argv[i]; - int opts; - - if (arg[0] != '-') { - argv[left++] = arg; - continue; - } - - if (!strcmp(arg, "--")) { - seen_dashdash = left; - memcpy(argv + left, argv + i, sizeof(*argv) * (argc - i)); - left += argc - i; - break; - } + int i, left, flags = 0; - opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv); - if (opts > 0) { - i += opts - 1; - continue; - } - if (opts < 0) - exit(128); - argv[left++] = arg; - } - argv[left] = NULL; - argc = left; - - /* Second, parse revisions arguments */ - flags = 0; for (left = i = 1; i < argc; i++) { const char *arg = argv[i]; @@ -1320,7 +1282,7 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) revs->no_walk = 0; continue; } - if (i == seen_dashdash) { + if (i == dashdash_pos) { argv[i] = NULL; argc = i; if (argv[i + 1]) @@ -1332,10 +1294,10 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) continue; } - if (handle_revision_arg(arg, revs, flags, seen_dashdash)) { + if (handle_revision_arg(arg, revs, flags, dashdash_pos)) { int j; - if (seen_dashdash || *arg == '^') - die("bad revision '%s'", arg); + if (dashdash_pos || *arg == '^') + return error("bad revision '%s'", arg); /* If we didn't have a "--": * (1) all filenames must exist; @@ -1351,7 +1313,50 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) break; } } - return left; + + return left; +} + +/* + * Parse revision information, filling in the "rev_info" structure, + * and removing the used arguments from the argument list. + * + * Returns the number of arguments left that weren't recognized + * (which are also moved to the head of the argument list) + */ +int parse_revisions(int argc, const char **argv, struct rev_info *revs) +{ + int i, left, dashdash_pos = 0; + + /* First, filter out revision options and look for "--" */ + for (left = i = 1; i < argc; i++) { + const char *arg = argv[i]; + int opts; + + if (arg[0] != '-') { + argv[left++] = arg; + continue; + } + + if (!strcmp(arg, "--")) { + dashdash_pos = left; + memcpy(argv + left, argv + i, sizeof(*argv) * (argc - i)); + left += argc - i; + break; + } + + opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv); + if (opts > 0) { + i += opts - 1; + continue; + } + if (opts < 0) + exit(128); + argv[left++] = arg; + } + argv[left] = NULL; + + return handle_revision_args(revs, left, argv, dashdash_pos); } void setup_revisions(struct rev_info *revs, const char *def) diff --git a/revision.h b/revision.h index 46ab713..c0f5df0 100644 --- a/revision.h +++ b/revision.h @@ -125,6 +125,7 @@ extern int parse_revisions(int argc, const char **argv, struct rev_info *revs); extern void setup_revisions(struct rev_info *revs, const char *def); extern int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, int *unkc, const char **unkv); +extern int handle_revision_args(struct rev_info *revs, int argc, const char **argv, int dashdash_pos); extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename); extern int prepare_revision_walk(struct rev_info *revs); -- 1.5.6.2.352.g416a6 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 5/6] git-blame: migrate to incremental parse-option [1/2] 2008-07-08 9:56 ` [PATCH 4/6] revisions: split handle_revision_args from parse_revisions Pierre Habouzit @ 2008-07-08 9:56 ` Pierre Habouzit 2008-07-08 9:56 ` [PATCH 6/6] git-blame: migrate to incremental parse-option [2/2] Pierre Habouzit 0 siblings, 1 reply; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 9:56 UTC (permalink / raw) To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit This step merely moves the parser to an incremental version, still using parse_revisions. Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- builtin-blame.c | 220 +++++++++++++++++++++++++++++------------------------- 1 files changed, 118 insertions(+), 102 deletions(-) diff --git a/builtin-blame.c b/builtin-blame.c index 1e26b88..6256341 100644 --- a/builtin-blame.c +++ b/builtin-blame.c @@ -18,24 +18,16 @@ #include "cache-tree.h" #include "path-list.h" #include "mailmap.h" +#include "parse-options.h" -static char blame_usage[] = -"git-blame [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-p] [-w] [-L n,m] [-S <revs-file>] [-M] [-C] [-C] [--contents <filename>] [--incremental] [commit] [--] file\n" -" -c Use the same output mode as git-annotate (Default: off)\n" -" -b Show blank SHA-1 for boundary commits (Default: off)\n" -" -l Show long commit SHA1 (Default: off)\n" -" --root Do not treat root commits as boundaries (Default: off)\n" -" -t Show raw timestamp (Default: off)\n" -" -f, --show-name Show original filename (Default: auto)\n" -" -n, --show-number Show original linenumber (Default: off)\n" -" -s Suppress author name and timestamp (Default: off)\n" -" -p, --porcelain Show in a format designed for machine consumption\n" -" -w Ignore whitespace differences\n" -" -L n,m Process only line range n,m, counting from 1\n" -" -M, -C Find line movements within and across files\n" -" --incremental Show blame entries as we find them, incrementally\n" -" --contents file Use <file>'s contents as the final image\n" -" -S revs-file Use revisions from revs-file instead of calling git-rev-list\n"; +static char blame_usage[] = "git-blame [options] [rev-opts] [rev] [--] file"; + +static const char *blame_opt_usage[] = { + blame_usage, + "", + "[rev-opts] are documented in git-rev-parse(1)", + NULL +}; static int longest_file; static int longest_author; @@ -2219,6 +2211,50 @@ static const char *prepare_initial(struct scoreboard *sb) return final_commit_name; } +static int blame_copy_callback(const struct option *option, const char *arg, int unset) +{ + int *opt = option->value; + + /* + * -C enables copy from removed files; + * -C -C enables copy from existing files, but only + * when blaming a new file; + * -C -C -C enables copy from existing files for + * everybody + */ + if (*opt & PICKAXE_BLAME_COPY_HARDER) + *opt |= PICKAXE_BLAME_COPY_HARDEST; + if (*opt & PICKAXE_BLAME_COPY) + *opt |= PICKAXE_BLAME_COPY_HARDER; + *opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE; + + if (arg) + blame_copy_score = parse_score(arg); + return 0; +} + +static int blame_move_callback(const struct option *option, const char *arg, int unset) +{ + int *opt = option->value; + + *opt |= PICKAXE_BLAME_MOVE; + + if (arg) + blame_move_score = parse_score(arg); + return 0; +} + +static int blame_bottomtop_callback(const struct option *option, const char *arg, int unset) +{ + const char **bottomtop = option->value; + if (!arg) + return -1; + if (*bottomtop) + die("More than one '-L n,m' option given"); + *bottomtop = arg; + return 0; +} + int cmd_blame(int argc, const char **argv, const char *prefix) { struct rev_info revs; @@ -2226,98 +2262,79 @@ int cmd_blame(int argc, const char **argv, const char *prefix) struct scoreboard sb; struct origin *o; struct blame_entry *ent; - int i, seen_dashdash, unk, opt; + int i, seen_dashdash, unk; long bottom, top, lno; - int output_option = 0; - int show_stats = 0; - const char *revs_file = NULL; const char *final_commit_name = NULL; enum object_type type; - const char *bottomtop = NULL; - const char *contents_from = NULL; + + static const char *bottomtop = NULL; + static int output_option = 0, opt = 0; + static int show_stats = 0; + static const char *revs_file = NULL; + static const char *contents_from = NULL; + static const struct option options[] = { + OPT_BOOLEAN(0, "incremental", &incremental, "Show blame entries as we find them, incrementally"), + OPT_BOOLEAN('b', NULL, &blank_boundary, "Show blank SHA-1 for boundary commits (Default: off)"), + OPT_BOOLEAN(0, "root", &show_root, "Do not treat root commits as boundaries (Default: off)"), + OPT_BOOLEAN(0, "show-stats", &show_stats, "Show work cost statistics"), + OPT_BIT(0, "score-debug", &output_option, "Show output score for blame entries", OUTPUT_SHOW_SCORE), + OPT_BIT('f', "show-name", &output_option, "Show original filename (Default: auto)", OUTPUT_SHOW_NAME), + OPT_BIT('n', "show-number", &output_option, "Show original linenumber (Default: off)", OUTPUT_SHOW_NUMBER), + OPT_BIT('p', "porcelain", &output_option, "Show in a format designed for machine consumption", OUTPUT_PORCELAIN), + OPT_BIT('c', NULL, &output_option, "Use the same output mode as git-annotate (Default: off)", OUTPUT_ANNOTATE_COMPAT), + OPT_BIT('t', NULL, &output_option, "Show raw timestamp (Default: off)", OUTPUT_RAW_TIMESTAMP), + OPT_BIT('l', NULL, &output_option, "Show long commit SHA1 (Default: off)", OUTPUT_LONG_OBJECT_NAME), + OPT_BIT('s', NULL, &output_option, "Suppress author name and timestamp (Default: off)", OUTPUT_NO_AUTHOR), + OPT_BIT('w', NULL, &xdl_opts, "Ignore whitespace differences", XDF_IGNORE_WHITESPACE), + OPT_STRING('S', NULL, &revs_file, "file", "Use revisions from <file> instead of calling git-rev-list"), + OPT_STRING(0, "contents", &contents_from, "file", "Use <file>'s contents as the final image"), + { OPTION_CALLBACK, 'C', NULL, &opt, "score", "Find line copies within and across files", PARSE_OPT_OPTARG, blame_copy_callback }, + { OPTION_CALLBACK, 'M', NULL, &opt, "score", "Find line movements within and across files", PARSE_OPT_OPTARG, blame_move_callback }, + OPT_CALLBACK('L', NULL, &bottomtop, "n,m", "Process only line range n,m, counting from 1", blame_bottomtop_callback), + OPT_END() + }; + + struct parse_opt_ctx_t ctx; cmd_is_annotate = !strcmp(argv[0], "annotate"); git_config(git_blame_config, NULL); + init_revisions(&revs, NULL); save_commit_buffer = 0; - opt = 0; + parse_options_start(&ctx, argc, argv, PARSE_OPT_KEEP_DASHDASH | + PARSE_OPT_KEEP_ARGV0); + for (;;) { + int n; + + switch (parse_options_step(&ctx, options, blame_opt_usage)) { + case PARSE_OPT_HELP: + exit(129); + case PARSE_OPT_DONE: + goto parse_done; + } + + if (!strcmp(ctx.argv[0], "--reverse")) { + ctx.argv[0] = "--children"; + reverse = 1; + } + n = handle_revision_opt(&revs, ctx.argc, ctx.argv, + &ctx.cpidx, ctx.out); + if (n <= 0) { + error("unknown option `%s'", ctx.argv[0]); + usage_with_options(blame_opt_usage, options); + } + ctx.argv += n; + ctx.argc -= n; + } +parse_done: + argc = parse_options_end(&ctx); + seen_dashdash = 0; for (unk = i = 1; i < argc; i++) { const char *arg = argv[i]; if (*arg != '-') break; - else if (!strcmp("-b", arg)) - blank_boundary = 1; - else if (!strcmp("--root", arg)) - show_root = 1; - else if (!strcmp("--reverse", arg)) { - argv[unk++] = "--children"; - reverse = 1; - } - else if (!strcmp(arg, "--show-stats")) - show_stats = 1; - else if (!strcmp("-c", arg)) - output_option |= OUTPUT_ANNOTATE_COMPAT; - else if (!strcmp("-t", arg)) - output_option |= OUTPUT_RAW_TIMESTAMP; - else if (!strcmp("-l", arg)) - output_option |= OUTPUT_LONG_OBJECT_NAME; - else if (!strcmp("-s", arg)) - output_option |= OUTPUT_NO_AUTHOR; - else if (!strcmp("-w", arg)) - xdl_opts |= XDF_IGNORE_WHITESPACE; - else if (!strcmp("-S", arg) && ++i < argc) - revs_file = argv[i]; - else if (!prefixcmp(arg, "-M")) { - opt |= PICKAXE_BLAME_MOVE; - blame_move_score = parse_score(arg+2); - } - else if (!prefixcmp(arg, "-C")) { - /* - * -C enables copy from removed files; - * -C -C enables copy from existing files, but only - * when blaming a new file; - * -C -C -C enables copy from existing files for - * everybody - */ - if (opt & PICKAXE_BLAME_COPY_HARDER) - opt |= PICKAXE_BLAME_COPY_HARDEST; - if (opt & PICKAXE_BLAME_COPY) - opt |= PICKAXE_BLAME_COPY_HARDER; - opt |= PICKAXE_BLAME_COPY | PICKAXE_BLAME_MOVE; - blame_copy_score = parse_score(arg+2); - } - else if (!prefixcmp(arg, "-L")) { - if (!arg[2]) { - if (++i >= argc) - usage(blame_usage); - arg = argv[i]; - } - else - arg += 2; - if (bottomtop) - die("More than one '-L n,m' option given"); - bottomtop = arg; - } - else if (!strcmp("--contents", arg)) { - if (++i >= argc) - usage(blame_usage); - contents_from = argv[i]; - } - else if (!strcmp("--incremental", arg)) - incremental = 1; - else if (!strcmp("--score-debug", arg)) - output_option |= OUTPUT_SHOW_SCORE; - else if (!strcmp("-f", arg) || - !strcmp("--show-name", arg)) - output_option |= OUTPUT_SHOW_NAME; - else if (!strcmp("-n", arg) || - !strcmp("--show-number", arg)) - output_option |= OUTPUT_SHOW_NUMBER; - else if (!strcmp("-p", arg) || - !strcmp("--porcelain", arg)) - output_option |= OUTPUT_PORCELAIN; else if (!strcmp("--", arg)) { seen_dashdash = 1; i++; @@ -2364,16 +2381,16 @@ int cmd_blame(int argc, const char **argv, const char *prefix) if (seen_dashdash) { /* (1) */ if (argc <= i) - usage(blame_usage); + usage_with_options(blame_opt_usage, options); path = add_prefix(prefix, argv[i]); if (i + 1 == argc - 1) { if (unk != 1) - usage(blame_usage); + usage_with_options(blame_opt_usage, options); argv[unk++] = argv[i + 1]; } else if (i + 1 != argc) /* garbage at end */ - usage(blame_usage); + usage_with_options(blame_opt_usage, options); } else { int j; @@ -2383,7 +2400,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) if (seen_dashdash) { /* (2) */ if (seen_dashdash + 1 != argc - 1) - usage(blame_usage); + usage_with_options(blame_opt_usage, options); path = add_prefix(prefix, argv[seen_dashdash + 1]); for (j = i; j < seen_dashdash; j++) argv[unk++] = argv[j]; @@ -2391,7 +2408,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) else { /* (3) */ if (argc <= i) - usage(blame_usage); + usage_with_options(blame_opt_usage, options); path = add_prefix(prefix, argv[i]); if (i + 1 == argc - 1) { final_commit_name = argv[i + 1]; @@ -2405,7 +2422,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) } } else if (i != argc - 1) - usage(blame_usage); /* garbage at end */ + usage_with_options(blame_opt_usage, options); setup_work_tree(); if (!has_path_in_work_tree(path)) @@ -2424,7 +2441,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix) argv[unk++] = "--"; /* terminate the rev name */ argv[unk] = NULL; - init_revisions(&revs, NULL); parse_revisions(unk, argv, &revs); setup_revisions(&revs, NULL); memset(&sb, 0, sizeof(sb)); -- 1.5.6.2.352.g416a6 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 6/6] git-blame: migrate to incremental parse-option [2/2] 2008-07-08 9:56 ` [PATCH 5/6] git-blame: migrate to incremental parse-option [1/2] Pierre Habouzit @ 2008-07-08 9:56 ` Pierre Habouzit 0 siblings, 0 replies; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 9:56 UTC (permalink / raw) To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit Now use handle_revision_args instead of parse_revisions, and simplify the handling of old-style arguments a lot thanks to the filtering. Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- builtin-blame.c | 132 +++++++++++++++++-------------------------------------- 1 files changed, 40 insertions(+), 92 deletions(-) diff --git a/builtin-blame.c b/builtin-blame.c index 6256341..bbbaea5 100644 --- a/builtin-blame.c +++ b/builtin-blame.c @@ -2262,8 +2262,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) struct scoreboard sb; struct origin *o; struct blame_entry *ent; - int i, seen_dashdash, unk; - long bottom, top, lno; + long dashdash_pos, bottom, top, lno; const char *final_commit_name = NULL; enum object_type type; @@ -2301,6 +2300,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) git_config(git_blame_config, NULL); init_revisions(&revs, NULL); save_commit_buffer = 0; + dashdash_pos = 0; parse_options_start(&ctx, argc, argv, PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0); @@ -2311,6 +2311,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix) case PARSE_OPT_HELP: exit(129); case PARSE_OPT_DONE: + if (ctx.argv[0]) + dashdash_pos = ctx.cpidx; goto parse_done; } @@ -2330,20 +2332,6 @@ int cmd_blame(int argc, const char **argv, const char *prefix) parse_done: argc = parse_options_end(&ctx); - seen_dashdash = 0; - for (unk = i = 1; i < argc; i++) { - const char *arg = argv[i]; - if (*arg != '-') - break; - else if (!strcmp("--", arg)) { - seen_dashdash = 1; - i++; - break; - } - else - argv[unk++] = arg; - } - if (!blame_move_score) blame_move_score = BLAME_DEFAULT_MOVE_SCORE; if (!blame_copy_score) @@ -2356,92 +2344,52 @@ parse_done: * * The remaining are: * - * (1) if seen_dashdash, its either - * "-options -- <path>" or - * "-options -- <path> <rev>". - * but the latter is allowed only if there is no - * options that we passed to revision machinery. + * (1) if dashdash_pos != 0, its either + * "blame [revisions] -- <path>" or + * "blame -- <path> <rev>" * - * (2) otherwise, we may have "--" somewhere later and - * might be looking at the first one of multiple 'rev' - * parameters (e.g. " master ^next ^maint -- path"). - * See if there is a dashdash first, and give the - * arguments before that to revision machinery. - * After that there must be one 'path'. + * (2) otherwise, its one of the two: + * "blame [revisions] <path>" + * "blame <path> <rev>" * - * (3) otherwise, its one of the three: - * "-options <path> <rev>" - * "-options <rev> <path>" - * "-options <path>" - * but again the first one is allowed only if - * there is no options that we passed to revision - * machinery. + * Note that we must strip out <path> from the arguments: we do not + * want the path pruning but we may want "bottom" processing. */ - - if (seen_dashdash) { - /* (1) */ - if (argc <= i) - usage_with_options(blame_opt_usage, options); - path = add_prefix(prefix, argv[i]); - if (i + 1 == argc - 1) { - if (unk != 1) + if (dashdash_pos) { + switch (argc - dashdash_pos - 1) { + case 2: /* (1b) */ + if (argc != 4) usage_with_options(blame_opt_usage, options); - argv[unk++] = argv[i + 1]; + /* reorder for the new way: <rev> -- <path> */ + argv[1] = argv[3]; + argv[3] = argv[2]; + argv[2] = "--"; + dashdash_pos = 2; + /* FALLTHROUGH */ + case 1: /* (1a) */ + path = add_prefix(prefix, argv[--argc]); + argv[argc] = NULL; + break; + default: + usage_with_options(blame_opt_usage, options); } - else if (i + 1 != argc) - /* garbage at end */ + } else { + if (argc < 2) usage_with_options(blame_opt_usage, options); - } - else { - int j; - for (j = i; !seen_dashdash && j < argc; j++) - if (!strcmp(argv[j], "--")) - seen_dashdash = j; - if (seen_dashdash) { - /* (2) */ - if (seen_dashdash + 1 != argc - 1) - usage_with_options(blame_opt_usage, options); - path = add_prefix(prefix, argv[seen_dashdash + 1]); - for (j = i; j < seen_dashdash; j++) - argv[unk++] = argv[j]; + path = add_prefix(prefix, argv[argc - 1]); + if (argc == 3 && !has_path_in_work_tree(path)) { /* (2b) */ + path = add_prefix(prefix, argv[1]); + argv[1] = argv[2]; } - else { - /* (3) */ - if (argc <= i) - usage_with_options(blame_opt_usage, options); - path = add_prefix(prefix, argv[i]); - if (i + 1 == argc - 1) { - final_commit_name = argv[i + 1]; - - /* if (unk == 1) we could be getting - * old-style - */ - if (unk == 1 && !has_path_in_work_tree(path)) { - path = add_prefix(prefix, argv[i + 1]); - final_commit_name = argv[i]; - } - } - else if (i != argc - 1) - usage_with_options(blame_opt_usage, options); + argv[argc - 1] = "--"; + dashdash_pos = argc - 1; - setup_work_tree(); - if (!has_path_in_work_tree(path)) - die("cannot stat path %s: %s", - path, strerror(errno)); - } + setup_work_tree(); + if (!has_path_in_work_tree(path)) + die("cannot stat path %s: %s", path, strerror(errno)); } - if (final_commit_name) - argv[unk++] = final_commit_name; - - /* - * Now we got rev and path. We do not want the path pruning - * but we may want "bottom" processing. - */ - argv[unk++] = "--"; /* terminate the rev name */ - argv[unk] = NULL; - - parse_revisions(unk, argv, &revs); + handle_revision_args(&revs, argc, argv, dashdash_pos); setup_revisions(&revs, NULL); memset(&sb, 0, sizeof(sb)); -- 1.5.6.2.352.g416a6 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions. 2008-07-08 9:56 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Pierre Habouzit 2008-07-08 9:56 ` [PATCH 3/6] revisions: parse_revisions refactor Pierre Habouzit @ 2008-07-08 10:51 ` Johannes Sixt 2008-07-08 11:00 ` Pierre Habouzit 1 sibling, 1 reply; 15+ messages in thread From: Johannes Sixt @ 2008-07-08 10:51 UTC (permalink / raw) To: Pierre Habouzit; +Cc: git, torvalds, gitster, peff Pierre Habouzit schrieb: > +int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, > + int *unkc, const char **unkv) > +{ > + const char *arg = argv[0]; > + > + /* pseudo revision arguments */ > + if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") || > + !strcmp(arg, "--tags") || !strcmp(arg, "--remotes") || > + !strcmp(arg, "--reflog") || !strcmp(arg, "--not") || > + !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) > + { > + unkv[*unkc++] = arg; + unkv[(*unkc)++] = arg; Hm? -- Hannes ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions. 2008-07-08 10:51 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Johannes Sixt @ 2008-07-08 11:00 ` Pierre Habouzit 2008-07-08 11:25 ` Pierre Habouzit 0 siblings, 1 reply; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 11:00 UTC (permalink / raw) To: Johannes Sixt; +Cc: git, torvalds, gitster, peff [-- Attachment #1: Type: text/plain, Size: 932 bytes --] On Tue, Jul 08, 2008 at 10:51:13AM +0000, Johannes Sixt wrote: > Pierre Habouzit schrieb: > > +int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, > > + int *unkc, const char **unkv) > > +{ > > + const char *arg = argv[0]; > > + > > + /* pseudo revision arguments */ > > + if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") || > > + !strcmp(arg, "--tags") || !strcmp(arg, "--remotes") || > > + !strcmp(arg, "--reflog") || !strcmp(arg, "--not") || > > + !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) > > + { > > + unkv[*unkc++] = arg; > > + unkv[(*unkc)++] = arg; Huh right. Good catch, I wonder why the testsuite failed to see that. Anyways I pushed a fix on my public repo. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions. 2008-07-08 11:00 ` Pierre Habouzit @ 2008-07-08 11:25 ` Pierre Habouzit 0 siblings, 0 replies; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 11:25 UTC (permalink / raw) To: Johannes Sixt, git, torvalds, gitster, peff [-- Attachment #1: Type: text/plain, Size: 6070 bytes --] On Tue, Jul 08, 2008 at 11:00:22AM +0000, Pierre Habouzit wrote: > On Tue, Jul 08, 2008 at 10:51:13AM +0000, Johannes Sixt wrote: > > Pierre Habouzit schrieb: > > > +int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, > > > + int *unkc, const char **unkv) > > > +{ > > > + const char *arg = argv[0]; > > > + > > > + /* pseudo revision arguments */ > > > + if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") || > > > + !strcmp(arg, "--tags") || !strcmp(arg, "--remotes") || > > > + !strcmp(arg, "--reflog") || !strcmp(arg, "--not") || > > > + !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk")) > > > + { > > > + unkv[*unkc++] = arg; > > > > + unkv[(*unkc)++] = arg; > > Huh right. Good catch, I wonder why the testsuite failed to see that. Okay, I know why, there is another bug that was hiding this one, parse_revisions whas doing argv[left++] = arg itself too, which was fixing this mistake. I've pushed it again, and here is the fixed patch 3/6 for reviewing. -----------8<----------- From d0a062617526128c4ec0245614eb9916b7996c38 Mon Sep 17 00:00:00 2001 From: Pierre Habouzit <madcoder@debian.org> Date: Tue, 8 Jul 2008 11:00:02 +0200 Subject: [PATCH] revisions: split handle_revision_args from parse_revisions. This new function is meant to only parse non option revision arguments in a row. IOW it's meant to parse what remains from a parse-opt filtering of the argument list, with the knowledge of the "--" position (0 means none). Signed-off-by: Pierre Habouzit <madcoder@debian.org> --- revision.c | 97 +++++++++++++++++++++++++++++++++++------------------------ revision.h | 1 + 2 files changed, 58 insertions(+), 40 deletions(-) diff --git a/revision.c b/revision.c index 99b3cc9..6958725 100644 --- a/revision.c +++ b/revision.c @@ -1241,39 +1241,15 @@ int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, return 1; } -/* - * Parse revision information, filling in the "rev_info" structure, - * and removing the used arguments from the argument list. - * - * Returns the number of arguments left that weren't recognized - * (which are also moved to the head of the argument list) - */ -int parse_revisions(int argc, const char **argv, struct rev_info *revs) +int handle_revision_args(struct rev_info *revs, + int argc, const char **argv, int dashdash_pos) { - int i, flags, seen_dashdash; - const char **unrecognized = argv + 1; - int left = 1; + int i, left, flags = 0; - /* First, search for "--" */ - seen_dashdash = 0; - for (i = 1; i < argc; i++) { + for (left = i = 1; i < argc; i++) { const char *arg = argv[i]; - if (strcmp(arg, "--")) - continue; - argv[i] = NULL; - argc = i; - if (argv[i + 1]) - revs->prune_data = get_pathspec(revs->prefix, argv + i + 1); - seen_dashdash = 1; - break; - } - flags = 0; - for (i = 1; i < argc; i++) { - const char *arg = argv[i]; if (*arg == '-') { - int opts; - if (!strcmp(arg, "--all")) { handle_refs(revs, flags, for_each_ref); continue; @@ -1306,23 +1282,22 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) revs->no_walk = 0; continue; } - - opts = handle_revision_opt(revs, argc - i, argv + i, NULL, NULL); - if (opts > 0) { - i += opts - 1; - continue; + if (i == dashdash_pos) { + argv[i] = NULL; + argc = i; + if (argv[i + 1]) + revs->prune_data = get_pathspec(revs->prefix, argv + i + 1); + break; } - if (opts < 0) - exit(128); - *unrecognized++ = arg; - left++; + + argv[left++] = arg; continue; } - if (handle_revision_arg(arg, revs, flags, seen_dashdash)) { + if (handle_revision_arg(arg, revs, flags, dashdash_pos)) { int j; - if (seen_dashdash || *arg == '^') - die("bad revision '%s'", arg); + if (dashdash_pos || *arg == '^') + return error("bad revision '%s'", arg); /* If we didn't have a "--": * (1) all filenames must exist; @@ -1338,9 +1313,51 @@ int parse_revisions(int argc, const char **argv, struct rev_info *revs) break; } } + return left; } +/* + * Parse revision information, filling in the "rev_info" structure, + * and removing the used arguments from the argument list. + * + * Returns the number of arguments left that weren't recognized + * (which are also moved to the head of the argument list) + */ +int parse_revisions(int argc, const char **argv, struct rev_info *revs) +{ + int i, left, dashdash_pos = 0; + + /* First, filter out revision options and look for "--" */ + for (left = i = 1; i < argc; i++) { + const char *arg = argv[i]; + int opts; + + if (arg[0] != '-') { + argv[left++] = arg; + continue; + } + + if (!strcmp(arg, "--")) { + dashdash_pos = left; + memcpy(argv + left, argv + i, sizeof(*argv) * (argc - i)); + left += argc - i; + break; + } + + opts = handle_revision_opt(revs, argc - i, argv + i, &left, argv); + if (opts > 0) { + i += opts - 1; + continue; + } + if (opts < 0) + exit(128); + } + argv[left] = NULL; + + return handle_revision_args(revs, left, argv, dashdash_pos); +} + void setup_revisions(struct rev_info *revs, const char *def) { if (revs->def == NULL) diff --git a/revision.h b/revision.h index 46ab713..c0f5df0 100644 --- a/revision.h +++ b/revision.h @@ -125,6 +125,7 @@ extern int parse_revisions(int argc, const char **argv, struct rev_info *revs); extern void setup_revisions(struct rev_info *revs, const char *def); extern int handle_revision_opt(struct rev_info *revs, int argc, const char **argv, int *unkc, const char **unkv); +extern int handle_revision_args(struct rev_info *revs, int argc, const char **argv, int dashdash_pos); extern int handle_revision_arg(const char *arg, struct rev_info *revs,int flags,int cant_be_filename); extern int prepare_revision_walk(struct rev_info *revs); -- 1.5.6.2.399.g15e15 [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/6] revisions: refactor init_revisions and setup_revisions. 2008-07-08 9:55 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Pierre Habouzit 2008-07-08 9:56 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Pierre Habouzit @ 2008-07-08 10:59 ` Johannes Schindelin 2008-07-08 11:06 ` Pierre Habouzit 1 sibling, 1 reply; 15+ messages in thread From: Johannes Schindelin @ 2008-07-08 10:59 UTC (permalink / raw) To: Pierre Habouzit; +Cc: git, torvalds, gitster, peff Hi, On Tue, 8 Jul 2008, Pierre Habouzit wrote: > setup_revisions has been split in two: parse_revisions that does > (almost) only argument parsing, to be more like what parse-options can > do, and setup_revisions that does the rest. I do not see the sense of this change, except - blowing up your patch unnecessarily, - making it more inconvenient for users, and - adding an opportunity for bugs to creep in. In other words, I would like to see the semantics of setup_revisions() untouched. Ciao, Dscho ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/6] revisions: refactor init_revisions and setup_revisions. 2008-07-08 10:59 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Johannes Schindelin @ 2008-07-08 11:06 ` Pierre Habouzit 2008-07-08 11:43 ` Johannes Schindelin 0 siblings, 1 reply; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 11:06 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, torvalds, gitster, peff [-- Attachment #1: Type: text/plain, Size: 1478 bytes --] On Tue, Jul 08, 2008 at 10:59:43AM +0000, Johannes Schindelin wrote: > Hi, > > On Tue, 8 Jul 2008, Pierre Habouzit wrote: > > > setup_revisions has been split in two: parse_revisions that does > > (almost) only argument parsing, to be more like what parse-options can > > do, and setup_revisions that does the rest. > > I do not see the sense of this change, except Well, it's required to remove "parse_revisions" at once if we one day reach the goal of having only parse-opt based parsers. > - blowing up your patch unnecessarily, > - making it more inconvenient for users, and It makes it convenient for parse-opt based parser, and it makes it convenient for people that don't have arguments to parse (and those are many). > - adding an opportunity for bugs to creep in. > > In other words, I would like to see the semantics of setup_revisions() > untouched. FWIW like said I already submitted that patch under different versions in the past, and I seem to recall that it was welcomed as it splits the rev_info final fixups from the argument parsing which are quite different steps. FWIW I really like the new semantics better. If you're worried about the fact that people may be confused about the new semantics, fine, let's rename it then. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/6] revisions: refactor init_revisions and setup_revisions. 2008-07-08 11:06 ` Pierre Habouzit @ 2008-07-08 11:43 ` Johannes Schindelin 2008-07-08 12:48 ` Pierre Habouzit 0 siblings, 1 reply; 15+ messages in thread From: Johannes Schindelin @ 2008-07-08 11:43 UTC (permalink / raw) To: Pierre Habouzit; +Cc: git, torvalds, gitster, peff Hi, On Tue, 8 Jul 2008, Pierre Habouzit wrote: > On Tue, Jul 08, 2008 at 10:59:43AM +0000, Johannes Schindelin wrote: > > > On Tue, 8 Jul 2008, Pierre Habouzit wrote: > > > > > setup_revisions has been split in two: parse_revisions that does > > > (almost) only argument parsing, to be more like what parse-options > > > can do, and setup_revisions that does the rest. > > > > I do not see the sense of this change, except > > Well, it's required to remove "parse_revisions" at once if we one day > reach the goal of having only parse-opt based parsers. We can do that, then. We do not need to do that, now. In the meantime, your patch series of already pretty large patches gets even larger, which prevents me from being able to review them. Sad, Dscho P.S.: Please do not pull things at me like "it is so convenient to be able to drop "0, NULL, " for many sites". That is not even funny. This case does not need more convenience. The other cases do. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/6] revisions: refactor init_revisions and setup_revisions. 2008-07-08 11:43 ` Johannes Schindelin @ 2008-07-08 12:48 ` Pierre Habouzit 0 siblings, 0 replies; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 12:48 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, torvalds, gitster, peff [-- Attachment #1: Type: text/plain, Size: 1041 bytes --] On mar, jui 08, 2008 at 11:43:15 +0000, Johannes Schindelin wrote: > Hi, > > On Tue, 8 Jul 2008, Pierre Habouzit wrote: > > > On Tue, Jul 08, 2008 at 10:59:43AM +0000, Johannes Schindelin wrote: > > > > > On Tue, 8 Jul 2008, Pierre Habouzit wrote: > > > > > > > setup_revisions has been split in two: parse_revisions that does > > > > (almost) only argument parsing, to be more like what parse-options > > > > can do, and setup_revisions that does the rest. > > > > > > I do not see the sense of this change, except > > > > Well, it's required to remove "parse_revisions" at once if we one day > > reach the goal of having only parse-opt based parsers. > > We can do that, then. > > We do not need to do that, now. hmmm right, I may be able to cook a simpler series without that indeed. Ill repost a new one with this idea. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Migration of builtin-blame to parse-option 2008-07-08 9:55 Migration of builtin-blame to parse-option Pierre Habouzit 2008-07-08 9:55 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Pierre Habouzit @ 2008-07-08 10:02 ` Pierre Habouzit 1 sibling, 0 replies; 15+ messages in thread From: Pierre Habouzit @ 2008-07-08 10:02 UTC (permalink / raw) To: git; +Cc: torvalds, gitster, peff [-- Attachment #1: Type: text/plain, Size: 522 bytes --] While rebasing this branch on the latest next, I had a conflict for the patch 1 that I fixed, and I also fixed spacing issues in patches 4 and 6. The fixed series is on my public git repo[0], but it's really like this one, so I don't see the point in flooding again with it ;) [0] git://git.madism.org/~madcoder/git.git#ph/parseopt -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org [-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2008-07-08 12:49 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-07-08 9:55 Migration of builtin-blame to parse-option Pierre Habouzit 2008-07-08 9:55 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Pierre Habouzit 2008-07-08 9:56 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Pierre Habouzit 2008-07-08 9:56 ` [PATCH 3/6] revisions: parse_revisions refactor Pierre Habouzit 2008-07-08 9:56 ` [PATCH 4/6] revisions: split handle_revision_args from parse_revisions Pierre Habouzit 2008-07-08 9:56 ` [PATCH 5/6] git-blame: migrate to incremental parse-option [1/2] Pierre Habouzit 2008-07-08 9:56 ` [PATCH 6/6] git-blame: migrate to incremental parse-option [2/2] Pierre Habouzit 2008-07-08 10:51 ` [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions Johannes Sixt 2008-07-08 11:00 ` Pierre Habouzit 2008-07-08 11:25 ` Pierre Habouzit 2008-07-08 10:59 ` [PATCH 1/6] revisions: refactor init_revisions and setup_revisions Johannes Schindelin 2008-07-08 11:06 ` Pierre Habouzit 2008-07-08 11:43 ` Johannes Schindelin 2008-07-08 12:48 ` Pierre Habouzit 2008-07-08 10:02 ` Migration of builtin-blame to parse-option Pierre Habouzit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).