Dongcan Jiang <dongcan.jiang@xxxxxxxxx> 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@xxxxxxxxx>
| | 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@xxxxxxxxx>
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html