From: Christian Couder <chriscool@tuxfamily.org>
To: sunshine@sunshineco.com
Cc: gitster@pobox.com, git@vger.kernel.org, philipoakley@iee.org,
trast@inf.ethz.ch, j6t@kdbg.org
Subject: Re: [PATCH v3 11/11] t6050-replace: use some long option names
Date: Sun, 01 Sep 2013 12:01:32 +0200 (CEST) [thread overview]
Message-ID: <20130901.120132.1611298105222242724.chriscool@tuxfamily.org> (raw)
In-Reply-To: <CAPig+cSwQdhybTzRwqGLbdxGRcAZfBeU6jjWYy43G0oT0hxkNw@mail.gmail.com>
From: Eric Sunshine <sunshine@sunshineco.com>
>
>> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
>> index 0b07a0b..5dc26e8 100755
>> --- a/t/t6050-replace.sh
>> +++ b/t/t6050-replace.sh
>> @@ -122,9 +122,9 @@ test_expect_success '"git replace" listing and deleting' '
>> test "$HASH2" = "$(git replace -l)" &&
>> test "$HASH2" = "$(git replace)" &&
>> aa=${HASH2%??????????????????????????????????????} &&
>> - test "$HASH2" = "$(git replace -l "$aa*")" &&
>> + test "$HASH2" = "$(git replace --list "$aa*")" &&
>> test_must_fail git replace -d $R &&
>> - test_must_fail git replace -d &&
>> + test_must_fail git replace --delete &&
>> test_must_fail git replace -l -d $HASH2 &&
>
> Is this sort of change a good idea? A person reading the code, but not
> familiar with this particular patch, might not understand the
> seemingly random choice of short versus long option usage. Such a
> person might be led to wonder if there is some subtle difference
> between the short and long forms,
I don't think that we should care about people who might wonder if
there are subtle differences between short and long forms, because of
such tests.
The doc says that there is no difference, and there is also no
difference in other git commands, and git replace is an advanced
command, and very few regular users probably read tests, ...
> and then unnecessarily spend time
> digging into the code and documentation in an attempt to figure it
> out. Alternately, someone reading the code might be led to assume that
> the person who added the tests was being sloppy.
Why not just assume that the person who added the tests is so great as
to even take care of testing the long options names?
> Hopefully, t0040-parse-options should already be proof enough that
> long options are correctly handled,
Yeah, indeed!
> but if you really want to test
> them here too, it seems like a separate test would be more appropriate
> than randomly changing short form options to long in various tests.
Introducing some randomness in tests also has some value, you
know. And at the same time it doesn't even slow down the test suite
while adding separate tests would slow it down, and make readers spend
more time reading the whole test suite, and would be a bigger burden
for git developers to maintain for no good reason.
Best,
Christian.
prev parent reply other threads:[~2013-09-01 10:01 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-31 19:12 [PATCH v3 00/11] Check replacement object type and minor updates Christian Couder
2013-08-31 19:12 ` [PATCH v3 01/11] replace: forbid replacing an object with one of a different type Christian Couder
2013-08-31 22:11 ` Philip Oakley
2013-09-01 11:53 ` Christian Couder
2013-09-01 19:26 ` Philip Oakley
2013-08-31 19:12 ` [PATCH v3 02/11] Documentation/replace: state that objects must be of the same type Christian Couder
2013-08-31 19:12 ` [PATCH v3 03/11] t6050-replace: test that objects are " Christian Couder
2013-08-31 19:12 ` [PATCH v3 04/11] t6050-replace: add test to clean up all the replace refs Christian Couder
2013-08-31 19:12 ` [PATCH v3 05/11] Documentation/replace: add Creating Replacement Objects section Christian Couder
2013-08-31 22:19 ` Philip Oakley
2013-09-01 10:27 ` Christian Couder
2013-08-31 19:12 ` [PATCH v3 06/11] replace: bypass the type check if -f option is used Christian Couder
2013-08-31 19:12 ` [PATCH v3 07/11] Documentation/replace: tell that -f option bypasses the type check Christian Couder
2013-08-31 22:16 ` Philip Oakley
2013-09-01 11:49 ` Christian Couder
2013-09-01 20:11 ` Philip Oakley
2013-09-02 6:11 ` Christian Couder
2013-09-02 21:50 ` Philip Oakley
2013-09-02 21:55 ` Jonathan Nieder
2013-09-02 22:13 ` Philip Oakley
2013-09-02 22:26 ` Jonathan Nieder
2013-09-02 22:45 ` Philip Oakley
2013-09-03 9:29 ` Christian Couder
2013-08-31 19:12 ` [PATCH v3 08/11] t6050-replace: check " Christian Couder
2013-09-01 7:50 ` Eric Sunshine
2013-09-01 10:02 ` Christian Couder
2013-08-31 19:12 ` [PATCH v3 09/11] replace: allow long option names Christian Couder
2013-08-31 19:12 ` [PATCH v3 10/11] Documentation/replace: list " Christian Couder
2013-08-31 19:12 ` [PATCH v3 11/11] t6050-replace: use some " Christian Couder
2013-08-31 22:19 ` Philip Oakley
2013-09-01 10:11 ` Christian Couder
2013-09-01 8:07 ` Eric Sunshine
2013-09-01 10:01 ` Christian Couder [this message]
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=20130901.120132.1611298105222242724.chriscool@tuxfamily.org \
--to=chriscool@tuxfamily.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=j6t@kdbg.org \
--cc=philipoakley@iee.org \
--cc=sunshine@sunshineco.com \
--cc=trast@inf.ethz.ch \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).