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

[Junio C Hamano <gitster@pobox.com>]

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