threads / patch / 38724

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

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

## tl;dr

13 messages between Mar 6, 2015 and Mar 18, 2015. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Dongcan Jiang· Mar 6, 2015, 08:55 UTC · lore

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(+)

Show changes to 4 files +10 −0

Documentation/rev-list-options.txt, builtin/log.c, revision.c, revision.h

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

Eric Sunshine· Mar 6, 2015, 09:56 UTC · re: Dongcan Jiang · lore

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

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).

Show 19 quoted lines

> 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.

Show 32 quoted lines

>  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)

Show 14 quoted lines

> +               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· Mar 6, 2015, 13:13 UTC · re: Eric Sunshine · lore

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

Hi, Eric and René

Thanks for your suggestions. Good ideas!

Show 5 quoted lines

> 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>:

Show 112 quoted lines

> 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

Junio C Hamano· Mar 6, 2015, 18:51 UTC · re: Dongcan Jiang · lore

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

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

Show 7 quoted lines

> 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.

René Scharfe· Mar 6, 2015, 10:07 UTC · re: Dongcan Jiang · lore

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

Am 06.03.2015 um 09:55 schrieb Dongcan Jiang:

Show 65 quoted lines

> 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?

Show 18 quoted lines

>   	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;
>

Dongcan Jiang· Mar 7, 2015, 04:56 UTC · re: Dongcan Jiang · lore

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

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(+)

Show changes to 6 files +22 −1

Documentation/rev-list-options.txt, builtin/log.c, revision.c, revision.h, t/t4202-log.sh, t/t6014-rev-list-all.sh

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

Junio C Hamano· Mar 8, 2015, 19:40 UTC · re: Dongcan Jiang · lore

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

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.

Show 13 quoted lines

> 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.

Show 5 quoted lines

> +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.

Dongcan Jiang· Mar 9, 2015, 04:09 UTC · re: Dongcan Jiang · lore

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

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(+)

Show changes to 6 files +18 −0

Documentation/rev-list-options.txt, builtin/log.c, revision.c, revision.h, t/t4202-log.sh, t/t6014-rev-list-all.sh

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

Junio C Hamano· Mar 10, 2015, 21:39 UTC · re: Dongcan Jiang · lore

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

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

Show 8 quoted lines

> 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.

Show 12 quoted lines

> 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.

Show 10 quoted lines

> 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.

Show 10 quoted lines

> 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.

Show 26 quoted lines

> 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

Dongcan Jiang· Mar 11, 2015, 02:13 UTC · re: Dongcan Jiang · lore

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

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.

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(-)

Show changes to 6 files +23 −7

Documentation/rev-list-options.txt, revision.c, t/t4052-stat-output.sh, t/t4202-log.sh, t/t6014-rev-list-all.sh, t/t7007-show.sh

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

Junio C Hamano· Mar 17, 2015, 23:18 UTC · re: Dongcan Jiang · lore

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

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

Show 9 quoted lines

> 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.

Show 11 quoted lines

>
> 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.

Eric Sunshine· Mar 17, 2015, 23:24 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 17, 2015 at 7:18 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> 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.

Show 8 quoted lines

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

Dongcan Jiang· Mar 18, 2015, 05:34 UTC · re: Dongcan Jiang · lore

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

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]

3 testcases have been added to test this patch.

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(-)

Show changes to 6 files +23 −7

Documentation/rev-list-options.txt, revision.c, t/t4052-stat-output.sh, t/t4202-log.sh, t/t6014-rev-list-all.sh, t/t7007-show.sh

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