Re: [PATCHv2 1/2] fast-import: test behavior of garbage after mark references
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Apr 3, 2012, 14:00 UTC
- Message-ID
- <20120403140055.GC15589@burratino>
- In-Reply-To
- <1333417910-17955-2-git-send-email-pw@padd.com>
Pete Wyckoff wrote:
Show 13 quoted lines
> --- 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...
Show 5 quoted lines
> + > +# > +# 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.
Show 10 quoted lines
> + 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?
Show 16 quoted lines
> +' > + > +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