From: Philip Oakley Date: Fri, 11 Mar 2016 18:39:56 GMT Subject: Re: [PATCH v4] commit: add a commit.verbose config variable Message-ID: <83A7534D5757482089D43FFE96FCA1C9@PhilipOakley> In-Reply-To: From: "Pranit Bauva" > On Fri, Mar 11, 2016 at 4:31 AM, Eric Sunshine > wrote: > >> Add commit.verbose configuration variable as a convenience >> for those who always prefer --verbose. >> >> or something. > > Sure! > >> As a convenience to reviewers, please use this area below the "---" >> line to provide links and explain what changed since the previous >> round rather than doing so in a separate email. > > Actually I am sending the patches with submitGit herokuapp because my > institute proxy does not allow IMAP/POP3 connections. You can still include the 'three dashes' and a commentary at the end of your (local) regular commit message, and then when it is sent as a patch it will have the right format. There are carried through rebases as well. There is a similar feature for attaching notes (though I haven't used it). Either should get around your institute's proxy issue. > >> The "permanently" bit sounds scary. A more concise way to state this >> might be: >> >> See the `commit.verbose` configuration variable in >> linkgit:git-config[1]. >> >> which doesn't bother spelling out what the intelligent reader should >> infer from the reference. >> Style: space before { > > Sure! > >>> +test_expect_success 'commit with commit.verbose true and no arguments' >>> ' >> >> "no arguments" doesn't convey much; how about "--verbose omitted" or >> something? Ditto for the titles of other tests. > > Will change the language construct. >>> + echo content >file && >>> + git add file && >>> + test_config commit.verbose true && >>> + ( >>> + GIT_EDITOR=cat && >>> + export GIT_EDITOR && >>> + test_must_fail git commit >output >>> + ) && >>> + test_i18ngrep "diff --git" output >>> +' >> >> Making git-commit fail unconditionally with "aborting due to empty >> commit message" is a rather sneaky way to perform this test. I would >> have expected to see these new tests re-use the existing machinery >> provided by this script (the check-for-diff "editor") rather than >> inventing an entirely new and unintuitive mechanism. Doing so would >> also reduce the size of each new test. > > I agree on the fact that making git-commit fail unconditionally is not > a good way to perform the test. "check-for-diff" is not really an > "editor" and it checks for the commit message after it has been > written to the history. The verbose output is stripped when it is > written to the history so we won't be able to test whether this patch > works. This is where purposely breaking the code is required as when > the commit fails, it gives the output of the contents present at that > time (which will contain the verbose output). More over the > 'check-for-diff' uses grep which is not preferred. Many tests are now > using test_i18ngrep (eg. f79ce8db). I had planned on using > 'check-for-diff' before but it took me some time to figure out this > behavior and thus I began searching for another mechanism (breaking > code). > >> Some additional tests[1][2] are probably warranted. >> >> [1]: http://article.gmane.org/gmane.comp.version-control.git/288648 >> [2]: http://article.gmane.org/gmane.comp.version-control.git/288657 > > I think these tests also are better included in this file as this > patch triggers it and it would not make much of a difference between > t7507 and t7502 but in fact improve its readability. >>> test_done