From: Junio C Hamano <hidden> Date: 2016-06-16 02:19:08
Pranit Bauva [off-list ref] writes:
Variable named 'verbose' in builtin/commit.c is consumed by git-status
and git-commit so if a new verbose related behavior is introduced in
git-commit, then it should not affect the behavior of git-status.
One previous commit (title: commit: add a commit.verbose config
variable) introduced a new config variable named commit.verbose,
so care should be taken that it would not affect the behavior of
status.
Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
respect "unspecified" values") changes the initial value of verbose
from 0 to -1. This can cause git-status to display a verbose output even
when it isn't supposed to.
Signed-off-by: Pranit Bauva <redacted>
---
This is a split off from the previous patch 6/6 as suggested by Eric
Sunshine.
If these are documenting what your previous patches broke, then
there test body should describe what should happen, and then if it
is broken, use test_expect_failure, no?
Your first test does "run status with commit.verbose is set, and
make sure the "diff --git" does not appear", which is correct, so if
it does not work, test_expect_failure would be the right thing to
use.
These, especially the latter, look rather unpleasant regressions to
me, and the main commit.verbose change would need to be held back
before they are fixed.
@@ -144,4 +144,14 @@ do"done+test_expect_success'status ignores commit.verbose=true''+git-ccommit.verbose=truestatus>actual&&+!grep"^diff --git"actual+'++test_expect_success'status does not verbose without --verbose''+gitstatus>actual&&+!grep"^diff --git"actual+'+ test_done
On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano [off-list ref] wrote:
Pranit Bauva [off-list ref] writes:
quoted
Variable named 'verbose' in builtin/commit.c is consumed by git-status
and git-commit so if a new verbose related behavior is introduced in
git-commit, then it should not affect the behavior of git-status.
One previous commit (title: commit: add a commit.verbose config
variable) introduced a new config variable named commit.verbose,
so care should be taken that it would not affect the behavior of
status.
Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
respect "unspecified" values") changes the initial value of verbose
from 0 to -1. This can cause git-status to display a verbose output even
when it isn't supposed to.
Signed-off-by: Pranit Bauva <redacted>
---
This is a split off from the previous patch 6/6 as suggested by Eric
Sunshine.
If these are documenting what your previous patches broke, then
there test body should describe what should happen, and then if it
is broken, use test_expect_failure, no?
Your first test does "run status with commit.verbose is set, and
make sure the "diff --git" does not appear", which is correct, so if
it does not work, test_expect_failure would be the right thing to
use.
These, especially the latter, look rather unpleasant regressions to
me, and the main commit.verbose change would need to be held back
before they are fixed.
I agree that using test_expect_failure would be a better way of going
with this thing. Thanks. Will send an updated patch for this.
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:08
On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva [off-list ref] wrote:
On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano [off-list ref] wrote:
quoted
Pranit Bauva [off-list ref] writes:
quoted
Variable named 'verbose' in builtin/commit.c is consumed by git-status
and git-commit so if a new verbose related behavior is introduced in
git-commit, then it should not affect the behavior of git-status.
One previous commit (title: commit: add a commit.verbose config
variable) introduced a new config variable named commit.verbose,
so care should be taken that it would not affect the behavior of
status.
Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
respect "unspecified" values") changes the initial value of verbose
from 0 to -1. This can cause git-status to display a verbose output even
when it isn't supposed to.
Signed-off-by: Pranit Bauva <redacted>
If these are documenting what your previous patches broke, then
there test body should describe what should happen, and then if it
is broken, use test_expect_failure, no?
Your first test does "run status with commit.verbose is set, and
make sure the "diff --git" does not appear", which is correct, so if
it does not work, test_expect_failure would be the right thing to
use.
These, especially the latter, look rather unpleasant regressions to
me, and the main commit.verbose change would need to be held back
before they are fixed.
I agree that using test_expect_failure would be a better way of going
with this thing. Thanks. Will send an updated patch for this.
Please don't. test_expect_failure() is not warranted.
Step back a moment and recall why these tests were added. Earlier
rounds of this series were buggy and caused regressions in git-status.
As a consequence, reviewers suggested[1,2] that you improve test
coverage to ensure that such breakage is caught early.
The problems which caused the regressions were addressed in later
versions of the series, thus using test_expect_success() is indeed
correct, whereas test_expect_failure(), which illustrates broken
behavior, would be the wrong choice.
The point of these new tests is to prevent regressions caused by
*subsequent* changes, which is why it was suggested that these tests
be added early (as a "preparatory patch"[3]), not at the very end of
the series as done here in v15.
This patch's commit message is perhaps a bit too detailed about what
could have gone wrong in earlier patches in this series; indeed, it
misled Junio into thinking that patches in this series did break
behavior, when in fact, it was instead previous rounds of this series
which were buggy. If you instead make this a preparatory patch[3],
then you can sell it more simply by explaining that git-commit and
git-status share implementation (without necessarily going into detail
about exactly what is shared), and that you're improving test coverage
to ensure that changes specific to git-commit don't accidentally
impact git-status, as well.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648
[2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730
[3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468
On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine [off-list ref] wrote:
On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva [off-list ref] wrote:
quoted
On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano [off-list ref] wrote:
quoted
Pranit Bauva [off-list ref] writes:
quoted
Variable named 'verbose' in builtin/commit.c is consumed by git-status
and git-commit so if a new verbose related behavior is introduced in
git-commit, then it should not affect the behavior of git-status.
One previous commit (title: commit: add a commit.verbose config
variable) introduced a new config variable named commit.verbose,
so care should be taken that it would not affect the behavior of
status.
Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
respect "unspecified" values") changes the initial value of verbose
from 0 to -1. This can cause git-status to display a verbose output even
when it isn't supposed to.
Signed-off-by: Pranit Bauva <redacted>
If these are documenting what your previous patches broke, then
there test body should describe what should happen, and then if it
is broken, use test_expect_failure, no?
Your first test does "run status with commit.verbose is set, and
make sure the "diff --git" does not appear", which is correct, so if
it does not work, test_expect_failure would be the right thing to
use.
These, especially the latter, look rather unpleasant regressions to
me, and the main commit.verbose change would need to be held back
before they are fixed.
I agree that using test_expect_failure would be a better way of going
with this thing. Thanks. Will send an updated patch for this.
Please don't. test_expect_failure() is not warranted.
I got confused between test_must_fail and test_expect_failure. I
thought Junio mentioned to use test_must_fail and remove the " ! "
sign.
Step back a moment and recall why these tests were added. Earlier
rounds of this series were buggy and caused regressions in git-status.
As a consequence, reviewers suggested[1,2] that you improve test
coverage to ensure that such breakage is caught early.
The problems which caused the regressions were addressed in later
versions of the series, thus using test_expect_success() is indeed
correct, whereas test_expect_failure(), which illustrates broken
behavior, would be the wrong choice.
The point of these new tests is to prevent regressions caused by
*subsequent* changes, which is why it was suggested that these tests
be added early (as a "preparatory patch"[3]), not at the very end of
the series as done here in v15.
This patch's commit message is perhaps a bit too detailed about what
could have gone wrong in earlier patches in this series; indeed, it
misled Junio into thinking that patches in this series did break
behavior, when in fact, it was instead previous rounds of this series
which were buggy. If you instead make this a preparatory patch[3],
then you can sell it more simply by explaining that git-commit and
git-status share implementation (without necessarily going into detail
about exactly what is shared), and that you're improving test coverage
to ensure that changes specific to git-commit don't accidentally
impact git-status, as well.
Sure! I just wanted the commit message to be detailed as per the
guidelines given by SubmittingPatches. I will swap the patch 6/7 and
patch 7/7 changing the commit message. Also I will make the commit
message less detailed.
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:08
On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva [off-list ref] wrote:
On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva [off-list ref] wrote:
quoted
I agree that using test_expect_failure would be a better way of going
with this thing. Thanks. Will send an updated patch for this.
Please don't. test_expect_failure() is not warranted.
I got confused between test_must_fail and test_expect_failure. I
thought Junio mentioned to use test_must_fail and remove the " ! "
sign.
quoted
Step back a moment and recall why these tests were added. Earlier
rounds of this series were buggy and caused regressions in git-status.
As a consequence, reviewers suggested[1,2] that you improve test
coverage to ensure that such breakage is caught early.
The problems which caused the regressions were addressed in later
versions of the series, thus using test_expect_success() is indeed
correct, whereas test_expect_failure(), which illustrates broken
behavior, would be the wrong choice.
The point of these new tests is to prevent regressions caused by
*subsequent* changes, which is why it was suggested that these tests
be added early (as a "preparatory patch"[3]), not at the very end of
the series as done here in v15.
This patch's commit message is perhaps a bit too detailed about what
could have gone wrong in earlier patches in this series; indeed, it
misled Junio into thinking that patches in this series did break
behavior, when in fact, it was instead previous rounds of this series
which were buggy. If you instead make this a preparatory patch[3],
then you can sell it more simply by explaining that git-commit and
git-status share implementation (without necessarily going into detail
about exactly what is shared), and that you're improving test coverage
to ensure that changes specific to git-commit don't accidentally
impact git-status, as well.
Sure! I just wanted the commit message to be detailed as per the
guidelines given by SubmittingPatches. I will swap the patch 6/7 and
patch 7/7 changing the commit message. Also I will make the commit
message less detailed.
This patch should be inserted before 4/7 since it needs to protect
against breakage which might occur when 4/7 changes the behavior of
OPTION_COUNTUP.
On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine [off-list ref] wrote:
On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva [off-list ref] wrote:
quoted
On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine [off-list ref] wrote:
quoted
On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva [off-list ref] wrote:
quoted
I agree that using test_expect_failure would be a better way of going
with this thing. Thanks. Will send an updated patch for this.
Please don't. test_expect_failure() is not warranted.
I got confused between test_must_fail and test_expect_failure. I
thought Junio mentioned to use test_must_fail and remove the " ! "
sign.
quoted
Step back a moment and recall why these tests were added. Earlier
rounds of this series were buggy and caused regressions in git-status.
As a consequence, reviewers suggested[1,2] that you improve test
coverage to ensure that such breakage is caught early.
The problems which caused the regressions were addressed in later
versions of the series, thus using test_expect_success() is indeed
correct, whereas test_expect_failure(), which illustrates broken
behavior, would be the wrong choice.
The point of these new tests is to prevent regressions caused by
*subsequent* changes, which is why it was suggested that these tests
be added early (as a "preparatory patch"[3]), not at the very end of
the series as done here in v15.
This patch's commit message is perhaps a bit too detailed about what
could have gone wrong in earlier patches in this series; indeed, it
misled Junio into thinking that patches in this series did break
behavior, when in fact, it was instead previous rounds of this series
which were buggy. If you instead make this a preparatory patch[3],
then you can sell it more simply by explaining that git-commit and
git-status share implementation (without necessarily going into detail
about exactly what is shared), and that you're improving test coverage
to ensure that changes specific to git-commit don't accidentally
impact git-status, as well.
Sure! I just wanted the commit message to be detailed as per the
guidelines given by SubmittingPatches. I will swap the patch 6/7 and
patch 7/7 changing the commit message. Also I will make the commit
message less detailed.
This patch should be inserted before 4/7 since it needs to protect
against breakage which might occur when 4/7 changes the behavior of
OPTION_COUNTUP.
I forgot to mention about this earlier. When I was rebasing, this stroked me.
I guess making any changes in ordering the commits will make one of
the test as absurd. One of the test uses a configuration variable
'commit.verbose' will won't be effective before the patch 6/7. So I
guess I will have to only change the commit message to reflect as
"improving test coverage".
From: Eric Sunshine <hidden> Date: 2016-06-16 02:19:09
On Tue, May 3, 2016 at 5:18 AM, Pranit Bauva [off-list ref] wrote:
On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine [off-list ref] wrote:
quoted
quoted
quoted
Step back a moment and recall why these tests were added. Earlier
rounds of this series were buggy and caused regressions in git-status.
As a consequence, reviewers suggested[1,2] that you improve test
coverage to ensure that such breakage is caught early.
The point of these new tests is to prevent regressions caused by
*subsequent* changes, which is why it was suggested that these tests
be added early (as a "preparatory patch"[3]), not at the very end of
the series as done here in v15.
Sure! I just wanted the commit message to be detailed as per the
guidelines given by SubmittingPatches. I will swap the patch 6/7 and
patch 7/7 changing the commit message. Also I will make the commit
message less detailed.
This patch should be inserted before 4/7 since it needs to protect
against breakage which might occur when 4/7 changes the behavior of
OPTION_COUNTUP.
I forgot to mention about this earlier. When I was rebasing, this stroked me.
I guess making any changes in ordering the commits will make one of
the test as absurd. One of the test uses a configuration variable
'commit.verbose' will won't be effective before the patch 6/7. So I
guess I will have to only change the commit message to reflect as
"improving test coverage".
I also had intended to talk about this but forgot. What would be quite
logical is to introduce only the "git-status without --verbose" test
in this new "improve coverage" patch before 4/7. The other test, which
ensures that git-status doesn't regress with commit.verbose, would
then very naturally be included in the patch which adds the
commit.verbose functionality (currently patch 6/7).
On Tue, May 3, 2016 at 9:47 PM, Eric Sunshine [off-list ref] wrote:
On Tue, May 3, 2016 at 5:18 AM, Pranit Bauva [off-list ref] wrote:
quoted
On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine [off-list ref] wrote:
quoted
quoted
quoted
Step back a moment and recall why these tests were added. Earlier
rounds of this series were buggy and caused regressions in git-status.
As a consequence, reviewers suggested[1,2] that you improve test
coverage to ensure that such breakage is caught early.
The point of these new tests is to prevent regressions caused by
*subsequent* changes, which is why it was suggested that these tests
be added early (as a "preparatory patch"[3]), not at the very end of
the series as done here in v15.
Sure! I just wanted the commit message to be detailed as per the
guidelines given by SubmittingPatches. I will swap the patch 6/7 and
patch 7/7 changing the commit message. Also I will make the commit
message less detailed.
This patch should be inserted before 4/7 since it needs to protect
against breakage which might occur when 4/7 changes the behavior of
OPTION_COUNTUP.
I forgot to mention about this earlier. When I was rebasing, this stroked me.
I guess making any changes in ordering the commits will make one of
the test as absurd. One of the test uses a configuration variable
'commit.verbose' will won't be effective before the patch 6/7. So I
guess I will have to only change the commit message to reflect as
"improving test coverage".
I also had intended to talk about this but forgot. What would be quite
logical is to introduce only the "git-status without --verbose" test
in this new "improve coverage" patch before 4/7. The other test, which
ensures that git-status doesn't regress with commit.verbose, would
then very naturally be included in the patch which adds the
commit.verbose functionality (currently patch 6/7).