git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v3 11/11] t6050-replace: use some long option names

From
Christian Couder <chriscool@tuxfamily.org>
Date
Sep 1, 2013, 10:01 UTC
Message-ID
<20130901.120132.1611298105222242724.chriscool@tuxfamily.org>
In-Reply-To
<CAPig+cSwQdhybTzRwqGLbdxGRcAZfBeU6jjWYy43G0oT0hxkNw@mail.gmail.com>
From: Eric Sunshine <sunshine@sunshineco.com>
Show 21 quoted lines
>
>> 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.

Previous: Eric Sunshine
Message 33 of 33 in “Check replacement object type and minor updates”
  1. 00/11 Check replacement object type and minor updatesChristian Couder, Aug 31, 2013
  2. 01/11 replace: forbid replacing an object with one of a different typeChristian Couder, Aug 31, 2013
  3. Philip OakleyAug 31, 2013
  4. Christian CouderSep 1, 2013
  5. Philip OakleySep 1, 2013
  6. 02/11 Documentation/replace: state that objects must be of the same typeChristian Couder, Aug 31, 2013
  7. 03/11 t6050-replace: test that objects are of the same typeChristian Couder, Aug 31, 2013
  8. 04/11 t6050-replace: add test to clean up all the replace refsChristian Couder, Aug 31, 2013
  9. 05/11 Documentation/replace: add Creating Replacement Objects sectionChristian Couder, Aug 31, 2013
  10. Philip OakleyAug 31, 2013
  11. Christian CouderSep 1, 2013
  12. 06/11 replace: bypass the type check if -f option is usedChristian Couder, Aug 31, 2013
  13. 07/11 Documentation/replace: tell that -f option bypasses the type checkChristian Couder, Aug 31, 2013
  14. Philip OakleyAug 31, 2013
  15. Christian CouderSep 1, 2013
  16. Philip OakleySep 1, 2013
  17. Christian CouderSep 2, 2013
  18. Philip OakleySep 2, 2013
  19. Jonathan NiederSep 2, 2013
  20. Philip OakleySep 2, 2013
  21. Jonathan NiederSep 2, 2013
  22. Philip OakleySep 2, 2013
  23. Christian CouderSep 3, 2013
  24. 08/11 t6050-replace: check that -f option bypasses the type checkChristian Couder, Aug 31, 2013
  25. Eric SunshineSep 1, 2013
  26. Christian CouderSep 1, 2013
  27. 09/11 replace: allow long option namesChristian Couder, Aug 31, 2013
  28. 10/11 Documentation/replace: list long option namesChristian Couder, Aug 31, 2013
  29. 11/11 t6050-replace: use some long option namesChristian Couder, Aug 31, 2013
  30. Philip OakleyAug 31, 2013
  31. Christian CouderSep 1, 2013
  32. Eric SunshineSep 1, 2013
  33. Christian CouderSep 1, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.