{"thread":{"id":"36047","subject":"[PATCH v3] commit.c: use skip_prefix() instead of starts_with()","startedAt":"2014-03-04T08:42:20Z","lastAt":"2014-03-04T20:38:32Z","messageCount":4,"participants":["Tanay Abhra","Max Horn","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"235979","messageId":"1393922540-13156-1-git-send-email-tanayabh@gmail.com","threadId":"36047","inReplyTo":null,"subject":"[PATCH v3] commit.c: use skip_prefix() instead of starts_with()","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-03-04T08:42:20Z","receivedAt":"2014-03-04T08:42:20Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"In record_author_date() & parse_gpg_output() ,using skip_prefix() instead of\nstarts_with() is a more suitable abstraction.\n\nHelped-by: Max Horn \nHelped-by: Junio C Hamano \nHelped-by: Michael Haggerty \nSigned-off-by: Tanay Abhra \n---\nPatch V3 Variable naming improved, removed assignments inside conditionals.\n\tThanks to Junio C Hamano and Max Horn.\n\t\nPatch V2 Corrected email formatting ,reapplied the implementation according to suggestions.\n\tThanks to Michael Haggerty.\n\nThis is in respect to GSoC microproject #10.\n\nIn record_author_date(), extra and useless calls to strlen due to using starts_with()\nwere removed by using skip_prefix(). Extra variable \"skip\" was used as \"buf\" is used in \nfor loop update check.\n\nOther usages of starts_with() in the same file can be found with,\n\n$ grep -n starts_with commit.c\n\n1116:\t\telse if (starts_with(line, gpg_sig_header) &&\n1196:\t\tif (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {\n\nThe starts_with() in line 1116 was left as it is, as strlen values were pre generated as \nglobal variables.\nThe starts_with() in line 1196 was replaced as it abstracts way the skip_prefix part by\ndirectly using the function.\nAlso skip_prefix() is inline when compared to starts_with().\n\n commit.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 6bf4fe0..6c92acb 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);\n static void record_author_date(struct author_date_slab *author_date,\n \t\t\t struct commit *commit)\n {\n-\tconst char *buf, *line_end;\n+\tconst char *buf, *line_end, *ident_line;\n \tchar *buffer = NULL;\n \tstruct ident_split ident;\n \tchar *date_end;\n@@ -566,14 +566,16 @@ static void record_author_date(struct author_date_slab *author_date,\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\tident_line = skip_prefix(buf, \"author \");\n+\t\tif (!ident_line) {\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\tbuf = ident_line;\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\t\t\t buf,\n+\t\t\t\t line_end - buf) ||\n \t\t !ident.date_begin || !ident.date_end)\n \t\t\tgoto fail_exit; /* malformed \"author\" line */\n \t\tbreak;\n@@ -1193,10 +1195,9 @@ 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\t/* At the very beginning of the buffer */\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\n"},{"id":"236021","messageId":"8CB399B0-6781-4702-9EC5-0D0A0CCC3450@quendi.de","threadId":"36047","inReplyTo":"1393922540-13156-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3] commit.c: use skip_prefix() instead of starts_with()","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2014-03-04T19:16:47Z","receivedAt":"2014-03-04T19:16:47Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"\nOn 04.03.2014, at 09:42, Tanay Abhra wrote:\n\n[...]\n\n> commit.c | 17 +++++++++--------\n> 1 file changed, 9 insertions(+), 8 deletions(-)\n> \n> diff --git a/commit.c b/commit.c\n> index 6bf4fe0..6c92acb 100644\n> --- a/commit.c\n> +++ b/commit.c\n\n[...]\n\n> @@ -566,14 +566,16 @@ static void record_author_date(struct author_date_slab *author_date,\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\tident_line = skip_prefix(buf, \"author \");\n> +\t\tif (!ident_line) {\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\tbuf = ident_line;\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\t\t\t buf,\n> +\t\t\t\t line_end - buf) ||\n> \t\t !ident.date_begin || !ident.date_end)\n> \t\t\tgoto fail_exit; /* malformed \"author\" line */\n> \t\tbreak;\n\nWhy not get rid of that assignment to \"buf\", and use ident_line instead of buf below? That seems like it would be more readable, wouldn't it?\n\n\n> @@ -1193,10 +1195,9 @@ 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\t/* At the very beginning of the buffer */\n\nDo we really need that comment, and in that spot? The code seemed clear enough to me without it. But if you think keeping is better, perhaps move it to *before* the skip_prefix, and add a trailing \"?\"\n\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\n\n"},{"id":"236023","messageId":"xmqqppm1kbc5.fsf@gitster.dls.corp.google.com","threadId":"36047","inReplyTo":"1393922540-13156-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH v3] commit.c: use skip_prefix() instead of starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-04T19:33:46Z","receivedAt":"2014-03-04T19:33:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra writes:\n\n> In record_author_date() & parse_gpg_output() ,using skip_prefix() instead of\n> starts_with() is a more suitable abstraction.\n\nThanks. Will queue with a reworded message to clarify what exactly\n\"A more suitable\" means.\n\nHere is what I tentatively came up with.\n\n-- >8 --\nFrom: Tanay Abhra \nDate: Tue, 4 Mar 2014 00:42:20 -0800\nSubject: [PATCH] commit.c: use skip_prefix() instead of starts_with()\n\nIn record_author_date() & parse_gpg_output(), the callers of\nstarts_with() not just want to know if the string starts with the\nprefix, but also can benefit from knowing the string that follows\nthe prefix.\n\nBy using skip_prefix(), we can do both at the same time.\n\nHelped-by: Max Horn \nHelped-by: Junio C Hamano \nHelped-by: Michael Haggerty \nSigned-off-by: Tanay Abhra \nSigned-off-by: Junio C Hamano \n---\n commit.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 6bf4fe0..6c92acb 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);\n static void record_author_date(struct author_date_slab *author_date,\n \t\t\t struct commit *commit)\n {\n-\tconst char *buf, *line_end;\n+\tconst char *buf, *line_end, *ident_line;\n \tchar *buffer = NULL;\n \tstruct ident_split ident;\n \tchar *date_end;\n@@ -566,14 +566,16 @@ static void record_author_date(struct author_date_slab *author_date,\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\tident_line = skip_prefix(buf, \"author \");\n+\t\tif (!ident_line) {\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\tbuf = ident_line;\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\t\t\t buf,\n+\t\t\t\t line_end - buf) ||\n \t\t !ident.date_begin || !ident.date_end)\n \t\t\tgoto fail_exit; /* malformed \"author\" line */\n \t\tbreak;\n@@ -1193,10 +1195,9 @@ 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\t/* At the very beginning of the buffer */\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-186-gd464cb7\n"},{"id":"236038","messageId":"xmqqha7dk8c7.fsf@gitster.dls.corp.google.com","threadId":"36047","inReplyTo":"8CB399B0-6781-4702-9EC5-0D0A0CCC3450@quendi.de","subject":"Re: [PATCH v3] commit.c: use skip_prefix() instead of starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-04T20:38:32Z","receivedAt":"2014-03-04T20:38:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn writes:\n\n>> +\t\tbuf = ident_line;\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\t\t\t buf,\n>> +\t\t\t\t line_end - buf) ||\n>> \t\t !ident.date_begin || !ident.date_end)\n>> \t\t\tgoto fail_exit; /* malformed \"author\" line */\n>> \t\tbreak;\n>\n> Why not get rid of that assignment to \"buf\", and use ident_line\n> instead of buf below? That seems like it would be more readable,\n> wouldn't it?\n\nYes, and also now the argument list is much shorter, you could\nprobably do it on two lines instead of three:\n\n if (split_ident_line(&ident,\n ident_line, line_end - ident_line) ||\n ...\n\n\n>> @@ -1193,10 +1195,9 @@ 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\t/* At the very beginning of the buffer */\n>\n> Do we really need that comment, and in that spot? The code seemed\n> clear enough to me without it. But if you think keeping is better,\n> perhaps move it to *before* the skip_prefix, and add a trailing\n> \"?\"\n\nBoth good suggestions (I tend to prefer the removal).\n\nThanks.\n"}]}