larsxschneider@xxxxxxxxx writes:
> From: Lars Schneider <larsxschneider@xxxxxxxxx>
>
> Use `test_config` to set the config, check that files are empty with
> `test_must_be_empty`, compare files with `test_cmp`, and remove spaces
> after ">" and "<".
All of the above are good things to do, but the first one needs to
be done a bit carefully.
It is unclear in the above description if you made sure that no
subsequent test depends on the settings left by earlier test before
replacing "git config" with "test_config".
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 7bac2bc..7b45136 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -13,8 +13,8 @@ EOF
> chmod +x rot13.sh
>
> test_expect_success setup '
> - git config filter.rot13.smudge ./rot13.sh &&
> - git config filter.rot13.clean ./rot13.sh &&
> + test_config filter.rot13.smudge ./rot13.sh &&
> + test_config filter.rot13.clean ./rot13.sh &&
>
> {
> echo "*.t filter=rot13"
For example, after this conversion, filter.rot13.* will be reverted
back to unconfigured once setup is done.
> test_expect_success check '
>
> - cmp test.o test &&
> - cmp test.o test.t &&
> + test_cmp test.o test &&
> + test_cmp test.o test.t &&
>
> # ident should be stripped in the repository
> git diff --raw --exit-code :test :test.i &&
It happens to be true that this subsequent test does not do any
operation to cause data come from and go into the object database
for any path that match the pattern "*.t", because for whatever
reason the previous "setup" step happens to do more than just
"setup". It already exercised the filtering by doing "git add" and
"git checkout".
If we were writing the script t0021 from scratch today, we would
have used test_config *AND* squashed there two tests into one
(i.e. making it a single "the filter and ident operation" test).
Then it is crystal clear that additional tests on commands that may
involve filtering will always be added to that combined test
(e.g. we may try "cat-file --filters" to ensure that rot13 filter is
excercised).
But because we are not doing that, it may become tempting to add
test for a new command that pays attention to a filter to either of
the test, and it would have worked OK if this patch weren't there.
Such an addition will break the test, because in the second "check"
test, the filter commands are no longer active with this patch.
So while I do appreciate the change for the longer term, I am not
quite happy with it. It just looks like an invitation for future
mistakes.
> @@ -130,7 +130,7 @@ test_expect_success 'filter shell-escaped filenames' '
>
> # delete the files and check them out again, using a smudge filter
> # that will count the args and echo the command-line back to us
> - git config filter.argc.smudge "sh ./argc.sh %f" &&
> + test_config filter.argc.smudge "sh ./argc.sh %f" &&
> rm "$normal" "$special" &&
> git checkout -- "$normal" "$special" &&
This one is clearly OK. Anything related to argc filter only
appears in this single test so it is not just OK to use test_config,
but it makes our intention very clear that nobody else would use the
argc filter. I think similar reasoning would apply to the test_config
conversion in the remainder of the script.
As to other types of changes you did in this, I see no problem with
them.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html