What's cooking in git.git (Aug 2010, #01; Wed, 4)

What's cooking in git.git (Aug 2010, #01; Wed, 4)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' while commits prefixed with '+' are in 'next'.  The ones
marked with '.' do not appear in any of the integration branches, but I am
still holding onto them.

The tip of 'next' has been rewound and rebuilt on top of v1.7.2; also a
few topics have been ejected as I've been warning to.  I'll start merging
more stuff to 'next' in the next round after reading them over again.

--------------------------------------------------
[Graduated to "master"]

* gp/pack-refs-remove-empty-dirs (2010-07-06) 1 commit
  (merged to 'next' on 2010-07-14 at 7d25131)
 + pack-refs: remove newly empty directories

* pt/git-gui (2010-08-02) 13 commits
 + Merge git://repo.or.cz/git-gui into pt/git-gui
 + git-gui: fix size and position of window panes on startup
 + git-gui: mc cannot be used before msgcat has been loaded
 + git-gui: use textconv filter for diff and blame
 + git-gui: Avoid using the <<Copy>> binding as a menu accelerator on win32
 + git-gui: fix shortcut creation on cygwin
 + git-gui: fix PATH environment for mingw development environment
 + git-gui: fix usage of _gitworktree when creating shortcut for windows
 + git-gui: fix "Explore Working Copy" for Windows again
 + git-gui: fix usage of themed widgets variable
 + git-gui: Handle failure of core.worktree to identify the working directory.
 + Merge branch 'maint'
 + git-gui: check whether systems nice command works or disable it

* rr/svn-fe (2010-07-29) 2 commits
 + contrib/svn-fe: Add the svn-fe target to .gitignore
 + contrib/svn-fe: Fix IncludePath

* sv/maint-diff-q-clear-fix (2010-08-02) 1 commit
 + Fix DIFF_QUEUE_CLEAR refactoring

--------------------------------------------------
[New Topics]

* jc/sha1-name-find-fix (2010-08-02) 1 commit
 - sha1_name.c: fix parsing of ":/token" syntax

* jn/doc-pull (2010-08-02) 1 commit
 - Documentation: flesh out “git pull” description

* jn/maint-gitweb-dynconf (2010-07-30) 1 commit
 - gitweb: allow configurations that change with each request

* sr/local-config (2010-08-03) 1 commit
 - config: add --local option

--------------------------------------------------
[Stalled]

* zl/mailinfo-recode-patch (2010-06-14) 2 commits
 - add --recode-patch option to git-am
 - add --recode-patch option to git-mailinfo

I recall there was another round of re-roll planned for this one.

* jk/tag-contains (2010-07-05) 4 commits
 - Why is "git tag --contains" so slow?
 - default core.clockskew variable to one day
 - limit "contains" traversals based on commit timestamp
 - tag: speed up --contains calculation

--------------------------------------------------
[Cooking]

* ab/test-coverage (2010-07-26) 8 commits
 - Makefile: make gcov invocation configurable
 - t/README: Add a note about the dangers of coverage chasing
 - t/README: A new section about test coverage
 - Makefile: Add cover_db_html target
 - Makefile: Add cover_db target
 - Makefile: Split out the untested functions target
 - Makefile: Include subdirectories in "make cover" reports
 - gitignore: Ignore files generated by "make coverage"

* ab/test-no-skip (2010-07-28) 5 commits
 - t/README: Update "Skipping tests" to align with best practices
 - t/t7800-difftool.sh: Skip with prereq on no PERL
 - t/t5800-remote-helpers.sh: Skip with prereq on python <2.4
 - t/t4004-diff-rename-symlink.sh: use three-arg <prereq>
 - tests: implicitly skip SYMLINKS tests using <prereq>

* bc/use-more-hardlinks-in-install (2010-07-23) 2 commits
 - Makefile: make hard/symbolic links for non-builtins too
 - Makefile: link builtins residing in bin directory to main git binary too

* cc/find-commit-subject (2010-07-22) 6 commits
 - blame: use find_commit_subject() instead of custom code
 - merge-recursive: use find_commit_subject() instead of custom code
 - bisect: use find_commit_subject() instead of custom code
 - revert: rename variables related to subject in get_message()
 - revert: refactor code to find commit subject in find_commit_subject()
 - revert: fix off by one read when searching the end of a commit subject

* gb/shell-ext (2010-07-28) 3 commits
 - Add sample commands for git-shell
 - Add interactive mode to git-shell for user-friendliness
 - Allow creation of arbitrary git-shell commands

* jc/log-grep (2010-07-19) 1 commit
 - git log: add -G<regexp> that greps in the patch text

* jh/clean-exclude (2010-07-20) 2 commits
 - Add test for git clean -e.
 - Add -e/--exclude to git-clean.

* jh/use-test-must-fail (2010-07-20) 1 commit
 - Convert "! git" to "test_must_fail git"

* jn/apply-filename-with-sp (2010-07-23) 4 commits
 - apply: Handle traditional patches with space in filename
 - t4135 (apply): use expand instead of pr for portability
 - tests: Test how well "git apply" copes with weird filenames
 - apply: Split quoted filename handling into new function

* jn/fix-abbrev (2010-07-27) 3 commits
 - examples/commit: use --abbrev for commit summary
 - checkout, commit: remove confusing assignments to rev.abbrev
 - archive: abbreviate substituted commit ids again

* jn/maint-setup-fix (2010-07-24) 11 commits
 - setup: split off a function to handle ordinary .git directories
 - Revert "rehabilitate 'git index-pack' inside the object store"
 - setup: do not forget working dir from subdir of gitdir
 - t4111 (apply): refresh index before applying patches to it
 - setup: split off get_device_or_die helper
 - setup: split off a function to handle hitting ceiling in repo search
 - setup: split off code to handle stumbling upon a repository
 - setup: split off a function to checks working dir for .git file
 - setup: split off $GIT_DIR-set case from setup_git_directory_gently
 - tests: try git apply from subdir of toplevel
 - t1501 (rev-parse): clarify

* jn/rebase-rename-am (2008-11-10) 5 commits
 - rebase: protect against diff.renames configuration
 - t3400 (rebase): whitespace cleanup
 - Teach "apply --index-info" to handle rename patches
 - t4150 (am): futureproof against failing tests
 - t4150 (am): style fix

* ml/rebase-x-strategy (2010-07-29) 1 commit
 - rebase: support -X to pass through strategy options

* mm/shortopt-detached (2010-07-30) 5 commits
 - log: parse detached option for --glob
 - log: parse detached options like git log --grep foo
 - diff: parse detached options --stat-width n, --stat-name-width n
 - diff: split off a function for --stat-* option parsing
 - diff: parse detached options like -S foo

* nd/fix-sparse-checkout (2010-07-31) 5 commits
 - unpack-trees: mark new entries skip-worktree appropriately
 - unpack-trees: do not check for conflict entries too early
 - unpack-trees: let read-tree -u remove index entries outside sparse area
 - unpack-trees: only clear CE_UPDATE|CE_REMOVE when skip-worktree is always set
 - t1011 (sparse checkout): style nitpicks

* tr/ab-i18n-fix (2010-07-25) 1 commit
 - tests: locate i18n lib&data correctly under --valgrind
 (this branch uses ab/i18n.)

* tr/maint-no-unquote-plus (2010-07-24) 1 commit
 - Do not unquote + into ' ' in URLs

* tr/xsize-bits (2010-07-28) 1 commit
 - xsize_t: check whether we lose bits

* vs/doc-spell (2010-07-20) 1 commit
 - Documentation: spelling fixes

* ab/report-corrupt-object-with-type (2010-06-10) 1 commit
 - sha1_file: Show the the type and path to corrupt objects

* cc/revert (2010-07-21) 5 commits
 - t3508: add check_head_differs_from() helper function and use it
 - revert: improve success message by adding abbreviated commit sha1
 - revert: don't print "Finished one cherry-pick." if commit failed
 - revert: refactor commit code into a new run_git_commit() function
 - revert: report success when using option --strategy

* en/fast-export-fix (2010-07-17) 2 commits
 - fast-export: Add a --full-tree option
 - fast-export: Fix dropping of files with --import-marks and path limiting

* jn/parse-date-basic (2010-07-15) 1 commit
 - Export parse_date_basic() to convert a date string to timestamp
 (this branch is used by rr/svn-export.)

* kf/post-receive-sample-hook (2010-07-16) 1 commit
 - post-receive-email: optional message line count limit

* tr/rfc-reset-doc (2010-07-18) 5 commits
 - Documentation/reset: move "undo permanently" example behind "make topic"
 - Documentation/reset: reorder examples to match description
 - Documentation/reset: promote 'examples' one section up
 - Documentation/reset: separate options by mode
 - Documentation/git-reset: reorder modes for soft-mixed-hard progression

* rr/svn-export (2010-07-29) 9 commits
 - vcs-svn: Remove stray calls to removed functions
 - Add SVN dump parser
 - Add infrastructure to write revisions in fast-export format
 - Add stream helper library
 - Add string-specific memory pool
 - vcs-svn: treap_search should return NULL for missing items
 - Add treap implementation
 - Add memory pool library
 - Introduce vcs-svn lib
 (this branch uses jn/parse-date-basic.)

* hv/autosquash-config (2010-07-14) 1 commit
 - add configuration variable for --autosquash option of interactive rebase

* jh/graph-next-line (2010-07-13) 2 commits
 - Enable custom schemes for column colors in the graph API
 - Make graph_next_line() available in the graph.h API

* ar/string-list-foreach (2010-07-03) 2 commits
 - Convert the users of for_each_string_list to for_each_string_list_item macro
 - Add a for_each_string_list_item macro
 (this branch is used by tf/string-list-init.)

* il/rfc-remote-fd-ext (2010-07-31) 4 commits
 - Rewrite bidirectional traffic loop
 - gitignore: Ignore the new /git-remote-{ext,fd} helpers
 - New remote helper: git-remote-ext
 - New remote helper git-remote-fd

* hv/submodule-find-ff-merge (2010-07-07) 3 commits
 - Implement automatic fast-forward merge for submodules
 - setup_revisions(): Allow walking history in a submodule
 - Teach ref iteration module about submodules

* jn/fast-import-subtree (2010-06-30) 1 commit
 - Teach fast-import to import subtrees named by tree id

* sg/rerere-gc-old-still-used (2010-07-13) 2 commits
 - rerere: fix overeager gc
 - mingw_utime(): handle NULL times parameter

* tf/string-list-init (2010-07-04) 1 commit
 - string_list: Add STRING_LIST_INIT macro and make use of it.
 (this branch uses ar/string-list-foreach.)

* en/d-f-conflict-fix (2010-07-27) 7 commits
  (merged to 'next' on 2010-08-03 at 7f78604)
 + t/t6035-merge-dir-to-symlink.sh: Remove TODO on passing test
 + fast-import: Improve robustness when D->F changes provided in wrong order
 + fast-export: Fix output order of D/F changes
 + merge_recursive: Fix renames across paths below D/F conflicts
 + merge-recursive: Fix D/F conflicts
 + Add a rename + D/F conflict testcase
 + Add additional testcases for D/F conflicts

* ab/i18n (2010-07-19) 2 commits
 - tests: rename test to work around GNU gettext bug
 - Add infrastructure for translating Git with gettext
 (this branch is used by tr/ab-i18n-fix.)

* tc/checkout-B (2010-06-24) 3 commits
 - builtin/checkout: learn -B
 - builtin/checkout: reword hint for -b
 - add tests for checkout -b

* eb/double-convert-before-merge (2010-07-02) 3 commits
 - Don't expand CRLFs when normalizing text during merge
 - Try normalizing files to avoid delete/modify conflicts when merging
 - Avoid conflicts when merging branches with mixed normalization

jk/tag-contains: stalled

[Ted Ts'o]

On Wed, Aug 04, 2010 at 03:24:23PM -0700, Junio C Hamano wrote:
> 
> * jk/tag-contains (2010-07-05) 4 commits
>  - Why is "git tag --contains" so slow?
>  - default core.clockskew variable to one day
>  - limit "contains" traversals based on commit timestamp
>  - tag: speed up --contains calculation

What needs to be fixed up before this effort can graduate?  I find the
fixups here to be really helpful, even without the automated skew
detection that has been proposed.  And even if we fix the root problem
with some new all-singing pack format, I suspect that may be a ways
out, so it would be nice if these patches could get included for now....

     	   	    	    	  	  	- Ted

tc/checkout-B

[Jonathan Nieder]

Junio C Hamano wrote:

> * tc/checkout-B (2010-06-24) 3 commits
>  - builtin/checkout: learn -B
>  - builtin/checkout: reword hint for -b
>  - add tests for checkout -b

I keep on trying to use this option and then remembering it wasn’t merged
yet.  So for what it’s worth, I like it.

Re: What's cooking in git.git (Aug 2010, #01; Wed, 4)

[Matthieu Moy]

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

> * mm/shortopt-detached (2010-07-30) 5 commits
>  - log: parse detached option for --glob
>  - log: parse detached options like git log --grep foo
>  - diff: parse detached options --stat-width n, --stat-name-width n
>  - diff: split off a function for --stat-* option parsing
>  - diff: parse detached options like -S foo

I'm pretty sure I had send a new version of this one, but judging from
the mailing list's archive, I guess I mis-sent it and it never went
through ;-).

Here's a new one, with very minor revisions :

* diff_long_opt renamed to parse_long_opt

* Use the same wording as api-parse-options.txt : separate/sticked
  forms (essentially in commit messages and comments).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[PATCH 1/5] diff: parse separate options like -S foo

[Matthieu Moy]

Change the option parsing logic in revision.c to accept separate forms
like `-S foo' in addition to `-Sfoo'. The rest of git already accepted
this form, but revision.c still used its own option parsing.

Short options affected are -S<string>, -l<num> and -O<orderfile>, for
which an empty string wouldn't make sense, hence -<option> <arg> isn't
ambiguous.

This patch does not handle --stat-name-width and --stat-width, which are
special-cases where diff_long_opt do not apply. They are handled in a
separate patch to ease review.

Original patch by Matthieu Moy, plus refactoring by Jonathan Nieder.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c                       |   87 ++++++++++++++++++++++++++++++++++--------
 diff.h                       |    7 +++
 t/t4013-diff-various.sh      |    5 ++
 t/t4013/diff.log_-S_F_master |    7 +++
 t/t4202-log.sh               |   12 ++---
 5 files changed, 95 insertions(+), 23 deletions(-)
 create mode 100644 t/t4013/diff.log_-S_F_master

diff --git a/diff.c b/diff.c
index 17873f3..bc8fa8e 100644
--- a/diff.c
+++ b/diff.c
@@ -2990,9 +2990,50 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
 
 static int diff_scoreopt_parse(const char *opt);
 
+static inline int short_opt(char opt, const char **argv,
+			    const char **optarg)
+{
+	const char *arg = argv[0];
+	if (arg[0] != '-' || arg[1] != opt)
+		return 0;
+	if (arg[2] != '\0') {
+		*optarg = arg + 2;
+		return 1;
+	}
+	if (!argv[1])
+		die("Option '%c' requires a value", opt);
+	*optarg = argv[1];
+	return 2;
+}
+
+int parse_long_opt(const char *opt, const char **argv,
+		   const char **optarg)
+{
+	const char *arg = argv[0];
+	if (arg[0] != '-' || arg[1] != '-')
+		return 0;
+	arg += strlen("--");
+	if (prefixcmp(arg, opt))
+		return 0;
+	arg += strlen(opt);
+	if (*arg == '=') { /* sticked form: --option=value */
+		*optarg = arg + 1;
+		return 1;
+	}
+	if (*arg != '\0')
+		return 0;
+	/* separate form: --option value */
+	if (!argv[1])
+		die("Option '--%s' requires a value", opt);
+	*optarg = argv[1];
+	return 2;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];
+	const char *optarg;
+	int argcount;
 
 	/* Output format options */
 	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch"))
@@ -3149,10 +3190,11 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		else
 			die("bad --word-diff argument: %s", type);
 	}
-	else if (!prefixcmp(arg, "--word-diff-regex=")) {
+	else if ((argcount = parse_long_opt("word-diff-regex", av, &optarg))) {
 		if (options->word_diff == DIFF_WORDS_NONE)
 			options->word_diff = DIFF_WORDS_PLAIN;
-		options->word_regex = arg + 18;
+		options->word_regex = optarg;
+		return argcount;
 	}
 	else if (!strcmp(arg, "--exit-code"))
 		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
@@ -3180,18 +3222,26 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	/* misc options */
 	else if (!strcmp(arg, "-z"))
 		options->line_termination = 0;
-	else if (!prefixcmp(arg, "-l"))
-		options->rename_limit = strtoul(arg+2, NULL, 10);
-	else if (!prefixcmp(arg, "-S"))
-		options->pickaxe = arg + 2;
+	else if ((argcount = short_opt('l', av, &optarg))) {
+		options->rename_limit = strtoul(optarg, NULL, 10);
+		return argcount;
+	}
+	else if ((argcount = short_opt('S', av, &optarg))) {
+		options->pickaxe = optarg;
+		return argcount;
+	}
 	else if (!strcmp(arg, "--pickaxe-all"))
 		options->pickaxe_opts = DIFF_PICKAXE_ALL;
 	else if (!strcmp(arg, "--pickaxe-regex"))
 		options->pickaxe_opts = DIFF_PICKAXE_REGEX;
-	else if (!prefixcmp(arg, "-O"))
-		options->orderfile = arg + 2;
-	else if (!prefixcmp(arg, "--diff-filter="))
-		options->filter = arg + 14;
+	else if ((argcount = short_opt('O', av, &optarg))) {
+		options->orderfile = optarg;
+		return argcount;
+	}
+	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
+		options->filter = optarg;
+		return argcount;
+	}
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
 	else if (!prefixcmp(arg, "--abbrev=")) {
@@ -3201,20 +3251,25 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		else if (40 < options->abbrev)
 			options->abbrev = 40;
 	}
-	else if (!prefixcmp(arg, "--src-prefix="))
-		options->a_prefix = arg + 13;
-	else if (!prefixcmp(arg, "--dst-prefix="))
-		options->b_prefix = arg + 13;
+	else if ((argcount = parse_long_opt("src-prefix", av, &optarg))) {
+		options->a_prefix = optarg;
+		return argcount;
+	}
+	else if ((argcount = parse_long_opt("dst-prefix", av, &optarg))) {
+		options->b_prefix = optarg;
+		return argcount;
+	}
 	else if (!strcmp(arg, "--no-prefix"))
 		options->a_prefix = options->b_prefix = "";
 	else if (opt_arg(arg, '\0', "inter-hunk-context",
 			 &options->interhunkcontext))
 		;
-	else if (!prefixcmp(arg, "--output=")) {
-		options->file = fopen(arg + strlen("--output="), "w");
+	else if ((argcount = parse_long_opt("output", av, &optarg))) {
+		options->file = fopen(optarg, "w");
 		if (!options->file)
 			die_errno("Could not open '%s'", arg + strlen("--output="));
 		options->close_file = 1;
+		return argcount;
 	} else
 		return 0;
 	return 1;
diff --git a/diff.h b/diff.h
index 063d10a..2b94ad6 100644
--- a/diff.h
+++ b/diff.h
@@ -214,6 +214,13 @@ extern void diff_unmerge(struct diff_options *,
 #define DIFF_SETUP_USE_CACHE		2
 #define DIFF_SETUP_USE_SIZE_CACHE	4
 
+/*
+ * Poor man's alternative to parse-option, to allow both sticked form
+ * (--option=value) and separate form (--option value).
+ */
+extern int parse_long_opt(const char *opt, const char **argv,
+			 const char **optarg);
+
 extern int git_diff_basic_config(const char *var, const char *value, void *cb);
 extern int git_diff_ui_config(const char *var, const char *value, void *cb);
 extern int diff_use_color_default;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index dae6358..19857f4 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -208,6 +208,7 @@ log -p --first-parent master
 log -m -p --first-parent master
 log -m -p master
 log -SF master
+log -S F master
 log -SF -p master
 log --decorate --all
 log --decorate=full --all
@@ -282,4 +283,8 @@ diff master master^ side
 diff --dirstat master~1 master~2
 EOF
 
+test_expect_success 'log -S requires an argument' '
+	test_must_fail git log -S
+'
+
 test_done
diff --git a/t/t4013/diff.log_-S_F_master b/t/t4013/diff.log_-S_F_master
new file mode 100644
index 0000000..978d2b4
--- /dev/null
+++ b/t/t4013/diff.log_-S_F_master
@@ -0,0 +1,7 @@
+$ git log -S F master
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+$
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 2230e60..c1abd31 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -100,13 +100,11 @@ test_expect_success 'oneline' '
 
 test_expect_success 'diff-filter=A' '
 
-	actual=$(git log --pretty="format:%s" --diff-filter=A HEAD) &&
-	expect=$(echo fifth ; echo fourth ; echo third ; echo initial) &&
-	test "$actual" = "$expect" || {
-		echo Oops
-		echo "Actual: $actual"
-		false
-	}
+	git log --pretty="format:%s" --diff-filter=A HEAD > actual &&
+	git log --pretty="format:%s" --diff-filter A HEAD > actual-separate &&
+	printf "fifth\nfourth\nthird\ninitial" > expect &&
+	test_cmp expect actual &&
+	test_cmp expect actual-separate
 
 '
 
-- 
1.7.2.1.30.g18195

[PATCH 2/5] diff: split off a function for --stat-* option parsing

[Matthieu Moy]

From: Jonathan Nieder <jrnieder@gmail.com>

As an optimization, the diff_opt_parse() switchboard has
a single case for all the --stat-* options.  Split it
off into a separate function so we can enhance it
without bringing code dangerously close to the right
margin.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c |   62 +++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/diff.c b/diff.c
index bc8fa8e..a08a56a 100644
--- a/diff.c
+++ b/diff.c
@@ -3029,6 +3029,38 @@ int parse_long_opt(const char *opt, const char **argv,
 	return 2;
 }
 
+static int stat_opt(struct diff_options *options, const char **av)
+{
+	const char *arg = av[0];
+	char *end;
+	int width = options->stat_width;
+	int name_width = options->stat_name_width;
+
+	arg += strlen("--stat");
+	end = (char *)arg;
+
+	switch (*arg) {
+	case '-':
+		if (!prefixcmp(arg, "-width="))
+			width = strtoul(arg + 7, &end, 10);
+		else if (!prefixcmp(arg, "-name-width="))
+			name_width = strtoul(arg + 12, &end, 10);
+		break;
+	case '=':
+		width = strtoul(arg+1, &end, 10);
+		if (*end == ',')
+			name_width = strtoul(end+1, &end, 10);
+	}
+
+	/* Important! This checks all the error cases! */
+	if (*end)
+		return 0;
+	options->output_format |= DIFF_FORMAT_DIFFSTAT;
+	options->stat_name_width = name_width;
+	options->stat_width = width;
+	return 1;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];
@@ -3070,33 +3102,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_NAME_STATUS;
 	else if (!strcmp(arg, "-s"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
-	else if (!prefixcmp(arg, "--stat")) {
-		char *end;
-		int width = options->stat_width;
-		int name_width = options->stat_name_width;
-		arg += 6;
-		end = (char *)arg;
-
-		switch (*arg) {
-		case '-':
-			if (!prefixcmp(arg, "-width="))
-				width = strtoul(arg + 7, &end, 10);
-			else if (!prefixcmp(arg, "-name-width="))
-				name_width = strtoul(arg + 12, &end, 10);
-			break;
-		case '=':
-			width = strtoul(arg+1, &end, 10);
-			if (*end == ',')
-				name_width = strtoul(end+1, &end, 10);
-		}
-
-		/* Important! This checks all the error cases! */
-		if (*end)
-			return 0;
-		options->output_format |= DIFF_FORMAT_DIFFSTAT;
-		options->stat_name_width = name_width;
-		options->stat_width = width;
-	}
+	else if (!prefixcmp(arg, "--stat"))
+		/* --stat, --stat-width, or --stat-name-width */
+		return stat_opt(options, av);
 
 	/* renames options */
 	else if (!prefixcmp(arg, "-B")) {
-- 
1.7.2.1.30.g18195

[PATCH 3/5] diff: parse separate options --stat-width n, --stat-name-width n

[Matthieu Moy]

Part of a campaign for unstuck forms of options.

[jn: with some refactoring]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 diff.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index a08a56a..e98d59b 100644
--- a/diff.c
+++ b/diff.c
@@ -3035,16 +3035,34 @@ static int stat_opt(struct diff_options *options, const char **av)
 	char *end;
 	int width = options->stat_width;
 	int name_width = options->stat_name_width;
+	int argcount = 1;
 
 	arg += strlen("--stat");
 	end = (char *)arg;
 
 	switch (*arg) {
 	case '-':
-		if (!prefixcmp(arg, "-width="))
-			width = strtoul(arg + 7, &end, 10);
-		else if (!prefixcmp(arg, "-name-width="))
-			name_width = strtoul(arg + 12, &end, 10);
+		if (!prefixcmp(arg, "-width")) {
+			arg += strlen("-width");
+			if (*arg == '=')
+				width = strtoul(arg + 1, &end, 10);
+			else if (!*arg && !av[1])
+				die("Option '--stat-width' requires a value");
+			else if (!*arg) {
+				width = strtoul(av[1], &end, 10);
+				argcount = 2;
+			}
+		} else if (!prefixcmp(arg, "-name-width")) {
+			arg += strlen("-name-width");
+			if (*arg == '=')
+				name_width = strtoul(arg + 1, &end, 10);
+			else if (!*arg && !av[1])
+				die("Option '--stat-name-width' requires a value");
+			else if (!*arg) {
+				name_width = strtoul(av[1], &end, 10);
+				argcount = 2;
+			}
+		}
 		break;
 	case '=':
 		width = strtoul(arg+1, &end, 10);
@@ -3058,7 +3076,7 @@ static int stat_opt(struct diff_options *options, const char **av)
 	options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	options->stat_name_width = name_width;
 	options->stat_width = width;
-	return 1;
+	return argcount;
 }
 
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
-- 
1.7.2.1.30.g18195

[PATCH 4/5] log: parse separate options like git log --grep foo

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 revision.c     |   74 ++++++++++++++++++++++++++++++++++---------------------
 t/t4202-log.sh |    7 +++++
 2 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/revision.c b/revision.c
index 7e82efd..489a3c2 100644
--- a/revision.c
+++ b/revision.c
@@ -1148,6 +1148,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			       int *unkc, const char **unkv)
 {
 	const char *arg = argv[0];
+	const char *optarg;
+	int argcount;
 
 	/* pseudo revision arguments */
 	if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") ||
@@ -1160,11 +1162,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		return 1;
 	}
 
-	if (!prefixcmp(arg, "--max-count=")) {
-		revs->max_count = atoi(arg + 12);
+	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
+		revs->max_count = atoi(optarg);
 		revs->no_walk = 0;
-	} else if (!prefixcmp(arg, "--skip=")) {
-		revs->skip_count = atoi(arg + 7);
+		return argcount;
+	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
+		revs->skip_count = atoi(optarg);
+		return argcount;
 	} else if ((*arg == '-') && isdigit(arg[1])) {
 	/* accept -<digit>, like traditional "head" */
 		revs->max_count = atoi(arg + 1);
@@ -1178,18 +1182,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!prefixcmp(arg, "-n")) {
 		revs->max_count = atoi(arg + 2);
 		revs->no_walk = 0;
-	} 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 ((argcount = parse_long_opt("max-age", argv, &optarg))) {
+		revs->max_age = atoi(optarg);
+		return argcount;
+	} else if ((argcount = parse_long_opt("since", argv, &optarg))) {
+		revs->max_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = parse_long_opt("after", argv, &optarg))) {
+		revs->max_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = parse_long_opt("min-age", argv, &optarg))) {
+		revs->min_age = atoi(optarg);
+		return argcount;
+	} else if ((argcount = parse_long_opt("before", argv, &optarg))) {
+		revs->min_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = parse_long_opt("until", argv, &optarg))) {
+		revs->min_age = approxidate(optarg);
+		return argcount;
 	} else if (!strcmp(arg, "--first-parent")) {
 		revs->first_parent_only = 1;
 	} else if (!strcmp(arg, "--ancestry-path")) {
@@ -1295,6 +1305,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->pretty_given = 1;
 		get_commit_format(arg+8, revs);
 	} else if (!prefixcmp(arg, "--pretty=") || !prefixcmp(arg, "--format=")) {
+		/*
+		 * Detached form ("--pretty X" as opposed to "--pretty=X")
+		 * not allowed, since the argument is optional.
+		 */
 		revs->verbose_header = 1;
 		revs->pretty_given = 1;
 		get_commit_format(arg+9, revs);
@@ -1359,21 +1373,25 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--relative-date")) {
 		revs->date_mode = DATE_RELATIVE;
 		revs->date_mode_explicit = 1;
-	} else if (!strncmp(arg, "--date=", 7)) {
-		revs->date_mode = parse_date_format(arg + 7);
+	} else if ((argcount = parse_long_opt("date", argv, &optarg))) {
+		revs->date_mode = parse_date_format(optarg);
 		revs->date_mode_explicit = 1;
+		return argcount;
 	} else if (!strcmp(arg, "--log-size")) {
 		revs->show_log_size = 1;
 	}
 	/*
 	 * Grepping the commit log
 	 */
-	else if (!prefixcmp(arg, "--author=")) {
-		add_header_grep(revs, GREP_HEADER_AUTHOR, arg+9);
-	} else if (!prefixcmp(arg, "--committer=")) {
-		add_header_grep(revs, GREP_HEADER_COMMITTER, arg+12);
-	} else if (!prefixcmp(arg, "--grep=")) {
-		add_message_grep(revs, arg+7);
+	else if ((argcount = parse_long_opt("author", argv, &optarg))) {
+		add_header_grep(revs, GREP_HEADER_AUTHOR, optarg);
+		return argcount;
+	} else if ((argcount = parse_long_opt("committer", argv, &optarg))) {
+		add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
+		return argcount;
+	} else if ((argcount = parse_long_opt("grep", argv, &optarg))) {
+		add_message_grep(revs, optarg);
+		return argcount;
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
 		revs->grep_filter.regflags |= REG_EXTENDED;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
@@ -1382,12 +1400,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->grep_filter.fixed = 1;
 	} else if (!strcmp(arg, "--all-match")) {
 		revs->grep_filter.all_match = 1;
-	} else if (!prefixcmp(arg, "--encoding=")) {
-		arg += 11;
-		if (strcmp(arg, "none"))
-			git_log_output_encoding = xstrdup(arg);
+	} else if ((argcount = parse_long_opt("encoding", argv, &optarg))) {
+		if (strcmp(optarg, "none"))
+			git_log_output_encoding = xstrdup(optarg);
 		else
 			git_log_output_encoding = "";
+		return argcount;
 	} else if (!strcmp(arg, "--reverse")) {
 		revs->reverse ^= 1;
 	} else if (!strcmp(arg, "--children")) {
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index c1abd31..95ac3f8 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -201,6 +201,13 @@ test_expect_success 'log --grep' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log --grep option parsing' '
+	echo second >expect &&
+	git log -1 --pretty="tformat:%s" --grep sec >actual &&
+	test_cmp expect actual &&
+	test_must_fail git log -1 --pretty="tformat:%s" --grep
+'
+
 test_expect_success 'log -i --grep' '
 	echo Second >expect &&
 	git log -1 --pretty="tformat:%s" -i --grep=sec >actual &&
-- 
1.7.2.1.30.g18195

[PATCH 5/5] log: parse separate option for --glob

[Matthieu Moy]

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 revision.c               |    7 +++++--
 t/t6018-rev-list-glob.sh |    6 ++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index 489a3c2..f241f34 100644
--- a/revision.c
+++ b/revision.c
@@ -1484,6 +1484,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 {
 	int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0;
 	const char **prune_data = NULL;
+	const char *optarg;
+	int argcount;
 
 	/* First, search for "--" */
 	seen_dashdash = 0;
@@ -1530,10 +1532,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				handle_refs(revs, flags, for_each_remote_ref);
 				continue;
 			}
-			if (!prefixcmp(arg, "--glob=")) {
+			if ((argcount = parse_long_opt("glob", argv + i, &optarg))) {
 				struct all_refs_cb cb;
+				i += argcount - 1;
 				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref(handle_one_ref, arg + 7, &cb);
+				for_each_glob_ref(handle_one_ref, optarg, &cb);
 				continue;
 			}
 			if (!prefixcmp(arg, "--branches=")) {
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 58428d9..fb8291c 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -123,6 +123,12 @@ test_expect_success 'rev-list --glob=refs/heads/subspace/*' '
 
 '
 
+test_expect_success 'rev-list --glob refs/heads/subspace/*' '
+
+	compare rev-list "subspace/one subspace/two" "--glob refs/heads/subspace/*"
+
+'
+
 test_expect_success 'rev-list --glob=heads/subspace/*' '
 
 	compare rev-list "subspace/one subspace/two" "--glob=heads/subspace/*"
-- 
1.7.2.1.30.g18195

Re: tc/checkout-B

[Tay Ray Chuan]

Hi,

On Thu, Aug 5, 2010 at 2:53 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>
>> * tc/checkout-B (2010-06-24) 3 commits
>>  - builtin/checkout: learn -B
>>  - builtin/checkout: reword hint for -b
>>  - add tests for checkout -b
>
> I keep on trying to use this option and then remembering it wasn’t merged
> yet.  So for what it’s worth, I like it.

Thanks, glad to know this was useful to you.

-- 
Cheers,
Ray Chuan

mm/shortopt-detached

[Jonathan Nieder]

Matthieu Moy wrote:
> Junio C Hamano <gitster@pobox.com> writes:
 
>> * mm/shortopt-detached (2010-07-30) 5 commits
>>  - log: parse detached option for --glob
>>  - log: parse detached options like git log --grep foo
>>  - diff: parse detached options --stat-width n, --stat-name-width n
>>  - diff: split off a function for --stat-* option parsing
>>  - diff: parse detached options like -S foo
[...]
> Here's a new one, with very minor revisions :
> 
> * diff_long_opt renamed to parse_long_opt
> 
> * Use the same wording as api-parse-options.txt : separate/sticked
>   forms (essentially in commit messages and comments).

Still looks good to me.  Thanks.

Re: [PATCH 1/5] diff: parse separate options like -S foo

[Jakub Narebski]

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

> +static inline int short_opt(char opt, const char **argv,
> +			    const char **optarg)
[...]
> +int parse_long_opt(const char *opt, const char **argv,
> +		   const char **optarg)

Just a nitpick question: why it is 'parse_long_opt' but just 'short_opt'?

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH 1/5] diff: parse separate options like -S foo

[Matthieu Moy]

Jakub Narebski <jnareb@gmail.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> +static inline int short_opt(char opt, const char **argv,
>> +			    const char **optarg)
> [...]
>> +int parse_long_opt(const char *opt, const char **argv,
>> +		   const char **optarg)
>
> Just a nitpick question: why it is 'parse_long_opt' but just
> 'short_opt'?

I initially made *_long_opt prefixed with something (diff_ in my first
version) because it's global, while short_opt is static.
parse_short_opt is already taken in parse-option.c, so I'd rather
avoid re-using it (it's static there, so technically, we can,
but ...).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: jk/tag-contains: stalled

[Junio C Hamano]

Ted Ts'o <tytso@mit.edu> writes:

> On Wed, Aug 04, 2010 at 03:24:23PM -0700, Junio C Hamano wrote:
>> 
>> * jk/tag-contains (2010-07-05) 4 commits
>>  - Why is "git tag --contains" so slow?
>>  - default core.clockskew variable to one day
>>  - limit "contains" traversals based on commit timestamp
>>  - tag: speed up --contains calculation
>
> What needs to be fixed up before this effort can graduate?  I find the
> fixups here to be really helpful, even without the automated skew
> detection that has been proposed.  And even if we fix the root problem
> with some new all-singing pack format, I suspect that may be a ways
> out, so it would be nice if these patches could get included for now....

I agree in principle; the log messages need to be cleaned up first
at the least, though.

Re: jk/tag-contains: stalled

[Junio C Hamano]

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

> Ted Ts'o <tytso@mit.edu> writes:
>
>> On Wed, Aug 04, 2010 at 03:24:23PM -0700, Junio C Hamano wrote:
>>> 
>>> * jk/tag-contains (2010-07-05) 4 commits
>>>  - Why is "git tag --contains" so slow?
>>>  - default core.clockskew variable to one day
>>>  - limit "contains" traversals based on commit timestamp
>>>  - tag: speed up --contains calculation
>>
>> What needs to be fixed up before this effort can graduate?  I find the
>> fixups here to be really helpful, even without the automated skew
>> detection that has been proposed.  And even if we fix the root problem
>> with some new all-singing pack format, I suspect that may be a ways
>> out, so it would be nice if these patches could get included for now....
>
> I agree in principle; the log messages need to be cleaned up first
> at the least, though.

To reduce the risk of double-work, I need to clarify.

I meant to say that I can find enough material, especially what Peff
wrote, in the discussion that followed in the thread to do the clean-up
myself.  No need to resend by anybody unless there are material
differences from what have been discussed so far that need to be
incorporated in the final series.

Re: jk/tag-contains: stalled

[Jeff King]

On Thu, Aug 05, 2010 at 10:05:58AM -0700, Junio C Hamano wrote:

> >>> * jk/tag-contains (2010-07-05) 4 commits
> >>>  - Why is "git tag --contains" so slow?
> >>>  - default core.clockskew variable to one day
> >>>  - limit "contains" traversals based on commit timestamp
> >>>  - tag: speed up --contains calculation
> [...]
> > I agree in principle; the log messages need to be cleaned up first
> > at the least, though.
> 
> To reduce the risk of double-work, I need to clarify.
> 
> I meant to say that I can find enough material, especially what Peff
> wrote, in the discussion that followed in the thread to do the clean-up
> myself.  No need to resend by anybody unless there are material
> differences from what have been discussed so far that need to be
> incorporated in the final series.

The only bad log message should be the final one, which should be
dropped anyway. I would recommend just merging the first two for now,
and Ted can tweak his core.clockskew manually.

-Peff

Re: jk/tag-contains: stalled

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 05, 2010 at 10:05:58AM -0700, Junio C Hamano wrote:
>
>> >>> * jk/tag-contains (2010-07-05) 4 commits
>> >>>  - Why is "git tag --contains" so slow?
>> >>>  - default core.clockskew variable to one day
>> >>>  - limit "contains" traversals based on commit timestamp
>> >>>  - tag: speed up --contains calculation
>> [...]
>> > I agree in principle; the log messages need to be cleaned up first
>> > at the least, though.
>> 
>> To reduce the risk of double-work, I need to clarify.
>> 
>> I meant to say that I can find enough material, especially what Peff
>> wrote, in the discussion that followed in the thread to do the clean-up
>> myself.  No need to resend by anybody unless there are material
>> differences from what have been discussed so far that need to be
>> incorporated in the final series.
>
> The only bad log message should be the final one, which should be
> dropped anyway. I would recommend just merging the first two for now,
> and Ted can tweak his core.clockskew manually.

After re-reviewing the one that is queued, the use of TMP_MARK smelled
somewhat bad to me.  It is named TMP_ exactly because it is meant to be
used in a closed callpath---you can use it but you are supposed to clean
it before you return the control to the caller, so that the caller can
rely on TMP_MARK absent from any objects.

Use of UNINTERESTING is similarly not kosher if this were to be used in
larger context outside of "do 'tags --contains' and exit".  You noted
these two points in your original RFC patch.

Besides, "contains()" is too generic a name to live in commit.h.

My gut feeling is that it is probably Ok if contains() and its recursive
helper are moved to builtin/tag.c and are made static, to make it clear
that this should not be reused outside the current context as a generic
"contains" function.  It would probably help to have a comment at the end
of list_tags() to say that TMP_MARK _ought_ to be cleaned before leaving
the function but we don't do that because we know it is the last function
in the callchain before we exit.

By the way, I wonder why pop_most_recent_commit() with a commit_list,
which is the usual revision traversal ingredient for doing something like
this, was not used in the patch, though.  Is it because depth-first was
necessary?

Re: jk/tag-contains: stalled

[Jeff King]

On Thu, Aug 05, 2010 at 11:47:09AM -0700, Junio C Hamano wrote:

> > The only bad log message should be the final one, which should be
> > dropped anyway. I would recommend just merging the first two for now,
> > and Ted can tweak his core.clockskew manually.
> 
> After re-reviewing the one that is queued, the use of TMP_MARK smelled
> somewhat bad to me.  It is named TMP_ exactly because it is meant to be
> used in a closed callpath---you can use it but you are supposed to clean
> it before you return the control to the caller, so that the caller can
> rely on TMP_MARK absent from any objects.
> 
> Use of UNINTERESTING is similarly not kosher if this were to be used in
> larger context outside of "do 'tags --contains' and exit".  You noted
> these two points in your original RFC patch.

Oops, thanks, I had forgotten that the marks needed to be addressed.
Should I be introducing new flags? We have 27 flag bits, but I would
hate to waste 2 of them.

> Besides, "contains()" is too generic a name to live in commit.h.

I agree it's a pretty generic name. I was trying to make this as generic
as possible, at least within the domain of commits, so it could be a
faster replacement for calls to is_descendant_of. Maybe commit_contains?

> My gut feeling is that it is probably Ok if contains() and its
> recursive helper are moved to builtin/tag.c and are made static, to
> make it clear that this should not be reused outside the current
> context as a generic "contains" function.  It would probably help to
> have a comment at the end of list_tags() to say that TMP_MARK _ought_
> to be cleaned before leaving the function but we don't do that because
> we know it is the last function in the callchain before we exit.

But my intent was to have a generic contains function. I was planning on
applying this to "git branch --contains", as well, but my initial
approach wasn't really any faster than the current code (probably
because the number of branches tends to be small compared to the number
of tags).

In an ideal object-oriented world, the interface would be:

  void contains_init(struct contains_context *c,
                     struct commit_list *needles);
  void contains_check(struct contains_context *c, struct commit *haystack);
  void contains_free(struct contains_context *c);

But for memory use reasons, we don't get our own private copy of each
commit. We can drop the "init" and have a "free" or "clear" which clears
marks on the global commit objects. But you also _must_ use the same
needle list for each contains check, or you will get bogus results
(since the marks are essentially partial cached answers).

I guess we could do:

  static struct commit_list *contains_needles;

  void contains_init(struct commit_list *needles)
  {
          if (contains_needles)
                  die("BUG: somebody else is already checking contains!");
          copy_commit_list(&contains_needles, needles);
  }

  void contains_check(struct commit *haystack)
  {
     /* like contains, but check against our static contains_needles */
  }

  void contains_clear(struct contains_context *c)
  {
          /* free contains_needles list, set it to NULL */
          /* clear commit marks */
  }

> By the way, I wonder why pop_most_recent_commit() with a commit_list,
> which is the usual revision traversal ingredient for doing something like
> this, was not used in the patch, though.  Is it because depth-first was
> necessary?

Yes, it is because of the depth-first nature. The intent is to mark
whole sections of the subgraph as "does not contain". If you can think
of a clever way around that, I would be interested to hear it. The fact
that it is a DFS is why we can possibly perform worse than the current
code (we might follow the wrong branch of a merge all the way down to
the root before realizing the commit in question is on the other side).

-Peff

Re: jk/tag-contains: stalled

[Jay Soffian]

On Thu, Aug 5, 2010 at 3:06 PM, Jeff King <peff@peff.net> wrote:
> I agree it's a pretty generic name. I was trying to make this as generic
> as possible, at least within the domain of commits, so it could be a
> faster replacement for calls to is_descendant_of. Maybe commit_contains?

I'm going to side-track this slightly. I wonder why branch and tag
have --contains, but it is not more generically available via
rev-list?  I needed it the other day and spent 5 minutes looking at
what it would take before I ended up just calling merge-base in a loop
for the commits I wanted to check.

j.

Re: jk/tag-contains: stalled

[Jeff King]

On Thu, Aug 05, 2010 at 03:18:15PM -0400, Jay Soffian wrote:

> On Thu, Aug 5, 2010 at 3:06 PM, Jeff King <peff@peff.net> wrote:
> > I agree it's a pretty generic name. I was trying to make this as generic
> > as possible, at least within the domain of commits, so it could be a
> > faster replacement for calls to is_descendant_of. Maybe commit_contains?
> 
> I'm going to side-track this slightly. I wonder why branch and tag
> have --contains, but it is not more generically available via
> rev-list?  I needed it the other day and spent 5 minutes looking at
> what it would take before I ended up just calling merge-base in a loop
> for the commits I wanted to check.

I'm not sure rev-list makes the most sense. We already have "show
commits in X, but not in Y". But I gather you wanted "from a list
(U,V,W,X), print each that contains Y". Which is not really a rev-list
function anymore, as it is not about listing revisions, but rather about
grepping a list you've given it.

Something like "git for-each-ref --contains" seems more sensible to me,
though it is not as generic as we could make it (I cannot use an
arbitrary list of commits to the "haystack", but only ones that have
refs pointing to them).

-Peff

Re: jk/tag-contains: stalled

[Jay Soffian]

On Thu, Aug 5, 2010 at 3:27 PM, Jeff King <peff@peff.net> wrote:
> I'm not sure rev-list makes the most sense. We already have "show
> commits in X, but not in Y". But I gather you wanted "from a list
> (U,V,W,X), print each that contains Y".

Correct.

> Which is not really a rev-list
> function anymore, as it is not about listing revisions, but rather about
> grepping a list you've given it.

Well maybe, but rev-list will already take a list of revs on stdin and
you can give it --no-walk, so it has already been abused to do more
than strictly list revisions. And what do you call this?

$ git rev-list --branches --no-walk --author=gitster

:-)


> Something like "git for-each-ref --contains" seems more sensible to me,
> though it is not as generic as we could make it (I cannot use an
> arbitrary list of commits to the "haystack", but only ones that have
> refs pointing to them).

Sure, and if I wanted to do that, I could've just created a bunch of
temporary light-weight tags for those commits I was potentially
interested in and then used tag --contains. :-)

So I don't think rev-list is such a bad place after all.

j.

Re: jk/tag-contains: stalled

[Ted Ts'o]

On Thu, Aug 05, 2010 at 03:06:54PM -0400, Jeff King wrote:
> But my intent was to have a generic contains function. I was planning on
> applying this to "git branch --contains", as well, but my initial
> approach wasn't really any faster than the current code (probably
> because the number of branches tends to be small compared to the number
> of tags).

At work we have some 100 topics branches per kernel revision, and I
have a repository with 332 branches in it at the moment.  So there may
very well be repo's where git branch --contains might be faster with
your approach.

							- Ted

Re: jk/tag-contains: stalled

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Oops, thanks, I had forgotten that the marks needed to be addressed.
> Should I be introducing new flags? We have 27 flag bits, but I would
> hate to waste 2 of them.

In the longer term it would be not just nice but necessary for us to come
up with a scheme where different codepaths can "allocate bits from object
flags", without having to fear stepping on each other's toes.  Some
possible approaches off the top of my head are:

 - Extend "struct object" by another uint64_t to give it 64 more bits?
   That would make the minimum object size from 24 bytes to 32 bytes and
   during a pack-object session we would keep a lot of objects (not just
   commits but trees and blobs) in core, so we probably would not want to
   do this.

 - Extend "struct commit" by another uint32_t?  Currently a "struct
   commit" is 72 bytes on x86_64 (there is an unfortunate 4-byte padding
   gap between indegree and date), and 48 bytes on i386 and this would
   enlarge the latter to 52 bytes (this comes free on 64-bit archs).

   As we need a lot more bits on commits than on other objects
   (e.g. left-right do not need to be placed on trees or blobs), this
   approach might be more space efficient.

 - Use one bit in the current flags section to signal "extended flag bits
   present on this object", and have a separate hashtable for minority
   objects that have that bit set?  This would work only for flag bits
   that are rarely used (otherwise the secondary hashtable will be full of
   objects and per-object overhead will kill us).

 - Migrate some users of flag bits that only mark small miniroty of
   commits to use dedicated hashtable to free their bits [*1*].  I don't
   know if there are candidates for doing this offhand.  Just uttering it
   as an idea.

Independent of this issue, I suspect that we might want to fold
object.used into the general set of flags---it is only used by fsck as far
as I remember.

[Footnote]

*1* Also we would want to do something similar to the commit.util field so
that more than one utility libraries can attach their own stuff to each
commit.  It _might_ make sense to instead get rid of commit.util and
migrate the users to a separate "one object hash per one type of info",
though.  In any case it is a separate topic.

Re: jc/sha1-name-find-fix

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 279 bytes --]

On Wed, Aug 04, 2010 at 03:24:23PM -0700, Junio C Hamano wrote:
[...]
> [New Topics]
> 
> * jc/sha1-name-find-fix (2010-08-02) 1 commit
>  - sha1_name.c: fix parsing of ":/token" syntax

Thank you for the fix.  BTW, it applies to maint with no regressions.


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: jk/tag-contains: stalled

[Thomas Rast]

Junio C Hamano wrote:
> *1* Also we would want to do something similar to the commit.util field so
> that more than one utility libraries can attach their own stuff to each
> commit.  It _might_ make sense to instead get rid of commit.util and
> migrate the users to a separate "one object hash per one type of info",
> though.  In any case it is a separate topic.

I thought this already existed in decorate.c?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: jk/tag-contains: stalled

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Junio C Hamano wrote:
>> *1* Also we would want to do something similar to the commit.util field so
>> that more than one utility libraries can attach their own stuff to each
>> commit.  It _might_ make sense to instead get rid of commit.util and
>> migrate the users to a separate "one object hash per one type of info",
>> though.  In any case it is a separate topic.
>
> I thought this already existed in decorate.c?

Yes, the point is that there may be cases where it may be better to
migrate some current commit.util users to that API.

Re: jk/tag-contains: stalled

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Aug 05, 2010 at 11:47:09AM -0700, Junio C Hamano wrote:
>
>> My gut feeling is that it is probably Ok if contains() and its
>> recursive helper are moved to builtin/tag.c and are made static, to
>> make it clear that this should not be reused outside the current
>> context as a generic "contains" function.  It would probably help to
>> have a comment at the end of list_tags() to say that TMP_MARK _ought_
>> to be cleaned before leaving the function but we don't do that because
>> we know it is the last function in the callchain before we exit.

After thinking about this a bit more, I changed my mind.

I think depth-first traversal from all tag tips, without running any
traversal from the wanted commit, has a serious downside.  What happens
when you run "git tag --contains master" in a Linus tree after a major
release but before he tags the -rc1 and closes the merge window?  Doesn't
the algorithm run all the way down to the root, only to say nothing?
Admittedly the traversal will visit each commit once (starting from
v2.6.12 down to v2.6.12-rc2, then skipping v2.6.12-rc2 through v2.6.12-rc6
because they are already seen and known not to contain the "master", then
starting from v2.6.13 down to v2.6.12, and so on), but visiting all the
commits down to root feels really wrong.

I think the merge-base traversal is essential.  Suppose we have hundreds
of commits with two tags 'T1' and 'T2':

                                  o---T2        o---o---Y
                                 /             /
 ---o---o---o---o---o---o---o---%---...---X---*---T1

If you are probing for 'X', traversing from all the tags until you hit a
wanted commit will stop at a reasonable point (namely, 'X') if you are
lucky and you started from 'T1' not 'T2', with your new algorithm.  If you
are unlucky and started from 'T2', you will however traverse down to root
from 'T2' which would be the bulk of the history.

Also, when you are probing for 'Y', you end up traversing all the way
down, whether you start from 'T1' or 'T2', don't you?

You can (and should) dig both from 'Y' and 'T1' and stop traversal at '*',
as that commit is an ancestor of 'Y' and at that point you have proven
that any ancestor of that you will find by traversing further will not
find 'Y'.  Similarly for 'Y' and 'T2' pair, which should stop at '%'.

As the author of show-branch, I think I know one trick that may speed this
up without compromising the worst case scenario.

We can run this traversal in parallel for many tags at once.  Use bit #0
of object.flags for the wanted commit, and bit #1..bit #N for N tags (if
you have more tags than that fit in the flags word, you run the algorithm
multiple times, N tags at a time, clearing the flags after each round).

The parallel traversal will run, smudging the parent commit's flags with
those of the child, in the usual way.

Suppose that you are probing for 'Y'.  You start traversal from 'Y', 'T1'
and 'T2' (as two tags will fit comfortably in a single flags word).  When
you come to commit '%', you will notice that all the ancestors of that
commit are contained in the wanted commit and all the tags you are probing
for ('T1' and 'T2').  There is no point to go further than that point
during this round, as you know you will not find 'Y' by digging further
from that point.

In general, when you find that a commit has both bit #0 and bit #i set,
that commit is known to be a common ancestor between 'Y' and 'Ti', so
there is no point going further from that point to see if 'Ti' contains
'Y'---you know it doesn't.  So you can stop when you see bit 0..N are all
set.  After traversing as many commits as possible this way, look at the
bits 1..i that is given to 'Y'.  They represent the set of tags that
contain 'Y' (you will find only bit #0 is set on 'Y').

Similarly, if you are probing for 'X', start the traversal from 'X', 'T1'
and 'T2'.  The traversal that begins at 'T1' will reach 'X' (so 'X' will
initially start with bit #0 set, but then when the traversal that started
from 'T1' reaches it, its bit #1 gets set) eventually.  Because bit #2 is
not set at that point, you don't stop and keep traversing.

You may already have started marking 'X' and its parent with bit #0 with
the traversal you started at 'X', but when you reach 'X' from 'T1' again,
you will be changing bit #1; anytime you change the flag, you would push
the commit back to the stack and re-traverse it.  Eventually, you will
smudge '%' with all 3 bits (depending on clock skew, you might traverse a
few more levels of its parents) and you can stop.  The flag word on 'X'
will have bis #0 and #1 set, so you know 'T1' reaches it but 'T2' doesn't.

To optimize this further, you may want to take advantage of the fact that
tags are not supposed to change, and have a cache that knows what tag is
contained in what other tag (e.g. if v1.0 is contained in v2.0, you do not
have to run the probe for v2.0 once you know the wanted commit is
reachable from v1.0).  But that can come later, I think.

But I haven't thought things really through.  If I am lucky, I may be able
to find time tomorrow during the day to try conjuring something out, but
no promises...