{"thread":{"id":"41786","subject":"[PATCH 0/3] builtin/apply: simplify some gitdiff_* functions","startedAt":"2016-03-22T20:58:39Z","lastAt":"2016-03-22T22:10:45Z","messageCount":8,"participants":["Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"281483","messageId":"1458680322-17681-1-git-send-email-chriscool@tuxfamily.org","threadId":"41786","inReplyTo":null,"subject":"[PATCH 0/3] builtin/apply: simplify some gitdiff_* functions","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-22T20:58:39Z","receivedAt":"2016-03-22T20:58:39Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"While working on libifying \"git apply\" it appeared that some gitdiff_*\nfunctions are unnecessarily complex.\n\nChristian Couder (3):\n  builtin/apply: get rid of useless 'name' variable\n  builtin/apply: make gitdiff_verify_name() return void\n  builtin/apply: simplify gitdiff_{old,new}name()\n\n builtin/apply.c | 39 +++++++++++++++------------------------\n 1 file changed, 15 insertions(+), 24 deletions(-)\n\n-- \n2.8.0.rc4.1.g302de0d.dirty\n"},{"id":"281485","messageId":"1458680322-17681-2-git-send-email-chriscool@tuxfamily.org","threadId":"41786","inReplyTo":"1458680322-17681-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH 1/3] builtin/apply: get rid of useless 'name' variable","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-22T20:58:40Z","receivedAt":"2016-03-22T20:58:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"While at it put an 'else' on the same line as the previous '}'.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/apply.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 42c610e..465f954 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -931,22 +931,19 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,\n \t\treturn find_name(line, NULL, p_value, TERM_TAB);\n \n \tif (orig_name) {\n-\t\tint len;\n-\t\tconst char *name;\n+\t\tint len = strlen(orig_name);\n \t\tchar *another;\n-\t\tname = orig_name;\n-\t\tlen = strlen(name);\n \t\tif (isnull)\n-\t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\"), name, linenr);\n+\t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\"),\n+\t\t\t    orig_name, linenr);\n \t\tanother = find_name(line, NULL, p_value, TERM_TAB);\n-\t\tif (!another || memcmp(another, name, len + 1))\n+\t\tif (!another || memcmp(another, orig_name, len + 1))\n \t\t\tdie((side == DIFF_NEW_NAME) ?\n \t\t\t    _(\"git apply: bad git-diff - inconsistent new filename on line %d\") :\n \t\t\t    _(\"git apply: bad git-diff - inconsistent old filename on line %d\"), linenr);\n \t\tfree(another);\n \t\treturn orig_name;\n-\t}\n-\telse {\n+\t} else {\n \t\t/* expect \"/dev/null\" */\n \t\tif (memcmp(\"/dev/null\", line, 9) || line[9] != '\\n')\n \t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null on line %d\"), linenr);\n-- \n2.8.0.rc4.1.g302de0d.dirty\n"},{"id":"281486","messageId":"1458680322-17681-3-git-send-email-chriscool@tuxfamily.org","threadId":"41786","inReplyTo":"1458680322-17681-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-22T20:58:41Z","receivedAt":"2016-03-22T20:58:41Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"As the value returned by gitdiff_verify_name() is put into the\nsame variable that is passed as a parameter to this function,\nit is simpler to pass the address of the variable and have\ngitdiff_verify_name() change the variable itself.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/apply.c | 26 +++++++++++++-------------\n 1 file changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 465f954..4cafdaf 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -925,37 +925,37 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)\n #define DIFF_OLD_NAME 0\n #define DIFF_NEW_NAME 1\n \n-static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, int side)\n+static void gitdiff_verify_name(const char *line, int isnull, char **name, int side)\n {\n-\tif (!orig_name && !isnull)\n-\t\treturn find_name(line, NULL, p_value, TERM_TAB);\n+\tif (!*name && !isnull) {\n+\t\t*name = find_name(line, NULL, p_value, TERM_TAB);\n+\t\treturn;\n+\t}\n \n-\tif (orig_name) {\n-\t\tint len = strlen(orig_name);\n+\tif (*name) {\n+\t\tint len = strlen(*name);\n \t\tchar *another;\n \t\tif (isnull)\n \t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\"),\n-\t\t\t    orig_name, linenr);\n+\t\t\t    *name, linenr);\n \t\tanother = find_name(line, NULL, p_value, TERM_TAB);\n-\t\tif (!another || memcmp(another, orig_name, len + 1))\n+\t\tif (!another || memcmp(another, *name, len + 1))\n \t\t\tdie((side == DIFF_NEW_NAME) ?\n \t\t\t    _(\"git apply: bad git-diff - inconsistent new filename on line %d\") :\n \t\t\t    _(\"git apply: bad git-diff - inconsistent old filename on line %d\"), linenr);\n \t\tfree(another);\n-\t\treturn orig_name;\n \t} else {\n \t\t/* expect \"/dev/null\" */\n \t\tif (memcmp(\"/dev/null\", line, 9) || line[9] != '\\n')\n \t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null on line %d\"), linenr);\n-\t\treturn NULL;\n \t}\n }\n \n static int gitdiff_oldname(const char *line, struct patch *patch)\n {\n \tchar *orig = patch->old_name;\n-\tpatch->old_name = gitdiff_verify_name(line, patch->is_new, patch->old_name,\n-\t\t\t\t\t      DIFF_OLD_NAME);\n+\tgitdiff_verify_name(line, patch->is_new, &patch->old_name,\n+\t\t\t    DIFF_OLD_NAME);\n \tif (orig != patch->old_name)\n \t\tfree(orig);\n \treturn 0;\n@@ -964,8 +964,8 @@ static int gitdiff_oldname(const char *line, struct patch *patch)\n static int gitdiff_newname(const char *line, struct patch *patch)\n {\n \tchar *orig = patch->new_name;\n-\tpatch->new_name = gitdiff_verify_name(line, patch->is_delete, patch->new_name,\n-\t\t\t\t\t      DIFF_NEW_NAME);\n+\tgitdiff_verify_name(line, patch->is_delete, &patch->new_name,\n+\t\t\t    DIFF_NEW_NAME);\n \tif (orig != patch->new_name)\n \t\tfree(orig);\n \treturn 0;\n-- \n2.8.0.rc4.1.g302de0d.dirty\n"},{"id":"281484","messageId":"1458680322-17681-4-git-send-email-chriscool@tuxfamily.org","threadId":"41786","inReplyTo":"1458680322-17681-1-git-send-email-chriscool@tuxfamily.org","subject":"[PATCH 3/3] builtin/apply: simplify gitdiff_{old,new}name()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-22T20:58:42Z","receivedAt":"2016-03-22T20:58:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"After the previous simplifications, it is easy to see that\nthere is no need to free the original string passed to\ngitdiff_verify_name(), because this string can be changed\nonly when it is NULL.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n builtin/apply.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 4cafdaf..9cfa9f4 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -953,21 +953,15 @@ static void gitdiff_verify_name(const char *line, int isnull, char **name, int s\n \n static int gitdiff_oldname(const char *line, struct patch *patch)\n {\n-\tchar *orig = patch->old_name;\n \tgitdiff_verify_name(line, patch->is_new, &patch->old_name,\n \t\t\t    DIFF_OLD_NAME);\n-\tif (orig != patch->old_name)\n-\t\tfree(orig);\n \treturn 0;\n }\n \n static int gitdiff_newname(const char *line, struct patch *patch)\n {\n-\tchar *orig = patch->new_name;\n \tgitdiff_verify_name(line, patch->is_delete, &patch->new_name,\n \t\t\t    DIFF_NEW_NAME);\n-\tif (orig != patch->new_name)\n-\t\tfree(orig);\n \treturn 0;\n }\n \n-- \n2.8.0.rc4.1.g302de0d.dirty\n"},{"id":"281488","messageId":"xmqq7fgugqfp.fsf@gitster.mtv.corp.google.com","threadId":"41786","inReplyTo":"1458680322-17681-2-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH 1/3] builtin/apply: get rid of useless 'name' variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T21:20:42Z","receivedAt":"2016-03-22T21:20:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> While at it put an 'else' on the same line as the previous '}'.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  builtin/apply.c | 13 +++++--------\n>  1 file changed, 5 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 42c610e..465f954 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -931,22 +931,19 @@ static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name,\n>  \t\treturn find_name(line, NULL, p_value, TERM_TAB);\n>  \n>  \tif (orig_name) {\n> -\t\tint len;\n> -\t\tconst char *name;\n> +\t\tint len = strlen(orig_name);\n>  \t\tchar *another;\n> -\t\tname = orig_name;\n> -\t\tlen = strlen(name);\n>  \t\tif (isnull)\n> -\t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\"), name, linenr);\n> +\t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\"),\n> +\t\t\t    orig_name, linenr);\n>  \t\tanother = find_name(line, NULL, p_value, TERM_TAB);\n> -\t\tif (!another || memcmp(another, name, len + 1))\n> +\t\tif (!another || memcmp(another, orig_name, len + 1))\n>  \t\t\tdie((side == DIFF_NEW_NAME) ?\n>  \t\t\t    _(\"git apply: bad git-diff - inconsistent new filename on line %d\") :\n>  \t\t\t    _(\"git apply: bad git-diff - inconsistent old filename on line %d\"), linenr);\n>  \t\tfree(another);\n>  \t\treturn orig_name;\n> -\t}\n> -\telse {\n> +\t} else {\n>  \t\t/* expect \"/dev/null\" */\n>  \t\tif (memcmp(\"/dev/null\", line, 9) || line[9] != '\\n')\n>  \t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null on line %d\"), linenr);\n\nLooks correct; back when 1e3f6b6e (git-apply: more consistency\nchecks on gitdiff filenames, 2005-05-23) introduced this function,\nthe variable \"name\" was in the outer scope, and a subsequent update\nnarrowed its scope at ea56188a (Update git-apply to use C-style\nquoting for funny pathnames., 2005-10-16) but it could have removed\nthe variable at the same time.\n"},{"id":"281489","messageId":"xmqq37rigq7o.fsf@gitster.mtv.corp.google.com","threadId":"41786","inReplyTo":"1458680322-17681-3-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T21:25:31Z","receivedAt":"2016-03-22T21:25:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> As the value returned by gitdiff_verify_name() is put into the\n> same variable that is passed as a parameter to this function,\n> it is simpler to pass the address of the variable and have\n> gitdiff_verify_name() change the variable itself.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n\n\nThis change makes the function less useful by restricting the\ncallers--only the ones that wants in-place update can call it after\nthis change, while the old function signature allowed a caller to\npass one variable as orig, receive the result in another variable\n(and probably compare them).\n\nIt does not matter very much for this file scope static helper\neither way, and I would probably say the same thing if the patch\nwere in reverse (i.e. if the patch were loosening the restriction),\nbut I cannot offhand see why this is an improvement.  Puzzled...\n\n>  builtin/apply.c | 26 +++++++++++++-------------\n>  1 file changed, 13 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 465f954..4cafdaf 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -925,37 +925,37 @@ static int gitdiff_hdrend(const char *line, struct patch *patch)\n>  #define DIFF_OLD_NAME 0\n>  #define DIFF_NEW_NAME 1\n>  \n> -static char *gitdiff_verify_name(const char *line, int isnull, char *orig_name, int side)\n> +static void gitdiff_verify_name(const char *line, int isnull, char **name, int side)\n>  {\n> -\tif (!orig_name && !isnull)\n> -\t\treturn find_name(line, NULL, p_value, TERM_TAB);\n> +\tif (!*name && !isnull) {\n> +\t\t*name = find_name(line, NULL, p_value, TERM_TAB);\n> +\t\treturn;\n> +\t}\n>  \n> -\tif (orig_name) {\n> -\t\tint len = strlen(orig_name);\n> +\tif (*name) {\n> +\t\tint len = strlen(*name);\n>  \t\tchar *another;\n>  \t\tif (isnull)\n>  \t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null, got %s on line %d\"),\n> -\t\t\t    orig_name, linenr);\n> +\t\t\t    *name, linenr);\n>  \t\tanother = find_name(line, NULL, p_value, TERM_TAB);\n> -\t\tif (!another || memcmp(another, orig_name, len + 1))\n> +\t\tif (!another || memcmp(another, *name, len + 1))\n>  \t\t\tdie((side == DIFF_NEW_NAME) ?\n>  \t\t\t    _(\"git apply: bad git-diff - inconsistent new filename on line %d\") :\n>  \t\t\t    _(\"git apply: bad git-diff - inconsistent old filename on line %d\"), linenr);\n>  \t\tfree(another);\n> -\t\treturn orig_name;\n>  \t} else {\n>  \t\t/* expect \"/dev/null\" */\n>  \t\tif (memcmp(\"/dev/null\", line, 9) || line[9] != '\\n')\n>  \t\t\tdie(_(\"git apply: bad git-diff - expected /dev/null on line %d\"), linenr);\n> -\t\treturn NULL;\n>  \t}\n>  }\n>  \n>  static int gitdiff_oldname(const char *line, struct patch *patch)\n>  {\n>  \tchar *orig = patch->old_name;\n> -\tpatch->old_name = gitdiff_verify_name(line, patch->is_new, patch->old_name,\n> -\t\t\t\t\t      DIFF_OLD_NAME);\n> +\tgitdiff_verify_name(line, patch->is_new, &patch->old_name,\n> +\t\t\t    DIFF_OLD_NAME);\n>  \tif (orig != patch->old_name)\n>  \t\tfree(orig);\n>  \treturn 0;\n> @@ -964,8 +964,8 @@ static int gitdiff_oldname(const char *line, struct patch *patch)\n>  static int gitdiff_newname(const char *line, struct patch *patch)\n>  {\n>  \tchar *orig = patch->new_name;\n> -\tpatch->new_name = gitdiff_verify_name(line, patch->is_delete, patch->new_name,\n> -\t\t\t\t\t      DIFF_NEW_NAME);\n> +\tgitdiff_verify_name(line, patch->is_delete, &patch->new_name,\n> +\t\t\t    DIFF_NEW_NAME);\n>  \tif (orig != patch->new_name)\n>  \t\tfree(orig);\n>  \treturn 0;\n"},{"id":"281491","messageId":"xmqqy49afb6k.fsf@gitster.mtv.corp.google.com","threadId":"41786","inReplyTo":"1458680322-17681-4-git-send-email-chriscool@tuxfamily.org","subject":"Re: [PATCH 3/3] builtin/apply: simplify gitdiff_{old,new}name()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-22T21:35:31Z","receivedAt":"2016-03-22T21:35:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <christian.couder@gmail.com> writes:\n\n> After the previous simplifications, it is easy to see that\n> there is no need to free the original string passed to\n> gitdiff_verify_name(), because this string can be changed\n> only when it is NULL.\n>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n\nI do not think you need either 1/3 or 2/3 to see that (I actually\nthink it is easier to see this without 2/3).  The caller passes\npatch->old_name and\n\n - if that is NULL, then we will either get NULL or a new string\n   from find_name(); either way, we do not have to worry about\n   calling free() on the original NULL;\n\n - if that is not NULL, then the only possible value returned from\n   the function is itself (otherwise it will die()), so we won't be\n   calling free() in this code.\n\nso I agree with the conclusion, i.e. the conditional free() can go\nfrom these places.\n\n>  builtin/apply.c | 6 ------\n>  1 file changed, 6 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 4cafdaf..9cfa9f4 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -953,21 +953,15 @@ static void gitdiff_verify_name(const char *line, int isnull, char **name, int s\n>  \n>  static int gitdiff_oldname(const char *line, struct patch *patch)\n>  {\n> -\tchar *orig = patch->old_name;\n>  \tgitdiff_verify_name(line, patch->is_new, &patch->old_name,\n>  \t\t\t    DIFF_OLD_NAME);\n> -\tif (orig != patch->old_name)\n> -\t\tfree(orig);\n>  \treturn 0;\n>  }\n>  \n>  static int gitdiff_newname(const char *line, struct patch *patch)\n>  {\n> -\tchar *orig = patch->new_name;\n>  \tgitdiff_verify_name(line, patch->is_delete, &patch->new_name,\n>  \t\t\t    DIFF_NEW_NAME);\n> -\tif (orig != patch->new_name)\n> -\t\tfree(orig);\n>  \treturn 0;\n>  }\n"},{"id":"281500","messageId":"CAP8UFD13ueLG1xdV7QYpiZLx9DZHgdrPcpi9zLc7NAh2BLd08w@mail.gmail.com","threadId":"41786","inReplyTo":"xmqq37rigq7o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] builtin/apply: make gitdiff_verify_name() return void","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-03-22T22:10:45Z","receivedAt":"2016-03-22T22:10:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Mar 22, 2016 at 10:25 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Christian Couder <christian.couder@gmail.com> writes:\n>\n>> As the value returned by gitdiff_verify_name() is put into the\n>> same variable that is passed as a parameter to this function,\n>> it is simpler to pass the address of the variable and have\n>> gitdiff_verify_name() change the variable itself.\n>>\n>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n>> ---\n>\n>\n> This change makes the function less useful by restricting the\n> callers--only the ones that wants in-place update can call it after\n> this change, while the old function signature allowed a caller to\n> pass one variable as orig, receive the result in another variable\n> (and probably compare them).\n>\n> It does not matter very much for this file scope static helper\n> either way, and I would probably say the same thing if the patch\n> were in reverse (i.e. if the patch were loosening the restriction),\n> but I cannot offhand see why this is an improvement.  Puzzled...\n\nIn my opinion it is an improvement because:\n\n- the feature of allowing a caller to pass one variable as orig and\nreceive the result in another is useless here and just makes it more\ndifficult to understand what the code is doing because you first have\nto realize that in both calls the argument that is passed is also\nassigned to, and\n\n- using up the return value to return a result prevents from using it\nto signal an error instead of die()ing, so it makes libifying the\nfunction more difficult.\n\nBut if you prefer I can just resend a series with only patchs 1/3 and\n3/3. And we can discuss later in the context of libifying \"git apply\"\nif this patch makes sense.\n"}]}