[PATCH 0/5 v3] log and diff: accept detached forms (--option value)

[PATCH 0/5 v3] log and diff: accept detached forms (--option value)

[Matthieu Moy]

Since last version :

* I had missed several optional arguments, for which detached form
  should not be allowed (otherwise, --option --other-option is
  ambiguous). In most cases, my changes were harmless since the code
  had already checked for the parameterless form before reaching mine,
  but that was definitely bad anyway.

* One missing "return optarg;" (that even Jonathan had missed ;-) )

* One more test for "git log -S" failure (no argument to -S)

* Refactoring by Jonathan Nieder for --stat-*

* Rewording of commit messages.

Jonathan Nieder (1):
  diff: split off a function for --stat-* option parsing

Matthieu Moy (4):
  diff: parse detached options like -S foo
  diff: parse detached options --stat-width n, --stat-name-width n
  log: parse detached options like git log --grep foo
  log: parse detached option for --glob

 diff.c                       |  167 +++++++++++++++++++++++++++++++-----------
 diff.h                       |    7 ++
 revision.c                   |   79 +++++++++++++-------
 t/t4013-diff-various.sh      |    5 +
 t/t4013/diff.log_-S_F_master |    7 ++
 t/t4202-log.sh               |   19 +++--
 t/t6018-rev-list-glob.sh     |    6 ++
 7 files changed, 211 insertions(+), 79 deletions(-)
 create mode 100644 t/t4013/diff.log_-S_F_master

-- 
1.7.2.21.ge9796

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

[Matthieu Moy]

Change the option parsing logic in revision.c to accept detached 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.

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..d89ea20 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 + strlen("-c");
+		return 1;
+	}
+	if (!argv[1])
+		die("Option '%c' requires a value", opt);
+	*optarg = argv[1];
+	return 2;
+}
+
+int diff_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 == '=') { /* Sticky form: --option=value */
+		*optarg = arg + 1;
+		return 1;
+	}
+	if (*arg != '\0')
+		return 0;
+	/* detached 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 = diff_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 = diff_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 = diff_long_opt("src-prefix", av, &optarg))) {
+		options->a_prefix = optarg;
+		return argcount;
+	}
+	else if ((argcount = diff_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 = diff_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..3b11732 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 sticky form
+ * (--option=value) and detached form (--option value).
+ */
+extern int diff_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..36870c5 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-detached &&
+	printf "fifth\nfourth\nthird\ninitial" > expect &&
+	test_cmp expect actual &&
+	test_cmp expect actual-detached
 
 '
 
-- 
1.7.2.21.ge9796

[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 d89ea20..d5f5d4b 100644
--- a/diff.c
+++ b/diff.c
@@ -3029,6 +3029,38 @@ int diff_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.21.ge9796

[PATCH 3/5] diff: parse detached 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 d5f5d4b..90b07e1 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.21.ge9796

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

[Matthieu Moy]

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

diff --git a/revision.c b/revision.c
index 7e82efd..7006340 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 = diff_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 = diff_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 = diff_long_opt("max-age", argv, &optarg))) {
+		revs->max_age = atoi(optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("since", argv, &optarg))) {
+		revs->max_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("after", argv, &optarg))) {
+		revs->max_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("min-age", argv, &optarg))) {
+		revs->min_age = atoi(optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("before", argv, &optarg))) {
+		revs->min_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = diff_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 = diff_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 = diff_long_opt("author", argv, &optarg))) {
+		add_header_grep(revs, GREP_HEADER_AUTHOR, optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("committer", argv, &optarg))) {
+		add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
+		return argcount;
+	} else if ((argcount = diff_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;
+	} else if ((argcount = diff_long_opt("encoding", argv, &optarg))) {
 		if (strcmp(arg, "none"))
-			git_log_output_encoding = xstrdup(arg);
+			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 36870c5..dfbc173 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.21.ge9796

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

[Jonathan Nieder]

Hi,

Glance over this again:

> +++ b/revision.c
> @@ -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;
> +	} else if ((argcount = diff_long_opt("encoding", argv, &optarg))) {
>  		if (strcmp(arg, "none"))
> -			git_log_output_encoding = xstrdup(arg);
> +			git_log_output_encoding = xstrdup(optarg);

I think arg should be optarg in the string comparison, too.  Sorry I
didn’t notice last time.

I wonder why t3900 and t8005 do not catch this.

>  		else
>  			git_log_output_encoding = "";

Curious,
Jonathan

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

[Matthieu Moy]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I think arg should be optarg in the string comparison, too.

Definitely, yes.

> Sorry I didn’t notice last time.

Ahem, sorry for making the bug in the first place ;-).

> I wonder why t3900 and t8005 do not catch this.

Good question. I don't understand exactly what's going on with commit
messages encoding, so I can't say.

Resend follows.

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

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

[Matthieu Moy]

Change the option parsing logic in revision.c to accept detached 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.

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..d89ea20 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 + strlen("-c");
+		return 1;
+	}
+	if (!argv[1])
+		die("Option '%c' requires a value", opt);
+	*optarg = argv[1];
+	return 2;
+}
+
+int diff_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 == '=') { /* Sticky form: --option=value */
+		*optarg = arg + 1;
+		return 1;
+	}
+	if (*arg != '\0')
+		return 0;
+	/* detached 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 = diff_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 = diff_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 = diff_long_opt("src-prefix", av, &optarg))) {
+		options->a_prefix = optarg;
+		return argcount;
+	}
+	else if ((argcount = diff_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 = diff_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..3b11732 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 sticky form
+ * (--option=value) and detached form (--option value).
+ */
+extern int diff_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..36870c5 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-detached &&
+	printf "fifth\nfourth\nthird\ninitial" > expect &&
+	test_cmp expect actual &&
+	test_cmp expect actual-detached
 
 '
 
-- 
1.7.2.252.gb848

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

[Junio C Hamano]

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

> Change the option parsing logic in revision.c to accept detached 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.
>
> 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..d89ea20 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 + strlen("-c");

Just a style thing, but I think "arg + 2" is much easier to read in this
particular case, as it won't risk tempting readers to go "Huh?  What does
that 'c' mean"?  I know the code wants to skip over an option that is a
single dash followed by a single byte and 'c' is just a placeholder for
that single unknown byte, but when the reader reaches that realization,
the reader already knows that the code wants to skip exactly two bytes,
no?  strlen("--") in diff_long_opt() is much easier to justify, though.  I
like the reduction of "arg + N"s that follow prefixcmp(arg, "--whatever")s
in diff_opt_parse() this patch gives us in general.  I just think the
above "-c" is overdoing it.

Do we have an option that can take a zero-length string as its value and
do something meaningful?  I don't think of any offhand ("log -S'' -p" is
not it---it may be meaningful but it is not useful), but this code would
start giving "-p" instead of "" to the option in such a case.

A longer option always required '=' before the value, so it won't share
the above issue (if it is an issue, that is).

Other than that minor style issue and a nagging -X<empty> worry, the
resulting code looks a lot nicer than the original.

Thanks.

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

[Matthieu Moy]

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

> Just a style thing, but I think "arg + 2" is much easier to read in this
> particular case, as it won't risk tempting readers to go "Huh?  What does
> that 'c' mean"?

I'll take this "+ 2" in the next round.

> Do we have an option that can take a zero-length string as its value and
> do something meaningful?  I don't think of any offhand ("log -S'' -p" is
> not it---it may be meaningful but it is not useful), but this code would
> start giving "-p" instead of "" to the option in such a case.

I make it explicit in the commit message: only -S, -l and -O are
touched by my change among the one-letter options => no problem here.

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

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Matthieu Moy <Matthieu.Moy@imag.fr> writes:

>> +	if (arg[2] != '\0') {
>> +		*optarg = arg + strlen("-c");
>
> Just a style thing, but I think "arg + 2" is much easier to read in this
> particular case, as it won't risk tempting readers to go "Huh?  What does
> that 'c' mean"?

Yeah, especially after the "if (arg[2] != ...)".  Thanks for a sanity
check.

> Do we have an option that can take a zero-length string as its value and
> do something meaningful?  I don't think of any offhand ("log -S'' -p" is
> not it---it may be meaningful but it is not useful), but this code would
> start giving "-p" instead of "" to the option in such a case.

 -l<num>:	"" is not a number.

 -S<artifact>:
		Could "" come up if a user tries to find a commit
		"Adding/removing string:" without typing a string to look
		for in gitk?  No, gitk protects against that.
		How about creating a view with an empty "Search string:"?
		No, that is protected against, too.

		Currently "git log -S" does not seem to do anything
		meaningful, anyway.

 -O<file>:	"" is not a file name.

[PATCH 2/5 v4] 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 d89ea20..d5f5d4b 100644
--- a/diff.c
+++ b/diff.c
@@ -3029,6 +3029,38 @@ int diff_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.252.gb848

[PATCH 3/5 v4] diff: parse detached 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 d5f5d4b..90b07e1 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.252.gb848

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

[Junio C Hamano]

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

> Part of a campaign for unstuck forms of options.
>
>  	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);

This will accept "--stat-width=40", "--stat-width 40", and "--stat=40,20"
but not "--stat 40,20" --- am I reading the patch correctly?

Not a complaint but trying to double-check.  I think it is Ok not to try
guessing wrong (the user may be interested in a path 40,20, for example).

Thanks.

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

[Matthieu Moy]

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

> This will accept "--stat-width=40", "--stat-width 40", and "--stat=40,20"
> but not "--stat 40,20" --- am I reading the patch correctly?

Yes.

> Not a complaint but trying to double-check.  I think it is Ok not to try
> guessing wrong (the user may be interested in a path 40,20, for example).

Right. Same goes for all optional arguments, the sticky form is
required.

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

[PATCH 4/5 v4] log: parse detached 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..359b1a1 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 = diff_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 = diff_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 = diff_long_opt("max-age", argv, &optarg))) {
+		revs->max_age = atoi(optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("since", argv, &optarg))) {
+		revs->max_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("after", argv, &optarg))) {
+		revs->max_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("min-age", argv, &optarg))) {
+		revs->min_age = atoi(optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("before", argv, &optarg))) {
+		revs->min_age = approxidate(optarg);
+		return argcount;
+	} else if ((argcount = diff_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 = diff_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 = diff_long_opt("author", argv, &optarg))) {
+		add_header_grep(revs, GREP_HEADER_AUTHOR, optarg);
+		return argcount;
+	} else if ((argcount = diff_long_opt("committer", argv, &optarg))) {
+		add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
+		return argcount;
+	} else if ((argcount = diff_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 = diff_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 36870c5..dfbc173 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.252.gb848

Re: [PATCH 4/5 v4] log: parse detached options like git log --grep foo

[Junio C Hamano]

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

> 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..359b1a1 100644
> --- a/revision.c
> +++ b/revision.c
> ...
> @@ -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);

The patch overall looks good, and this comments illustrates the issue
rather well.  When the user wants to use "--longopt val" syntax, s/he
needs to know that "--longopt" will always take a value.  Arguably
majority of options that can take value will, but like "--stat X,Y" this
leaves things inconsistent.  Without "--longopt value" patch there won't
be such an inconsistency, but I think this patch series is lessor of two
evils.

Don't you by the way regret the naming of the parsing function by now?
There is nothing "diff" about it anymore.

Re: [PATCH 4/5 v4] log: parse detached options like git log --grep foo

[Matthieu Moy]

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

> The patch overall looks good, and this comments illustrates the issue
> rather well.  When the user wants to use "--longopt val" syntax, s/he
> needs to know that "--longopt" will always take a value.  Arguably
> majority of options that can take value will, but like "--stat X,Y" this
> leaves things inconsistent.  Without "--longopt value" patch there won't
> be such an inconsistency, but I think this patch series is lessor of two
> evils.

... especially when parse-option already does this:

git commit --message foo   => works
git gc --prune 'last week' => doesn't

Just like most GNU tools:

grep --regexp foo => works
grep --color auto => doesn't

> Don't you by the way regret the naming of the parsing function by now?
> There is nothing "diff" about it anymore.

Right. I'll rename them to "parse_long_opt".

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

Re: [PATCH 4/5 v4] log: parse detached options like git log --grep foo

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> The patch overall looks good, and this comments illustrates the issue
>> rather well.  When the user wants to use "--longopt val" syntax, s/he
>> needs to know that "--longopt" will always take a value.  Arguably
>> majority of options that can take value will, but like "--stat X,Y" this
>> leaves things inconsistent.  Without "--longopt value" patch there won't
>> be such an inconsistency, but I think this patch series is lessor of two
>> evils.
>
> ... especially when parse-option already does this:
>
> git commit --message foo   => works
> git gc --prune 'last week' => doesn't
>
> Just like most GNU tools:
>
> grep --regexp foo => works
> grep --color auto => doesn't

Hmm.

Are you hinting that we should keep "you can say '-Ofoo' and '-O foo'"
bits but we should drop "you can also say '--opt=foo' and '--opt foo' as
long as --opt always takes an argument"?

I actually think that may make sense.

>> Don't you by the way regret the naming of the parsing function by now?
>> There is nothing "diff" about it anymore.
>
> Right. I'll rename them to "parse_long_opt".

Thanks.

Re: [PATCH 4/5 v4] log: parse detached options like git log --grep foo

[Matthieu Moy]

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> The patch overall looks good, and this comments illustrates the issue
>>> rather well.  When the user wants to use "--longopt val" syntax, s/he
>>> needs to know that "--longopt" will always take a value.  Arguably
>>> majority of options that can take value will, but like "--stat X,Y" this
>>> leaves things inconsistent.  Without "--longopt value" patch there won't
>>> be such an inconsistency, but I think this patch series is lessor of two
>>> evils.
>>
>> ... especially when parse-option already does this:
>>
>> git commit --message foo   => works
>> git gc --prune 'last week' => doesn't
>>
>> Just like most GNU tools:
>>
>> grep --regexp foo => works
>> grep --color auto => doesn't
>
> Hmm.
>
> Are you hinting that we should keep "you can say '-Ofoo' and '-O foo'"
> bits but we should drop "you can also say '--opt=foo' and '--opt foo' as
> long as --opt always takes an argument"?
>
> I actually think that may make sense.

I'm not sure I parsed your sentence correctly, but if I did, then no,
I don't think we should drop separate forms when --opt always takes an
argument, just the opposite.

-O<string> => always accepted
-O <string> => OK if <string> is mandatory and always non-empty
--opt=<sting> => always accepted
--opt <string> => OK if <string> is mandatory (possibly empty)

My remark was that there's nothing very original here, what my patch
does is what other places of Git did, and that other common tools did
years before.

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

Re: [PATCH 4/5 v4] log: parse detached options like git log --grep foo

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

>>> ... especially when parse-option already does this:
>>>
>>> git commit --message foo   => works
>>> git gc --prune 'last week' => doesn't
>>>
>>> Just like most GNU tools:
>>>
>>> grep --regexp foo => works
>>> grep --color auto => doesn't
>>
>> Hmm.
>>
>> Are you hinting that we should keep "you can say '-Ofoo' and '-O foo'"
>> bits but we should drop "you can also say '--opt=foo' and '--opt foo' as
>> long as --opt always takes an argument"?
>>
>> I actually think that may make sense.
>
> I'm not sure I parsed your sentence correctly, but if I did, then no,
> I don't think we should drop separate forms when --opt always takes an
> argument, just the opposite.

What I meant was somewhat different.  I agree that we _could_ take
separate "--opt <string>" form when --opt is known to always take an
argument without ambiguity (the same goes for "-O <string>").

But I thought you were suggesting to accept:

    -O<string>
    -O <string>
    --opt=<string>

but never take:

    --opt <string> (even when --opt always takes an argument)

because that would further reduce one source of potential confusion
(i.e. the user needs to remember if --opt always takes an argument or
not).  I thought you mentioned that neither parse-options nor GNU tools
take "--prune last", "--color auto" as a good supporting argument for
this---the users won't miss the "--opt <string>" feature because that is
not a common practice.

And I was agreeing to that.

Re: [PATCH 4/5 v4] log: parse detached options like git log --grep foo

[Matthieu Moy]

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

> But I thought you were suggesting to [...] never take:
>
>     --opt <string> (even when --opt always takes an argument)
>
> because that would further reduce one source of potential confusion
> (i.e. the user needs to remember if --opt always takes an argument or
> not).  I thought you mentioned that neither parse-options nor GNU tools
> take "--prune last",

No, I said the opposite: both do.

> "--color auto" as a good supporting argument for this

"grep --color auto" is refused because the "auto" part is optional.
But both "grep --regex foo" and "grep --regexp=foo" are accepted. Same
for "git commit --message=foo" and "git commit --message foo".

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

Re: [PATCH 4/5 v4] log: parse detached options like git log --grep foo

[Miles Bader]

Junio C Hamano <gitster@pobox.com> writes:
> this---the users won't miss the "--opt <string>" feature because that is
> not a common practice.
>
> And I was agreeing to that.

No, that would be not good -- GNU getopt_long supports that form, and
AFAICT, both the "--foo=bar" and "--foo bar" forms are widely used (I
certainly use both all the time), for roughly the same reasons both
"-Oarg" and "-O arg" are both common[*].

In the case of "optional-argument options", the "--foo bar" (and "-O
bar") form doesn't work, which is an unfortunate (but apparently
unavoidable) inconsistency, but optional-argument options are rare
enough that this doesn't really effect the usage patterns of more
conventional options.

[*] both forms are useful -- sometimes it's handy to bundle up the
option-argument with the option, e.g., when writing a throw-away script,
to avoid whitespace issues, but sometimes it's useful to keep it
separate, e.g. so that shell filename completion works on the
option-argument.

-Miles

-- 
Justice, n. A commodity which in a more or less adulterated condition the
State sells to the citizen as a reward for his allegiance, taxes and personal
service.

[PATCH 5/5 v4] log: parse detached 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 359b1a1..e7a367a 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 = diff_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.252.gb848

[PATCH 5/5] log: parse detached 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 7006340..188fa21 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 = diff_long_opt("glob", argv + i, &optarg))) {
+				i += argcount - 1;
 				struct all_refs_cb cb;
 				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.21.ge9796