Re: [PATCHv2 2/2] fast-import: tighten parsing of mark references

[Jonathan Nieder <jrnieder@gmail.com>]

Pete Wyckoff wrote:
> jrnieder@gmail.com wrote on Tue, 03 Apr 2012 09:20 -0500:

>> Simpler:
>> 
>> 	if (*p == ':') {
>> 		oe = find_mark(parse_mark_ref_space(p, &p));
>> 		hashcpy(sha1, oe->idx.sha1);
>> 	} else if ...
>
> Yes.  I thought about just passing in plain old &p.  Even though
> these approaches would work, it is a bit more difficult for
> novice C coders to read.  Figured we should err on the side of
> helping future code readers.  I can add more cleverness if you
> feel strongly.

It would be clearest with one argument, like so:

		oe = find_mark(parse_mark_...(&p));
		hashcpy(sha1, oe->idx.sha1);

[...]
> Insead of "Missing space after 'inline'", you'll get "Invalid
> SHA1".  You misspelled "inline" with "inliness"?  And would
> prefer to be told you provided an invalid SHA1?

It wasn't a great example, but what I meant is that if someone
asked me, a human, to parse

	M 100644 foobar path/to/file

I would assume that foobar is a <dataref>.  Likewise, for any
string baz in

	M 100644 baz path/to/file

including strings that start with "inline", except for "inline"
itself.

To put it another way: checking for 'inline' at the start of a word as
a way to check for typos seems odd to me.  We do not diagnose

	M 100644 Inline path/to/file

as a misspelled version of "inline", nor

	M 100644inline path/to/file

as an instance of a missing space character, and we shouldn't.

The goal in fast-import's behavior is usually predictability and
simplicity in terms of the mental model of the person writing a
frontend.  Trying to guess the user's intention on malformed input
only takes away from that goal.

Why I care: if some day git permits other kinds of <dataref> (for
example if it supports refnames some day), I do not want datarefs
beginning with "inline" to be forbidden.

[...]
> There are two cases it handles:  mark and sha1.  The mark case
> uses the handy new parse_mark_ref_space(), which does the space
> checking.  The sha1 branch had no check in this function.  So
> I hoisted the space check up to make the branches symmetrical.

I think it's ok to sacrifice symmetry here, but:

[...]
> I would prefer just to inline the whole thing.  Or new name
> parse_ls_dataref() if you have a preference.

if changing the behavior of the function that parses a treeish dataref
seems right, that's fine with me as long as its name or signature
changes.

For example, it could become

	static struct object_entry *parse_treeish(const char **p);

Hope that helps,
Jonathan