Re: [PATCHv3] fast-import: tighten parsing of mark references

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 5, 2012, 02:24 UTC
Message-ID: <20120405022422.GB20687@burratino>
In-Reply-To: <20120405015121.GA10945@padd.com>

Pete Wyckoff wrote:

> This addresses all of Jonathan's comments, in particular:

Nice.  Thanks much.  I only have a few small worries left:

[...]

> +++ b/t/t9300-fast-import.sh
> @@ -2635,4 +2635,280 @@ test_expect_success \

[...]

> +test_expect_success 'S: filemodify with garbage after sha1 must fail' '
> +	sha1=$(grep -w :103 marks | cut -d\  -f2) &&

"grep -w" isn't used elsewhere in the testsuite.  Is it portable?

[...]

Show 17 quoted lines

> +# inline is misspelled; fast-import thinks it is some unknown dataref
> +# and complains "Invalid SHA1"
> +test_expect_success 'S: notemodify with garbage after inline dataref must fail' '
> +	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 S note dataref inline
> +	COMMIT
> +	N inlineX :2
> +	data <<BLOB
> +	note blob
> +	BLOB
> +	EOF
> +	cat err &&
> +	test_i18ngrep "nvalid SHA1" err
> +'

If I understood the discussion before correctly, this error message is suboptimal and something like "invalid dataref" would be a little clearer, right?

That's orthogonal to what this patch is about so I'm not suggesting changing it. But shouldn't the test just check that fast-import fails without committing to any particular message?

Cheers, Jonathan