[PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk"

[PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk"

[Dongcan Jiang]

Because --graph is about connected history while --no-walk is about discrete points.

revision.c: Judge whether --graph and --no-walk come together when running git-log.
buildin/log.c: Set git-log cmd flag.
Documentation/rev-list-options.txt: Add specification on the forbidden usage.

Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
---
 Documentation/rev-list-options.txt | 2 ++
 builtin/log.c                      | 1 +
 revision.c                         | 4 ++++
 revision.h                         | 3 +++
 4 files changed, 10 insertions(+)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..eea2c0a 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -679,6 +679,7 @@ endif::git-rev-list[]
 	given on the command line. Otherwise (if `sorted` or no argument
 	was given), the commits are shown in reverse chronological order
 	by commit time.
+	Cannot be combined with `--graph` when running git-log.
 
 --do-walk::
 	Overrides a previous `--no-walk`.
@@ -781,6 +782,7 @@ you would get an output like this:
 	on the left hand side of the output.  This may cause extra lines
 	to be printed in between commits, in order for the graph history
 	to be drawn properly.
+	Cannot be combined with `--no-walk` when running git-log.
 +
 This enables parent rewriting, see 'History Simplification' below.
 +
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..7bf5adb 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
 	git_config(git_log_config, NULL);
 
 	init_revisions(&rev, prefix);
+	rev.cmd_is_log = 1;
 	rev.always_show_header = 1;
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
diff --git a/revision.c b/revision.c
index 66520c6..5f62c89 100644
--- a/revision.c
+++ b/revision.c
@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
+	revs->cmd_is_log = 0;
+
 	init_grep_defaults();
 	grep_init(&revs->grep_filter, prefix);
 	revs->grep_filter.status_only = 1;
@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	if (revs->reflog_info && revs->graph)
 		die("cannot combine --walk-reflogs with --graph");
+	if (revs->no_walk && revs->graph && revs->cmd_is_log)
+		die("cannot combine --no-walk with --graph when running git-log");
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
 		die("cannot use --grep-reflog without --walk-reflogs");
 
diff --git a/revision.h b/revision.h
index 0ea8b4e..255982a 100644
--- a/revision.h
+++ b/revision.h
@@ -146,6 +146,9 @@ struct rev_info {
 			track_first_time:1,
 			linear:1;
 
+	/* cmd type */
+	unsigned int  cmd_is_log:1;
+
 	enum date_mode date_mode;
 
 	unsigned int	abbrev;
-- 
2.3.1.251.g83036f8

Re: [PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk"

[Eric Sunshine]

On Fri, Mar 6, 2015 at 3:55 AM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
> Forbid "log --graph --no-walk

Style: drop capitalization in the Subject: line. Also prefix with the
command or module being modified, followed by a colon. So:

    log: forbid combining --graph and --no-walk

or:

    revision: forbid combining --graph and --no-walk

> Because --graph is about connected history while --no-walk is about discrete points.

Okay. You might also want to cite the wider discussion[1].

[1]: http://article.gmane.org/gmane.comp.version-control.git/216083

> revision.c: Judge whether --graph and --no-walk come together when running git-log.
> buildin/log.c: Set git-log cmd flag.
> Documentation/rev-list-options.txt: Add specification on the forbidden usage.

No need to repeat in prose what the patch itself states more clearly
and concisely.

Also, such a change should be accompanied by new test(s).

> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
> ---
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 4ed8587..eea2c0a 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -679,6 +679,7 @@ endif::git-rev-list[]
>         given on the command line. Otherwise (if `sorted` or no argument
>         was given), the commits are shown in reverse chronological order
>         by commit time.
> +       Cannot be combined with `--graph` when running git-log.
>
>  --do-walk::
>         Overrides a previous `--no-walk`.
> @@ -781,6 +782,7 @@ you would get an output like this:
>         on the left hand side of the output.  This may cause extra lines
>         to be printed in between commits, in order for the graph history
>         to be drawn properly.
> +       Cannot be combined with `--no-walk` when running git-log.

Nice to see documentation updates. More below.

>  This enables parent rewriting, see 'History Simplification' below.
>  +
> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..7bf5adb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
>         git_config(git_log_config, NULL);
>
>         init_revisions(&rev, prefix);
> +       rev.cmd_is_log = 1;
>         rev.always_show_header = 1;
>         memset(&opt, 0, sizeof(opt));
>         opt.def = "HEAD";
> diff --git a/revision.c b/revision.c
> index 66520c6..5f62c89 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)
>
>         revs->commit_format = CMIT_FMT_DEFAULT;
>
> +       revs->cmd_is_log = 0;
> +
>         init_grep_defaults();
>         grep_init(&revs->grep_filter, prefix);
>         revs->grep_filter.status_only = 1;
> @@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>
>         if (revs->reflog_info && revs->graph)
>                 die("cannot combine --walk-reflogs with --graph");
> +       if (revs->no_walk && revs->graph && revs->cmd_is_log)

Placing 'revs->cmd_is_log' first would make it clear at a glance that
this restriction impacts 'log' only (but see question below):

    if (revs->cmd_is_log && revs->no_walk && revs->graph)

> +               die("cannot combine --no-walk with --graph when running git-log");
>         if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
>                 die("cannot use --grep-reflog without --walk-reflogs");
>
> diff --git a/revision.h b/revision.h
> index 0ea8b4e..255982a 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -146,6 +146,9 @@ struct rev_info {
>                         track_first_time:1,
>                         linear:1;
>
> +       /* cmd type */
> +       unsigned int  cmd_is_log:1;

Genuine question: Despite the GSoC micro-project mentioning only
'log', is it ever meaningful for these two options to be specified
together? I suspect not, but it would be nice to hear from someone
more familiar with the issue. If not specific to 'log', then the patch
can be simplified a good deal.

>         enum date_mode date_mode;
>
>         unsigned int    abbrev;
> --

Re: [PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk"

[René Scharfe]

Am 06.03.2015 um 09:55 schrieb Dongcan Jiang:
> Because --graph is about connected history while --no-walk is about discrete points.
>
> revision.c: Judge whether --graph and --no-walk come together when running git-log.
> buildin/log.c: Set git-log cmd flag.
> Documentation/rev-list-options.txt: Add specification on the forbidden usage.
>
> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
> ---
>   Documentation/rev-list-options.txt | 2 ++
>   builtin/log.c                      | 1 +
>   revision.c                         | 4 ++++
>   revision.h                         | 3 +++
>   4 files changed, 10 insertions(+)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index 4ed8587..eea2c0a 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -679,6 +679,7 @@ endif::git-rev-list[]
>   	given on the command line. Otherwise (if `sorted` or no argument
>   	was given), the commits are shown in reverse chronological order
>   	by commit time.
> +	Cannot be combined with `--graph` when running git-log.
>
>   --do-walk::
>   	Overrides a previous `--no-walk`.
> @@ -781,6 +782,7 @@ you would get an output like this:
>   	on the left hand side of the output.  This may cause extra lines
>   	to be printed in between commits, in order for the graph history
>   	to be drawn properly.
> +	Cannot be combined with `--no-walk` when running git-log.
>   +
>   This enables parent rewriting, see 'History Simplification' below.
>   +
> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..7bf5adb 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
>   	git_config(git_log_config, NULL);
>
>   	init_revisions(&rev, prefix);
> +	rev.cmd_is_log = 1;
>   	rev.always_show_header = 1;
>   	memset(&opt, 0, sizeof(opt));
>   	opt.def = "HEAD";
> diff --git a/revision.c b/revision.c
> index 66520c6..5f62c89 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)
>
>   	revs->commit_format = CMIT_FMT_DEFAULT;
>
> +	revs->cmd_is_log = 0;
> +
>   	init_grep_defaults();
>   	grep_init(&revs->grep_filter, prefix);
>   	revs->grep_filter.status_only = 1;
> @@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>
>   	if (revs->reflog_info && revs->graph)
>   		die("cannot combine --walk-reflogs with --graph");
> +	if (revs->no_walk && revs->graph && revs->cmd_is_log)
> +		die("cannot combine --no-walk with --graph when running git-log");

Why only for git log?  Doesn't the justification given in the commit 
message above apply in general?

>   	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
>   		die("cannot use --grep-reflog without --walk-reflogs");
>
> diff --git a/revision.h b/revision.h
> index 0ea8b4e..255982a 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -146,6 +146,9 @@ struct rev_info {
>   			track_first_time:1,
>   			linear:1;
>
> +	/* cmd type */
> +	unsigned int  cmd_is_log:1;
> +
>   	enum date_mode date_mode;
>
>   	unsigned int	abbrev;
>

Re: [PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk"

[Dongcan Jiang]

Hi, Eric and René

Thanks for your suggestions. Good ideas!

> Genuine question: Despite the GSoC micro-project mentioning only
> 'log', is it ever meaningful for these two options to be specified
> together? I suspect not, but it would be nice to hear from someone
> more familiar with the issue. If not specific to 'log', then the > patch
> can be simplified a good deal.

> Why only for git log?  Doesn't the justification given in the
> commit message above apply in general?

At first, I also tried to only judge the value of "revs->no_walk &&
revs->graph", but unfortunately, it failed to pass all cases in
t4052-stat-output.sh.
e.g. command "git show --stat --graph" failed to get the correct result.

Finally, this is because that "revs->no_walk" gets set when it comes
to "git show --stat". That's why I add the parameter
"revs->cmd_is_log" as judgement. Of course, it causes the limitation
you've mentioned. I will consider better solution in the next patch
edition as soon as possible.

Best Regards

Dongcan

2015-03-06 17:56 GMT+08:00 Eric Sunshine <sunshine@sunshineco.com>:
> On Fri, Mar 6, 2015 at 3:55 AM, Dongcan Jiang <dongcan.jiang@gmail.com> wrote:
>> Forbid "log --graph --no-walk
>
> Style: drop capitalization in the Subject: line. Also prefix with the
> command or module being modified, followed by a colon. So:
>
>     log: forbid combining --graph and --no-walk
>
> or:
>
>     revision: forbid combining --graph and --no-walk
>
>> Because --graph is about connected history while --no-walk is about discrete points.
>
> Okay. You might also want to cite the wider discussion[1].
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/216083
>
>> revision.c: Judge whether --graph and --no-walk come together when running git-log.
>> buildin/log.c: Set git-log cmd flag.
>> Documentation/rev-list-options.txt: Add specification on the forbidden usage.
>
> No need to repeat in prose what the patch itself states more clearly
> and concisely.
>
> Also, such a change should be accompanied by new test(s).
>
>> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
>> ---
>> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
>> index 4ed8587..eea2c0a 100644
>> --- a/Documentation/rev-list-options.txt
>> +++ b/Documentation/rev-list-options.txt
>> @@ -679,6 +679,7 @@ endif::git-rev-list[]
>>         given on the command line. Otherwise (if `sorted` or no argument
>>         was given), the commits are shown in reverse chronological order
>>         by commit time.
>> +       Cannot be combined with `--graph` when running git-log.
>>
>>  --do-walk::
>>         Overrides a previous `--no-walk`.
>> @@ -781,6 +782,7 @@ you would get an output like this:
>>         on the left hand side of the output.  This may cause extra lines
>>         to be printed in between commits, in order for the graph history
>>         to be drawn properly.
>> +       Cannot be combined with `--no-walk` when running git-log.
>
> Nice to see documentation updates. More below.
>
>>  This enables parent rewriting, see 'History Simplification' below.
>>  +
>> diff --git a/builtin/log.c b/builtin/log.c
>> index dd8f3fc..7bf5adb 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -627,6 +627,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
>>         git_config(git_log_config, NULL);
>>
>>         init_revisions(&rev, prefix);
>> +       rev.cmd_is_log = 1;
>>         rev.always_show_header = 1;
>>         memset(&opt, 0, sizeof(opt));
>>         opt.def = "HEAD";
>> diff --git a/revision.c b/revision.c
>> index 66520c6..5f62c89 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)
>>
>>         revs->commit_format = CMIT_FMT_DEFAULT;
>>
>> +       revs->cmd_is_log = 0;
>> +
>>         init_grep_defaults();
>>         grep_init(&revs->grep_filter, prefix);
>>         revs->grep_filter.status_only = 1;
>> @@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>
>>         if (revs->reflog_info && revs->graph)
>>                 die("cannot combine --walk-reflogs with --graph");
>> +       if (revs->no_walk && revs->graph && revs->cmd_is_log)
>
> Placing 'revs->cmd_is_log' first would make it clear at a glance that
> this restriction impacts 'log' only (but see question below):
>
>     if (revs->cmd_is_log && revs->no_walk && revs->graph)
>
>> +               die("cannot combine --no-walk with --graph when running git-log");
>>         if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
>>                 die("cannot use --grep-reflog without --walk-reflogs");
>>
>> diff --git a/revision.h b/revision.h
>> index 0ea8b4e..255982a 100644
>> --- a/revision.h
>> +++ b/revision.h
>> @@ -146,6 +146,9 @@ struct rev_info {
>>                         track_first_time:1,
>>                         linear:1;
>>
>> +       /* cmd type */
>> +       unsigned int  cmd_is_log:1;
>
> Genuine question: Despite the GSoC micro-project mentioning only
> 'log', is it ever meaningful for these two options to be specified
> together? I suspect not, but it would be nice to hear from someone
> more familiar with the issue. If not specific to 'log', then the patch
> can be simplified a good deal.
>
>>         enum date_mode date_mode;
>>
>>         unsigned int    abbrev;
>> --



-- 
江东灿（Dongcan Jiang）
Team of Search Engine & Web Mining
School of Electronic Engineering & Computer Science
Peking University, Beijing, 100871, P.R.China

Re: [PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk"

[Junio C Hamano]

Dongcan Jiang <dongcan.jiang@gmail.com> writes:

> At first, I also tried to only judge the value of "revs->no_walk &&
> revs->graph", but unfortunately, it failed to pass all cases in
> t4052-stat-output.sh.
> e.g. command "git show --stat --graph" failed to get the correct result.
>
> Finally, this is because that "revs->no_walk" gets set when it comes
> to "git show --stat".

When "git show" is given a range, it turns no-walk off and becomes a
command about a connected history.  Otherwise, it is a command about
discrete point(s).

Because "git show --graph A B C" and "git log --graph --no-walk A B
C" are moral equivalents, if we are forbidding the latter, we should
forbid the former.

"git show A" (and no other revs, just a single point), however,
could be thought of as an equivalent for "git log -1 A", even though
that interpretation is based on a wrong world view (because "show
one and stop" is not the reason why the result "git show A" gives
has only A and nothing else).  And it makes sort of sense to allow
"git show --graph A" that is an equivalent to "git log -1 --graph A"
under that interpretation.

I was tempted to say the existing test is wrong to expect that
"--graph" does anything meaningful for "git show" that is run for a
non-range.  But doing the "use of both is wrong for log" change
without special casing "show" (and instead "fixing" t4052) would be
a change in behaviour, which we try to avoid in general.

I'd prefer a solution that does not waste a new bit in revision
structure in general, but if we were to waste one bit to special
case "show", it shouldn't be "fail only for the log command".  It
should be "allow this stupidity only for the show command for
backward compatibility" bit instead, I think.

[PATCH v2/GSoC/MICRO] revision: forbid combining --graph and --no-walk

[Dongcan Jiang]

Because --graph is about connected history while
--no-walk is about discrete points. [1]

It's a pity that git-show has to allow such combination
in order to make t4052-stat-output.sh compatible. [2]

2 testcases have been added to test this patch.

[1]: http://article.gmane.org/gmane.comp.version-control.git/216083
[2]: http://article.gmane.org/gmane.comp.version-control.git/264950

Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>

Thanks-to: Eric Sunshine, René Scharfe, Junio C Hamano
---
 Documentation/rev-list-options.txt | 2 ++
 builtin/log.c                      | 1 +
 revision.c                         | 4 ++++
 revision.h                         | 3 +++
 t/t4202-log.sh                     | 6 ++++++
 t/t6014-rev-list-all.sh            | 6 ++++++
 6 files changed, 22 insertions(+)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..136abdf 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -679,6 +679,7 @@ endif::git-rev-list[]
 	given on the command line. Otherwise (if `sorted` or no argument
 	was given), the commits are shown in reverse chronological order
 	by commit time.
+	Cannot be combined with `--graph`.

 --do-walk::
 	Overrides a previous `--no-walk`.
@@ -781,6 +782,7 @@ you would get an output like this:
 	on the left hand side of the output.  This may cause extra lines
 	to be printed in between commits, in order for the graph history
 	to be drawn properly.
+	Cannot be combined with `--no-walk`.
 +
 This enables parent rewriting, see 'History Simplification' below.
 +
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..5b5d028 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -524,6 +524,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)

 	memset(&match_all, 0, sizeof(match_all));
 	init_revisions(&rev, prefix);
+	rev.cmd_is_show = 1;
 	rev.diff = 1;
 	rev.always_show_header = 1;
 	rev.no_walk = REVISION_WALK_NO_WALK_SORTED;
diff --git a/revision.c b/revision.c
index 66520c6..5d6fbef 100644
--- a/revision.c
+++ b/revision.c
@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)

 	revs->commit_format = CMIT_FMT_DEFAULT;

+	revs->cmd_is_show = 0;
+
 	init_grep_defaults();
 	grep_init(&revs->grep_filter, prefix);
 	revs->grep_filter.status_only = 1;
@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s

 	if (revs->reflog_info && revs->graph)
 		die("cannot combine --walk-reflogs with --graph");
+	if (!revs->cmd_is_show && revs->no_walk && revs->graph)
+		die("cannot combine --no-walk with --graph");
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
 		die("cannot use --grep-reflog without --walk-reflogs");

diff --git a/revision.h b/revision.h
index 0ea8b4e..378c3bf 100644
--- a/revision.h
+++ b/revision.h
@@ -146,6 +146,9 @@ struct rev_info {
 			track_first_time:1,
 			linear:1;

+	/* cmd type */
+	unsigned int  cmd_is_show:1;
+
 	enum date_mode date_mode;

 	unsigned int	abbrev;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 5f2b290..fed162e 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -887,4 +887,10 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	grep "^| | gpg: Good signature" actual
 '

+test_expect_success 'log --graph --no-walk is forbidden' '
+	echo "fatal: cannot combine --no-walk with --graph" >expect-error &&
+	test_must_fail git log --graph --no-walk 2>error &&
+	test_cmp expect-error error
+'
+
 test_done
diff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh
index 991ab4a..574e8d9 100755
--- a/t/t6014-rev-list-all.sh
+++ b/t/t6014-rev-list-all.sh
@@ -35,4 +35,10 @@ test_expect_success 'repack does not lose detached HEAD' '

 '

+test_expect_success 'rev-list --graph --no-walk is forbidden' '
+	echo "fatal: cannot combine --no-walk with --graph" >expect-error &&
+	test_must_fail git rev-list --graph --no-walk 2>error &&
+	test_cmp expect-error error
+'
+
 test_done
--
2.3.1.253.g3de5837

Re: [PATCH v2/GSoC/MICRO] revision: forbid combining --graph and --no-walk

[Junio C Hamano]

Dongcan Jiang <dongcan.jiang@gmail.com> writes:

> Because --graph is about connected history while
> --no-walk is about discrete points. [1]

The convention around here is that the title of the patch on the
Subject line is *not* the beginning part of the first sentence, you
would need to phrase the above more like:

        Because "--graph" is about ... discrete points, it does not
        make sense to allow giving these two options at the same
        time.

to make it a complete sentence.

> It's a pity that git-show has to allow such combination
> in order to make t4052-stat-output.sh compatible. [2]

If you feel "It's a pity", it actually is OK to make a good argument
for "fixing" the test without adding the workaround.  A replacement
commit message for the above two lines for such an approach might
look like:

        This change makes a few calls to "show --graph" fail in
        t4052, but asking to show one commit _with_ graph is a
        nonsensical thing to do.  Correct these offending tests that
        expected "--graph" to be useful by removing "--graph" in
        their invocations (and adjusting the expected output, of
        course).

That way, people who do find it actually useful that "show --graph HEAD"
that shows something like this

        $ git show --graph -s 52d5bf778
        *   commit 52d5bf77875275bbfc1bf1d7b690f355d5c869e4
        |\  Merge: 36ab768 189c860
        | | Author: Junio C Hamano <gitster@pobox.com>
        | | Date:   Fri Mar 6 15:02:33 2015 -0800
        | | 
        | |     Merge branch 'bw/kwset-use-unsigned'
        ...

can react and argue why it is useful to them.  

It is important to notice that your "It's a pity" will no longer
apply, if we are keeping the feature because it is useful.  It would
be clear that we would be keeping the feature not because we are too
lazy to correct tests, but because it is actually useful.

> Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
>
> Thanks-to: Eric Sunshine, René Scharfe, Junio C Hamano

These look unusual for a few reasons: S-o-b is not at the end, we
usually say Helped-by: instead, and we do not use Thanks-to: with
multiple names on a single line.

Please do not try to be original without a good reason.  We may
start counting the number of times people appear on these footers to
see how much contribution those who do not directly author commits
(read: those who mentor others) are making.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 5f2b290..fed162e 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -887,4 +887,10 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
>  	grep "^| | gpg: Good signature" actual
>  '
>
> +test_expect_success 'log --graph --no-walk is forbidden' '
> +	echo "fatal: cannot combine --no-walk with --graph" >expect-error &&
> +	test_must_fail git log --graph --no-walk 2>error &&
> +	test_cmp expect-error error
> +'

I do not think we want to check exact phrasing of the error
message here.  Just the second line (without the " 2>error &&"
at the end) should be sufficient.

> +test_expect_success 'rev-list --graph --no-walk is forbidden' '
> +	echo "fatal: cannot combine --no-walk with --graph" >expect-error &&
> +	test_must_fail git rev-list --graph --no-walk 2>error &&
> +	test_cmp expect-error error
> +'

The same comment (about preferring not to check the error message)
applies here, and more importantly, this is not a good test because
"git rev-list --graph" without the forbidden "--no-walk" would fail
for other reasons.  Perhaps

	test_must_fail git rev-list --graph --no-walk HEAD

or something, assuming that there is already a commit pointed by
HEAD at this point in the test (I didn't check).

Thanks.

[PATCH v3/GSoC/MICRO] revision: forbid combining --graph and --no-walk

[Dongcan Jiang]

Because "--graph" is about connected history while --no-walk
is about discrete points, it does not make sense to allow
giving these two options at the same time. [1]

This change allows git-show to have such options' combination
as a special case, because git-show itself has underlying
--no-walk option, while "git show --graph" is a legal and
useful operation which is tested in t4052. [2,3]

2 testcases have been added to test this patch.

[1]: http://article.gmane.org/gmane.comp.version-control.git/216083
[2]: http://article.gmane.org/gmane.comp.version-control.git/264950
[3]: http://article.gmane.org/gmane.comp.version-control.git/265107

Helped-By: Eric Sunshine <sunshine@sunshineco.com>
Helped-By: René Scharfe <l.s.r@web.de>
Helped-By: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
---
 Documentation/rev-list-options.txt | 2 ++
 builtin/log.c                      | 1 +
 revision.c                         | 4 ++++
 revision.h                         | 3 +++
 t/t4202-log.sh                     | 4 ++++
 t/t6014-rev-list-all.sh            | 4 ++++
 6 files changed, 18 insertions(+)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..136abdf 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -679,6 +679,7 @@ endif::git-rev-list[]
 	given on the command line. Otherwise (if `sorted` or no argument
 	was given), the commits are shown in reverse chronological order
 	by commit time.
+	Cannot be combined with `--graph`.
 
 --do-walk::
 	Overrides a previous `--no-walk`.
@@ -781,6 +782,7 @@ you would get an output like this:
 	on the left hand side of the output.  This may cause extra lines
 	to be printed in between commits, in order for the graph history
 	to be drawn properly.
+	Cannot be combined with `--no-walk`.
 +
 This enables parent rewriting, see 'History Simplification' below.
 +
diff --git a/builtin/log.c b/builtin/log.c
index dd8f3fc..5b5d028 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -524,6 +524,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 
 	memset(&match_all, 0, sizeof(match_all));
 	init_revisions(&rev, prefix);
+	rev.cmd_is_show = 1;
 	rev.diff = 1;
 	rev.always_show_header = 1;
 	rev.no_walk = REVISION_WALK_NO_WALK_SORTED;
diff --git a/revision.c b/revision.c
index 66520c6..5d6fbef 100644
--- a/revision.c
+++ b/revision.c
@@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
+	revs->cmd_is_show = 0;
+
 	init_grep_defaults();
 	grep_init(&revs->grep_filter, prefix);
 	revs->grep_filter.status_only = 1;
@@ -2339,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	if (revs->reflog_info && revs->graph)
 		die("cannot combine --walk-reflogs with --graph");
+	if (!revs->cmd_is_show && revs->no_walk && revs->graph)
+		die("cannot combine --no-walk with --graph");
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
 		die("cannot use --grep-reflog without --walk-reflogs");
 
diff --git a/revision.h b/revision.h
index 0ea8b4e..378c3bf 100644
--- a/revision.h
+++ b/revision.h
@@ -146,6 +146,9 @@ struct rev_info {
 			track_first_time:1,
 			linear:1;
 
+	/* cmd type */
+	unsigned int  cmd_is_show:1;
+
 	enum date_mode date_mode;
 
 	unsigned int	abbrev;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 5f2b290..f111705 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -887,4 +887,8 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	grep "^| | gpg: Good signature" actual
 '
 
+test_expect_success 'log --graph --no-walk is forbidden' '
+	test_must_fail git log --graph --no-walk
+'
+
 test_done
diff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh
index 991ab4a..c9bedd2 100755
--- a/t/t6014-rev-list-all.sh
+++ b/t/t6014-rev-list-all.sh
@@ -35,4 +35,8 @@ test_expect_success 'repack does not lose detached HEAD' '
 
 '
 
+test_expect_success 'rev-list --graph --no-walk is forbidden' '
+	test_must_fail git rev-list --graph --no-walk HEAD
+'
+
 test_done
-- 
2.3.1.252.ge67f612

Re: [PATCH v3/GSoC/MICRO] revision: forbid combining --graph and --no-walk

[Junio C Hamano]

Dongcan Jiang <dongcan.jiang@gmail.com> writes:

> Because "--graph" is about connected history while --no-walk
> is about discrete points, it does not make sense to allow
> giving these two options at the same time. [1]
>
> This change allows git-show to have such options' combination
> as a special case, because git-show itself has underlying
> --no-walk option, while "git show --graph" is a legal and
> useful operation which is tested in t4052. [2,3]

Hmph, I actually was hoping to see that you would either (1) explain
why this special case is not useful, fix t4052 and do without
cmd_is_show bit, or (2) explain why this special case _is_ useful in
a more concrete terms.

"X is legal and tested" does not automatically imply that whatever
random thing the implementation does, and the test whose expectation
matches what it does, is a well-thought-out and a useful operation.
If you are going in the direction (2), it would have been better if
the reason why "git show --graph one_commit" is useful is given here
in your own words.

You do not want to force those who are reading this log message 6
months down the road to visit [2,3] for more details.

> diff --git a/builtin/log.c b/builtin/log.c
> index dd8f3fc..5b5d028 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -524,6 +524,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  
>  	memset(&match_all, 0, sizeof(match_all));
>  	init_revisions(&rev, prefix);
> +	rev.cmd_is_show = 1;
>  	rev.diff = 1;
>  	rev.always_show_header = 1;
>  	rev.no_walk = REVISION_WALK_NO_WALK_SORTED;

OK.

> diff --git a/revision.c b/revision.c
> index 66520c6..5d6fbef 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1399,6 +1399,8 @@ void init_revisions(struct rev_info *revs, const char *prefix)
>  
>  	revs->commit_format = CMIT_FMT_DEFAULT;
>  
> +	revs->cmd_is_show = 0;
> +

The new assignment and a blank line shouldn't be necessary; the
memset() at the beginning is there so that you do not have to do
this.

> diff --git a/revision.h b/revision.h
> index 0ea8b4e..378c3bf 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -146,6 +146,9 @@ struct rev_info {
>  			track_first_time:1,
>  			linear:1;
>  
> +	/* cmd type */
> +	unsigned int  cmd_is_show:1;

If you are going to comment, imagine that somebody will want to add
a new subcommand in "git log" family in the future, and try to help
him decide if he wants to flip this bit for his subcommand with that
comment.  He would scratch his head, reading "cmd type?", and wonder
"Hmmmm, what makes 'show' special?  Is my new command also special
like 'show' is?  What makes my new command the same cmd type as
'show' (or different)?"  The comment does not help him answer these
questions very much.

An alternative is to not to add the misleading comment; cmd_is_show
is clear enough indication for such a person that he does not want
the bit set because whatever new subcommand he is adding is not
'show'.

This is becoming to appear more and more that cmd_is_show is "allow
combined use of graph and no-walk only to avoid breaking a few
tests", not "in the context of show, graph and no-walk is useful",
at least to me.  Perhaps the comment should say

	/*
         * special case to prevent 'git show --graph' that does not
         * walk from triggering the usual "--no-walk and --graph cannot
         * be used together" error.
         */
	unsigned int cmd_is_show:1;

or even name the variable more explicitly, i.e.

	unsigned int allow_graph_and_no_walk:1;

I dunno.

The tests look fine.  Thanks.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 5f2b290..f111705 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -887,4 +887,8 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
>  	grep "^| | gpg: Good signature" actual
>  '
>  
> +test_expect_success 'log --graph --no-walk is forbidden' '
> +	test_must_fail git log --graph --no-walk
> +'
> +
>  test_done
> diff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh
> index 991ab4a..c9bedd2 100755
> --- a/t/t6014-rev-list-all.sh
> +++ b/t/t6014-rev-list-all.sh
> @@ -35,4 +35,8 @@ test_expect_success 'repack does not lose detached HEAD' '
>  
>  '
>  
> +test_expect_success 'rev-list --graph --no-walk is forbidden' '
> +	test_must_fail git rev-list --graph --no-walk HEAD
> +'
> +
>  test_done

[PATCH v4/GSoC/MICRO] revision: forbid combining --graph and --no-walk

[Dongcan Jiang]

Because "--graph" is about connected history while --no-walk
is about discrete points, it does not make sense to allow
giving these two options at the same time. [1]

This change makes a few calls to "show --graph" fail in
t4052, but asking to show one commit with graph is a
nonsensical thing to do. Thus, tests on "show --graph" in
t4052 have been removed. [2,3] Same tests on "show" without
--graph option have already been tested in 4052.

3 testcases have been added to test this patch.

[1]: http://article.gmane.org/gmane.comp.version-control.git/216083
[2]: http://article.gmane.org/gmane.comp.version-control.git/264950
[3]: http://article.gmane.org/gmane.comp.version-control.git/265107

Helped-By: Eric Sunshine <sunshine@sunshineco.com>
Helped-By: René Scharfe <l.s.r@web.de>
Helped-By: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
---
 Documentation/rev-list-options.txt |  2 ++
 revision.c                         |  2 ++
 t/t4052-stat-output.sh             | 14 +++++++-------
 t/t4202-log.sh                     |  4 ++++
 t/t6014-rev-list-all.sh            |  4 ++++
 t/t7007-show.sh                    |  4 ++++
 6 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..136abdf 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -679,6 +679,7 @@ endif::git-rev-list[]
 	given on the command line. Otherwise (if `sorted` or no argument
 	was given), the commits are shown in reverse chronological order
 	by commit time.
+	Cannot be combined with `--graph`.
 
 --do-walk::
 	Overrides a previous `--no-walk`.
@@ -781,6 +782,7 @@ you would get an output like this:
 	on the left hand side of the output.  This may cause extra lines
 	to be printed in between commits, in order for the graph history
 	to be drawn properly.
+	Cannot be combined with `--no-walk`.
 +
 This enables parent rewriting, see 'History Simplification' below.
 +
diff --git a/revision.c b/revision.c
index 66520c6..6cd91dd 100644
--- a/revision.c
+++ b/revision.c
@@ -2339,6 +2339,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	if (revs->reflog_info && revs->graph)
 		die("cannot combine --walk-reflogs with --graph");
+	if (revs->no_walk && revs->graph)
+		die("cannot combine --no-walk with --graph");
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
 		die("cannot use --grep-reflog without --walk-reflogs");
 
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index b68afef..a989e8f 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -99,7 +99,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff -a "$cmd" != show || continue
 
 	test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
 		COLUMNS=200 git $cmd $args --graph >output
@@ -127,7 +127,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff -a "$cmd" != show || continue
 
 	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
 		COLUMNS=40 git $cmd $args --graph >output
@@ -155,7 +155,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff -a "$cmd" != show || continue
 
 	test_expect_success "$cmd --graph $verb statGraphWidth config" '
 		git -c diff.statGraphWidth=26 $cmd $args --graph >output
@@ -196,7 +196,7 @@ do
 		test_cmp expect actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff -a "$cmd" != show || continue
 
 	test_expect_success "$cmd --stat-width=width --graph with big change" '
 		git $cmd $args --stat-width=40 --graph >output
@@ -236,7 +236,7 @@ do
 		test_cmp expect actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff -a "$cmd" != show || continue
 
 	test_expect_success "$cmd --stat=width --graph with big change is balanced" '
 		git $cmd $args --stat-width=60 --graph >output &&
@@ -270,7 +270,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff -a "$cmd" != show || continue
 
 	test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
 		COLUMNS=200 git $cmd $args --graph >output
@@ -299,7 +299,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff -a "$cmd" != show || continue
 
 	test_expect_success COLUMNS_CAN_BE_1 \
 		"$cmd --graph $verb prefix greater than COLUMNS (big change)" '
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 5f2b290..f111705 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -887,4 +887,8 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	grep "^| | gpg: Good signature" actual
 '
 
+test_expect_success 'log --graph --no-walk is forbidden' '
+	test_must_fail git log --graph --no-walk
+'
+
 test_done
diff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh
index 991ab4a..c9bedd2 100755
--- a/t/t6014-rev-list-all.sh
+++ b/t/t6014-rev-list-all.sh
@@ -35,4 +35,8 @@ test_expect_success 'repack does not lose detached HEAD' '
 
 '
 
+test_expect_success 'rev-list --graph --no-walk is forbidden' '
+	test_must_fail git rev-list --graph --no-walk HEAD
+'
+
 test_done
diff --git a/t/t7007-show.sh b/t/t7007-show.sh
index 1b824fe..42d3db6 100755
--- a/t/t7007-show.sh
+++ b/t/t7007-show.sh
@@ -124,4 +124,8 @@ test_expect_success '--quiet suppresses diff' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show --graph is forbidden' '
+  test_must_fail git show --graph HEAD
+'
+
 test_done
-- 
2.3.1.252.ge67f612.dirty

Re: [PATCH v4/GSoC/MICRO] revision: forbid combining --graph and --no-walk

[Junio C Hamano]

Dongcan Jiang <dongcan.jiang@gmail.com> writes:

> Because "--graph" is about connected history while --no-walk
> is about discrete points, it does not make sense to allow
> giving these two options at the same time. [1]
>
> This change makes a few calls to "show --graph" fail in
> t4052, but asking to show one commit with graph is a
> nonsensical thing to do. Thus, tests on "show --graph" in
> t4052 have been removed. [2,3] Same tests on "show" without
> --graph option have already been tested in 4052.

This looks almost perfect.
>
> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
> index b68afef..a989e8f 100755
> --- a/t/t4052-stat-output.sh
> +++ b/t/t4052-stat-output.sh
> @@ -99,7 +99,7 @@ do
>  		test_cmp "$expect" actual
>  	'
>  
> -	test "$cmd" != diff || continue
> +	test "$cmd" != diff -a "$cmd" != show || continue

I think we avoid -a and -o used with test (don't we have a write-up
on this somewhere?).  Write it like this

	test "$cmd" != diff &&
        test "$cmd" != show || continue

or perhaps like this

	case "$cmd" in diff|show) continue ;; esac

Other than that I do not see anything objectionable.

Thanks, good job.

Re: [PATCH v4/GSoC/MICRO] revision: forbid combining --graph and --no-walk

[Eric Sunshine]

On Tue, Mar 17, 2015 at 7:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dongcan Jiang <dongcan.jiang@gmail.com> writes:
>> diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
>> index b68afef..a989e8f 100755
>> --- a/t/t4052-stat-output.sh
>> +++ b/t/t4052-stat-output.sh
>> @@ -99,7 +99,7 @@ do
>>               test_cmp "$expect" actual
>>       '
>>
>> -     test "$cmd" != diff || continue
>> +     test "$cmd" != diff -a "$cmd" != show || continue
>
> I think we avoid -a and -o used with test (don't we have a write-up
> on this somewhere?).

The very last item in the shell script section of CodingGuidelines discusses it.

> Write it like this
>
>         test "$cmd" != diff &&
>         test "$cmd" != show || continue
>
> or perhaps like this
>
>         case "$cmd" in diff|show) continue ;; esac

[PATCH v5/GSoC/MICRO] revision: forbid combining --graph and --no-walk

[Dongcan Jiang]

Because "--graph" is about connected history while --no-walk
is about discrete points, it does not make sense to allow
giving these two options at the same time. [1]

This change makes a few calls to "show --graph" fail in
t4052, but asking to show one commit with graph is a
nonsensical thing to do. Thus, tests on "show --graph" in
t4052 have been removed. [2,3] Same tests on "show" without
--graph option have already been tested in 4052.

3 testcases have been added to test this patch.

[1]: http://article.gmane.org/gmane.comp.version-control.git/216083
[2]: http://article.gmane.org/gmane.comp.version-control.git/264950
[3]: http://article.gmane.org/gmane.comp.version-control.git/265107

Helped-By: Eric Sunshine <sunshine@sunshineco.com>
Helped-By: René Scharfe <l.s.r@web.de>
Helped-By: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Dongcan Jiang <dongcan.jiang@gmail.com>
---
 Documentation/rev-list-options.txt |  2 ++
 revision.c                         |  2 ++
 t/t4052-stat-output.sh             | 14 +++++++-------
 t/t4202-log.sh                     |  4 ++++
 t/t6014-rev-list-all.sh            |  4 ++++
 t/t7007-show.sh                    |  4 ++++
 6 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 4ed8587..136abdf 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -679,6 +679,7 @@ endif::git-rev-list[]
 	given on the command line. Otherwise (if `sorted` or no argument
 	was given), the commits are shown in reverse chronological order
 	by commit time.
+	Cannot be combined with `--graph`.
 
 --do-walk::
 	Overrides a previous `--no-walk`.
@@ -781,6 +782,7 @@ you would get an output like this:
 	on the left hand side of the output.  This may cause extra lines
 	to be printed in between commits, in order for the graph history
 	to be drawn properly.
+	Cannot be combined with `--no-walk`.
 +
 This enables parent rewriting, see 'History Simplification' below.
 +
diff --git a/revision.c b/revision.c
index 66520c6..6cd91dd 100644
--- a/revision.c
+++ b/revision.c
@@ -2339,6 +2339,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 
 	if (revs->reflog_info && revs->graph)
 		die("cannot combine --walk-reflogs with --graph");
+	if (revs->no_walk && revs->graph)
+		die("cannot combine --no-walk with --graph");
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
 		die("cannot use --grep-reflog without --walk-reflogs");
 
diff --git a/t/t4052-stat-output.sh b/t/t4052-stat-output.sh
index b68afef..54f10cf 100755
--- a/t/t4052-stat-output.sh
+++ b/t/t4052-stat-output.sh
@@ -99,7 +99,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff && test "$cmd" != show || continue
 
 	test_expect_success "$cmd --graph $verb COLUMNS (big change)" '
 		COLUMNS=200 git $cmd $args --graph >output
@@ -127,7 +127,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff && test "$cmd" != show || continue
 
 	test_expect_success "$cmd --graph $verb not enough COLUMNS (big change)" '
 		COLUMNS=40 git $cmd $args --graph >output
@@ -155,7 +155,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff && test "$cmd" != show || continue
 
 	test_expect_success "$cmd --graph $verb statGraphWidth config" '
 		git -c diff.statGraphWidth=26 $cmd $args --graph >output
@@ -196,7 +196,7 @@ do
 		test_cmp expect actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff && test "$cmd" != show || continue
 
 	test_expect_success "$cmd --stat-width=width --graph with big change" '
 		git $cmd $args --stat-width=40 --graph >output
@@ -236,7 +236,7 @@ do
 		test_cmp expect actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff && test "$cmd" != show || continue
 
 	test_expect_success "$cmd --stat=width --graph with big change is balanced" '
 		git $cmd $args --stat-width=60 --graph >output &&
@@ -270,7 +270,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff && test "$cmd" != show || continue
 
 	test_expect_success "$cmd --graph $verb COLUMNS (long filename)" '
 		COLUMNS=200 git $cmd $args --graph >output
@@ -299,7 +299,7 @@ do
 		test_cmp "$expect" actual
 	'
 
-	test "$cmd" != diff || continue
+	test "$cmd" != diff && test "$cmd" != show || continue
 
 	test_expect_success COLUMNS_CAN_BE_1 \
 		"$cmd --graph $verb prefix greater than COLUMNS (big change)" '
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 5f2b290..f111705 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -887,4 +887,8 @@ test_expect_success GPG 'log --graph --show-signature for merged tag' '
 	grep "^| | gpg: Good signature" actual
 '
 
+test_expect_success 'log --graph --no-walk is forbidden' '
+	test_must_fail git log --graph --no-walk
+'
+
 test_done
diff --git a/t/t6014-rev-list-all.sh b/t/t6014-rev-list-all.sh
index 991ab4a..c9bedd2 100755
--- a/t/t6014-rev-list-all.sh
+++ b/t/t6014-rev-list-all.sh
@@ -35,4 +35,8 @@ test_expect_success 'repack does not lose detached HEAD' '
 
 '
 
+test_expect_success 'rev-list --graph --no-walk is forbidden' '
+	test_must_fail git rev-list --graph --no-walk HEAD
+'
+
 test_done
diff --git a/t/t7007-show.sh b/t/t7007-show.sh
index 1b824fe..42d3db6 100755
--- a/t/t7007-show.sh
+++ b/t/t7007-show.sh
@@ -124,4 +124,8 @@ test_expect_success '--quiet suppresses diff' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show --graph is forbidden' '
+  test_must_fail git show --graph HEAD
+'
+
 test_done
-- 
2.3.2.224.g6c2f212