Re: [PATCH] GSoC 2014 Microproject 1 rewrite skip_prefix() as loop
- From
- Faiz Kothari <faiz.off93@gmail.com>
- Date
- Feb 27, 2014, 11:40 UTC
- Message-ID
- <CAFbjVcmbF4MYj1t2v1_dxQ0z4mqMfFChK2HdCFiEqWL6Wo2vGg@mail.gmail.com>
- In-Reply-To
- <530F2238.4090303@alum.mit.edu>
Thanks for the reply, I was unable to get git send-email working. Now its working, I'll resend the patch. I ran all the tests, they are working properly. About the comment, I meant, there is a similar function strbuf.c:starts_with() which does the exact same job, but it returns 0 or 1. I just changed it to return a (const char *) accordingly.
On Thu, Feb 27, 2014 at 5:02 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 65 quoted lines
> On 02/26/2014 05:46 PM, Faiz Kothari wrote:
>> I am Faiz Kothari, I am a GSoC aspirant and want to contribute to git.
>> I am submitting the patch in reponse to Microproject 1,
>> rewrite git-compat-util.h:skip_prefix() as a loop.
>>
>> Signed-off-by: Faiz Kothari <faiz.off93@gmail.com>
>
> The subject of your email plus the part above the "---" line will be
> taken directly to be used as the commit message. So it should not
> include information that is inappropriate for a commit message.
>
> You can put such information directly below the "---" line.
>
> Please also see my comments below.
>
>> ---
>> git-compat-util.h | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index cbd86c3..bb2582a 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char
>> *suffix);
>>
>> static inline const char *skip_prefix(const char *str, const char
>> *prefix)
>
> The line above seems to have been broken by your email program. It is
> important for efficiency reasons that patches be readable directly out
> of emails (e.g., by using "git am"). Please practice by sending the
> patch to yourself different ways until "git am" works on it correctly.
>
>> {
>> - size_t len = strlen(prefix);
>> - return strncmp(str, prefix, len) ? NULL : str + len;
>> + for (; ; str++, prefix++)
>> + if (!*prefix)
>> + return str;//code same as strbuf.c:starts_with()
>
> We don't use "//" for comments, and please space things out the way
> other code does it. But actually, IMO this particular comment doesn't
> really belong permanently in the code. It rather belongs in the commit
> message, or in the discussion (under the "---"), or maybe it should be
> taken as an indication of a deeper problem (see below).
>
>> + else if (*str != *prefix)
>> + return NULL;
>> }
>>
>> #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
>>
>
> The code itself looks correct.
>
> But, considering your comment, would it be appropriate for one of the
> functions to call the other?
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/