Re: [PATCH 10/16] fast-import: use skip_prefix for parsing input

2014-06-20 Thread Eric Sunshine 
On Fri, Jun 20, 2014 at 1:45 AM, Jeff King p...@peff.net wrote:
 On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote:

  -   if (starts_with(command_buf.buf, M ))
  -   file_change_m(b);
  -   else if (starts_with(command_buf.buf, D ))
  -   file_change_d(b);
  -   else if (starts_with(command_buf.buf, R ))
  -   file_change_cr(b, 1);
  -   else if (starts_with(command_buf.buf, C ))
  -   file_change_cr(b, 0);
  -   else if (starts_with(command_buf.buf, N ))
  -   note_change_n(b, prev_fanout);
  +   const char *v;

 This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in
 the function.

 Thanks.  I reordered the patches before sending, so when this one was
 originally written, there was no v at the top-level of the function.
 I think we can just drop this interior one. The point of the short v
 is that it can be used as a temporary value for prefix matches, so I
 think we can just reuse the same one.

Agreed. The intended usage of 'v' is clear enough and the code simple
enough that confusion is unlikely.

 I tried compiling with -Wshadow (which I don't usually do), but we're
 not even close to compiling clean there. Some of them are legitimately
 confusing (e.g., try figuring out end in parse_rev_note). But others
 look just annoying (e.g., complaining that a local usage conflicts
 with the global function). I don't know if we want to put effort into
 being -Wshadow clean or not.

I just happened to notice the shadowing declaration while reading the
patch, but don't feel strongly about existing cases. It makes sense to
clean up confusing cases, such 'end' in parse_rev_note(), when working
on that code (just as with style cleanups), but thus far nobody has
been complaining about existing shadowed variables, so global cleanup
would likely be considered churn.
--
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 10/16] fast-import: use skip_prefix for parsing input

2014-06-19 Thread Eric Sunshine 
On Wed, Jun 18, 2014 at 3:49 PM, Jeff King p...@peff.net wrote:
 Fast-import does a lot of parsing of commands and
 dispatching to sub-functions. For example, given option
 foo, we might recognize option  using starts_with, and
 then hand it off to parse_option() to do the rest.

 However, we do not let parse_option know that we have parsed
 the first part already. It gets the full buffer, and has to
 skip past the uninteresting bits. Some functions simply add
 a magic constant:

   char *option = command_buf.buf + 7;

 Others use strlen:

   char *option = command_buf.buf + strlen(option );

 And others use strchr:

   char *option = strchr(command_buf.buf, ' ') + 1;

 All of these are brittle and easy to get wrong (especially
 given that the starts_with call and the code that assumes
 the presence of the prefix are far apart). Instead, we can
 use skip_prefix, and just pass each handler a pointer to its
 arguments.

 Signed-off-by: Jeff King p...@peff.net
 ---
 diff --git a/fast-import.c b/fast-import.c
 index a3ffe30..5f17adb 100644
 --- a/fast-import.c
 +++ b/fast-import.c
 @@ -2713,20 +2706,22 @@ static void parse_new_commit(void)

 /* file_change* */
 while (command_buf.len  0) {
 -   if (starts_with(command_buf.buf, M ))
 -   file_change_m(b);
 -   else if (starts_with(command_buf.buf, D ))
 -   file_change_d(b);
 -   else if (starts_with(command_buf.buf, R ))
 -   file_change_cr(b, 1);
 -   else if (starts_with(command_buf.buf, C ))
 -   file_change_cr(b, 0);
 -   else if (starts_with(command_buf.buf, N ))
 -   note_change_n(b, prev_fanout);
 +   const char *v;

This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in
the function.

 +   if (skip_prefix(command_buf.buf, M , v))
 +   file_change_m(v, b);
 +   else if (skip_prefix(command_buf.buf, D , v))
 +   file_change_d(v, b);
 +   else if (skip_prefix(command_buf.buf, R , v))
 +   file_change_cr(v, b, 1);
 +   else if (skip_prefix(command_buf.buf, C , v))
 +   file_change_cr(v, b, 0);
 +   else if (skip_prefix(command_buf.buf, N , v))
 +   note_change_n(v, b, prev_fanout);
 else if (!strcmp(deleteall, command_buf.buf))
 file_change_deleteall(b);
 -   else if (starts_with(command_buf.buf, ls ))
 -   parse_ls(b);
 +   else if (skip_prefix(command_buf.buf, ls , v))
 +   parse_ls(v, b);
 else {
 unread_command_buf = 1;
 break;
--
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 10/16] fast-import: use skip_prefix for parsing input

2014-06-19 Thread Jeff King 
On Thu, Jun 19, 2014 at 11:19:09PM -0400, Eric Sunshine wrote:

  -   if (starts_with(command_buf.buf, M ))
  -   file_change_m(b);
  -   else if (starts_with(command_buf.buf, D ))
  -   file_change_d(b);
  -   else if (starts_with(command_buf.buf, R ))
  -   file_change_cr(b, 1);
  -   else if (starts_with(command_buf.buf, C ))
  -   file_change_cr(b, 0);
  -   else if (starts_with(command_buf.buf, N ))
  -   note_change_n(b, prev_fanout);
  +   const char *v;
 
 This declaration of 'v' shadows the 'v' added by patch 8/16 earlier in
 the function.

Thanks.  I reordered the patches before sending, so when this one was
originally written, there was no v at the top-level of the function.
I think we can just drop this interior one. The point of the short v
is that it can be used as a temporary value for prefix matches, so I
think we can just reuse the same one.

I tried compiling with -Wshadow (which I don't usually do), but we're
not even close to compiling clean there. Some of them are legitimately
confusing (e.g., try figuring out end in parse_rev_note). But others
look just annoying (e.g., complaining that a local usage conflicts
with the global function). I don't know if we want to put effort into
being -Wshadow clean or not.

-Peff
--
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 10/16] fast-import: use skip_prefix for parsing input

2014-06-18 Thread Jeff King 
Fast-import does a lot of parsing of commands and
dispatching to sub-functions. For example, given option
foo, we might recognize option  using starts_with, and
then hand it off to parse_option() to do the rest.

However, we do not let parse_option know that we have parsed
the first part already. It gets the full buffer, and has to
skip past the uninteresting bits. Some functions simply add
a magic constant:

  char *option = command_buf.buf + 7;

Others use strlen:

  char *option = command_buf.buf + strlen(option );

And others use strchr:

  char *option = strchr(command_buf.buf, ' ') + 1;

All of these are brittle and easy to get wrong (especially
given that the starts_with call and the code that assumes
the presence of the prefix are far apart). Instead, we can
use skip_prefix, and just pass each handler a pointer to its
arguments.

Signed-off-by: Jeff King p...@peff.net
---
 fast-import.c | 125 +-
 1 file changed, 53 insertions(+), 72 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index a3ffe30..5f17adb 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -371,8 +371,8 @@ static volatile sig_atomic_t checkpoint_requested;
 static int cat_blob_fd = STDOUT_FILENO;
 
 static void parse_argv(void);
-static void parse_cat_blob(void);
-static void parse_ls(struct branch *b);
+static void parse_cat_blob(const char *p);
+static void parse_ls(const char *p, struct branch *b);
 
 static void write_branch_report(FILE *rpt, struct branch *b)
 {
@@ -1861,6 +1861,8 @@ static int read_next_command(void)
}
 
for (;;) {
+   const char *p;
+
if (unread_command_buf) {
unread_command_buf = 0;
} else {
@@ -1893,8 +1895,8 @@ static int read_next_command(void)
rc-prev-next = rc;
cmd_tail = rc;
}
-   if (starts_with(command_buf.buf, cat-blob )) {
-   parse_cat_blob();
+   if (skip_prefix(command_buf.buf, cat-blob , p)) {
+   parse_cat_blob(p);
continue;
}
if (command_buf.buf[0] == '#')
@@ -2273,9 +2275,8 @@ static uintmax_t parse_mark_ref_space(const char **p)
return mark;
 }
 
-static void file_change_m(struct branch *b)
+static void file_change_m(const char *p, struct branch *b)
 {
-   const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
const char *endp;
struct object_entry *oe;
@@ -2376,9 +2377,8 @@ static void file_change_m(struct branch *b)
tree_content_set(b-branch_tree, p, sha1, mode, NULL);
 }
 
-static void file_change_d(struct branch *b)
+static void file_change_d(const char *p, struct branch *b)
 {
-   const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
const char *endp;
 
@@ -2391,15 +2391,14 @@ static void file_change_d(struct branch *b)
tree_content_remove(b-branch_tree, p, NULL, 1);
 }
 
-static void file_change_cr(struct branch *b, int rename)
+static void file_change_cr(const char *s, struct branch *b, int rename)
 {
-   const char *s, *d;
+   const char *d;
static struct strbuf s_uq = STRBUF_INIT;
static struct strbuf d_uq = STRBUF_INIT;
const char *endp;
struct tree_entry leaf;
 
-   s = command_buf.buf + 2;
strbuf_reset(s_uq);
if (!unquote_c_style(s_uq, s, endp)) {
if (*endp != ' ')
@@ -2444,9 +2443,8 @@ static void file_change_cr(struct branch *b, int rename)
leaf.tree);
 }
 
-static void note_change_n(struct branch *b, unsigned char *old_fanout)
+static void note_change_n(const char *p, struct branch *b, unsigned char 
*old_fanout)
 {
-   const char *p = command_buf.buf + 2;
static struct strbuf uq = STRBUF_INIT;
struct object_entry *oe;
struct branch *s;
@@ -2587,7 +2585,7 @@ static int parse_from(struct branch *b)
const char *from;
struct branch *s;
 
-   if (!starts_with(command_buf.buf, from ))
+   if (!skip_prefix(command_buf.buf, from , from))
return 0;
 
if (b-branch_tree.tree) {
@@ -2595,7 +2593,6 @@ static int parse_from(struct branch *b)
b-branch_tree.tree = NULL;
}
 
-   from = strchr(command_buf.buf, ' ') + 1;
s = lookup_branch(from);
if (b == s)
die(Can't create a branch from itself: %s, b-name);
@@ -2636,8 +2633,7 @@ static struct hash_list *parse_merge(unsigned int *count)
struct branch *s;
 
*count = 0;
-   while (starts_with(command_buf.buf, merge )) {
-   from = strchr(command_buf.buf, ' ') + 1;
+   while (skip_prefix(command_buf.buf, merge , from)) {
n = xmalloc(sizeof(*n));
s = lookup_branch(from);
if (s)
@@ -2668,11 +2664,10 @@