{"thread":{"id":"33201","subject":"[PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()","startedAt":"2013-03-16T18:29:31Z","lastAt":"2013-03-17T11:38:44Z","messageCount":3,"participants":["René Scharfe","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"211466","messageId":"5144BA0B.2040109@lsrfire.ath.cx","threadId":"33201","inReplyTo":null,"subject":"[PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-03-16T18:29:31Z","receivedAt":"2013-03-16T18:29:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The only caller of diagnose_invalid_sha1_path() extracts a substring from\nan object name by creating a NUL-terminated copy of the interesting part.\nAdd a length parameter to the function and thus avoid the need for an\nallocation, thereby simplifying the code.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\n sha1_name.c | 32 ++++++++++++++------------------\n 1 file changed, 14 insertions(+), 18 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 95003c7..4cea6d3 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1137,7 +1137,8 @@ int get_sha1_blob(const char *name, unsigned char *sha1)\n static void diagnose_invalid_sha1_path(const char *prefix,\n \t\t\t\t       const char *filename,\n \t\t\t\t       const unsigned char *tree_sha1,\n-\t\t\t\t       const char *object_name)\n+\t\t\t\t       const char *object_name,\n+\t\t\t\t       int object_name_len)\n {\n \tstruct stat st;\n \tunsigned char sha1[20];\n@@ -1147,8 +1148,8 @@ static void diagnose_invalid_sha1_path(const char *prefix,\n \t\tprefix = \"\";\n \n \tif (!lstat(filename, &st))\n-\t\tdie(\"Path '%s' exists on disk, but not in '%s'.\",\n-\t\t    filename, object_name);\n+\t\tdie(\"Path '%s' exists on disk, but not in '%.*s'.\",\n+\t\t    filename, object_name_len, object_name);\n \tif (errno == ENOENT || errno == ENOTDIR) {\n \t\tchar *fullname = xmalloc(strlen(filename)\n \t\t\t\t\t     + strlen(prefix) + 1);\n@@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix,\n \t\tif (!get_tree_entry(tree_sha1, fullname,\n \t\t\t\t    sha1, &mode)) {\n \t\t\tdie(\"Path '%s' exists, but not '%s'.\\n\"\n-\t\t\t    \"Did you mean '%s:%s' aka '%s:./%s'?\",\n+\t\t\t    \"Did you mean '%.*s:%s' aka '.*%.*s:./%s'?\",\n \t\t\t    fullname,\n \t\t\t    filename,\n-\t\t\t    object_name,\n+\t\t\t    object_name_len, object_name,\n \t\t\t    fullname,\n-\t\t\t    object_name,\n+\t\t\t    object_name_len, object_name,\n \t\t\t    filename);\n \t\t}\n-\t\tdie(\"Path '%s' does not exist in '%s'\",\n-\t\t    filename, object_name);\n+\t\tdie(\"Path '%s' does not exist in '%.*s'\",\n+\t\t    filename, object_name_len, object_name);\n \t}\n }\n \n@@ -1332,13 +1333,8 @@ static int get_sha1_with_context_1(const char *name,\n \t}\n \tif (*cp == ':') {\n \t\tunsigned char tree_sha1[20];\n-\t\tchar *object_name = NULL;\n-\t\tif (only_to_die) {\n-\t\t\tobject_name = xmalloc(cp-name+1);\n-\t\t\tstrncpy(object_name, name, cp-name);\n-\t\t\tobject_name[cp-name] = '\\0';\n-\t\t}\n-\t\tif (!get_sha1_1(name, cp-name, tree_sha1, GET_SHA1_TREEISH)) {\n+\t\tint len = cp - name;\n+\t\tif (!get_sha1_1(name, len, tree_sha1, GET_SHA1_TREEISH)) {\n \t\t\tconst char *filename = cp+1;\n \t\t\tchar *new_filename = NULL;\n \n@@ -1348,8 +1344,8 @@ static int get_sha1_with_context_1(const char *name,\n \t\t\tret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);\n \t\t\tif (ret && only_to_die) {\n \t\t\t\tdiagnose_invalid_sha1_path(prefix, filename,\n-\t\t\t\t\t\t\t   tree_sha1, object_name);\n-\t\t\t\tfree(object_name);\n+\t\t\t\t\t\t\t   tree_sha1,\n+\t\t\t\t\t\t\t   name, len);\n \t\t\t}\n \t\t\thashcpy(oc->tree, tree_sha1);\n \t\t\tstrncpy(oc->path, filename,\n@@ -1360,7 +1356,7 @@ static int get_sha1_with_context_1(const char *name,\n \t\t\treturn ret;\n \t\t} else {\n \t\t\tif (only_to_die)\n-\t\t\t\tdie(\"Invalid object name '%s'.\", object_name);\n+\t\t\t\tdie(\"Invalid object name '%.*s'.\", len, name);\n \t\t}\n \t}\n \treturn ret;\n-- \n1.8.2\n"},{"id":"211499","messageId":"7vzjy2qz6g.fsf@alter.siamese.dyndns.org","threadId":"33201","inReplyTo":"5144BA0B.2040109@lsrfire.ath.cx","subject":"Re: [PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-17T07:10:31Z","receivedAt":"2013-03-17T07:10:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:\n\n> The only caller of diagnose_invalid_sha1_path() extracts a substring from\n> an object name by creating a NUL-terminated copy of the interesting part.\n> Add a length parameter to the function and thus avoid the need for an\n> allocation, thereby simplifying the code.\n>\n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n> ---\n>  sha1_name.c | 32 ++++++++++++++------------------\n>  1 file changed, 14 insertions(+), 18 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 95003c7..4cea6d3 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1137,7 +1137,8 @@ int get_sha1_blob(const char *name, unsigned char *sha1)\n>  static void diagnose_invalid_sha1_path(const char *prefix,\n>  \t\t\t\t       const char *filename,\n>  \t\t\t\t       const unsigned char *tree_sha1,\n> -\t\t\t\t       const char *object_name)\n> +\t\t\t\t       const char *object_name,\n> +\t\t\t\t       int object_name_len)\n>  {\n>  \tstruct stat st;\n>  \tunsigned char sha1[20];\n> @@ -1147,8 +1148,8 @@ static void diagnose_invalid_sha1_path(const char *prefix,\n>  \t\tprefix = \"\";\n>  \n>  \tif (!lstat(filename, &st))\n> -\t\tdie(\"Path '%s' exists on disk, but not in '%s'.\",\n> -\t\t    filename, object_name);\n> +\t\tdie(\"Path '%s' exists on disk, but not in '%.*s'.\",\n> +\t\t    filename, object_name_len, object_name);\n>  \tif (errno == ENOENT || errno == ENOTDIR) {\n>  \t\tchar *fullname = xmalloc(strlen(filename)\n>  \t\t\t\t\t     + strlen(prefix) + 1);\n> @@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix,\n>  \t\tif (!get_tree_entry(tree_sha1, fullname,\n>  \t\t\t\t    sha1, &mode)) {\n>  \t\t\tdie(\"Path '%s' exists, but not '%s'.\\n\"\n> -\t\t\t    \"Did you mean '%s:%s' aka '%s:./%s'?\",\n> +\t\t\t    \"Did you mean '%.*s:%s' aka '.*%.*s:./%s'?\",\n\nThis is so unlike what I call \"Scharfe patch\", which I can apply\nwith my eyes closed and expect everything to be perfect.\n\nOther than that, I see this as a usual \"Scharfe patch\" ;-)  Will\nsquash an obvious fix in and apply.\n\nThanks.\n\n>  \t\t\t    fullname,\n>  \t\t\t    filename,\n> -\t\t\t    object_name,\n> +\t\t\t    object_name_len, object_name,\n>  \t\t\t    fullname,\n> -\t\t\t    object_name,\n> +\t\t\t    object_name_len, object_name,\n>  \t\t\t    filename);\n>  \t\t}\n> -\t\tdie(\"Path '%s' does not exist in '%s'\",\n> -\t\t    filename, object_name);\n> +\t\tdie(\"Path '%s' does not exist in '%.*s'\",\n> +\t\t    filename, object_name_len, object_name);\n>  \t}\n>  }\n>  \n> @@ -1332,13 +1333,8 @@ static int get_sha1_with_context_1(const char *name,\n>  \t}\n>  \tif (*cp == ':') {\n>  \t\tunsigned char tree_sha1[20];\n> -\t\tchar *object_name = NULL;\n> -\t\tif (only_to_die) {\n> -\t\t\tobject_name = xmalloc(cp-name+1);\n> -\t\t\tstrncpy(object_name, name, cp-name);\n> -\t\t\tobject_name[cp-name] = '\\0';\n> -\t\t}\n> -\t\tif (!get_sha1_1(name, cp-name, tree_sha1, GET_SHA1_TREEISH)) {\n> +\t\tint len = cp - name;\n> +\t\tif (!get_sha1_1(name, len, tree_sha1, GET_SHA1_TREEISH)) {\n>  \t\t\tconst char *filename = cp+1;\n>  \t\t\tchar *new_filename = NULL;\n>  \n> @@ -1348,8 +1344,8 @@ static int get_sha1_with_context_1(const char *name,\n>  \t\t\tret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);\n>  \t\t\tif (ret && only_to_die) {\n>  \t\t\t\tdiagnose_invalid_sha1_path(prefix, filename,\n> -\t\t\t\t\t\t\t   tree_sha1, object_name);\n> -\t\t\t\tfree(object_name);\n> +\t\t\t\t\t\t\t   tree_sha1,\n> +\t\t\t\t\t\t\t   name, len);\n>  \t\t\t}\n>  \t\t\thashcpy(oc->tree, tree_sha1);\n>  \t\t\tstrncpy(oc->path, filename,\n> @@ -1360,7 +1356,7 @@ static int get_sha1_with_context_1(const char *name,\n>  \t\t\treturn ret;\n>  \t\t} else {\n>  \t\t\tif (only_to_die)\n> -\t\t\t\tdie(\"Invalid object name '%s'.\", object_name);\n> +\t\t\t\tdie(\"Invalid object name '%.*s'.\", len, name);\n>  \t\t}\n>  \t}\n>  \treturn ret;\n"},{"id":"211514","messageId":"5145AB44.8050704@lsrfire.ath.cx","threadId":"33201","inReplyTo":"7vzjy2qz6g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-03-17T11:38:44Z","receivedAt":"2013-03-17T11:38:44Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.03.2013 08:10, schrieb Junio C Hamano:\n>> @@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix,\n>>   \t\tif (!get_tree_entry(tree_sha1, fullname,\n>>   \t\t\t\t    sha1, &mode)) {\n>>   \t\t\tdie(\"Path '%s' exists, but not '%s'.\\n\"\n>> -\t\t\t    \"Did you mean '%s:%s' aka '%s:./%s'?\",\n>> +\t\t\t    \"Did you mean '%.*s:%s' aka '.*%.*s:./%s'?\",\n>\n> Will squash an obvious fix in and apply.\n\nDid I try to make a point there?  Certainly not.  It seems I need to go \nback to http://vim-adventures.com/.\n\nThank you for spotting this!\nRené\n"}]}