{"thread":{"id":"36039","subject":"[PATCH V2] commit.c: Use skip_prefix() instead of starts_with()","startedAt":"2014-03-03T15:59:58Z","lastAt":"2014-03-03T23:45:34Z","messageCount":4,"participants":["Tanay Abhra","Junio C Hamano","Max Horn"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"235881","messageId":"1393862398-2989-1-git-send-email-tanayabh@gmail.com","threadId":"36039","inReplyTo":null,"subject":"[PATCH V2] commit.c: Use skip_prefix() instead of starts_with()","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-03-03T15:59:58Z","receivedAt":"2014-03-03T15:59:58Z","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 more elegant and abstracts away the details.\n\nHelped-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\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 | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 6bf4fe0..668c703 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, *skip;\n \tchar *buffer = NULL;\n \tstruct ident_split ident;\n \tchar *date_end;\n@@ -566,14 +566,15 @@ 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\tif (!(skip = skip_prefix(buf, \"author \"))) {\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 = skip;\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,9 +1194,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\tif (found = skip_prefix(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\t;\n \t\t} else {\n \t\t\tfound = strstr(buf, sigcheck_gpg_status[i].check);\n \t\t\tif (!found)\n-- \n1.9.0\n"},{"id":"235909","messageId":"xmqqiorvoyoo.fsf@gitster.dls.corp.google.com","threadId":"36039","inReplyTo":"1393862398-2989-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH V2] commit.c: Use skip_prefix() instead of starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-03T19:43:35Z","receivedAt":"2014-03-03T19:43:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tanay Abhra <tanayabh@gmail.com> writes:\n\n> In record_author_date() & parse_gpg_output() ,using skip_prefix() instead of\n> starts_with() is more elegant and abstracts away the details.\n\nAvoid subjective judgement like \"more elegant\" when justifying your\nchange; you are not your own judge.\n\nThe caller of starts_with() actually can use the string that follows\nthe expected prefix and that is the reason why using skip_prefix()\nin these places is a good idea.  There is no need to be subjective\nto justify that change.\n\nI do not think there is any more abstracting away of the details in\nthis change.  The updated uses a different and more suitable\nabstraction than the original.\n\n> diff --git a/commit.c b/commit.c\n> index 6bf4fe0..668c703 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, *skip;\n>  \tchar *buffer = NULL;\n>  \tstruct ident_split ident;\n>  \tchar *date_end;\n> @@ -566,14 +566,15 @@ 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\tif (!(skip = skip_prefix(buf, \"author \"))) {\n\nWe tend to avoid assignments in conditionals.\n\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 = skip;\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\nIf you give a sensible name to what 'buf + strlen(\"author \")' is,\nthen the result becomes a lot more readable compared to the\noriginal, and I think that is what this change is about.\n\nAnd \"skip\" is not a good name for that.  'but + strlen(\"author \")'\nis what split_ident_line() expects its input to be split; let's\ntentatively call it \"ident_line\" and see what the call looks like:\n\n\tsplit_ident_line(&ident, ident_line, line_end - ident_line))\n\nAnd that is what we want to see here.  It is a bit more clear than\nthe original that we are splitting the ident information on the line,\nident_line (you could call it ident_begin) points at the beginning\nand line_end points at the end of that ident information.\n\nUse of skip_prefix(), which I am sure you took the name of the new\nvariable \"skip\" from, is merely an implementation detail of finding\nwhere the ident begins.  A good rule of thumb to remember is to name\nthings after what they are, not how you obtain them, how they are\nused or what they are used for/as.\n\n> @@ -1193,9 +1194,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\tif (found = skip_prefix(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\t;\n>  \t\t} else {\n>  \t\t\tfound = strstr(buf, sigcheck_gpg_status[i].check);\n>  \t\t\tif (!found)\n\nThis hunk looks good.  It can be a separate patch but they are both\nminor changes so it is OK to have it in a single patch.\n"},{"id":"235949","messageId":"176A3D83-AFFB-4EF4-A1CB-3A953F692166@quendi.de","threadId":"36039","inReplyTo":"xmqqiorvoyoo.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH V2] commit.c: Use skip_prefix() instead of starts_with()","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2014-03-03T22:59:20Z","receivedAt":"2014-03-03T22:59:20Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"\nOn 03.03.2014, at 20:43, Junio C Hamano <gitster@pobox.com> wrote:\n\n> Tanay Abhra <tanayabh@gmail.com> writes:\n> \n>> @@ -1193,9 +1194,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\tif (found = skip_prefix(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\t;\n>> \t\t} else {\n>> \t\t\tfound = strstr(buf, sigcheck_gpg_status[i].check);\n>> \t\t\tif (!found)\n> \n> This hunk looks good.  It can be a separate patch but they are both\n> minor changes so it is OK to have it in a single patch.\n\nHm, but that hunk also introduces an assignment in a conditional, and introduces an empty block. Maybe like this?\n\n\ndiff --git a/commit.c b/commit.c\nindex 6bf4fe0..0ee0725 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1193,10 +1193,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\n"},{"id":"235963","messageId":"xmqqbnxmn8wx.fsf@gitster.dls.corp.google.com","threadId":"36039","inReplyTo":"176A3D83-AFFB-4EF4-A1CB-3A953F692166@quendi.de","subject":"Re: [PATCH V2] commit.c: Use skip_prefix() instead of starts_with()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-03T23:45:34Z","receivedAt":"2014-03-03T23:45:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn <max@quendi.de> writes:\n\n> On 03.03.2014, at 20:43, Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> Tanay Abhra <tanayabh@gmail.com> writes:\n>> \n>>> @@ -1193,9 +1194,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\tif (found = skip_prefix(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\t;\n>>> \t\t} else {\n>>> \t\t\tfound = strstr(buf, sigcheck_gpg_status[i].check);\n>>> \t\t\tif (!found)\n>> \n>> This hunk looks good.  It can be a separate patch but they are both\n>> minor changes so it is OK to have it in a single patch.\n>\n> Hm, but that hunk also introduces an assignment in a conditional,\n> and introduces an empty block. Maybe like this?\n\nMuch better.\n\nIf we anticipate that we may add _more_ ways to find the thing\nlater, then I would say this code structure is better:\n\n\t/* Is it at the beginning of the buffer? */\n\tfound = skip_prefix(...);\n        if (!found) {\n\t\t/* Oh, maybe it is on the second or later line? */\n        \tfound = ... find it on a later line...\n\t}\n\tif (!found)\n        \tcontinue;\n\nbut I do not think that is the case for this particular one.  We\neither try to find it at the very beginning (i.e. no leading\nnewline), or we have some other lines before it (i.e. require\nleading newline), and there will be no future extension, so what you\nsuggested below looks sensible.\n\n> diff --git a/commit.c b/commit.c\n> index 6bf4fe0..0ee0725 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1193,10 +1193,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"}]}