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

Re: [PATCH v14 6/6] commit: add a commit.verbose config variable

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Apr 13, 2016, 17:44 UTC
Message-ID
<CAPig+cQjOq_cKCa7_9CVG0tzd7x1aW9Wg4tZ+UvQUAvkPRWWHA@mail.gmail.com>
In-Reply-To
<CAFZEwPN+N+21CN+P3oegaxEve59BKHabdfyM_c7QdjV+bonpQA@mail.gmail.com>
On Wed, Apr 13, 2016 at 5:15 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 18 quoted lines
> On Wed, Apr 13, 2016 at 11:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> +test_expect_success 'status does not verbose without --verbose' '
>>> +       git status >actual &&
>>> +       ! grep "^diff --git" actual
>>> +'
>>
>> But what is this test checking?
>
> status is also a consumer of the verbose whose initial value is set to
> -1. This makes it include verbose in status output. This bug was fixed
> by explicitly initializing verbose to 0 if -1. SZEDER pointed out a
> bug[1] which broke some tests in and then when I fixed it, you
> requested me to include tests even in this patch[2] which I found
> convincing enough.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289730
> [2]: http://article.gmane.org/gmane.comp.version-control.git/289993

Okay, makes sense, but it's not at all obvious from the context of this patch or its commit message. It probably would have been clearer had the two git-status tests been added in a separate preparatory test with a commit message explaining that the tests are to ensure that a subsequent patch (adding commit.verbose) won't break the existing behavior of git-status. Having a separate commit explaining that would also help future readers of the test script who wonder what this test is doing (since it's not obvious and it's not explained by the current commit message) when they use git-blame to try to figure out its purpose. If you do re-roll, you might consider breaking them out to a new patch or, at the very least, document their purpose in the commit message of this patch.

Previous: Pranit BauvaNext: Pranit Bauva
Message 13 of 22 in “t0040-test-parse-options.sh: fix style issues”
  1. 1/6 t0040-test-parse-options.sh: fix style issuesPranit Bauva, Apr 12, 2016
  2. 2/6 test-parse-options: print quiet as integerPranit Bauva, Apr 12, 2016
  3. 3/6 t0040-parse-options: improve test coveragePranit Bauva, Apr 12, 2016
  4. Eric SunshineApr 13, 2016
  5. Pranit BauvaApr 13, 2016
  6. Eric SunshineApr 13, 2016
  7. Pranit BauvaApr 25, 2016
  8. Eric SunshineApr 27, 2016
  9. Pranit BauvaApr 27, 2016
  10. 6/6 commit: add a commit.verbose config variablePranit Bauva, Apr 12, 2016
  11. Eric SunshineApr 13, 2016
  12. Pranit BauvaApr 13, 2016
  13. Eric SunshineApr 13, 2016
  14. 4/6 parse-options.c: make OPTION_COUNTUP respect "unspecified" valuesPranit Bauva, Apr 12, 2016
  15. Eric SunshineApr 13, 2016
  16. Pranit BauvaApr 13, 2016
  17. Eric SunshineApr 13, 2016
  18. Pranit BauvaApr 13, 2016
  19. Eric SunshineApr 13, 2016
  20. 5/6 t7507-commit-verbose: improve test coverage by testing number of diffsPranit Bauva, Apr 12, 2016
  21. Eric SunshineApr 13, 2016
  22. Pranit BauvaApr 13, 2016

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.