2014-03-17 18:52 GMT-04:00 Junio C Hamano <[email protected]>:
> Thanks. This probably needs retitled, though (hint: "replaces"?
> who does so?) and the message rewritten (see numerous reviews on
> other GSoC micros from Eric).
I found some messages [1] by Eric concerning imperative voice ("simplify"
rather than "simplifies/ed").
Other than the change of verb, what sort of changes are you looking for in
the description? It doesn't look much different than, for instance, this
[2] commit in the log.
[1]: http://article.gmane.org/gmane.comp.version-control.git/243848
[2]: https://github.com/git/git/commit/0eea5a6e91d3da6932c13f16fdf4b4e5ed91b93c
> I sense that there is a bonus point for an independent follow-up
> patch to unify the conflicting definitions of what an incomplete
> line should look like. Hint, hint...
I'll try to make the time to follow up on that, if I can think of a good
clear solution for the conflict. I'm also a full-time student, but I will
certainly give it a shot.
>> @@ -1673,7 +1673,7 @@ static int parse_single_patch(const char *line,
>> unsigned long size, struct patch
>> unsigned long oldlines = 0, newlines = 0, context = 0;
>> struct fragment **fragp = &patch->fragments;
>>
>> - while (size > 4 && !memcmp(line, "@@ -", 4)) {
>> + while (size > 4 && starts_with(line, "@@ -")) {
>
> If there were a variant of starts_with() that works on a counted
> string, and rewriting this using it to
>
> while (starts_with_counted(line, size, "@@ -")) {
>
> would make perfect sense, but as written above, I do not think it is
> an improvement.
This still feels to me like an improvement from the !memcmp line, but if
you think we need to wait for a full helper-function revamp, let's drop it.
>> @@ -66,7 +66,7 @@ static int verify_tag(char *buffer, unsigned long size)
>> return error("char%"PRIuMAX": could not find next \"\\n\"",
>> (uintmax_t) (type_line - buffer));
>> tag_line++;
>> - if (memcmp(tag_line, "tag ", 4) || tag_line[4] == '\n')
>> + if (!starts_with(tag_line, "tag ") || tag_line[4] == '\n')
>> return error("char%"PRIuMAX": no \"tag \" found",
>> (uintmax_t) (tag_line - buffer));
>
> Not quite. I suspect that what actually makes this strange and
> tricky is that this "no tag found" check is misplaced. It found the
> type line, expects that the next line is a tag line, and instead of
> validating the remainder of type line, it does this thing, and then
> verifies the actual type string, and for that, it needs tag_line
> variable to stay where it is.
>
> If we flipped the order of things around the codepath a bit, then we
> should be able to first validate the type line, and then use
> skip-prefix to skip the "tag " part (while validating that that line
> actually begins with "tag ") and check the tag name is a non-empty
> string that consists of a good character. All of that is a topic
> for a separate patch.
That's tricky. Okay, let's definitely drop this hunk.
Shall I submit a new [PATCH v5] with these changes to the mailing list or
directly to you, or is everything in order?
Thanks for taking the time to review this. I really appreciate the
feedback.
Quint
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
