Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
On Wed, Aug 22, 2018 at 2:59 PM SZEDER Gábor wrote: > On Wed, Aug 22, 2018 at 7:53 PM Eric Sunshine wrote: > > Can you say a word or two (here in the email thread) about how you're > > finding these failures (across the various test fixes you've posted > > recently)? Are you instrumenting the code in some fashion? Or, finding > > them by visual inspection? > > Errors from system commands in our tests look like these: > grep: file3: No such file or directory > i.e. lines starting with various system commands' or test scripts' > names, followed by ': '. > > So I've modified t/Makefile to not remove the 'test-results' directory > after a successful 'make test': > And then scanned the results of a '--verbose-log -x' test run with: > grep -E [...] > and then, for lack of something better to do ;), I started looking at > the simpler looking errors. Thanks for the explanation. That makes a lot of sense.
Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
On Wed, Aug 22, 2018 at 11:50:46AM -0700, Junio C Hamano wrote:
> > test_expect_failure 'rev-list should succeed with empty output on empty
> > stdin' '
> > - git rev-list --stdin actual &&
> > + git rev-list --stdin actual &&
> > test_must_be_empty actual
> > '
>
> By the way, it may be about time to turn that expect-failure into
> expect-success. It is somewhat unfortunate that 0c5dc743 ("t6018:
> flesh out empty input/output rev-list tests", 2017-08-02) removed
> the comment that said "we _might_ want to change the behaviour in
> these cases" and explained the tests as reminders, anticipating that
> the series will change the behaviour for three cases where the
> pending list ends up empty to make the discussion moot, but it
> changed the behaviour of only two of them, leaving the "--stdin
> reads empty" case behind.
Yeah, I think I had intended to make --stdin work there, and when I
realized at the end of the series it did not, I failed to go back and
modify that initial commit touching the test.
More discussion in the original thread:
https://public-inbox.org/git/20170802223416.gwiezhbuxbdmb...@sigill.intra.peff.net/
> It may be just the matter of doing something like the attached
> patch. I won't be committing such a behaviour change during the
> pre-release feature freeze, but we may want to consider doing this
> early in the next cycle.
Yes, definitely this is tricky enough to avoid doing during the freeze.
> diff --git a/revision.c b/revision.c
> index d12e6d8a4a..21fb413511 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2441,6 +2441,19 @@ int setup_revisions(int argc, const char **argv,
> struct rev_info *revs, struct s
> object = get_reference(revs, revs->def, &oid, 0);
> add_pending_object_with_mode(revs, object, revs->def, oc.mode);
> }
> + /*
> + * Even if revs->pending is empty after all the above, if we
> + * handled "--stdin", then the caller really meant to give us
> + * an empty commit range. Just let the traversal give an
> + * empty result without causing a "no input? do you know how
> + * to use this command?" failure.
> + *
> + * NOTE!!! Because "--stdin + * default to HEAD, this must come _after_ the above block
> + * that deals with revs->ref fallback.
> + */
> + if (read_from_stdin)
> + revs->rev_input_given = 1;
I think this should do the right thing. All of the issues discussed in
the earlier thread were about using revs->def, and this neatly sidesteps
that by touching the flag afterwards. It's a little funny that the flag
now means two things (earlier in the function it is "we should use the
default" and later it becomes "the caller may complain").
It may be worth splitting it into two flags: rev_input_given and
rev_read_stdin. That puts the responsibility on the caller to check both
flags. But really, rev-list.c is the only caller that checks it anyway.
And it would mean this scary comment can go away. ;)
It also _could_ be useful to other callers to distinguish the two cases
(e.g., if they wanted to know whether they were free to use stdin
themselves). But I don't offhand know of any callers that need that (I
have a vague recollection that it might have come up once over the
years).
-Peff
Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
On Wed, Aug 22, 2018 at 7:53 PM Eric Sunshine wrote: > Can you say a word or two (here in the email thread) about how you're > finding these failures (across the various test fixes you've posted > recently)? Are you instrumenting the code in some fashion? Or, finding > them by visual inspection? Errors from system commands in our tests look like these: grep: file3: No such file or directory sed: -e expression #1, char 2: extra characters after command diff: sub1/.git: No such file or directory tar: rmtlseek not stopped at a record boundary tar: Error is not recoverable: exiting now while errors from the shell running the test like these: t0020-crlf.sh: 8: eval: cannot open two: No such file t6018-rev-list-glob.sh: 4: eval: cannot open expect: No such file t7408-submodule-reference.sh: 615: test: =: unexpected operator i.e. lines starting with various system commands' or test scripts' names, followed by ': '. So I've modified t/Makefile to not remove the 'test-results' directory after a successful 'make test': diff --git a/t/Makefile b/t/Makefile index ea36cf7ac7..c7b1655593 100644 --- a/t/Makefile +++ b/t/Makefile @@ -54,10 +54,11 @@ pre-clean: $(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)' clean-except-prove-cache: - $(RM) -r 'trash directory'.* '$(TEST_RESULTS_DIRECTORY_SQ)' + $(RM) -r 'trash directory'.* $(RM) -r valgrind/bin clean: clean-except-prove-cache + $(RM) '$(TEST_RESULTS_DIRECTORY_SQ)' distclean: clean $(RM) .prove And then scanned the results of a '--verbose-log -x' test run with: grep -E '^(awk|basename|cat|cd|chmod|cmp|cp|cut|diff|dirname|egrep|find|fgrep|grep|gunzip|gzip|ln|mkdir|mkfifo|mktemp|mv|readlink|rmdir|sed|sort|tar|touch|tr|ulimit|umask|uniq|unzip|wc|zipinfo|t[0-9][0-9][0-9][0-9]-[^:]*\.sh): ' test-results/*.out and then, for lack of something better to do ;), I started looking at the simpler looking errors. I've though about how a check like this could be automated, but haven't had any workable idea yet. There are commands that can legitimately print errors, e.g. when checking for a prereq which the system doesn't have (e.g. the 'tar' errors above, I think). And the list of system commands in the grep pattern above is surely incomplete and will likely change in the future...
Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
SZEDER Gábor writes:
> Redirect 'git rev-list's stdin explicitly from /dev/null to provide
> empty input. (Strictly speaking we don't need this redirection,
> because the test script's stdin is already redirected from /dev/null
> anyway, but I think it's better to be explicit about it.)
Yes.
> test_expect_failure 'rev-list should succeed with empty output on empty
> stdin' '
> - git rev-list --stdin actual &&
> + git rev-list --stdin actual &&
> test_must_be_empty actual
> '
By the way, it may be about time to turn that expect-failure into
expect-success. It is somewhat unfortunate that 0c5dc743 ("t6018:
flesh out empty input/output rev-list tests", 2017-08-02) removed
the comment that said "we _might_ want to change the behaviour in
these cases" and explained the tests as reminders, anticipating that
the series will change the behaviour for three cases where the
pending list ends up empty to make the discussion moot, but it
changed the behaviour of only two of them, leaving the "--stdin
reads empty" case behind.
It may be just the matter of doing something like the attached
patch. I won't be committing such a behaviour change during the
pre-release feature freeze, but we may want to consider doing this
early in the next cycle.
revision.c | 13 +
t/t6018-rev-list-glob.sh | 4 ++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index d12e6d8a4a..21fb413511 100644
--- a/revision.c
+++ b/revision.c
@@ -2441,6 +2441,19 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
object = get_reference(revs, revs->def, &oid, 0);
add_pending_object_with_mode(revs, object, revs->def, oc.mode);
}
+ /*
+* Even if revs->pending is empty after all the above, if we
+* handled "--stdin", then the caller really meant to give us
+* an empty commit range. Just let the traversal give an
+* empty result without causing a "no input? do you know how
+* to use this command?" failure.
+*
+* NOTE!!! Because "--stdin ref fallback.
+*/
+ if (read_from_stdin)
+ revs->rev_input_given = 1;
/* Did the user ask for any diff output? Run the diff! */
if (revs->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT)
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 02936c2f24..db8a7834d8 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -255,8 +255,8 @@ test_expect_success 'rev-list accumulates multiple
--exclude' '
compare rev-list "--exclude=refs/remotes/* --exclude=refs/tags/* --all"
--branches
'
-test_expect_failure 'rev-list should succeed with empty output on empty stdin'
'
+test_expect_success 'rev-list should succeed with empty output on empty stdin'
'
git rev-list --stdin actual &&
test_must_be_empty actual
'
Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
SZEDER Gábor writes: > Prior to d3c6751b18 (tests: make use of the test_must_be_empty > function, 2018-07-27), in the test 'rev-list should succeed with empty > output on empty stdin' in 't6018-rev-list-glob' the empty 'expect' > file served dual purpose: besides specifying the expected output, as > usual, it also served as empty input for 'git rev-list --stdin'. Thanks for a correction to a breakage on 'master' that was introduced during this cycle.
Re: [PATCH] t6018-rev-list-glob: fix 'empty stdin' test
On Wed, Aug 22, 2018 at 1:48 PM SZEDER Gábor wrote: > Prior to d3c6751b18 (tests: make use of the test_must_be_empty > function, 2018-07-27), in the test 'rev-list should succeed with empty > output on empty stdin' in 't6018-rev-list-glob' the empty 'expect' > file served dual purpose: besides specifying the expected output, as > usual, it also served as empty input for 'git rev-list --stdin'. > > Then d3c6751b18 came along, and, as part of the conversion to > 'test_must_be_empty', removed this empty 'expect' file, not realizing > its secondary purpose. Redirecting stdin from the now non-existing > file failed the test, but since this test expects failure in the first > place, this issue went unnoticed. Can you say a word or two (here in the email thread) about how you're finding these failures (across the various test fixes you've posted recently)? Are you instrumenting the code in some fashion? Or, finding them by visual inspection? Thanks.
