Re: [PATCHv2 1/2] fast-import: test behavior of garbage after mark references

[Jonathan Nieder <jrnieder@gmail.com>]

Pete Wyckoff wrote:

> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -2635,4 +2635,271 @@ test_expect_success \
>  	'n=$(grep $a verify | wc -l) &&
>  	 test 1 = $n'
>  
> +###
> +### series S
> +###
> +#
> +# Set up is roughly this.  Commits marked 1,2,3,4.  Blobs
> +# marked 100 + commit.  Notes 200 +.  Make sure missing spaces
> +# and EOLs after mark references cause errors.

Nit: "Set up" should be "Setup" when it is a noun.

[...]
> +test_expect_success 'S: add commits 1 and 2, and blob 103' '
> +	git fast-import --export-marks=marks <input
> +'

Ok, this one sets up for later ones...

> +
> +#
> +# filemodify, three datarefs
> +#
> +test_expect_failure 'S: filemodify markref no space' '

What is this testing for?  The ideal is that each test_expect_foo line
contains a proposition and the test checks whether that proposition is
true or false.  For example:

	test_expect_failure 'S: filemodify with garbage after mark errors out' '

Likewise in later tests.

> +	test_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&
> +	commit refs/heads/S
> +	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +	data <<COMMIT
> +	commit N
> +	COMMIT
> +	M 100644 :103x hello.c
> +	EOF
> +	cat err &&
> +	grep -q "Missing space after mark" err

Is this using "grep -q" to avoid repeating the same line in the output
twice?  It seems better to use plain grep or test_i18ngrep.

I'm also worried that if someone wants to change these messages
(perhaps to make the 'm' in "Missing" lowercase or something), they
will have to change all of these tests.  If we want to be absolutely
sure that git detects the right error instead of something else, I
would suggest

	test_i18ngrep "space after mark" message

I'm also not convinced the error message is worth checking at all ---
as long as fast-import errors out, won't the frontend author be able
to look in the logs to find out the problematic line anyway?

> +'
> +
> +test_expect_failure 'S: filemodify inline no space' '
> +	test_must_fail git fast-import --import-marks=marks <<-EOF 2>err &&
> +	commit refs/heads/S
> +	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +	data <<COMMIT
> +	commit N
> +	COMMIT
> +	M 100644 inlineX hello.c
> +	data <<BLOB
> +	inline
> +	BLOB
> +	EOF
> +	cat err &&
> +	grep -q "Missing space after .inline." err

Does this fail because the error message is "Missing space after SHA1"
instead?  I'm not sure that's actually a bug, unless we want to
correctly nitpick that the keyword "inline" that is a stand-in for an
object name is not itself one.

I don't think the tests for exact error messages make too much sense
without the next patch, so I would suggest leaving them out if this
patch is supposed to be applicable on its own.

Thanks for some thorough tests.

Hope that helps,
Jonathan