threads / patch / 24542

patch, 4 partsAllow detached form for git diff --stat-name-width and --stat-width.

Subject: [PATCH 2/4] Allow detached form for git diff --stat-name-width and --stat-width.

## tl;dr

24 messages between Jul 27, 2010 and Jul 29, 2010. Diffs are folded; open one to read it.

replies: 23people: 5as markdown or json

Matthieu Moy· Jul 27, 2010, 21:21 UTC · lore

[PATCH 0/4] Allow detached forms (--option arg) for git log and friends

After discssion on the early RFC, I decided that a migration to parse-option would be too much work given my git time budget and the expected benefits. I'm accomplishing the same goal with very simple macros, and one can now do e.g.

  git log -S foo
  git log --grep bar
Options with optional arguments do not accept this form.

To ease review, I'm splitting the serie into 4 batches of applications of the same pattern.

Matthieu Moy (4):
  Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff
    options
  Allow detached form for git diff --stat-name-width and --stat-width.
  Allow detached form (e.g. "git log --grep foo") in log options.
  Allow detached form for --glob, --branches, --tags and --remote.
 diff.c                       |   75 +++++++++++++++++++++------
 diff.h                       |   15 +++++
 revision.c                   |  117 +++++++++++++++++++++++++++---------------
 t/t4013-diff-various.sh      |    1 +
 t/t4013/diff.log_-S_F_master |    7 +++
 t/t4202-log.sh               |   12 ++++
 t/t6018-rev-list-glob.sh     |    6 ++
 7 files changed, 176 insertions(+), 57 deletions(-)
 create mode 100644 t/t4013/diff.log_-S_F_master
-- 
1.7.2.25.g9ebe3
Matthieu Moy· Jul 27, 2010, 21:21 UTC · re: Matthieu Moy · lore

[PATCH 1/4] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options

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

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c                       |   61 +++++++++++++++++++++++++++++++-----------
 diff.h                       |   15 ++++++++++
 t/t4013-diff-various.sh      |    1 +
 t/t4013/diff.log_-S_F_master |    7 +++++
 t/t4202-log.sh               |    5 +++
 5 files changed, 73 insertions(+), 16 deletions(-)
 create mode 100644 t/t4013/diff.log_-S_F_master
Show changes to 5 files +73 −16

diff.c, diff.h, t/t4013-diff-various.sh, t/t4013/diff.log_-S_F_master, t/t4202-log.sh

diff --git a/diff.c b/diff.c
index 17873f3..41b39be 100644
--- a/diff.c
+++ b/diff.c
@@ -2990,9 +2990,23 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
 
 static int diff_scoreopt_parse(const char *opt);
 
-int diff_opt_parse(struct diff_options *options, const char **av, int ac)
+int diff_opt_parse(struct diff_options *options, const char **argv, int ac)
 {
-	const char *arg = av[0];
+	const char *arg = argv[0];
+	const char *optarg;
+	int argcount;
+
+#define IF_SHORT_OPT(optname)			\
+	((!strcmp(arg, "-" #optname)		\
+	  && (argv[1] || (die("Option `" #optname "' requires a value"), 1),	\
+	      optarg   = argv[1],	       		\
+	      argcount = 2,			\
+	      1)) ||				\
+	 (!prefixcmp(arg, "-" #optname)		\
+	  && (optarg = arg + strlen("-" #optname),	\
+	      argcount = 1,			\
+	       1)))
+
 
 	/* Output format options */
 	if (!strcmp(arg, "-p") || !strcmp(arg, "-u") || !strcmp(arg, "--patch"))
@@ -3149,10 +3163,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 (IF_LONG_OPT(word-diff-regex)) {
 		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 +3195,28 @@ 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 (!strcmp(arg, "-l")) {
+		options->rename_limit = strtoul(optarg, NULL, 10);
+		return 2;
+	}
 	else if (!prefixcmp(arg, "-l"))
 		options->rename_limit = strtoul(arg+2, NULL, 10);
-	else if (!prefixcmp(arg, "-S"))
-		options->pickaxe = arg + 2;
+	else if (IF_SHORT_OPT(S)) {
+		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 (IF_SHORT_OPT(O)) {
+		options->orderfile = optarg;
+		return argcount;
+	}
+	else if (IF_LONG_OPT(diff-filter)) {
+		options->filter = optarg;
+		return argcount;
+	}
 	else if (!strcmp(arg, "--abbrev"))
 		options->abbrev = DEFAULT_ABBREV;
 	else if (!prefixcmp(arg, "--abbrev=")) {
@@ -3201,20 +3226,24 @@ 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 (IF_LONG_OPT(src-prefix)) {
+		options->a_prefix = optarg;
+		return argcount;
+	}
+	else if (IF_LONG_OPT(dst-prefix)) {
+		options->b_prefix = optarg;
+	}
 	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 (IF_LONG_OPT(output)) {
+		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..ca1a693 100644
--- a/diff.h
+++ b/diff.h
@@ -214,6 +214,21 @@ 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, only meant to be used in
+ * handle_revision_opt and  diff_opt_parse.
+ */
+#define IF_LONG_OPT(optname)					\
+	((!strcmp(arg, "--" #optname)				\
+	  && (argv[1] || (die("Option `" #optname "' requires a value"), 1), \
+	      optarg   = argv[1],	       			\
+	      argcount = 2,					\
+	      1)) ||						\
+	 (!prefixcmp(arg, "--" #optname "=")			\
+	  && (optarg = arg + strlen("--" #optname "="),		\
+	      argcount = 1,					\
+	      1)))
+
 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..8036e00 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
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..3352935 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -101,11 +101,16 @@ test_expect_success 'oneline' '
 test_expect_success 'diff-filter=A' '
 
 	actual=$(git log --pretty="format:%s" --diff-filter=A HEAD) &&
+	actual_detached=$(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
+	} &&
+	test "$actual" = "$actual_detached" || {
+		echo Oops. Detached form broken
+		echo "Actual_detached: $actual_detached"
 	}
 
 '
-- 
1.7.2.25.g9ebe3
Jonathan Nieder· Jul 27, 2010, 21:37 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/4] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options

Matthieu Moy wrote:
> +++ b/diff.c
> @@ -2990,9 +2990,23 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
[...]
Show 11 quoted lines
> +#define IF_SHORT_OPT(optname)			\
> +	((!strcmp(arg, "-" #optname)		\
> +	  && (argv[1] || (die("Option `" #optname "' requires a value"), 1),	\
> +	      optarg   = argv[1],	       		\
> +	      argcount = 2,			\
> +	      1)) ||				\
> +	 (!prefixcmp(arg, "-" #optname)		\
> +	  && (optarg = arg + strlen("-" #optname),	\
> +	      argcount = 1,			\
> +	       1)))
> +
Why not something like this?
	static inline int short_opt(char opt, const char *arg,
					const char *argv, const char **optarg)
	{
		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;
	}
and then:
	if ((argcount = short_opt('S', arg, argv, &optarg))) {
		...
Show 19 quoted lines
> +++ b/diff.h
> @@ -214,6 +214,21 @@ 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, only meant to be used in
> + * handle_revision_opt and  diff_opt_parse.
> + */
> +#define IF_LONG_OPT(optname)					\
> +	((!strcmp(arg, "--" #optname)				\
> +	  && (argv[1] || (die("Option `" #optname "' requires a value"), 1), \
> +	      optarg   = argv[1],	       			\
> +	      argcount = 2,					\
> +	      1)) ||						\
> +	 (!prefixcmp(arg, "--" #optname "=")			\
> +	  && (optarg = arg + strlen("--" #optname "="),		\
> +	      argcount = 1,					\
> +	      1)))
 static int long_opt(const char *opt, const char *arg,
				const char **argv, const char **optarg)
 {
	if (arg[0] != '-' || arg[1] != '-')
		return 0;
	if (prefixcmp(arg, opt))
		return 0;
	arg += strlen("--") + strlen(opt);
	if (*arg == '=') {
		*optarg = arg + 1;
		return 1;
	}
	if (*arg != '\0')
		return 0;
	if (!argv[1])
		die("Option '--%s' requires a value", opt);
	*optarg = argv[1];
	return 2;
 }
Jonathan
Matthieu Moy· Jul 28, 2010, 07:38 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/4] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 21 quoted lines
> Matthieu Moy wrote:
>
>> +++ b/diff.c
>> @@ -2990,9 +2990,23 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
> [...]
>> +#define IF_SHORT_OPT(optname)			\
>> +	((!strcmp(arg, "-" #optname)		\
>> +	  && (argv[1] || (die("Option `" #optname "' requires a value"), 1),	\
>> +	      optarg   = argv[1],	       		\
>> +	      argcount = 2,			\
>> +	      1)) ||				\
>> +	 (!prefixcmp(arg, "-" #optname)		\
>> +	  && (optarg = arg + strlen("-" #optname),	\
>> +	      argcount = 1,			\
>> +	       1)))
>> +
>
> Why not something like this?
>
> 	static inline int short_opt(char opt, const char *arg,
> 					const char *argv, const char **optarg)
Sounds nice too. I'll do that in the next round.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Jul 28, 2010, 09:40 UTC · re: Matthieu Moy · lore

[PATCH 1/4 v2] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options

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.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c                       |   86 ++++++++++++++++++++++++++++++++++--------
 diff.h                       |    7 +++
 t/t4013-diff-various.sh      |    1 +
 t/t4013/diff.log_-S_F_master |    7 +++
 t/t4202-log.sh               |    5 ++
 5 files changed, 90 insertions(+), 16 deletions(-)
 create mode 100644 t/t4013/diff.log_-S_F_master
Show changes to 5 files +90 −16

diff.c, diff.h, t/t4013-diff-various.sh, t/t4013/diff.log_-S_F_master, t/t4202-log.sh

diff --git a/diff.c b/diff.c
index 17873f3..1868b3e 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,24 @@ 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;
+	}
 	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..8036e00 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
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..3352935 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -101,11 +101,16 @@ test_expect_success 'oneline' '
 test_expect_success 'diff-filter=A' '
 
 	actual=$(git log --pretty="format:%s" --diff-filter=A HEAD) &&
+	actual_detached=$(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
+	} &&
+	test "$actual" = "$actual_detached" || {
+		echo Oops. Detached form broken
+		echo "Actual_detached: $actual_detached"
 	}
 
 '
-- 
1.7.2.25.g9ebe3
Jonathan Nieder· Jul 29, 2010, 02:00 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/4 v2] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options

Matthieu Moy wrote:
>  5 files changed, 90 insertions(+), 16 deletions(-)
>  create mode 100644 t/t4013/diff.log_-S_F_master
Well, I like it, though I’m obviously biased.
Show 5 quoted lines
> --- /dev/null
> +++ b/t/t4013/diff.log_-S_F_master
> @@ -0,0 +1,7 @@
> +$ git log -S F master
> +commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
Note to self: a nice mini-series might be to blur these out.
Show 16 quoted lines
> +++ b/t/t4202-log.sh
> @@ -101,11 +101,16 @@ test_expect_success 'oneline' '
>  test_expect_success 'diff-filter=A' '
>  
>  	actual=$(git log --pretty="format:%s" --diff-filter=A HEAD) &&
> +	actual_detached=$(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
> +	} &&
> +	test "$actual" = "$actual_detached" || {
> +		echo Oops. Detached form broken
> +		echo "Actual_detached: $actual_detached"
>  	}
You left out the crucial "false"! :)

Below is an add-on patch to use a more readable style. With or without that change:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Show changes to t/t4202-log.sh +5 −12
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 3352935..36870c5 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -100,18 +100,11 @@ test_expect_success 'oneline' '
 
 test_expect_success 'diff-filter=A' '
 
-	actual=$(git log --pretty="format:%s" --diff-filter=A HEAD) &&
-	actual_detached=$(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
-	} &&
-	test "$actual" = "$actual_detached" || {
-		echo Oops. Detached form broken
-		echo "Actual_detached: $actual_detached"
-	}
+	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
 
 '
 
Matthieu Moy· Jul 29, 2010, 07:19 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/4 v2] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 7 quoted lines
>> +	} &&
>> +	test "$actual" = "$actual_detached" || {
>> +		echo Oops. Detached form broken
>> +		echo "Actual_detached: $actual_detached"
>>  	}
>
> You left out the crucial "false"! :)
Oups, right.
> Below is an add-on patch to use a more readable style.

I was mimicking the style right above, but that makes sense to clean-up while we're there, yes. I'll squash your change in the next serie.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jul 29, 2010, 16:54 UTC · re: Matthieu Moy · lore

Re: [PATCH 1/4 v2] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 11 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>> You left out the crucial "false"! :)
>
> Oups, right.
>
>> Below is an add-on patch to use a more readable style.
>
> I was mimicking the style right above, but that makes sense to
> clean-up while we're there, yes. I'll squash your change in the next
> serie.

Thanks both; I've also seen Jonathan's --stat-* rewrite and agree with him. Looking forward to see a re-roll.

Matthieu Moy· Jul 28, 2010, 09:41 UTC · re: Matthieu Moy · lore

[PATCH 2/4 v2] Allow detached form for git diff --stat-name-width and --stat-width.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Show changes to diff.c +14 −0
diff --git a/diff.c b/diff.c
index 1868b3e..04e97ef 100644
--- a/diff.c
+++ b/diff.c
@@ -3071,6 +3071,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!strcmp(arg, "-s"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
 	else if (!prefixcmp(arg, "--stat")) {
+		argcount = 1;
 		char *end;
 		int width = options->stat_width;
 		int name_width = options->stat_name_width;
@@ -3079,8 +3080,20 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 
 		switch (*arg) {
 		case '-':
+			if (!strcmp(arg, "-width")) {
+				if (!av[1])
+					die("Option `--stat-width' requires a value");
+				width = strtoul(av[1], &end, 10);
+				argcount = 2;
+			}
 			if (!prefixcmp(arg, "-width="))
 				width = strtoul(arg + 7, &end, 10);
+			else if (!strcmp(arg, "-name-width")) {
+				if (!av[1])
+					die("Option `--stat-name-width' requires a value");
+				name_width = strtoul(av[1], &end, 10);
+				argcount = 2;
+			}				
 			else if (!prefixcmp(arg, "-name-width="))
 				name_width = strtoul(arg + 12, &end, 10);
 			break;
@@ -3096,6 +3109,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->output_format |= DIFF_FORMAT_DIFFSTAT;
 		options->stat_name_width = name_width;
 		options->stat_width = width;
+		return argcount;
 	}
 
 	/* renames options */
-- 
1.7.2.25.g9ebe3
Jonathan Nieder· Jul 29, 2010, 02:36 UTC · re: Matthieu Moy · lore

Re: [PATCH 2/4 v2] Allow detached form for git diff --stat-name-width and --stat-width.

Matthieu Moy wrote:
>  diff.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
Just some nitpicks:
 
Show 7 quoted lines
> +++ b/diff.c
> @@ -3071,6 +3071,7 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>  	else if (!strcmp(arg, "-s"))
>  		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
>  	else if (!prefixcmp(arg, "--stat")) {
> +		argcount = 1;
>  		char *end;
declaration after statement.
Show 11 quoted lines
> @@ -3079,8 +3080,20 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>  
>  		switch (*arg) {
>  		case '-':
> +			if (!strcmp(arg, "-width")) {
> +				if (!av[1])
> +					die("Option `--stat-width' requires a value");
> +				width = strtoul(av[1], &end, 10);
> +				argcount = 2;
> +			}
>  			if (!prefixcmp(arg, "-width="))
Could save an extra string comparison with an "else" here.
[...]
> +				name_width = strtoul(av[1], &end, 10);
> +				argcount = 2;
> +			}				
Whitespace damage: trailing tabs.
How about something like the following?
Jonathan Nieder (1):
  diff: split off a function for --stat-* option parsing
Matthieu Moy (1):
  diff: allow --stat-width n, --stat-name-width n
 diff.c |   80 ++++++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 53 insertions(+), 27 deletions(-)
Jonathan Nieder· Jul 29, 2010, 02:37 UTC · re: Jonathan Nieder · lore

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

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>
---
 diff.c |   62 +++++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 35 insertions(+), 27 deletions(-)
Show changes to diff.c +35 −27
diff --git a/diff.c b/diff.c
index 1868b3e..641a326 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.6.5
Jonathan Nieder· Jul 29, 2010, 02:38 UTC · re: Jonathan Nieder · lore

[PATCH 2/2] diff: allow --stat-width n, --stat-name-width n

From: Matthieu Moy <Matthieu.Moy@imag.fr>
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(-)
Show changes to diff.c +23 −5
diff --git a/diff.c b/diff.c
index 641a326..55edc3e 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.6.5
Matthieu Moy· Jul 28, 2010, 09:41 UTC · re: Matthieu Moy · lore

[PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 revision.c     |   86 ++++++++++++++++++++++++++++++++++---------------------
 t/t4202-log.sh |    7 ++++
 2 files changed, 60 insertions(+), 33 deletions(-)
Show changes to 2 files +60 −33

revision.c, t/t4202-log.sh

diff --git a/revision.c b/revision.c
index 7e82efd..92035a3 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,27 +1305,32 @@ 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);
 	} else if (!strcmp(arg, "--show-notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
-	} else if (!prefixcmp(arg, "--show-notes=")) {
+	} else if ((argcount = diff_long_opt("show-notes", argv, &optarg))) {
 		struct strbuf buf = STRBUF_INIT;
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
 		if (!revs->notes_opt.extra_notes_refs)
 			revs->notes_opt.extra_notes_refs = xcalloc(1, sizeof(struct string_list));
-		if (!prefixcmp(arg+13, "refs/"))
+		if (!prefixcmp(optarg, "refs/"))
 			/* happy */;
-		else if (!prefixcmp(arg+13, "notes/"))
+		else if (!prefixcmp(optarg, "notes/"))
 			strbuf_addstr(&buf, "refs/");
 		else
 			strbuf_addstr(&buf, "refs/notes/");
-		strbuf_addstr(&buf, arg+13);
+		strbuf_addstr(&buf, optarg);
 		string_list_append(revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));
+		return argcount;
 	} else if (!strcmp(arg, "--no-notes")) {
 		revs->show_notes = 0;
 		revs->show_notes_given = 1;
@@ -1343,12 +1358,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->abbrev = 0;
 	} else if (!strcmp(arg, "--abbrev")) {
 		revs->abbrev = DEFAULT_ABBREV;
-	} else if (!prefixcmp(arg, "--abbrev=")) {
-		revs->abbrev = strtoul(arg + 9, NULL, 10);
+	} else if ((argcount = diff_long_opt("abbrev", argv, &optarg))) {
+		revs->abbrev = strtoul(optarg, NULL, 10);
 		if (revs->abbrev < MINIMUM_ABBREV)
 			revs->abbrev = MINIMUM_ABBREV;
 		else if (revs->abbrev > 40)
 			revs->abbrev = 40;
+		return argcount;
 	} else if (!strcmp(arg, "--abbrev-commit")) {
 		revs->abbrev_commit = 1;
 	} else if (!strcmp(arg, "--full-diff")) {
@@ -1359,21 +1375,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 +1402,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 3352935..f912589 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -208,6 +208,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.25.g9ebe3
Ævar Arnfjörð Bjarmason· Jul 28, 2010, 10:11 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.

On Wed, Jul 28, 2010 at 09:41, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:

Here's a better commit message, the subject was >50 chars> (see Documentation/SubmittingPatches):

    Subject: git-log: Parse detached options like --grep foo
    Change the option parsing logic in revision.c to accept detached forms
    like `--grep foo' in addition to `--grep=foo'. The rest of git already
    accepted this form, but revision.c still used its own option parsing.
Show 6 quoted lines
> +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
> +'

There's a lot of behavior change in this series, but only two small tests that I can see. It would be easy to change the parsing code back without triggering a regression test.

Matthieu Moy· Jul 28, 2010, 11:29 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Wed, Jul 28, 2010 at 09:41, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>
> Here's a better commit message, the subject was >50 chars> (see
> Documentation/SubmittingPatches):

I don't see a mention of 50 chars there. Conventions usually limit lines to <80 chars (sometimes 72), and my subject line is 64 chars-wide if you don't count [PATCH], which won't end up in git.git.

Show 9 quoted lines
>> +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
>> +'
>
> There's a lot of behavior change in this series, but only two small
> tests that I can see.
4, not 2.
> It would be easy to change the parsing code back without triggering
> a regression test.

I've introduced a test for each pattern, but deliberately did not do exhaustive testing: I'm testing the patterns, not the way they are applied. In the same way, I don't think there's a testcase for each use of parse-option in the testsuite.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Ævar Arnfjörð Bjarmason· Jul 28, 2010, 12:56 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.

On Wed, Jul 28, 2010 at 11:29, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 10 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Wed, Jul 28, 2010 at 09:41, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:
>>
>> Here's a better commit message, the subject was >50 chars> (see
>> Documentation/SubmittingPatches):
>
> I don't see a mention of 50 chars there. Conventions usually limit
> lines to <80 chars (sometimes 72), and my subject line is 64
> chars-wide if you don't count [PATCH], which won't end up in git.git.
Hrm, I thought it was explicitly mentioned there, but it says:
	- the first line of the commit message should be a short
	  description and should skip the full stop
Since your patch subject had a full stop I thought I'd mention it.

DISCUSSION in git-commit(1) and gittutorial(7) mention 50 characters explicitly though, and a lot of tools that present commits in short form use that as a soft limit, e.g. gitweb and github.

I'll submit a patch to SubmittingPatches citing the 50 char soft limit.

Show 11 quoted lines
>>> +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
>>> +'
>>
>> There's a lot of behavior change in this series, but only two small
>> tests that I can see.
>
> 4, not 2.

I only saw two test_expect_success additions in your v2 series, maybe I missed one.

Show 7 quoted lines
>> It would be easy to change the parsing code back without triggering
>> a regression test.
>
> I've introduced a test for each pattern, but deliberately did not do
> exhaustive testing: I'm testing the patterns, not the way they are
> applied. In the same way, I don't think there's a testcase for each
> use of parse-option in the testsuite.
Not for everything it seems, but the coverage is pretty good:
    http://v.nix.is/~avar/cover_db_html/parse-options-c.html
    http://v.nix.is/~avar/cover_db_html/test-parse-options-c.html

Anyway. Regression tests can be added later, and I think this series is good enough for inclusion as-is. But option parsing has been a moving target in Git for a while now, and it's quite likely that this code will get rewritten down the line to use a new library that can parse all of Git's option syntax.

More tests coverage would be valuable for catching regressions in such future rewrites. In general that's the main value, not making sure *your* code works, but that someone else doesn't break it.

Anyway, here's how you can easily get almost complete coverage at almost no cost for this series, first make a t/lib-log.sh like this:

    #!/bin/sh
    log_expect_success() {
        desc=$1
        text=$2
        test_expect_success "Phony test with attached options: $1" "$2"
        mocked=$(echo "$2" | sed -r 's/([A-Za-z-]+)=/\1 /')
        test_expect_success "Phony test with detached options: $1" "$mocked"
    }

Then just change the t4*log*.sh tests that aren't doing setup work (like git init) to use it:

    diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
    index cb9f2bd..7ccc4bb 100755
    --- a/t/t4205-log-pretty-formats.sh
    +++ b/t/t4205-log-pretty-formats.sh
    @@ -20 +20 @@ test_expect_success 'set up basic repos' '
    -test_expect_success 'alias builtin format' '
    +log_expect_success 'alias builtin format' '
We'll be running tests twice, but it'll catch these at least:
    $ ack -h -i '(--[a-z-]+=)' --output '$1' t4*log*.sh | sort | uniq
    --abbrev=
    --author=
    --color=
    --decorate=
    --diff-filter=
    --format=
    --grep=
    --pretty=

I can submit something like that later if I get around to it, or you can include it in your series if you want.

Matthieu Moy· Jul 28, 2010, 14:00 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> DISCUSSION in git-commit(1) and gittutorial(7) mention 50 characters
> explicitly though, and a lot of tools that present commits in short
> form use that as a soft limit, e.g. gitweb and github.
Where does github have this limitation?

On http://github.com/git/git/commits/master, I can see messages like this:

git-read-tree.txt: acknowledge the directory matching bug in sparse checkout
76 chars-wide, untruncated.
> I'll submit a patch to SubmittingPatches citing the 50 char soft
> limit.

git$ git log --format='%s' | wc -l 22700 git$ git log --format='%s' | grep '..................................................' | wc -l 9392

Almost half of the commits already there do not comply with this supposed rule.

I'm fine with rewriting the subject, but the claim that commit messages should be <50 chars is just not reasonable and has never been applied to git.git.

> I only saw two test_expect_success additions in your v2 series, maybe
> I missed one.

I'm putting the diff for the t/ directory below to sum up the tests added.

> Not for everything it seems, but the coverage is pretty good:
>
>     http://v.nix.is/~avar/cover_db_html/parse-options-c.html
>     http://v.nix.is/~avar/cover_db_html/test-parse-options-c.html

You're precisely illustrating my point: these coverage results are about parse-option, not about the places where it is used. For example:

git/t$ git grep -e '--message' git/t$

=> git commit --message is not tested, at all (but git commit -m obviously is).

In my case, I didn't gcov it, but I think I've got 100% coverage on the helper functions (short_opt and diff_long_opt), just not 100% on the places where they're used.

Show 12 quoted lines
> Anyway, here's how you can easily get almost complete coverage at
> almost no cost for this series, first make a t/lib-log.sh like this:
>
>     #!/bin/sh
>
>     log_expect_success() {
>         desc=$1
>         text=$2
>         test_expect_success "Phony test with attached options: $1" "$2"
>         mocked=$(echo "$2" | sed -r 's/([A-Za-z-]+)=/\1 /')
>         test_expect_success "Phony test with detached options: $1" "$mocked"
>     }
You should check whether "$mocked" == "$2" to avoid useless re-run.
> I can submit something like that later if I get around to it,
Go ahead, I've exhausted my git time budget ;-).
Show changes to 4 files +26 −0

t/t4013-diff-various.sh, t/t4013/diff.log_-S_F_master, t/t4202-log.sh, t/t6018-rev-list-glob.sh

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index dae6358..8036e00 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
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..f912589 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -101,11 +101,16 @@ test_expect_success 'oneline' '
 test_expect_success 'diff-filter=A' '
 
 	actual=$(git log --pretty="format:%s" --diff-filter=A HEAD) &&
+	actual_detached=$(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
+	} &&
+	test "$actual" = "$actual_detached" || {
+		echo Oops. Detached form broken
+		echo "Actual_detached: $actual_detached"
 	}
 
 '
@@ -203,6 +208,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 &&
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/*"
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Ævar Arnfjörð Bjarmason· Jul 28, 2010, 15:03 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.

On Wed, Jul 28, 2010 at 14:00, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 14 quoted lines
> Ęvar Arnfjörš Bjarmason <avarab@gmail.com> writes:
>
>> DISCUSSION in git-commit(1) and gittutorial(7) mention 50 characters
>> explicitly though, and a lot of tools that present commits in short
>> form use that as a soft limit, e.g. gitweb and github.
>
> Where does github have this limitation?
>
> On http://github.com/git/git/commits/master, I can see messages like
> this:
>
> git-read-tree.txt: acknowledge the directory matching bug in sparse checkout
>
> 76 chars-wide, untruncated.
A lot of applications do it in their public timeline, e.g. at GitHub:
    http://github.com/mirrors

Although their limit seems to be closer to 65 characters now, but I'm fairly sure that it was 50 before. It's also overflowing my screen now as a result.

Show 10 quoted lines
>> I'll submit a patch to SubmittingPatches citing the 50 char soft
>> limit.
>
> git$ git log --format='%s' | wc -l
> 22700
> git$ git log --format='%s' | grep '..................................................' | wc -l
> 9392
>
> Almost half of the commits already there do not comply with this
> supposed rule.

It's not a rule, just a soft limit. Anyway, I just mentioned it because the subject ended with a full stop. Which indicated that you hadn't spotted that bit in SubmittingPatches, and might want to know for future submissions.

There's also a quickly decreasing long tail after 50 characters.
    $ for i in {50..70}; do git log --format='%s' | egrep ".{$i}" | wc
-l | tr '\n' ' '; done && echo
    9391 8847 8341 7838 7338 6840 6379 5915 5456 5018 4585 4184 3779
3418 3045 2751 2434 2153 1871 1655 1438
(Use Extended Regular Expressions, your "." key will thank you >:)
Show 9 quoted lines
> I'm fine with rewriting the subject, but the claim that commit
> messages should be <50 chars is just not reasonable and has never been
> applied to git.git.
>
>> I only saw two test_expect_success additions in your v2 series, maybe
>> I missed one.
>
> I'm putting the diff for the t/ directory below to sum up the tests
> added.

Thanks. I missed 2/4 because I only grep-ed for the test harness functions.

Show 11 quoted lines
>> Not for everything it seems, but the coverage is pretty good:
>>
>>     http://v.nix.is/~avar/cover_db_html/parse-options-c.html
>>     http://v.nix.is/~avar/cover_db_html/test-parse-options-c.html
>
> You're precisely illustrating my point: these coverage results are
> about parse-option, not about the places where it is used. For
> example:
>
> git/t$ git grep -e '--message'
> git/t$
*added to my list of stuff to test*
Show 6 quoted lines
> => git commit --message is not tested, at all (but git commit -m
> obviously is).
>
> In my case, I didn't gcov it, but I think I've got 100% coverage on
> the helper functions (short_opt and diff_long_opt), just not 100% on
> the places where they're used.

I just mentioned the coverage as an aside (and probably shouldn't have. What I mainly meant is that it would be easy to regress on option parsing in revision.c in the future because e.g. nothing tests both "--abbrev foo' and "--abbrev=foo".

Show 14 quoted lines
>> Anyway, here's how you can easily get almost complete coverage at
>> almost no cost for this series, first make a t/lib-log.sh like this:
>>
>>     #!/bin/sh
>>
>>     log_expect_success() {
>>         desc=$1
>>         text=$2
>>         test_expect_success "Phony test with attached options: $1" "$2"
>>         mocked=$(echo "$2" | sed -r 's/([A-Za-z-]+)=/\1 /')
>>         test_expect_success "Phony test with detached options: $1" "$mocked"
>>     }
>
> You should check whether "$mocked" == "$2" to avoid useless re-run.

Yeah, it's just pseudocode. Assigning to desc and text and not using them is also pretty useless. I didn't even run it, but I'm delighted to find that it at least compiles and executes.

>> I can submit something like that later if I get around to it,
>
> Go ahead, I've exhausted my git time budget ;-).

Maybe I will. Anyway, like I said this patch looks just fine. I mainly just wanted to note that it was under-tested (but that it should go in anyway, IMO), if for no other reason than as a future note to myself. Or anyone else wanting to improve our test coverage.

Jonathan Nieder· Jul 29, 2010, 02:46 UTC · re: Matthieu Moy · lore

Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.

Matthieu Moy wrote:
> +++ b/revision.c
[...]
Show 16 quoted lines
> @@ -1295,27 +1305,32 @@ 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);
>  	} else if (!strcmp(arg, "--show-notes")) {
>  		revs->show_notes = 1;
>  		revs->show_notes_given = 1;
> -	} else if (!prefixcmp(arg, "--show-notes=")) {
> +	} else if ((argcount = diff_long_opt("show-notes", argv, &optarg))) {
Why is this not like the --pretty case?
[...]
Show 8 quoted lines
> @@ -1343,12 +1358,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  		revs->abbrev = 0;
>  	} else if (!strcmp(arg, "--abbrev")) {
>  		revs->abbrev = DEFAULT_ABBREV;
> -	} else if (!prefixcmp(arg, "--abbrev=")) {
> -		revs->abbrev = strtoul(arg + 9, NULL, 10);
> +	} else if ((argcount = diff_long_opt("abbrev", argv, &optarg))) {
> +		revs->abbrev = strtoul(optarg, NULL, 10);
Likewise.
> +	test_must_fail git log -1 --pretty="tformat:%s" --grep
Thanks for checking an edge case.
With or without the following change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Show changes to revision.c +6 −8
diff --git a/revision.c b/revision.c
index 92035a3..7006340 100644
--- a/revision.c
+++ b/revision.c
@@ -1315,22 +1315,21 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!strcmp(arg, "--show-notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
-	} else if ((argcount = diff_long_opt("show-notes", argv, &optarg))) {
+	} else if (!prefixcmp(arg, "--show-notes=")) {
 		struct strbuf buf = STRBUF_INIT;
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
 		if (!revs->notes_opt.extra_notes_refs)
 			revs->notes_opt.extra_notes_refs = xcalloc(1, sizeof(struct string_list));
-		if (!prefixcmp(optarg, "refs/"))
+		if (!prefixcmp(arg+13, "refs/"))
 			/* happy */;
-		else if (!prefixcmp(optarg, "notes/"))
+		else if (!prefixcmp(arg+13, "notes/"))
 			strbuf_addstr(&buf, "refs/");
 		else
 			strbuf_addstr(&buf, "refs/notes/");
-		strbuf_addstr(&buf, optarg);
+		strbuf_addstr(&buf, arg+13);
 		string_list_append(revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));
-		return argcount;
 	} else if (!strcmp(arg, "--no-notes")) {
 		revs->show_notes = 0;
 		revs->show_notes_given = 1;
@@ -1358,13 +1357,12 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->abbrev = 0;
 	} else if (!strcmp(arg, "--abbrev")) {
 		revs->abbrev = DEFAULT_ABBREV;
-	} else if ((argcount = diff_long_opt("abbrev", argv, &optarg))) {
-		revs->abbrev = strtoul(optarg, NULL, 10);
+	} else if (!prefixcmp(arg, "--abbrev=")) {
+		revs->abbrev = strtoul(arg + 9, NULL, 10);
 		if (revs->abbrev < MINIMUM_ABBREV)
 			revs->abbrev = MINIMUM_ABBREV;
 		else if (revs->abbrev > 40)
 			revs->abbrev = 40;
-		return argcount;
 	} else if (!strcmp(arg, "--abbrev-commit")) {
 		revs->abbrev_commit = 1;
 	} else if (!strcmp(arg, "--full-diff")) {
Matthieu Moy· Jul 28, 2010, 09:41 UTC · re: Matthieu Moy · lore

[PATCH 4/4 v2] Allow detached form for --glob, --branches, --tags and --remote.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 revision.c               |   22 ++++++++++++++--------
 t/t6018-rev-list-glob.sh |    6 ++++++
 2 files changed, 20 insertions(+), 8 deletions(-)
Show changes to 2 files +20 −8

revision.c, t/t6018-rev-list-glob.sh

diff --git a/revision.c b/revision.c
index 92035a3..33fa0b5 100644
--- a/revision.c
+++ b/revision.c
@@ -1486,6 +1486,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;
@@ -1532,28 +1534,32 @@ 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=")) {
+			if ((argcount = diff_long_opt("branches", argv + i, &optarg))) {
+				i += argcount - 1;
 				struct all_refs_cb cb;
 				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
+				for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
 				continue;
 			}
-			if (!prefixcmp(arg, "--tags=")) {
+			if ((argcount = diff_long_opt("tags", argv + i, &optarg))) {
+				i += argcount - 1;
 				struct all_refs_cb cb;
 				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
+				for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
 				continue;
 			}
-			if (!prefixcmp(arg, "--remotes=")) {
+			if ((argcount = diff_long_opt("remotes", argv + i, &optarg))) {
+				i += argcount - 1;
 				struct all_refs_cb cb;
 				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
+				for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
 				continue;
 			}
 			if (!strcmp(arg, "--reflog")) {
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.25.g9ebe3
Jonathan Nieder· Jul 29, 2010, 02:48 UTC · re: Matthieu Moy · lore

Re: [PATCH 4/4 v2] Allow detached form for --glob, --branches, --tags and --remote.

Matthieu Moy wrote:
> -			if (!prefixcmp(arg, "--branches=")) {
> +			if ((argcount = diff_long_opt("branches", argv + i, &optarg))) {
> +				i += argcount - 1;
>  				struct all_refs_cb cb;

As before: is this the right change where the argument is optional?

Thanks for a pleasant read. Jonathan

Matthieu Moy· Jul 27, 2010, 21:21 UTC · re: Matthieu Moy · lore
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 diff.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Show changes to diff.c +14 −0
diff --git a/diff.c b/diff.c
index 41b39be..c8ee603 100644
--- a/diff.c
+++ b/diff.c
@@ -3044,6 +3044,7 @@ int diff_opt_parse(struct diff_options *options, const char **argv, int ac)
 	else if (!strcmp(arg, "-s"))
 		options->output_format |= DIFF_FORMAT_NO_OUTPUT;
 	else if (!prefixcmp(arg, "--stat")) {
+		argcount = 1;
 		char *end;
 		int width = options->stat_width;
 		int name_width = options->stat_name_width;
@@ -3052,8 +3053,20 @@ int diff_opt_parse(struct diff_options *options, const char **argv, int ac)
 
 		switch (*arg) {
 		case '-':
+			if (!strcmp(arg, "-width")) {
+				if (!argv[1])
+					die("Option `--stat-width' requires a value");
+				width = strtoul(argv[1], &end, 10);
+				argcount = 2;
+			}
 			if (!prefixcmp(arg, "-width="))
 				width = strtoul(arg + 7, &end, 10);
+			else if (!strcmp(arg, "-name-width")) {
+				if (!argv[1])
+					die("Option `--stat-name-width' requires a value");
+				name_width = strtoul(argv[1], &end, 10);
+				argcount = 2;
+			}				
 			else if (!prefixcmp(arg, "-name-width="))
 				name_width = strtoul(arg + 12, &end, 10);
 			break;
@@ -3069,6 +3082,7 @@ int diff_opt_parse(struct diff_options *options, const char **argv, int ac)
 		options->output_format |= DIFF_FORMAT_DIFFSTAT;
 		options->stat_name_width = name_width;
 		options->stat_width = width;
+		return argcount;
 	}
 
 	/* renames options */
-- 
1.7.2.25.g9ebe3
Matthieu Moy· Jul 27, 2010, 21:21 UTC · re: Matthieu Moy · lore

[PATCH 3/4] Allow detached form (e.g. "git log --grep foo") in log options.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 revision.c     |   86 ++++++++++++++++++++++++++++++++++---------------------
 t/t4202-log.sh |    7 ++++
 2 files changed, 60 insertions(+), 33 deletions(-)
Show changes to 2 files +60 −33

revision.c, t/t4202-log.sh

diff --git a/revision.c b/revision.c
index 7e82efd..5d62340 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 (IF_LONG_OPT(max-count)) {
+		revs->max_count = atoi(optarg);
 		revs->no_walk = 0;
-	} else if (!prefixcmp(arg, "--skip=")) {
-		revs->skip_count = atoi(arg + 7);
+		return argcount;
+	} else if (IF_LONG_OPT(skip)) {
+		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 (IF_LONG_OPT(max-age)) {
+		revs->max_age = atoi(optarg);
+		return argcount;
+	} else if (IF_LONG_OPT(since)) {
+		revs->max_age = approxidate(optarg);
+		return argcount;
+	} else if (IF_LONG_OPT(after)) {
+		revs->max_age = approxidate(optarg);
+		return argcount;
+	} else if (IF_LONG_OPT(min-age)) {
+		revs->min_age = atoi(optarg);
+		return argcount;
+	} else if (IF_LONG_OPT(before)) {
+		revs->min_age = approxidate(optarg);
+		return argcount;
+	} else if (IF_LONG_OPT(until)) {
+		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,27 +1305,32 @@ 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);
 	} else if (!strcmp(arg, "--show-notes")) {
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
-	} else if (!prefixcmp(arg, "--show-notes=")) {
+	} else if (IF_LONG_OPT(show-notes)) {
 		struct strbuf buf = STRBUF_INIT;
 		revs->show_notes = 1;
 		revs->show_notes_given = 1;
 		if (!revs->notes_opt.extra_notes_refs)
 			revs->notes_opt.extra_notes_refs = xcalloc(1, sizeof(struct string_list));
-		if (!prefixcmp(arg+13, "refs/"))
+		if (!prefixcmp(optarg, "refs/"))
 			/* happy */;
-		else if (!prefixcmp(arg+13, "notes/"))
+		else if (!prefixcmp(optarg, "notes/"))
 			strbuf_addstr(&buf, "refs/");
 		else
 			strbuf_addstr(&buf, "refs/notes/");
-		strbuf_addstr(&buf, arg+13);
+		strbuf_addstr(&buf, optarg);
 		string_list_append(revs->notes_opt.extra_notes_refs,
 				   strbuf_detach(&buf, NULL));
+		return argcount;
 	} else if (!strcmp(arg, "--no-notes")) {
 		revs->show_notes = 0;
 		revs->show_notes_given = 1;
@@ -1343,12 +1358,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->abbrev = 0;
 	} else if (!strcmp(arg, "--abbrev")) {
 		revs->abbrev = DEFAULT_ABBREV;
-	} else if (!prefixcmp(arg, "--abbrev=")) {
-		revs->abbrev = strtoul(arg + 9, NULL, 10);
+	} else if (IF_LONG_OPT(abbrev)) {
+		revs->abbrev = strtoul(optarg, NULL, 10);
 		if (revs->abbrev < MINIMUM_ABBREV)
 			revs->abbrev = MINIMUM_ABBREV;
 		else if (revs->abbrev > 40)
 			revs->abbrev = 40;
+		return argcount;
 	} else if (!strcmp(arg, "--abbrev-commit")) {
 		revs->abbrev_commit = 1;
 	} else if (!strcmp(arg, "--full-diff")) {
@@ -1359,21 +1375,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 (IF_LONG_OPT(date)) {
+		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 (IF_LONG_OPT(author)) {
+		add_header_grep(revs, GREP_HEADER_AUTHOR, optarg);
+		return argcount;
+	} else if (IF_LONG_OPT(committer)) {
+		add_header_grep(revs, GREP_HEADER_COMMITTER, optarg);
+		return argcount;
+	} else if (IF_LONG_OPT(grep)) {
+		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 +1402,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 (IF_LONG_OPT(encoding)) {
 		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 3352935..f912589 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -208,6 +208,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.25.g9ebe3
Matthieu Moy· Jul 27, 2010, 21:21 UTC · re: Matthieu Moy · lore

[PATCH 4/4] Allow detached form for --glob, --branches, --tags and --remote.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 revision.c               |   31 +++++++++++++++++++++++--------
 t/t6018-rev-list-glob.sh |    6 ++++++
 2 files changed, 29 insertions(+), 8 deletions(-)
Show changes to 2 files +29 −8

revision.c, t/t6018-rev-list-glob.sh

diff --git a/revision.c b/revision.c
index 5d62340..93e6233 100644
--- a/revision.c
+++ b/revision.c
@@ -1486,6 +1486,7 @@ 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;
 
 	/* First, search for "--" */
 	seen_dashdash = 0;
@@ -1501,6 +1502,20 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		break;
 	}
 
+	/*
+	 * Variant of IF_LONG_OPT, for setup_revisions (use argv[i+1]
+	 * instead of argv[1], increment i directly)
+	 */
+#define IF_LONG_OPT_SR(optname)					\
+	((!strcmp(arg, "--" #optname)				\
+	  && (argv[i+1] || (die("Option `" #optname "' requires a value"), 1), \
+	      optarg   = argv[i+1],	       			\
+	      i++,						\
+	      1)) ||						\
+	 (!prefixcmp(arg, "--" #optname "=")			\
+	  && (optarg = arg + strlen("--" #optname "="),		\
+	      1)))
+
 	/* Second, deal with arguments and options */
 	flags = 0;
 	read_from_stdin = 0;
@@ -1532,28 +1547,28 @@ 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 (IF_LONG_OPT_SR(glob)) {
 				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=")) {
+			if (IF_LONG_OPT_SR(branches)) {
 				struct all_refs_cb cb;
 				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", &cb);
+				for_each_glob_ref_in(handle_one_ref, optarg, "refs/heads/", &cb);
 				continue;
 			}
-			if (!prefixcmp(arg, "--tags=")) {
+			if (IF_LONG_OPT_SR(tags)) {
 				struct all_refs_cb cb;
 				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", &cb);
+				for_each_glob_ref_in(handle_one_ref, optarg, "refs/tags/", &cb);
 				continue;
 			}
-			if (!prefixcmp(arg, "--remotes=")) {
+			if (IF_LONG_OPT_SR(remotes)) {
 				struct all_refs_cb cb;
 				init_all_refs_cb(&cb, revs, flags);
-				for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", &cb);
+				for_each_glob_ref_in(handle_one_ref, optarg, "refs/remotes/", &cb);
 				continue;
 			}
 			if (!strcmp(arg, "--reflog")) {
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.25.g9ebe3

← back to recent threads