* [PATCH 1/4 v2] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options
2010-07-28 7:38 ` Matthieu Moy
@ 2010-07-28 9:40 ` Matthieu Moy
2010-07-29 2:00 ` Jonathan Nieder
2010-07-28 9:41 ` [PATCH 2/4 v2] Allow detached form for git diff --stat-name-width and --stat-width Matthieu Moy
` (2 subsequent siblings)
3 siblings, 1 reply; 24+ messages in thread
From: Matthieu Moy @ 2010-07-28 9:40 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
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
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 1/4 v2] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options
2010-07-28 9:40 ` [PATCH 1/4 v2] " Matthieu Moy
@ 2010-07-29 2:00 ` Jonathan Nieder
2010-07-29 7:19 ` Matthieu Moy
0 siblings, 1 reply; 24+ messages in thread
From: Jonathan Nieder @ 2010-07-29 2:00 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster
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.
> --- /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.
> +++ 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.
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
'
--
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 1/4 v2] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options
2010-07-29 2:00 ` Jonathan Nieder
@ 2010-07-29 7:19 ` Matthieu Moy
2010-07-29 16:54 ` Junio C Hamano
0 siblings, 1 reply; 24+ messages in thread
From: Matthieu Moy @ 2010-07-29 7:19 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, gitster
Jonathan Nieder <jrnieder@gmail.com> writes:
>> + } &&
>> + 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/
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/4 v2] Allow detached form (e.g. "-S foo" instead of "-Sfoo") for diff options
2010-07-29 7:19 ` Matthieu Moy
@ 2010-07-29 16:54 ` Junio C Hamano
0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2010-07-29 16:54 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Jonathan Nieder, git
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/4 v2] Allow detached form for git diff --stat-name-width and --stat-width.
2010-07-28 7:38 ` Matthieu Moy
2010-07-28 9:40 ` [PATCH 1/4 v2] " Matthieu Moy
@ 2010-07-28 9:41 ` Matthieu Moy
2010-07-29 2:36 ` Jonathan Nieder
2010-07-28 9:41 ` [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options Matthieu Moy
2010-07-28 9:41 ` [PATCH 4/4 v2] Allow detached form for --glob, --branches, --tags and --remote Matthieu Moy
3 siblings, 1 reply; 24+ messages in thread
From: Matthieu Moy @ 2010-07-28 9:41 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
diff.c | 14 ++++++++++++++
1 files changed, 14 insertions(+), 0 deletions(-)
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 2/4 v2] Allow detached form for git diff --stat-name-width and --stat-width.
2010-07-28 9:41 ` [PATCH 2/4 v2] Allow detached form for git diff --stat-name-width and --stat-width Matthieu Moy
@ 2010-07-29 2:36 ` Jonathan Nieder
2010-07-29 2:37 ` [PATCH 1/2] diff: split off a function for --stat-* option parsing Jonathan Nieder
2010-07-29 2:38 ` [PATCH 2/2] diff: allow --stat-width n, --stat-name-width n Jonathan Nieder
0 siblings, 2 replies; 24+ messages in thread
From: Jonathan Nieder @ 2010-07-29 2:36 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster
Matthieu Moy wrote:
> diff.c | 14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
Just some nitpicks:
> +++ 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.
> @@ -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(-)
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH 1/2] diff: split off a function for --stat-* option parsing
2010-07-29 2:36 ` Jonathan Nieder
@ 2010-07-29 2:37 ` Jonathan Nieder
2010-07-29 2:38 ` [PATCH 2/2] diff: allow --stat-width n, --stat-name-width n Jonathan Nieder
1 sibling, 0 replies; 24+ messages in thread
From: Jonathan Nieder @ 2010-07-29 2:37 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster
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(-)
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH 2/2] diff: allow --stat-width n, --stat-name-width n
2010-07-29 2:36 ` Jonathan Nieder
2010-07-29 2:37 ` [PATCH 1/2] diff: split off a function for --stat-* option parsing Jonathan Nieder
@ 2010-07-29 2:38 ` Jonathan Nieder
1 sibling, 0 replies; 24+ messages in thread
From: Jonathan Nieder @ 2010-07-29 2:38 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster
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(-)
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.
2010-07-28 7:38 ` Matthieu Moy
2010-07-28 9:40 ` [PATCH 1/4 v2] " Matthieu Moy
2010-07-28 9:41 ` [PATCH 2/4 v2] Allow detached form for git diff --stat-name-width and --stat-width Matthieu Moy
@ 2010-07-28 9:41 ` Matthieu Moy
2010-07-28 10:11 ` Ævar Arnfjörð Bjarmason
2010-07-29 2:46 ` Jonathan Nieder
2010-07-28 9:41 ` [PATCH 4/4 v2] Allow detached form for --glob, --branches, --tags and --remote Matthieu Moy
3 siblings, 2 replies; 24+ messages in thread
From: Matthieu Moy @ 2010-07-28 9:41 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
revision.c | 86 ++++++++++++++++++++++++++++++++++---------------------
t/t4202-log.sh | 7 ++++
2 files changed, 60 insertions(+), 33 deletions(-)
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.
2010-07-28 9:41 ` [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options Matthieu Moy
@ 2010-07-28 10:11 ` Ævar Arnfjörð Bjarmason
2010-07-28 11:29 ` Matthieu Moy
2010-07-29 2:46 ` Jonathan Nieder
1 sibling, 1 reply; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-07-28 10:11 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster
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.
> +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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.
2010-07-28 10:11 ` Ævar Arnfjörð Bjarmason
@ 2010-07-28 11:29 ` Matthieu Moy
2010-07-28 12:56 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 24+ messages in thread
From: Matthieu Moy @ 2010-07-28 11:29 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, gitster
Æ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.
>> +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/
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.
2010-07-28 11:29 ` Matthieu Moy
@ 2010-07-28 12:56 ` Ævar Arnfjörð Bjarmason
2010-07-28 14:00 ` Matthieu Moy
0 siblings, 1 reply; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-07-28 12:56 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster
On Wed, Jul 28, 2010 at 11:29, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Æ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.
>>> +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.
>> 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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.
2010-07-28 12:56 ` Ævar Arnfjörð Bjarmason
@ 2010-07-28 14:00 ` Matthieu Moy
2010-07-28 15:03 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 24+ messages in thread
From: Matthieu Moy @ 2010-07-28 14:00 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, gitster
Æ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.
> 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 ;-).
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/
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.
2010-07-28 14:00 ` Matthieu Moy
@ 2010-07-28 15:03 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 24+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2010-07-28 15:03 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster
On Wed, Jul 28, 2010 at 14:00, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Ę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.
>> 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 >:)
> 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.
>> 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*
> => 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".
>> 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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.
2010-07-28 9:41 ` [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options Matthieu Moy
2010-07-28 10:11 ` Ævar Arnfjörð Bjarmason
@ 2010-07-29 2:46 ` Jonathan Nieder
1 sibling, 0 replies; 24+ messages in thread
From: Jonathan Nieder @ 2010-07-29 2:46 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git, gitster
Matthieu Moy wrote:
> +++ b/revision.c
[...]
> @@ -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?
[...]
> @@ -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>
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")) {
--
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 4/4 v2] Allow detached form for --glob, --branches, --tags and --remote.
2010-07-28 7:38 ` Matthieu Moy
` (2 preceding siblings ...)
2010-07-28 9:41 ` [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options Matthieu Moy
@ 2010-07-28 9:41 ` Matthieu Moy
2010-07-29 2:48 ` Jonathan Nieder
3 siblings, 1 reply; 24+ messages in thread
From: Matthieu Moy @ 2010-07-28 9:41 UTC (permalink / raw)
To: git, gitster; +Cc: Matthieu Moy
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(-)
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
^ permalink raw reply related [flat|nested] 24+ messages in thread