All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Dongcan Jiang <dongcan.jiang@gmail.com>
Cc: git@vger.kernel.org, sunshine@sunshineco.com, l.s.r@web.de
Subject: Re: [PATCH v4/GSoC/MICRO] revision: forbid combining --graph and --no-walk
Date: Tue, 17 Mar 2015 16:18:28 -0700	[thread overview]
Message-ID: <xmqq4mpjmcvv.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <c6c08cdbbc0060c6d834c86d13a142cde290dc5c.1426039099.git.dongcan.jiang@gmail.com> (Dongcan Jiang's message of "Wed, 11 Mar 2015 10:13:02 +0800")

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.

  reply	other threads:[~2015-03-17 23:18 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-06  8:55 [PATCH] [GSoC][MICRO] Forbid "log --graph --no-walk" Dongcan Jiang
2015-03-06  9:56 ` Eric Sunshine
2015-03-06 13:13   ` Dongcan Jiang
2015-03-06 18:51     ` Junio C Hamano
2015-03-06 10:07 ` René Scharfe
2015-03-07  4:56 ` [PATCH v2/GSoC/MICRO] revision: forbid combining --graph and --no-walk Dongcan Jiang
2015-03-08 19:40   ` Junio C Hamano
2015-03-09  4:09 ` [PATCH v3/GSoC/MICRO] " Dongcan Jiang
2015-03-10 21:39   ` Junio C Hamano
2015-03-11  2:13 ` [PATCH v4/GSoC/MICRO] " Dongcan Jiang
2015-03-17 23:18   ` Junio C Hamano [this message]
2015-03-17 23:24     ` Eric Sunshine
2015-03-18  5:34 ` [PATCH v5/GSoC/MICRO] " Dongcan Jiang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq4mpjmcvv.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=dongcan.jiang@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=l.s.r@web.de \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.