From: Junio C Hamano <gitster@pobox•com>
To: Dongcan Jiang <dongcan.jiang@gmail•com>
Cc: sunshine@sunshineco•com, l.s.r@web•de, git@vger•kernel.org
Subject: Re: [PATCH v2/GSoC/MICRO] revision: forbid combining --graph and --no-walk
Date: Sun, 08 Mar 2015 12:40:10 -0700 [thread overview]
Message-ID: <xmqqwq2rqnvp.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <3de5837561c07fbf8d6187578fc37b3dbf2ea5f7.1425702676.git.dongcan.jiang@gmail.com> (Dongcan Jiang's message of "Sat, 7 Mar 2015 12:56:47 +0800")
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.
next prev parent reply other threads:[~2015-03-08 19:40 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 [this message]
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
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=xmqqwq2rqnvp.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=dongcan.jiang@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=l.s.r@web$(echo .)de \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox