Git development
 help / color / mirror / Atom feed
* Re: Migration of builtin-blame to parse-option
From: Pierre Habouzit @ 2008-07-08 10:02 UTC (permalink / raw)
  To: git; +Cc: torvalds, gitster, peff
In-Reply-To: <1215510964-16664-1-git-send-email-madcoder@debian.org>

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

* [PATCH 2/6] revisions: split the pure option parsing out from parse_revisions.
From: Pierre Habouzit @ 2008-07-08  9:56 UTC (permalink / raw)
  To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit
In-Reply-To: <1215510964-16664-2-git-send-email-madcoder@debian.org>

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

* [PATCH 6/6] git-blame: migrate to incremental parse-option [2/2]
From: Pierre Habouzit @ 2008-07-08  9:56 UTC (permalink / raw)
  To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit
In-Reply-To: <1215510964-16664-6-git-send-email-madcoder@debian.org>

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

* [PATCH 5/6] git-blame: migrate to incremental parse-option [1/2]
From: Pierre Habouzit @ 2008-07-08  9:56 UTC (permalink / raw)
  To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit
In-Reply-To: <1215510964-16664-5-git-send-email-madcoder@debian.org>

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

* [PATCH 1/6] revisions: refactor init_revisions and setup_revisions.
From: Pierre Habouzit @ 2008-07-08  9:55 UTC (permalink / raw)
  To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit
In-Reply-To: <1215510964-16664-1-git-send-email-madcoder@debian.org>

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

* [PATCH 3/6] revisions: parse_revisions refactor.
From: Pierre Habouzit @ 2008-07-08  9:56 UTC (permalink / raw)
  To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit
In-Reply-To: <1215510964-16664-3-git-send-email-madcoder@debian.org>

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

* [PATCH 4/6] revisions: split handle_revision_args from parse_revisions.
From: Pierre Habouzit @ 2008-07-08  9:56 UTC (permalink / raw)
  To: git; +Cc: torvalds, gitster, peff, Pierre Habouzit
In-Reply-To: <1215510964-16664-4-git-send-email-madcoder@debian.org>

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

* Migration of builtin-blame to parse-option
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

* Re: Test breakage [Re: [ANNOUNCE] GIT 1.5.6.2]
From: Michael J Gruber @ 2008-07-08  9:47 UTC (permalink / raw)
  To: git
In-Reply-To: <20080708051622.GC2037@sigill.intra.peff.net>

Jeff King venit, vidit, dixit 08.07.2008 07:16:
> On Mon, Jul 07, 2008 at 02:44:49PM +0200, Michael J Gruber wrote:
> 
>> On Kubuntu 7.10 (not my choice, this is at work...), "make test" gives:
>>
>> *** t9600-cvsimport.sh ***
>> [...]
>> * FAIL 6: update git module
>>                 cd module-git &&
>>                 git cvsimport -a -z 0 module &&
>>                 git merge origin &&
>>                 cd .. &&
>>                 test_cmp module-cvs/o_fortuna module-git/o_fortuna
>>
>> On the second (and subsequent) run, "make test" succeeds. I tried again  
>> with a fresh copy from the tarball, same effect: failure on 1, success on 
>> 2 and following.
> 
> I tried reproducing on my Debian box with various weaks of $LANG, but I
> couldn't. Can you please try running "./t9600-cvsimport.sh -v -i" and
> send us the output of a failing instance. It's not clear which step in
> failing test is causing the problem, or if cvsps is giving off any
> useful errors.
> 
>> This is with LANG=de_DE.UTF-8, whereas with LANG=C the tests succeed the  
>> first time already. Are tests supposed to be done in C locale only? Do  
>> they reuse data from a previous test run? I didn't notice this with 
>> 1.5.6.1.
> 
> They should be perfectly repeatable, but I have run into problems in the
> past with cruft in my ~/.cvsps directory. However, we should be setting
> $HOME properly in t9600 to avoid this, so I'm not sure what the culprit
> is.

Sure enough, I can't reproduce it today. I reproduced that effect 
several times yesterday, or else I would not have reported anything. 
Trying the very same steps today the tests succeed on the first run. 
Only differences between yesterday and today are be:

- I installed git 1.5.6.2 now (had 1.5.6.1 before) under $HOME, with 
PATH, PERL5LIB, LD_LIBRARY_PATH set accordingly; but tests should pick 
up git stuff from the local build dir, right?

- There may have been an automated update to kubuntu meanwhile, but I 
don't think so; cvsps is under $HOME anyways, and what else would 
matter? Perl version maybe?

So it's WORKSFORME now. Sorry for the noise. Next time I'll not only try 
to reproduce (as I did) but wait a day also!

Best
Michael

^ permalink raw reply

* Re: [PATCH] fix "git-submodule add a/b/c/repository"
From: Sylvain Joyeux @ 2008-07-08  8:08 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: Junio C Hamano, Lars Hjemli, Ping Yin, git
In-Reply-To: <4872DE54.5010804@gmail.com>

On Mon, Jul 07, 2008 at 11:26:12PM -0400, Mark Levedahl wrote:
> Junio C Hamano wrote:
>> I'd like to hear clarifications on two counts, please?
>>  (1) If Sylvain wanted to have that appear at dir0/dir1/init not init,
>>      would it have been sufficient to give that path twice (once for
>>      <repository> and another for <path> parameter) to make things work as
>>      expected?
>>   
> git-submodule really requires two arguments:

Then it should raise an error when the following is given

  git submodule add ./relative-path-in-repo

which, for now, is accepted as

  git submodule add ./relative-path-in-repo ./relative-path-in-repo

(and confused me into thinking it was a normal behaviour)

Sylvain

^ permalink raw reply

* Re: [PATCH] better git-submodule status output
From: Sylvain Joyeux @ 2008-07-08  8:00 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, Avery Pennarun, Lars Hjemli, Ping Yin,
	Mark Levedahl, git
In-Reply-To: <7v8wwd4kvb.fsf@gitster.siamese.dyndns.org>

On Mon, Jul 07, 2008 at 11:20:08AM -0700, Junio C Hamano wrote:
> > Is it possible to make 'fetch' only .. well .. fetch objects, without
> > updating any refs?
> 
> By honestly saying "The superproject binds a commit you do not have
> locally at these paths", the user *can* choose *when* to go there and
> update, or have "git submodule update" command to that for him.
Agreed. The problem is that git submodule update does not only fetches
the new refs but *also* checks out the commit currently registered in
the superproject. Therefore git-submodule status *and* git-submodule
summary cannot tell you anything after a git-submodule update.

What I was missing is, given the current discussion, a git-submodule
fetch alike to what Avery was proposing, which fetches all submodules in
one command. Having to do the fetch/update manually for each submodule
is really cumbersome -- I have around 30 modules in the superproject.

Sylvain

^ permalink raw reply

* Re* [PATCH] builtin-rerere: fix conflict markers parsing
From: Junio C Hamano @ 2008-07-08  7:52 UTC (permalink / raw)
  To: Olivier Marin; +Cc: Johannes Schindelin, git
In-Reply-To: <7vy74d4mr8.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> So what about
>>
>> 	<<<<<<< This hunk contains =====
>> 	anythin
>> 	=======
>>
>> 	Hello
>> 	=======
>> 	somethin else
>> 	>>>>>>> problem!
>> ...
> I however think detecting that we have this ambiguous hunk is easy, and
> punting gracefully and not re-resolving in such a case is million times
> better than producing random results that the users need to be worried
> about.

I am wondering if a patch like this on top of your patch may make things
even safer.  The idea is the same as the earlier a1b32fd (git-rerere:
detect unparsable conflicts, 2008-06-22) to fail rerere unless the markers
are unambiguous.

Thanks to your isspace(buf[7]), it is slightly less likely that this
safety triggers on false positives.

Thoughts?

-- >8 --
rerere: punt and do not resolve if conflict markers are ambiguous

Especially because we are introducing rerere.autoupdate configuration
(which is off by default for safety) that automatically stages the
resolution made by rerere, it is necessary to make sure that we do not
autoresolve when there is any ambiguity.

 builtin-rerere.c |   17 +++++++++++++----
 1 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/builtin-rerere.c b/builtin-rerere.c
index e618862..69c3a52 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -112,12 +112,17 @@ static int handle_file(const char *path,
 	strbuf_init(&one, 0);
 	strbuf_init(&two,  0);
 	while (fgets(buf, sizeof(buf), f)) {
-		if (hunk == 0 && !prefixcmp(buf, "<<<<<<< "))
+		if (!prefixcmp(buf, "<<<<<<< ")) {
+			if (hunk)
+				goto bad;
 			hunk = 1;
-		else if (hunk == 1 && !prefixcmp(buf, "=======") &&
-			 isspace(buf[7]))
+		} else if (!prefixcmp(buf, "=======") && isspace(buf[7])) {
+			if (hunk != 1)
+				goto bad;
 			hunk = 2;
-		else if (hunk == 2 && !prefixcmp(buf, ">>>>>>> ")) {
+		} else if (!prefixcmp(buf, ">>>>>>> ")) {
+			if (hunk != 2)
+				goto bad;
 			if (strbuf_cmp(&one, &two) > 0)
 				strbuf_swap(&one, &two);
 			hunk_no++;
@@ -143,6 +148,10 @@ static int handle_file(const char *path,
 			strbuf_addstr(&two, buf);
 		else if (out)
 			fputs(buf, out);
+		continue;
+	bad:
+		hunk = 99; /* force error exit */
+		break;
 	}
 	strbuf_release(&one);
 	strbuf_release(&two);

^ permalink raw reply related

* Re: PATCH] Documentation: Tweak use case in "git stash save --keep-index"
From: Eric Raible @ 2008-07-08  7:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, szeder
In-Reply-To: <7vlk0dvsr5.fsf@gitster.siamese.dyndns.org>

The documentation suggests using "git stash apply" in the
--keep-index workflow even though doing so will lead to clutter
in the stash.  And given that the changes are about to be
committed anyway "git stash pop" is more sensible.

Additionally the text preceeding the example claims that it
works for "two or more commits", but the example itself is
really tailored for just two.  Expanding it just a little
makes it clear how the procedure generalizes to N commits.

Finally the example is annotated with some commentary to
explain things on a line-by-line basis.
---
[Note: If we can't rely on fixed-width fonts then the resulting
example will look stupid]

On Mon, Jul 7, 2008 at 10:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Also this may be a good way to split an existing commit into five during a
> "rebase -i" session, and the example in the documentation might want to
> talk about it in that larger picture.

If you're talking about 'edit' mode, doing a "reset HEAD^", and then
using the same "add --patch"/"stash save -k-i"/commit/"stash pop"
cycle in question then I understand and agree.  But only if the
user wants to test each part separately.

Otherwise why bother with stash-k-i when you can just loop
on add-patch/commit or use git-gui?
--
 Documentation/git-stash.txt |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index df26901..7d50d74 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -197,13 +197,14 @@ each change before committing:
 +
 ----------------------------------------------------------------
 ... hack hack hack ...
-$ git add --patch foo
-$ git stash save --keep-index
-$ build && run tests
-$ git commit -m 'First part'
-$ git stash apply
-$ build && run tests
-$ git commit -a -m 'Second part'
+$ git add --patch foo            # add just first part to the index
+$ git stash save --keep-index    # save all other changes to the stash
+$ edit/build/test first part
+$ git commit foo -m 'First part' # commit fully tested change
+$ git stash pop                  # prepare to work on all other changes
+... repeat above five steps until one commit remains ...
+$ edit/build/test remaining parts
+$ git commit foo -m 'Remaining parts'
 ----------------------------------------------------------------

 SEE ALSO
-- 
1.5.6.1.1073.g489ff.dirty

^ permalink raw reply related

* Re: [GSoC] What is status of Git's Google Summer of Code 2008 projects?
From: Jakub Narebski @ 2008-07-08  7:39 UTC (permalink / raw)
  To: Stephan Beyer; +Cc: Junio C Hamano, git, Christian Couder, Daniel Barkalow
In-Reply-To: <20080708014734.GE6726@leksak.fem-net>

On Tue, 8 July 2008, Stephan Beyer wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> I _really_ wanted to merge the basic bits and rewrite of "am" at least to
>> pu tonight, but I had to drop them after noticing that it does not seem to
>> handle --rebasing at all (it parses to set $rebasing but after that where
>> does that bit go?
> 
> Yes, you are right that am --rebasing is a no-op.
> That option was a little mystery to me, because it seemed to do nothing
> special, but I'll check again (bash-completion etc) and do appropriate
> changes.

Undocumented option '--rebasing' to git-am is internal option changing
git-am behavior to be better used by git-rebase, namely it does not
change commit message even if it doesn't follow git commit message
convention, for example if it begins not with single line summary
of commit, separated by empty line, but by multi-line paragraph.
See also t/t3405-rebase-malformed.sh

Although I am not sure if when rebase is rewritten using git-sequencer
implementing "git am --rebasing" would be truly needed.  On the other
hand side it would be nice to have some _documented_ option which would
allow to git-am mail messages with commits not following git commit
messages convention...

-- 
Jakub Narebski
Poland

^ permalink raw reply

* Re: [GSoC] What is status of Git's Google Summer of Code 2008 projects?
From: J.H. @ 2008-07-08  7:20 UTC (permalink / raw)
  To: Lea Wiemann
  Cc: Jakub Narebski, git, Sam Vilain, Joshua Roys, Sverre Rabbelier,
	Sverre Rabbelier, David Symonds, Marek Zawirski, Shawn O. Pearce,
	Miklos Vajna, Johannes Schindelin, Stephan Beyer,
	Christian Couder, Daniel Barkalow
In-Reply-To: <4872E858.4080607@gmail.com>

>From a quick an cursory glance I think we are well on track with the
Gitweb stuff (though I'll let Lea do a full status update).  It looks
like most of the code is in place, and there is a test site up and
running on one of the kernel.org machines (though not anywhere near
production yet - I still need to port some of my other changes to Lea's
change set so we can try this out for real on Kernel.org)

- John 'Warthog9' Hawley

On Tue, 2008-07-08 at 06:08 +0200, Lea Wiemann wrote:
> Jakub Narebski wrote:
> > 3. Gitweb caching
> 
> I'll post a complete status update in the next few days.  And three
> large patches (including the mechanize test). ;-)
> 
> > Lea has chosen caching data and memcached as primary caching engine,
> > and is working on object layer on top of Git.pm, namely Git::Repo and 
> > friends, which will be used by gitweb.  If I understand correctly 
> > caching is to be done, or at least helped by this layer.
> 
> That's correct, except that I'm not using Git.pm anywhere; Git::Repo is
> independent of Git.pm.  More about that later...
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: stgit error on status command
From: Karl Hasselström @ 2008-07-08  7:02 UTC (permalink / raw)
  To: Jon Smirl; +Cc: Git Mailing List, Catalin Marinas
In-Reply-To: <9e4733910807070810i14c8856vfbec24127e711285@mail.gmail.com>

Thanks for the report.

The problem is that we mis-parse the output of git diff-index when
rename detection is on (and it prints more than one filename on one
line). This happens if you give stg status the --diff-opts=-M flag,
but you didn't -- but it could also happen if you have the
stgit.diff-opts config variable set.

I'll try to get a patch out tonight. In the mean time, if you like you
should be able to work around the problem by unsetting
stgit.diff-opts.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle

^ permalink raw reply

* Re: Cloning marks pack for .keep
From: Teemu Likonen @ 2008-07-08  6:57 UTC (permalink / raw)
  To: Shawn O. Pearce
  Cc: Jean-Luc Herren, Junio C Hamano, Git Mailing List,
	Daniel Barkalow
In-Reply-To: <20080708044606.GC2542@spearce.org>

Shawn O. Pearce wrote (2008-07-08 04:46 +0000):

> Jean-Luc Herren <jlh@gmx.ch> wrote:
> > After cloning a local repository with "git clone file://...", the
> > resulting repo had one big pack file, as expected, but also
> > a matching ".keep" file.  Certainly this is a bug, isn't it?  The
> > same happens if I clone git.git.  I used git 1.5.6.1 but observed
> > the same with the current master.  I bisected this behavior to
> > commit fa740529 by Shawn O. Pearce (CC'ing him).  Since this dates
> > back to 2007, I wonder if maybe only I am seeing this, but I cannot
> > think of any reason for it.
> 
> This is a known issue to me; I have been seeing this behavior myself
> since probably fa74 hit next.  I just don't clone often so I've never
> thought about it much.  ;-)

Earlier I noticed that this issue sometimes causes the repository to
grow pretty much after "git gc" because .keep packs are not touched.
This was discussed two months ago:

http://thread.gmane.org/gmane.comp.version-control.git/81856

I tend to think that .keep packs are not good idea at least for small
and medium sized repos. For _really_ big ones I'm not sure.

^ permalink raw reply

* What should "git branch --merged master" do?
From: Junio C Hamano @ 2008-07-08  6:49 UTC (permalink / raw)
  To: git; +Cc: Pierre Habouzit, Lars Hjemli

e8b404c (git-branch: add support for --merged and --no-merged, 2008-04-17)
introduced "git branch --merged" to show the branches that are contained
within the current HEAD.  I.e. the ones that you can say "git branch -d"
and do not have to say "-D" to delete.

Currently "git branch --merged master" fails with a message:

	fatal: A branch named 'master' already exists.

but --merged and its opposite --no-merged are in spirit very similar to
another option --contains in that it is meant to be used as a way to
influence which branches are _reported_, and not about creating a new
branch.

Perhaps we should change them to default to HEAD (to retain the current
behaviour) but take a commit, and show branches that are merged to the
commit or not yet fully merged to the commit, respectively?

Incidentally, "git branch --with" fails without the mandatory commit
argument, and if we are going to do the above, we probably should default
the argument to HEAD as well.

Here is an attempt to update --with but I am not happy with it.

The patch makes

	$ git branch --contains

work as expected, but breaks

	$ git branch --contains master

You need to spell "git branch --contains=master" instead.  Which means it
is a regression and cannot be applied.  I suspect I am not using
parse_options() in an optimal way, but I did not find a way for my
callback to tell "I consumed the next parameter and it was mine" or "The
next one is not my optional parameter" back to the parse_options(), so
probably parse_options() need to be fixed to update this without
regression, I suspect.

 builtin-branch.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index d279702..ee722a2 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -411,7 +411,7 @@ static int opt_parse_with_commit(const struct option *opt, const char *arg, int
 	struct commit *commit;
 
 	if (!arg)
-		return -1;
+		arg = "HEAD";
 	if (get_sha1(arg, sha1))
 		die("malformed object name %s", arg);
 	commit = lookup_commit_reference(sha1);
@@ -438,13 +438,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_BOOLEAN( 0 , "color",  &branch_use_color, "use colored output"),
 		OPT_SET_INT('r', NULL,     &kinds, "act on remote-tracking branches",
 			REF_REMOTE_BRANCH),
-		OPT_CALLBACK(0, "contains", &with_commit, "commit",
-			     "print only branches that contain the commit",
-			     opt_parse_with_commit),
+		{
+			OPTION_CALLBACK, 0, "contains", &with_commit, "commit",
+			"print only branches that contain the commit",
+			PARSE_OPT_OPTARG, opt_parse_with_commit,
+		},
 		{
 			OPTION_CALLBACK, 0, "with", &with_commit, "commit",
 			"print only branches that contain the commit",
-			PARSE_OPT_HIDDEN, opt_parse_with_commit,
+			PARSE_OPT_HIDDEN | PARSE_OPT_OPTARG, opt_parse_with_commit,
 		},
 		OPT__ABBREV(&abbrev),
 

^ permalink raw reply related

* Re: uninstalling Git
From: Matthieu Moy @ 2008-07-08  6:28 UTC (permalink / raw)
  To: Eric Hanchrow; +Cc: git
In-Reply-To: <87od59c58s.fsf@offby1.atm01.sea.blarg.net>

Eric Hanchrow <offby1@blarg.net> writes:

> * Failing that, I run
>
>         find /usr -printf '%T@ ' -ls | sort -n | cut -d\  -f 2-
>

I usualy do

touch /tmp/dumb.txt
make install
find /usr -newer /tmp/dumb.txt

for that.

-- 
Matthieu

^ permalink raw reply

* Re: [PATCH] fix "git-submodule add a/b/c/repository"
From: Junio C Hamano @ 2008-07-08  6:02 UTC (permalink / raw)
  To: Mark Levedahl; +Cc: Sylvain Joyeux, Lars Hjemli, Ping Yin, git
In-Reply-To: <4872DE54.5010804@gmail.com>

Mark Levedahl <mlevedahl@gmail.com> writes:

> Junio C Hamano wrote:
>> I'd like to hear clarifications on two counts, please?
>>  (1) If Sylvain wanted to have that appear at dir0/dir1/init not init,
>>      would it have been sufficient to give that path twice (once for
>>      <repository> and another for <path> parameter) to make things work as
>>      expected?
>>
> git-submodule really requires two arguments:
>
>    $ git submodule add <URL> <relative-path-to-module-in-tree>
>
> and supports two modes:
>
> 1) relative-path exists and is a valid repo: just add the module, it
> was created in tree, the user is expected to eventually push this to
> the given URL so other users will get this as normal. This exists to
> simplify the process of creating a repo to begin with.
>
> 2) relative-path doesn't exist: clone from the URL. This is the normal use.
> submodule supports adding a module in one of two ways:
>
> So,
>
>    $ git submodule add   dir0/dir1/init   dir0/dir1/init
>
> will add the repo, but also makes the repo its own origin. I don't
> think this makes sense.
>>  (2) Is it generally considered a sane use case to specify an existing
>>      repository inside the working tree of a superproject as a submodule
>>      using "git submodule add" like Sylvain's example did?
>>
>>      I would have understood if the command were "git add dir0/dir1/init",
>>      but I have this vague recolleciton that "git submodule add" is about
>>      telling our repository about a submodule that comes from _outside_.
>>
>>
>>
> Adding an existing in-tree repo, ala
>
> $ git submodule add <intended-URL> <path>
>
> is there to ease the initial creation of a submodule. It can be
> created and registered in-tree, and later pushed to the server. This
> is sane, but is not the normal usage (makes sense only on creation).

Thanks.

The above is quite a bit more information than I can read from
Documentation/git-submodule.txt; care to send it in in a patch form?

^ permalink raw reply

* Re: git push to amazon s3 [was: [GSoC] What is status of Git's Google Summer of Code 2008 projects?]
From: Mike Hommey @ 2008-07-08  5:56 UTC (permalink / raw)
  To: Tarmigan
  Cc: Shawn O. Pearce, Marek Zawirski, git, Daniel Barkalow,
	Nick Hengeveld, Johannes Schindelin
In-Reply-To: <905315640807072248w44ccdc4y2f1cf54a10c50c43@mail.gmail.com>

On Mon, Jul 07, 2008 at 10:48:59PM -0700, Tarmigan wrote:
> (trimmed cc list to folks who've touched http-push.c)
> 
> On Mon, Jul 7, 2008 at 9:19 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Using Marek's pack generation code I added support for push over
> > the dumb sftp:// and amazon-s3:// protocols, with the latter also
> > supporting transparent client side encryption.
> >
> > I chose to add these features to jgit partly as an exercise to prove
> > that Marek's code was built well enough to be reused for this task,
> > partly because I wanted to backup some private personal repositories
> > to Amazon S3, and partly to prove that multiple dumb transports
> > could implement push support.
> 
> That sounds cool.  I've been looking into adding s3 push into cgit,
> and was looking into modifying http-push.c, but got in over my head.
> I had trouble trying to make it fit into the DAV model that http-push
> is built around, in part because s3 doesn't seem to support any
> locking and a lot of the http-push code seems to be around the
> locking.
> 
> Can you describe the s3 support that you added?  Did you do any
> locking when you pushed?  The objects and packs seem likely to be
> naturally OK, but I was worried about refs/ and especially
> objects/info/packs and info/refs (fetch over http works currently out
> of the box with publicly accessable s3 repos).

FWIW, I'm starting to work again on the http backend overhaul. My idea
is to provide a generic dumb protocol vfs-like interface, so that other
dumb protocols could be built out of it.

Mike

^ permalink raw reply

* git push to amazon s3 [was: [GSoC] What is status of Git's Google Summer of Code 2008 projects?]
From: Tarmigan @ 2008-07-08  5:48 UTC (permalink / raw)
  To: Shawn O. Pearce
  Cc: Marek Zawirski, git, Daniel Barkalow, Nick Hengeveld,
	Johannes Schindelin

(trimmed cc list to folks who've touched http-push.c)

On Mon, Jul 7, 2008 at 9:19 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Using Marek's pack generation code I added support for push over
> the dumb sftp:// and amazon-s3:// protocols, with the latter also
> supporting transparent client side encryption.
>
> I chose to add these features to jgit partly as an exercise to prove
> that Marek's code was built well enough to be reused for this task,
> partly because I wanted to backup some private personal repositories
> to Amazon S3, and partly to prove that multiple dumb transports
> could implement push support.

That sounds cool.  I've been looking into adding s3 push into cgit,
and was looking into modifying http-push.c, but got in over my head.
I had trouble trying to make it fit into the DAV model that http-push
is built around, in part because s3 doesn't seem to support any
locking and a lot of the http-push code seems to be around the
locking.

Can you describe the s3 support that you added?  Did you do any
locking when you pushed?  The objects and packs seem likely to be
naturally OK, but I was worried about refs/ and especially
objects/info/packs and info/refs (fetch over http works currently out
of the box with publicly accessable s3 repos).

Thanks,
Tarmigan

PS For anyone else who's interested, here's some instructions on how I
got started with s3 and git:

Start by creating an amazon s3 account

Next download and install "aws" from http://timkay.com/aws/
Set it up and install your amazon keys as specified.

# I setup a bucket named git_test.
s3mkdir git_test

#  Run this script to upload a git repo to amazon (run
update-server-info first):
#!/bin/bash
for i in $(tree -fi --noreport git_test_orig.git) ; do
    #exclude directories
    if [ ! -d $i ] ; then
        echo $i
        s3put "x-amz-acl: public-read" git_test/ $i
    fi
done

# Then you can clone (really, feel free to clone from this url.  It
should just work):
git clone http://s3.amazonaws.com/git_test/git_test_orig.git

# This experimenting on the git.git repo will set you back about US$0.05

^ permalink raw reply

* Re: PATCH] Documentation: Tweak use case in "git stash save --keep-index"
From: Junio C Hamano @ 2008-07-08  5:39 UTC (permalink / raw)
  To: Eric Raible; +Cc: Git Mailing List, Junio C Hamano, szeder
In-Reply-To: <279b37b20807072218o19dabd97y2c4edc62fb980ca4@mail.gmail.com>

"Eric Raible" <raible@gmail.com> writes:

> The documentation suggests using "git stash apply" in the
> --keep-index workflow even though doing so will lead to clutter
> in the stash.  And given that the changes are about to be
> committed anyway "git stash pop" is more sensible.

Yeah, I was pondering about this myself.  After popping the remaining
part, you would "git add -p" the next batch, the same "stash save -k-i" to
save the remaining bits away, and continue.  Will queue.

BUT

It is very likely that in this workflow you would sometimes find that what
you staged (and left in the working tree after "save -k-i") is faulty and
you need to tweak it in place to make it into a good enough shape for
committing.  The example probably should talk about what happens.
Editing, testing and committing is fine, but then what?  Will the "pop"
wipe that unplanned change you made after "save -k-i" out? (the answer is
no and this is safe, but the reader of the documentaiton needs it
explained)

Also this may be a good way to split an existing commit into five during a
"rebase -i" session, and the example in the documentation might want to
talk about it in that larger picture.

>  Documentation/git-stash.txt |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index df26901..bf241da 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -201,9 +201,10 @@ $ git add --patch foo
>  $ git stash save --keep-index
>  $ build && run tests
>  $ git commit -m 'First part'
> -$ git stash apply
> +$ git stash pop
> +... repeat above five steps until one commit remains ...
>  $ build && run tests
> -$ git commit -a -m 'Second part'
> +$ git commit foo -m 'Remaining parts'
>  ----------------------------------------------------------------

^ permalink raw reply

* Re: uninstalling Git
From: Eric Hanchrow @ 2008-07-08  5:30 UTC (permalink / raw)
  To: git
In-Reply-To: <4872F3B0.6050406@jaeger.mine.nu>

>>>>> "Christian" == Christian Jaeger <christian@jaeger.mine.nu> writes:

    Christian> I wonder why there's no "uninstall" make target.  

This is a problem with lots of packages, not just git.  Here are two
things I do:
            
* When I remember, I use "gnu stow" to manage installations; doing so
  makes uninstallation easy.

* Failing that, I run

        find /usr -printf '%T@ ' -ls | sort -n | cut -d\  -f 2-

  and assume that all the installed files have the same last-modified
  timestamps -- once you've found one of them in that pipeline's output,
  you've found them all, since they'll all be adjacent.

-- 
Asking the Iraqi people to assume Saddam's debts
is rather like telling a man who has been shot in the head
that he has to pay for the bullet.
        -- James Surowiecki

^ permalink raw reply

* Re: [PATCH] completion: add branch options --contains --merged --no-merged
From: Junio C Hamano @ 2008-07-08  5:30 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Eric Raible, Git Mailing List, szeder
In-Reply-To: <20080708044922.GD2542@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Eric Raible <raible@gmail.com> wrote:
>> Signed-off-by: Eric Raible <raible@gmail.com>
>
> Trivially-Acked-by: Shawn O. Pearce <spearce@spearce.org>
>
> ;-)
>
> More completion support that probably should go to maint, as the
> functionality in git-branch is in 1.5.6 but we (again) forgot to
> make sure the completion was up-to-date prior to release.

I am actually getting more worried about completion code getting larger
and larger without its performance impact not being looked at nor
addressed adequately.  In my regular working tree:

	$ echo Docu<TAB>

completes "mentation/" instantly, but:

	$ git log -- Docu<TAB>

takes about 1.5 to 2 seconds to complete the same.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox