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

2014-03-17 Thread Michael Haggerty 
Quint,

Thanks for v3 of the patch and for sticking with it.  See a few comments
below.

On 03/15/2014 05:39 PM, Quint Guvernator wrote:
 memcmp() is replaced with negated starts_with() when comparing strings
 from the beginning and when it is logical to do so. starts_with() looks
 nicer and it saves the extra argument of the length of the comparing
 string.
 
 Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
 ---
  builtin/apply.c   |  6 +++---
  builtin/for-each-ref.c|  2 +-
  builtin/get-tar-commit-id.c   |  2 +-
  builtin/mktag.c   |  8 
  builtin/patch-id.c| 18 +-
  commit.c  |  4 ++--
  connect.c |  6 +++---
  contrib/convert-objects/convert-objects.c |  6 +++---
  convert.c |  2 +-
  http-walker.c |  2 +-
  imap-send.c   |  6 +++---
  pack-write.c  |  2 +-
  remote.c  |  2 +-
  13 files changed, 33 insertions(+), 33 deletions(-)
 
 diff --git a/builtin/apply.c b/builtin/apply.c
 index 0189523..de84dce 100644
 --- a/builtin/apply.c
 +++ b/builtin/apply.c
 @@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned 
 long size,
* l10n of \ No newline... is at least that long.
*/
   case '\\':
 - if (len  12 || memcmp(line, \\ , 2))
 + if (len  12 || !starts_with(line, \\ ))
   return -1;
   break;
   }
 @@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned 
 long size,
* it in the above loop because we hit oldlines == newlines == 0
* before seeing it.
*/
 - if (12  size  !memcmp(line, \\ , 2))
 + if (12  size  starts_with(line, \\ ))
   offset += linelen(line, size);
  
   patch-lines_added += added;
 @@ -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, @@ -)) {
   struct fragment *fragment;
   int len;
  
 diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
 index 3e1d5c3..4135980 100644
 --- a/builtin/for-each-ref.c
 +++ b/builtin/for-each-ref.c
 @@ -193,7 +193,7 @@ static int verify_format(const char *format)
   at = parse_atom(sp + 2, ep);
   cp = ep + 1;
  
 - if (!memcmp(used_atom[at], color:, 6))
 + if (starts_with(used_atom[at], color:))
   need_color_reset_at_eol = !!strcmp(used_atom[at], 
 color_reset);
   }
   return 0;
 diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
 index aa72596..6409c26 100644
 --- a/builtin/get-tar-commit-id.c
 +++ b/builtin/get-tar-commit-id.c
 @@ -29,7 +29,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, 
 const char *prefix)
   die(git get-tar-commit-id: read error);
   if (header-typeflag[0] != 'g')
   return 1;
 - if (memcmp(content, 52 comment=, 11))
 + if (!starts_with(content, 52 comment=))
   return 1;
  
   n = write_in_full(1, content + 11, 41);

This hunk uses the magic number 11 a couple lines later.  Junio (I
think rightly) objected [1] to rewrites in these circumstances because
they make it even less obvious where the constant came from and thereby
make the complete elimination of the hard-coded constant *more* difficult.

 diff --git a/builtin/mktag.c b/builtin/mktag.c
 index 640ab64..70385ac 100644
 --- a/builtin/mktag.c
 +++ b/builtin/mktag.c
 @@ -49,7 +49,7 @@ static int verify_tag(char *buffer, unsigned long size)
  
   /* Verify object line */
   object = buffer;
 - if (memcmp(object, object , 7))
 + if (!starts_with(object, object ))
   return error(char%d: does not start with \object \, 0);
  
   if (get_sha1_hex(object + 7, sha1))

Ditto.

 @@ -57,7 +57,7 @@ static int verify_tag(char *buffer, unsigned long size)
  
   /* Verify type line */
   type_line = object + 48;
 - if (memcmp(type_line - 1, \ntype , 6))
 + if (!starts_with(type_line - 1, \ntype ))
   return error(char%d: could not find \\\ntype \, 47);
  
   /* Verify tag-line */
 @@ -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 ) ||

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

2014-03-17 Thread Quint Guvernator 
2014-03-17 12:29 GMT-04:00 Michael Haggerty mhag...@alum.mit.edu:
 This hunk uses the magic number 11 a couple lines later.  Junio (I
 think rightly) objected [1] to rewrites in these circumstances because
 they make it even less obvious where the constant came from and thereby
 make the complete elimination of the hard-coded constant *more* difficult.

 [1] http://article.gmane.org/gmane.comp.version-control.git/244005

Sure, I can understand that. I'll look through the hunks again with
more context in the diff to try to look for more cases where magic
numbers are used more than once. One of the goals of this revision is
to minimize that, see the commit message.

 In any open-source project it is important to optimize for the time of
 the reviewer, because code-review is a relatively tedious task and is
 therefore often the bottleneck for progress.  Therefore I suggest that
 you turn your approach on its head.  Don't remove the most
 objectionable hunks but rather *include only the best hunks*--the ones
 that you can stand behind 100%, which you think are an unambiguous
 improvement, and that the reviewer can accept without reservations.
...
 It would be much better for you to submit only a handful of changes (or
 even only one!) that is perfect, rather than throwing a bunch at the
 wall and asking the reviewer to pick between the good and the bad.  As
 you gain experience and learn about the preferences of the Git project,
 you will get a better feel for the boundary between
 acceptable/unacceptable patches, and *then* you will be able to start
 submitting patches closer to the boundary.

I see. For v4, I will be more discerning as to what I include. I will
try to keep the scope of future patches down and err on the side of
caution when I review my own changes before submitting. Thank you for
the *pointers.

I'm going to give v4 a shot. It should be on the mailing list in an hour or so.

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

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

2014-03-15 Thread Quint Guvernator 
memcmp() is replaced with negated starts_with() when comparing strings
from the beginning and when it is logical to do so. starts_with() looks
nicer and it saves the extra argument of the length of the comparing
string.

Signed-off-by: Quint Guvernator quintus.pub...@gmail.com
---
 builtin/apply.c   |  6 +++---
 builtin/for-each-ref.c|  2 +-
 builtin/get-tar-commit-id.c   |  2 +-
 builtin/mktag.c   |  8 
 builtin/patch-id.c| 18 +-
 commit.c  |  4 ++--
 connect.c |  6 +++---
 contrib/convert-objects/convert-objects.c |  6 +++---
 convert.c |  2 +-
 http-walker.c |  2 +-
 imap-send.c   |  6 +++---
 pack-write.c  |  2 +-
 remote.c  |  2 +-
 13 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 0189523..de84dce 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -1631,7 +1631,7 @@ static int parse_fragment(const char *line, unsigned long 
size,
 * l10n of \ No newline... is at least that long.
 */
case '\\':
-   if (len  12 || memcmp(line, \\ , 2))
+   if (len  12 || !starts_with(line, \\ ))
return -1;
break;
}
@@ -1646,7 +1646,7 @@ static int parse_fragment(const char *line, unsigned long 
size,
 * it in the above loop because we hit oldlines == newlines == 0
 * before seeing it.
 */
-   if (12  size  !memcmp(line, \\ , 2))
+   if (12  size  starts_with(line, \\ ))
offset += linelen(line, size);
 
patch-lines_added += added;
@@ -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, @@ -)) {
struct fragment *fragment;
int len;
 
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 3e1d5c3..4135980 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -193,7 +193,7 @@ static int verify_format(const char *format)
at = parse_atom(sp + 2, ep);
cp = ep + 1;
 
-   if (!memcmp(used_atom[at], color:, 6))
+   if (starts_with(used_atom[at], color:))
need_color_reset_at_eol = !!strcmp(used_atom[at], 
color_reset);
}
return 0;
diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index aa72596..6409c26 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -29,7 +29,7 @@ int cmd_get_tar_commit_id(int argc, const char **argv, const 
char *prefix)
die(git get-tar-commit-id: read error);
if (header-typeflag[0] != 'g')
return 1;
-   if (memcmp(content, 52 comment=, 11))
+   if (!starts_with(content, 52 comment=))
return 1;
 
n = write_in_full(1, content + 11, 41);
diff --git a/builtin/mktag.c b/builtin/mktag.c
index 640ab64..70385ac 100644
--- a/builtin/mktag.c
+++ b/builtin/mktag.c
@@ -49,7 +49,7 @@ static int verify_tag(char *buffer, unsigned long size)
 
/* Verify object line */
object = buffer;
-   if (memcmp(object, object , 7))
+   if (!starts_with(object, object ))
return error(char%d: does not start with \object \, 0);
 
if (get_sha1_hex(object + 7, sha1))
@@ -57,7 +57,7 @@ static int verify_tag(char *buffer, unsigned long size)
 
/* Verify type line */
type_line = object + 48;
-   if (memcmp(type_line - 1, \ntype , 6))
+   if (!starts_with(type_line - 1, \ntype ))
return error(char%d: could not find \\\ntype \, 47);
 
/* Verify tag-line */
@@ -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));
 
@@ -98,7 +98,7 @@ static int verify_tag(char *buffer, unsigned long size)
/* Verify the tagger line */
tagger_line = tag_line;
 
-   if (memcmp(tagger_line, tagger , 7))
+   if (!starts_with(tagger_line, tagger ))
return error(char%PRIuMAX: could not find