Re: [PATCH] GSoC 2014 Microproject 1 rewrite skip_prefix() as loop

2014-02-27 Thread Michael Haggerty
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.of...@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
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
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


Re: [PATCH] GSoC 2014 Microproject 1 rewrite skip_prefix() as loop

2014-02-27 Thread Faiz Kothari
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 mhag...@alum.mit.edu wrote:
 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.of...@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
 mhag...@alum.mit.edu
 http://softwareswirl.blogspot.com/
--
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