From: Junio C Hamano <hidden> Date: 2016-06-15 22:49:09
Ævar Arnfjörð Bjarmason [off-list ref] writes:
On Tue, Jul 20, 2010 at 18:00, Junio C Hamano [off-list ref] wrote:
quoted
Run a git command and ensure it fails in a controlled way. Use
this instead of "! <git-command>". When git-command dies due to a
segfault, test_must_fail diagnoses it as an error; "! <git-command>"
treats it as just another expected failure. letting such a bug go
unnoticed.
To add to that:
Don't use test_must_fail to negate the return values of commands
on the system like grep, sed etc. If we can't trust that the core
utilities won't randomly segfault we might as well die horribly.
I think you are being incoherent. If we can't trust system "grep" and it
randomly segfaults, then a test:
git some-command >actual &&
! grep string-that-should-not-be-in-the-output actual
would _pass_ when the command segfaults. I do agree with you that "We
might as well die horribly", and the way you do so is by protecting the
test with test_must_fail, like this:
git some-command >actual &&
test_must_fail grep string-that-should-not-be-in-the-output actual
Having said that, as we _do_ trust system tools to a certain degree, we do
not care very deeply about this. IOW, I wouldn't want to see a patch that
rewrites "! grep" to "test_must_fail grep".
Thanks.
On Tue, Jul 20, 2010 at 18:34, Junio C Hamano [off-list ref] wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
On Tue, Jul 20, 2010 at 18:00, Junio C Hamano [off-list ref] wrote:
quoted
Run a git command and ensure it fails in a controlled way. Use
this instead of "! <git-command>". When git-command dies due to a
segfault, test_must_fail diagnoses it as an error; "! <git-command>"
treats it as just another expected failure. letting such a bug go
unnoticed.
To add to that:
Don't use test_must_fail to negate the return values of commands
on the system like grep, sed etc. If we can't trust that the core
utilities won't randomly segfault we might as well die horribly.
I think you are being incoherent. If we can't trust system "grep" and it
randomly segfaults, then a test:
git some-command >actual &&
! grep string-that-should-not-be-in-the-output actual
would _pass_ when the command segfaults. I do agree with you that "We
might as well die horribly", and the way you do so is by protecting the
test with test_must_fail, like this:
git some-command >actual &&
test_must_fail grep string-that-should-not-be-in-the-output actual
Having said that, as we _do_ trust system tools to a certain degree, we do
not care very deeply about this. IOW, I wouldn't want to see a patch that
rewrites "! grep" to "test_must_fail grep".
An individual test would pass, yes. But if test or grep are
segfaulting we're going to bail out horribly eventually anyway, so I
don't think it's worth the effort to guard them with test_must_fail,
and I wouldn't write tests to do that. I'd just use !.
That's what we seem to be doing in the tests so far, i.e. test_must_fail
is reserved for git commands only.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:09
Ævar Arnfjörð Bjarmason wrote:
That's what we seem to be doing in the tests so far, i.e. test_must_fail
is reserved for git commands only.
test_must_fail relies on conventions for return value that cannot
necessarily be relied on from outside utilities.
Thanks for cleaning up my mess.
Jonathan
On 07/20/2010 03:49 PM, Ævar Arnfjörð Bjarmason wrote:
On Tue, Jul 20, 2010 at 19:16, Jonathan Nieder [off-list ref] wrote:
quoted
Ęvar Arnfjörš Bjarmason wrote:
quoted
That's what we seem to be doing in the tests so far, i.e. test_must_fail
is reserved for git commands only.
test_must_fail relies on conventions for return value that cannot
necessarily be relied on from outside utilities.
Right, someone should send a patch for these:
ack 'test_must_fail (?!git)' *sh
:)
You joke, but thanks to your prodding, I discovered these broken
tests that should definitely all be fixed:
$ perl -ne 'm/test_must_fail +[^ ]+=/ && print' *sh
test_must_fail PAGER= git reflog show delta &&
test_must_fail PAGER= git reflog show epsilon &&
test_must_fail PAGER= git reflog show epsilon
test_must_fail PAGER= git reflog show zeta &&
test_must_fail PAGER= git reflog show eta &&
test_must_fail PAGER= git reflog show eta
test_must_fail PAGER= git reflog show beta
test_must_fail MSG="yet another note" git notes add -c deadbeef &&
one-shot variable assignment does not work with test_must_fail.
See e2007832552ccea9befed9003580c494f09e666e for an explanation.
-brandon
On Tue, Jul 20, 2010 at 21:12, Brandon Casey [off-list ref] wrote:
On 07/20/2010 03:49 PM, Ævar Arnfjörð Bjarmason wrote:
quoted
On Tue, Jul 20, 2010 at 19:16, Jonathan Nieder [off-list ref] wrote:
quoted
Ęvar Arnfjörš Bjarmason wrote:
quoted
That's what we seem to be doing in the tests so far, i.e. test_must_fail
is reserved for git commands only.
test_must_fail relies on conventions for return value that cannot
necessarily be relied on from outside utilities.
Right, someone should send a patch for these:
ack 'test_must_fail (?!git)' *sh
:)
You joke, but thanks to your prodding, I discovered these broken
tests that should definitely all be fixed:
Oh I'm completely serious, I'm just too lazy to do these myself today :)
$ perl -ne 'm/test_must_fail +[^ ]+=/ && print' *sh
test_must_fail PAGER= git reflog show delta &&
test_must_fail PAGER= git reflog show epsilon &&
test_must_fail PAGER= git reflog show epsilon
test_must_fail PAGER= git reflog show zeta &&
test_must_fail PAGER= git reflog show eta &&
test_must_fail PAGER= git reflog show eta
test_must_fail PAGER= git reflog show beta
test_must_fail MSG="yet another note" git notes add -c deadbeef &&
one-shot variable assignment does not work with test_must_fail.
See e2007832552ccea9befed9003580c494f09e666e for an explanation.
No time to investigate, but here is an example patch and the
results of running the affected tests. Looks like reflog may
be creating a reflog when it is not supposed to.
Erick, I added you to cc since you appear to be the author of the tests
in question.
$ ./t2017-checkout-orphan.sh
<snip>
not ok - 8 --orphan does not make reflog when core.logAllRefUpdates = false
#
# git checkout master &&
# git config core.logAllRefUpdates false &&
# git checkout --orphan epsilon &&
# ! test -f .git/logs/refs/heads/epsilon &&
# (
# PAGER= &&
# export PAGER &&
# test_must_fail git reflog show epsilon
# ) &&
# git commit -m Epsilon &&
# ! test -f .git/logs/refs/heads/epsilon &&
# (
# PAGER= &&
# export PAGER &&
# test_must_fail git reflog show epsilon
# )
#
<snip>
$ ./t3200-branch.sh
<snip>
not ok - 32 checkout -b does not make reflog when core.logAllRefUpdates = false
#
# git checkout master &&
# git config core.logAllRefUpdates false &&
# git checkout -b beta &&
# ! test -f .git/logs/refs/heads/beta &&
# (
# PAGER= &&
# export PAGER &&
# test_must_fail git reflog show beta
# )
#
<snip>
--->8---
From: Brandon Casey <redacted>
See e2007832552ccea9befed9003580c494f09e666e
---
t/t2017-checkout-orphan.sh | 36 ++++++++++++++++++++++++++++++------
t/t3200-branch.sh | 6 +++++-
t/t3301-notes.sh | 6 +++++-
3 files changed, 40 insertions(+), 8 deletions(-)
@@ -69,7 +69,11 @@ test_expect_success '--orphan makes reflog by default' 'gitconfig--unsetcore.logAllRefUpdates&&gitcheckout--orphandelta&&!test-f.git/logs/refs/heads/delta&&-test_must_failPAGER=gitreflogshowdelta&&+(+PAGER=&&+exportPAGER&&+test_must_failgitreflogshowdelta+)&&gitcommit-mDelta&&test-f.git/logs/refs/heads/delta&&PAGER=gitreflogshowdelta
@@ -80,17 +84,29 @@ test_expect_success '--orphan does not make reflog when core.logAllRefUpdates =gitconfigcore.logAllRefUpdatesfalse&&gitcheckout--orphanepsilon&&!test-f.git/logs/refs/heads/epsilon&&-test_must_failPAGER=gitreflogshowepsilon&&+(+PAGER=&&+exportPAGER&&+test_must_failgitreflogshowepsilon+)&&gitcommit-mEpsilon&&!test-f.git/logs/refs/heads/epsilon&&-test_must_failPAGER=gitreflogshowepsilon+(+PAGER=&&+exportPAGER&&+test_must_failgitreflogshowepsilon+)' test_expect_success'--orphan with -l makes reflog when core.logAllRefUpdates = false''gitcheckoutmaster&&gitcheckout-l--orphanzeta&&test-f.git/logs/refs/heads/zeta&&-test_must_failPAGER=gitreflogshowzeta&&+(+PAGER=&&+exportPAGER&&+test_must_failgitreflogshowzeta+)&&gitcommit-mZeta&&PAGER=gitreflogshowzeta'
@@ -99,10 +115,18 @@ test_expect_success 'giving up --orphan not committed when -l and core.logAllRefgitcheckoutmaster&&gitcheckout-l--orphaneta&&test-f.git/logs/refs/heads/eta&&-test_must_failPAGER=gitreflogshoweta&&+(+PAGER=&&+exportPAGER&&+test_must_failgitreflogshoweta+)&&gitcheckoutmaster&&!test-f.git/logs/refs/heads/eta&&-test_must_failPAGER=gitreflogshoweta+(+PAGER=&&+exportPAGER&&+test_must_failgitreflogshoweta+)' test_expect_success'--orphan is rejected with an existing name''
@@ -237,7 +237,11 @@ test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdatesgitconfigcore.logAllRefUpdatesfalse&&gitcheckout-bbeta&&!test-f.git/logs/refs/heads/beta&&-test_must_failPAGER=gitreflogshowbeta+(+PAGER=&&+exportPAGER&&+test_must_failgitreflogshowbeta+)' test_expect_success'checkout -b with -l makes reflog when core.logAllRefUpdates = false''
@@ -693,7 +693,11 @@ test_expect_success 'create note from non-existing note with "git notes add -c"gitadda10&&test_tick&&gitcommit-m10th&&-test_must_failMSG="yet another note"gitnotesadd-cdeadbeef&&+(+MSG="yet another note"&&+exportMSG&&+test_must_failgitnotesadd-cdeadbeef+)&&test_must_failgitnoteslistHEAD'
No time to investigate, but here is an example patch and the
results of running the affected tests. Looks like reflog may
be creating a reflog when it is not supposed to.
Erick, I added you to cc since you appear to be the author of the tests
in question.
Thanks for your kindness.
$ ./t2017-checkout-orphan.sh
<snip>
not ok - 8 --orphan does not make reflog when core.logAllRefUpdates = false
#
# git checkout master &&
# git config core.logAllRefUpdates false &&
# git checkout --orphan epsilon &&
# ! test -f .git/logs/refs/heads/epsilon &&
# (
# PAGER= &&
# export PAGER &&
# test_must_fail git reflog show epsilon
# ) &&
# git commit -m Epsilon &&
# ! test -f .git/logs/refs/heads/epsilon &&
# (
# PAGER= &&
# export PAGER &&
# test_must_fail git reflog show epsilon
# )
#
<snip>
$ ./t3200-branch.sh
<snip>
not ok - 32 checkout -b does not make reflog when core.logAllRefUpdates = false
#
# git checkout master &&
# git config core.logAllRefUpdates false &&
# git checkout -b beta &&
# ! test -f .git/logs/refs/heads/beta &&
# (
# PAGER= &&
# export PAGER &&
# test_must_fail git reflog show beta
# )
#
<snip>
You have made cosmetic changes which do not do the same as the original.
I don't like this proposed patch because I don't see an improvement
when:
* you change a line to three;
* the original line is clear enough;
* the resulting lines improves nothing;
* it is for cosmetic purposes only inside a script;
* and it uses more resources, even if little more as by creating a
subshell environment, to do the same.
Best regards
You must use "export PAGER;", not "export PAGER &&". export doesn't
return zero on all systems when exporting, see previous changes in
this regard in t/.
You must use "export PAGER;", not "export PAGER &&". export doesn't
return zero on all systems when exporting, see previous changes in
this regard in t/.
Actually, see the t/README docs which explicitly mention this. Yay docs.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:09
Ævar Arnfjörð Bjarmason wrote:
You must use "export PAGER;", not "export PAGER &&". export doesn't
return zero on all systems when exporting, see previous changes in
this regard in t/.
@@ -259,11 +259,11 @@ Do: test ... That way all of the commands in your tests will succeed or fail. If- you must ignore the return value of something (e.g. the return- value of export is unportable) it's best to indicate so explicitly- with a semicolon:+ you must ignore the return value of something (e.g., the return+ after unsetting a variable that was already unset is unportable) it's+ best to indicate so explicitly with a semicolon:- export HLAGH;+ unset HLAGH; git merge hla && git push gh && test ...
On Wed, Jul 21, 2010 at 00:01, Jonathan Nieder [off-list ref] wrote:
quoted hunk
Ævar Arnfjörð Bjarmason wrote:
quoted
You must use "export PAGER;", not "export PAGER &&". export doesn't
return zero on all systems when exporting, see previous changes in
this regard in t/.
test ...
That way all of the commands in your tests will succeed or fail. If
- you must ignore the return value of something (e.g. the return
- value of export is unportable) it's best to indicate so explicitly
- with a semicolon:
+ you must ignore the return value of something (e.g., the return
+ after unsetting a variable that was already unset is unportable) it's
+ best to indicate so explicitly with a semicolon:
We should have examples for both export and unset, but the prose
should mention both IMO
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:09
Ævar Arnfjörð Bjarmason wrote:
We should have examples for both export and unset
What is unportable for “export” is the effect of exporting an unset
variable. I am not even sure whether the return value is unportable,
but it doesn’t matter; that is an example of a “Don’t” rather than a
“Do it this way”.
See v1.5.6-rc0~61 (filter-branch: fix variable export logic,
2008-05-13) for an example.
but the prose
should mention both IMO
Yes, thanks for putting this portability guide together.
Jonathan
On Wed, Jul 21, 2010 at 00:14, Jonathan Nieder [off-list ref] wrote:
Ævar Arnfjörð Bjarmason wrote:
quoted
We should have examples for both export and unset
What is unportable for “export” is the effect of exporting an unset
variable. I am not even sure whether the return value is unportable,
but it doesn’t matter; that is an example of a “Don’t” rather than a
“Do it this way”.
I didn't know that. It'd be good if it were mentioned in the docs. I
thought it was just export in general.
See v1.5.6-rc0~61 (filter-branch: fix variable export logic,
2008-05-13) for an example.
As an aside, how do you make these 61-commits-after-rc0 commit ids. Is
there a sha1->that tool that I haven't spotted?
quoted
but the prose
should mention both IMO
Yes, thanks for putting this portability guide together.
Thanks for improving it, this change is a definite improvement, I just
had a "wait, what's the export doing there then?" question when
reading it.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:09
Ævar Arnfjörð Bjarmason wrote:
As an aside, how do you make these 61-commits-after-rc0 commit ids. Is
there a sha1->that tool that I haven't spotted?
GIT_PAGER_IN_USE=1 git log --all-match --grep=shell --grep=export |
git -p name-rev --tags --stdin
v1.5.6-rc0~61 is 61 commits before rc0 (well, the 61st-generation
grandparent using the first parent where there is a choice). The
distinction matters because “61 commits _after_ 1.5.6-rc0” is not
well-defined.
If you want the latter sort of description anyway, ‘git describe’
might help.
On Wed, Jul 21, 2010 at 01:05, Jonathan Nieder [off-list ref] wrote:
Ævar Arnfjörð Bjarmason wrote:
quoted
As an aside, how do you make these 61-commits-after-rc0 commit ids. Is
there a sha1->that tool that I haven't spotted?
GIT_PAGER_IN_USE=1 git log --all-match --grep=shell --grep=export |
git -p name-rev --tags --stdin
v1.5.6-rc0~61 is 61 commits before rc0 (well, the 61st-generation
grandparent using the first parent where there is a choice). The
distinction matters because “61 commits _after_ 1.5.6-rc0” is not
well-defined.
Thanks, added that as an alias in my .gitconfig:
abbrev-commit = "!f() { git name-rev --tags \"$@\" | sed
's|.*tags/||;s|\\([0-9a-f]\\{7\\}\\).*|\\1|'; }; f"
If you want the latter sort of description anyway, ‘git describe’
might help.
git describe was actually more like what I wanted. I'd used it before,
but forgotten it. Thanks.
No time to investigate, but here is an example patch and the
results of running the affected tests.
You have made cosmetic changes which do not do the same as the original.
Nope, look closer. The changes are not cosmetic.
Try this:
run_it () { "$@"; }; run_it foo= true && echo success || echo failure
You probably get something like this (if you're using bash):
bash: foo=: command not found
That's because the one-shot variable assignment doesn't work when
used like this. It also means that the original tests which do:
test_must_fail PAGER= git ...
are broken. We ran into this problem a while back and fixed it in
the commit that I referenced (e2007832). I fixed the new instances
in t2017, t3200, and t3301 in the patch that I sent.
For the tests in t2017 and t3200 that now fail, the originals seem to
expect 'git reflog show' to return non-zero when asked to show the reflog
for a ref which doesn't have a log. reflog does not currently return
non-zero in this case. Either the tests should be updated to reflect
the actual behavior of 'reflog show', or 'reflog show' should be updated
to return non-zero when passed a ref without a log.
-brandon
@@ -237,7 +237,11 @@ test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdatesgitconfigcore.logAllRefUpdatesfalse&&gitcheckout-bbeta&&!test-f.git/logs/refs/heads/beta&&-test_must_failPAGER=gitreflogshowbeta+(+PAGER=&&+exportPAGER&&+test_must_failgitreflogshowbeta+)'
No time to investigate, but here is an example patch and the
results of running the affected tests.
quoted
You have made cosmetic changes which do not do the same as the original.
?
Nope, look closer. The changes are not cosmetic.
Now I see it. I was not completely aware of the problem, only of your
email. Maybe I should subscribe to the list at last... ;-D
Try this:
run_it () { "$@"; }; run_it foo= true && echo success || echo failure
You probably get something like this (if you're using bash):
bash: foo=: command not found
It would work if it was like:
run_it () { eval "$@"; }; run_it "foo= true" && echo success || echo failure
But I think your approach is better shaped for a fast solution.
That's because the one-shot variable assignment doesn't work when
used like this. It also means that the original tests which do:
test_must_fail PAGER= git ...
are broken. We ran into this problem a while back and fixed it in
the commit that I referenced (e2007832). I fixed the new instances
in t2017, t3200, and t3301 in the patch that I sent.
For the tests in t2017 and t3200 that now fail, the originals seem to
expect 'git reflog show' to return non-zero when asked to show the reflog
for a ref which doesn't have a log. reflog does not currently return
non-zero in this case. Either the tests should be updated to reflect
the actual behavior of 'reflog show', or 'reflog show' should be updated
to return non-zero when passed a ref without a log.
So fixing it is a need.
On t2017 and t3200 the problem that appeared was because no message
and error had been generated when there is no reflog. Git reflog when
run on a ref with a nonexistent reflog file exits with 0 saying
nothing.
I don't see this as a correct behavior but as those tests were just to
enforce the previous "! test -f..." test which is already enough for
checking the intended behavior then I would think it was good enough
just to wipe out the "problematic git reflog" commands until deciding
how quiet git reflog command should be when there is no reflog file to
show.
Although I just realized Junio sent an email following this thread and
I bet he could give a better solution or his directions. Going to
read it now.
Thanks for your clarifications.
Regards