2014-03-17 18:52 GMT-04:00 Junio C Hamano <gits...@pobox.com>:
> 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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to