Pete Wyckoff wrote:
> jrnieder@xxxxxxxxx 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html