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 feedback. Quint -- 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