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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 4, 2012, 05:32 UTC
Message-ID: <20120404053236.GA2460@burratino>
In-Reply-To: <20120404012037.GB4124@padd.com>

Pete Wyckoff wrote:

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

Show 12 quoted lines

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