Re: [PATCH 1/1] general style: replaces memcmp() with starts_with()

From: Quint Guvernator <quintus.public@gmail.com>
Date: Mar 17, 2014, 23:46 UTC
Message-ID: <CALs4jVGxmirDdO03kaKco7=NrTaXBinY=1U71=fYwX1mbqNDOA@mail.gmail.com>
In-Reply-To: <xmqqob14a14s.fsf@gitster.dls.corp.google.com>

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

Show 14 quoted lines

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

Show 22 quoted lines

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