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

[Pete Wyckoff <pw@padd.com>]

jrnieder@gmail.com wrote on Tue, 03 Apr 2012 09:00 -0500:
> Pete Wyckoff wrote:
> > +
> > +#
> > +# 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.

I've fixed these locally, thanks for reading them!

> > +	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?

I'm a bit confused.  I read through 5e9637c (i18n: add
infrastructure for translating Git with gettext, 2011-11-18), and
GETTEXT_POISON seems to be used to find untranslated messages.
What I want to test here is that the functionality works: do the
right untranslated messages get printed.

Changing the "Missing" to "missing" would require fixing the
tests, and that seems okay.

I'm happy to drop the "-q" and drop the "Missing", but wonder if
you're looking for something deeper.

> > +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.

The form of the message matters.  Datarefs can be inline, SHA1s,
or marks.  It is confusing to see an error message about a SHA1
when the input stream has no SHA1s.  The existing code always
says "SHA1", which is wrong if you gave it a mark or an inline.

> 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.

You think I should squash it all together, then?  Or factor the
tests into two chunks:
    - tests for behavior that silently accepts broken input; and,
    - tests for behavior where the bogus input is detcetd, but
      incorrect error messages are given?

		-- Pete