{"thread":{"id":"36065","subject":"[PATCH v2] commit.c: Replace starts_with() with skip_prefix()","startedAt":"2014-03-05T14:06:12Z","lastAt":"2014-03-06T09:38:48Z","messageCount":3,"participants":["Karthik Nayak","karthik nayak","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"236085","messageId":"1394028372-29990-1-git-send-email-karthik.188@gmail.com","threadId":"36065","inReplyTo":null,"subject":"[PATCH v2] commit.c: Replace starts_with() with skip_prefix()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2014-03-05T14:06:12Z","receivedAt":"2014-03-05T14:06:12Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Replaces all instances of starts_with() by skip_prefix(),\nwhich can not only be used to check presence of a prefix,\nbut also used further on as it returns the string after the prefix,\nif the prefix is present.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n\nHey Eric,\nHere are the changes i have made in this Patch v2:\n1. Edited the variables names to fit their usage\n2. set my emacs to indent 8 tabs, so proper indentation\n3. fixed my error where i increased the value by 1 in parse_signed_commit().\nThanks again for your patience.\n\n---\n commit.c | 23 ++++++++++++-----------\n 1 file changed, 12 insertions(+), 11 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 6bf4fe0..f006490 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -553,6 +553,7 @@ static void record_author_date(struct author_date_slab *author_date,\n \tstruct ident_split ident;\n \tchar *date_end;\n \tunsigned long date;\n+\tconst char *buf_split;\n \n \tif (!commit->buffer) {\n \t\tunsigned long size;\n@@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab *author_date,\n \t\t\treturn;\n \t}\n \n+\tbuf_split = skip_prefix(buf, \"author \");\n+\n \tfor (buf = commit->buffer ? commit->buffer : buffer;\n \t     buf;\n \t     buf = line_end + 1) {\n \t\tline_end = strchrnul(buf, '\\n');\n-\t\tif (!starts_with(buf, \"author \")) {\n+\t\tif (!buf_split) {\n \t\t\tif (!line_end[0] || line_end[1] == '\\n')\n \t\t\t\treturn; /* end of header */\n \t\t\tcontinue;\n \t\t}\n-\t\tif (split_ident_line(&ident,\n-\t\t\t\t     buf + strlen(\"author \"),\n-\t\t\t\t     line_end - (buf + strlen(\"author \"))) ||\n+\t\tif (split_ident_line(&ident, buf_split,\n+\t\t\t\t     line_end - buf_split) ||\n \t\t    !ident.date_begin || !ident.date_end)\n \t\t\tgoto fail_exit; /* malformed \"author\" line */\n \t\tbreak;\n@@ -1098,6 +1100,7 @@ int parse_signed_commit(const unsigned char *sha1,\n \tchar *buffer = read_sha1_file(sha1, &type, &size);\n \tint in_signature, saw_signature = -1;\n \tchar *line, *tail;\n+\tconst char *line_split;\n \n \tif (!buffer || type != OBJ_COMMIT)\n \t\tgoto cleanup;\n@@ -1111,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,\n \t\tchar *next = memchr(line, '\\n', tail - line);\n \n \t\tnext = next ? next + 1 : tail;\n+\t\tline_split = skip_prefix(line, gpg_sig_header);\n \t\tif (in_signature && line[0] == ' ')\n \t\t\tsig = line + 1;\n-\t\telse if (starts_with(line, gpg_sig_header) &&\n-\t\t\t line[gpg_sig_header_len] == ' ')\n-\t\t\tsig = line + gpg_sig_header_len + 1;\n+\t\telse if (line_split && line_split[0] == ' ')\n+\t\t\tsig = line_split + 1;\n \t\tif (sig) {\n \t\t\tstrbuf_add(signature, sig, next - sig);\n \t\t\tsaw_signature = 1;\n@@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check *sigc)\n \tfor (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n \t\tconst char *found, *next;\n \n-\t\tif (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {\n-\t\t\t/* At the very beginning of the buffer */\n-\t\t\tfound = buf + strlen(sigcheck_gpg_status[i].check + 1);\n-\t\t} else {\n+\t\tfound = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);\n+\t\tif(!found) {\n \t\t\tfound = strstr(buf, sigcheck_gpg_status[i].check);\n \t\t\tif (!found)\n \t\t\t\tcontinue;\n-- \n1.9.0.138.g2de3478\n"},{"id":"236086","messageId":"CAOLa=ZT5fnnXzM6rCAj2eUBM+CSeLwSOveLzKoT8u7ekjHZvSw@mail.gmail.com","threadId":"36065","inReplyTo":"1394028372-29990-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v2] commit.c: Replace starts_with() with skip_prefix()","fromName":"karthik nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2014-03-05T14:10:49Z","receivedAt":"2014-03-05T14:10:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hey Eric,\nSorry about not cc'ing you again , still figuring out \"git send-email\".\n\n\nOn Wed, Mar 5, 2014 at 7:36 PM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> Replaces all instances of starts_with() by skip_prefix(),\n> which can not only be used to check presence of a prefix,\n> but also used further on as it returns the string after the prefix,\n> if the prefix is present.\n>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n>\n> Hey Eric,\n> Here are the changes i have made in this Patch v2:\n> 1. Edited the variables names to fit their usage\n> 2. set my emacs to indent 8 tabs, so proper indentation\n> 3. fixed my error where i increased the value by 1 in parse_signed_commit().\n> Thanks again for your patience.\n>\n> ---\n>  commit.c | 23 ++++++++++++-----------\n>  1 file changed, 12 insertions(+), 11 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index 6bf4fe0..f006490 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -553,6 +553,7 @@ static void record_author_date(struct author_date_slab *author_date,\n>         struct ident_split ident;\n>         char *date_end;\n>         unsigned long date;\n> +       const char *buf_split;\n>\n>         if (!commit->buffer) {\n>                 unsigned long size;\n> @@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab *author_date,\n>                         return;\n>         }\n>\n> +       buf_split = skip_prefix(buf, \"author \");\n> +\n>         for (buf = commit->buffer ? commit->buffer : buffer;\n>              buf;\n>              buf = line_end + 1) {\n>                 line_end = strchrnul(buf, '\\n');\n> -               if (!starts_with(buf, \"author \")) {\n> +               if (!buf_split) {\n>                         if (!line_end[0] || line_end[1] == '\\n')\n>                                 return; /* end of header */\n>                         continue;\n>                 }\n> -               if (split_ident_line(&ident,\n> -                                    buf + strlen(\"author \"),\n> -                                    line_end - (buf + strlen(\"author \"))) ||\n> +               if (split_ident_line(&ident, buf_split,\n> +                                    line_end - buf_split) ||\n>                     !ident.date_begin || !ident.date_end)\n>                         goto fail_exit; /* malformed \"author\" line */\n>                 break;\n> @@ -1098,6 +1100,7 @@ int parse_signed_commit(const unsigned char *sha1,\n>         char *buffer = read_sha1_file(sha1, &type, &size);\n>         int in_signature, saw_signature = -1;\n>         char *line, *tail;\n> +       const char *line_split;\n>\n>         if (!buffer || type != OBJ_COMMIT)\n>                 goto cleanup;\n> @@ -1111,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,\n>                 char *next = memchr(line, '\\n', tail - line);\n>\n>                 next = next ? next + 1 : tail;\n> +               line_split = skip_prefix(line, gpg_sig_header);\n>                 if (in_signature && line[0] == ' ')\n>                         sig = line + 1;\n> -               else if (starts_with(line, gpg_sig_header) &&\n> -                        line[gpg_sig_header_len] == ' ')\n> -                       sig = line + gpg_sig_header_len + 1;\n> +               else if (line_split && line_split[0] == ' ')\n> +                       sig = line_split + 1;\n>                 if (sig) {\n>                         strbuf_add(signature, sig, next - sig);\n>                         saw_signature = 1;\n> @@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check *sigc)\n>         for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n>                 const char *found, *next;\n>\n> -               if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {\n> -                       /* At the very beginning of the buffer */\n> -                       found = buf + strlen(sigcheck_gpg_status[i].check + 1);\n> -               } else {\n> +               found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);\n> +               if(!found) {\n>                         found = strstr(buf, sigcheck_gpg_status[i].check);\n>                         if (!found)\n>                                 continue;\n> --\n> 1.9.0.138.g2de3478\n>\n"},{"id":"236156","messageId":"CAPig+cSp6ckBifayD5a0Z=+xWVfRiVcEyf6rEi_4M-fqWJ9TBw@mail.gmail.com","threadId":"36065","inReplyTo":"1394028372-29990-1-git-send-email-karthik.188@gmail.com","subject":"Re: [PATCH v2] commit.c: Replace starts_with() with skip_prefix()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-06T09:38:48Z","receivedAt":"2014-03-06T09:38:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 5, 2014 at 9:06 AM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> Replaces all instances of starts_with() by skip_prefix(),\n\nUse imperative mode: \"Replace all...\"\n\n> which can not only be used to check presence of a prefix,\n> but also used further on as it returns the string after the prefix,\n> if the prefix is present.\n\nThis is a lot better than previous versions. It could be improved a\nbit by more directly stating that skip_prefix() singly does what the\ncurrent code is doing in two steps. (See Tanay's submission [1] for an\nexample of a well-crafted commit message). However, we're probably at\nthe point of diminishing returns, so no need to reroll just for this.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243395\n\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n>\n> Hey Eric,\n> Here are the changes i have made in this Patch v2:\n> 1. Edited the variables names to fit their usage\n> 2. set my emacs to indent 8 tabs, so proper indentation\n> 3. fixed my error where i increased the value by 1 in parse_signed_commit().\n> Thanks again for your patience.\n\nThanks for the reroll and for explaining the changes. More below.\n\n> ---\n>  commit.c | 23 ++++++++++++-----------\n>  1 file changed, 12 insertions(+), 11 deletions(-)\n>\n> diff --git a/commit.c b/commit.c\n> index 6bf4fe0..f006490 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -553,6 +553,7 @@ static void record_author_date(struct author_date_slab *author_date,\n>         struct ident_split ident;\n>         char *date_end;\n>         unsigned long date;\n> +       const char *buf_split;\n\nIn a previous review, I suggested reading Junio's response [1] to a\nsimilar submission. Of particular interest, Junio says:\n\n    A good rule of thumb to remember is to name things after what\n    they are, not how you obtain them, how they are used or what\n    they are used for/as.\n\nThe name 'buf_split' is clearly a \"how you obtain them\", which does\nnot convey much. Better would be to name the variable to reflect what\nit references once assigned.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/243231/focus=243259\n\n>         if (!commit->buffer) {\n>                 unsigned long size;\n> @@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab *author_date,\n>                         return;\n>         }\n>\n> +       buf_split = skip_prefix(buf, \"author \");\n> +\n>         for (buf = commit->buffer ? commit->buffer : buffer;\n>              buf;\n>              buf = line_end + 1) {\n>                 line_end = strchrnul(buf, '\\n');\n> -               if (!starts_with(buf, \"author \")) {\n> +               if (!buf_split) {\n>                         if (!line_end[0] || line_end[1] == '\\n')\n>                                 return; /* end of header */\n>                         continue;\n>                 }\n> -               if (split_ident_line(&ident,\n> -                                    buf + strlen(\"author \"),\n> -                                    line_end - (buf + strlen(\"author \"))) ||\n> +               if (split_ident_line(&ident, buf_split,\n> +                                    line_end - buf_split) ||\n>                     !ident.date_begin || !ident.date_end)\n>                         goto fail_exit; /* malformed \"author\" line */\n>                 break;\n> @@ -1098,6 +1100,7 @@ int parse_signed_commit(const unsigned char *sha1,\n>         char *buffer = read_sha1_file(sha1, &type, &size);\n>         int in_signature, saw_signature = -1;\n>         char *line, *tail;\n> +       const char *line_split;\n\nDitto.\n\n>         if (!buffer || type != OBJ_COMMIT)\n>                 goto cleanup;\n> @@ -1111,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,\n>                 char *next = memchr(line, '\\n', tail - line);\n>\n>                 next = next ? next + 1 : tail;\n> +               line_split = skip_prefix(line, gpg_sig_header);\n>                 if (in_signature && line[0] == ' ')\n>                         sig = line + 1;\n> -               else if (starts_with(line, gpg_sig_header) &&\n> -                        line[gpg_sig_header_len] == ' ')\n> -                       sig = line + gpg_sig_header_len + 1;\n> +               else if (line_split && line_split[0] == ' ')\n> +                       sig = line_split + 1;\n\nA shortcoming of this change is that skip_prefix() is now being called\nunconditionally, even when its result won't be used (as in the first\n'if' statement). The original code did the work of checking the prefix\nonly if the first 'if' statement evaluated to false.\n\n>                 if (sig) {\n>                         strbuf_add(signature, sig, next - sig);\n>                         saw_signature = 1;\n> @@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check *sigc)\n>         for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {\n>                 const char *found, *next;\n>\n> -               if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {\n> -                       /* At the very beginning of the buffer */\n> -                       found = buf + strlen(sigcheck_gpg_status[i].check + 1);\n> -               } else {\n> +               found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);\n> +               if(!found) {\n\nSpace after 'if'.\n\n>                         found = strstr(buf, sigcheck_gpg_status[i].check);\n>                         if (!found)\n>                                 continue;\n> --\n> 1.9.0.138.g2de3478\n"}]}