[PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()

[PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()

[René Scharfe]

The only caller of diagnose_invalid_sha1_path() extracts a substring from
an object name by creating a NUL-terminated copy of the interesting part.
Add a length parameter to the function and thus avoid the need for an
allocation, thereby simplifying the code.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 sha1_name.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 95003c7..4cea6d3 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1137,7 +1137,8 @@ int get_sha1_blob(const char *name, unsigned char *sha1)
 static void diagnose_invalid_sha1_path(const char *prefix,
 				       const char *filename,
 				       const unsigned char *tree_sha1,
-				       const char *object_name)
+				       const char *object_name,
+				       int object_name_len)
 {
 	struct stat st;
 	unsigned char sha1[20];
@@ -1147,8 +1148,8 @@ static void diagnose_invalid_sha1_path(const char *prefix,
 		prefix = "";
 
 	if (!lstat(filename, &st))
-		die("Path '%s' exists on disk, but not in '%s'.",
-		    filename, object_name);
+		die("Path '%s' exists on disk, but not in '%.*s'.",
+		    filename, object_name_len, object_name);
 	if (errno == ENOENT || errno == ENOTDIR) {
 		char *fullname = xmalloc(strlen(filename)
 					     + strlen(prefix) + 1);
@@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix,
 		if (!get_tree_entry(tree_sha1, fullname,
 				    sha1, &mode)) {
 			die("Path '%s' exists, but not '%s'.\n"
-			    "Did you mean '%s:%s' aka '%s:./%s'?",
+			    "Did you mean '%.*s:%s' aka '.*%.*s:./%s'?",
 			    fullname,
 			    filename,
-			    object_name,
+			    object_name_len, object_name,
 			    fullname,
-			    object_name,
+			    object_name_len, object_name,
 			    filename);
 		}
-		die("Path '%s' does not exist in '%s'",
-		    filename, object_name);
+		die("Path '%s' does not exist in '%.*s'",
+		    filename, object_name_len, object_name);
 	}
 }
 
@@ -1332,13 +1333,8 @@ static int get_sha1_with_context_1(const char *name,
 	}
 	if (*cp == ':') {
 		unsigned char tree_sha1[20];
-		char *object_name = NULL;
-		if (only_to_die) {
-			object_name = xmalloc(cp-name+1);
-			strncpy(object_name, name, cp-name);
-			object_name[cp-name] = '\0';
-		}
-		if (!get_sha1_1(name, cp-name, tree_sha1, GET_SHA1_TREEISH)) {
+		int len = cp - name;
+		if (!get_sha1_1(name, len, tree_sha1, GET_SHA1_TREEISH)) {
 			const char *filename = cp+1;
 			char *new_filename = NULL;
 
@@ -1348,8 +1344,8 @@ static int get_sha1_with_context_1(const char *name,
 			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
 			if (ret && only_to_die) {
 				diagnose_invalid_sha1_path(prefix, filename,
-							   tree_sha1, object_name);
-				free(object_name);
+							   tree_sha1,
+							   name, len);
 			}
 			hashcpy(oc->tree, tree_sha1);
 			strncpy(oc->path, filename,
@@ -1360,7 +1356,7 @@ static int get_sha1_with_context_1(const char *name,
 			return ret;
 		} else {
 			if (only_to_die)
-				die("Invalid object name '%s'.", object_name);
+				die("Invalid object name '%.*s'.", len, name);
 		}
 	}
 	return ret;
-- 
1.8.2

Re: [PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> The only caller of diagnose_invalid_sha1_path() extracts a substring from
> an object name by creating a NUL-terminated copy of the interesting part.
> Add a length parameter to the function and thus avoid the need for an
> allocation, thereby simplifying the code.
>
> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
>  sha1_name.c | 32 ++++++++++++++------------------
>  1 file changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 95003c7..4cea6d3 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -1137,7 +1137,8 @@ int get_sha1_blob(const char *name, unsigned char *sha1)
>  static void diagnose_invalid_sha1_path(const char *prefix,
>  				       const char *filename,
>  				       const unsigned char *tree_sha1,
> -				       const char *object_name)
> +				       const char *object_name,
> +				       int object_name_len)
>  {
>  	struct stat st;
>  	unsigned char sha1[20];
> @@ -1147,8 +1148,8 @@ static void diagnose_invalid_sha1_path(const char *prefix,
>  		prefix = "";
>  
>  	if (!lstat(filename, &st))
> -		die("Path '%s' exists on disk, but not in '%s'.",
> -		    filename, object_name);
> +		die("Path '%s' exists on disk, but not in '%.*s'.",
> +		    filename, object_name_len, object_name);
>  	if (errno == ENOENT || errno == ENOTDIR) {
>  		char *fullname = xmalloc(strlen(filename)
>  					     + strlen(prefix) + 1);
> @@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix,
>  		if (!get_tree_entry(tree_sha1, fullname,
>  				    sha1, &mode)) {
>  			die("Path '%s' exists, but not '%s'.\n"
> -			    "Did you mean '%s:%s' aka '%s:./%s'?",
> +			    "Did you mean '%.*s:%s' aka '.*%.*s:./%s'?",

This is so unlike what I call "Scharfe patch", which I can apply
with my eyes closed and expect everything to be perfect.

Other than that, I see this as a usual "Scharfe patch" ;-)  Will
squash an obvious fix in and apply.

Thanks.

>  			    fullname,
>  			    filename,
> -			    object_name,
> +			    object_name_len, object_name,
>  			    fullname,
> -			    object_name,
> +			    object_name_len, object_name,
>  			    filename);
>  		}
> -		die("Path '%s' does not exist in '%s'",
> -		    filename, object_name);
> +		die("Path '%s' does not exist in '%.*s'",
> +		    filename, object_name_len, object_name);
>  	}
>  }
>  
> @@ -1332,13 +1333,8 @@ static int get_sha1_with_context_1(const char *name,
>  	}
>  	if (*cp == ':') {
>  		unsigned char tree_sha1[20];
> -		char *object_name = NULL;
> -		if (only_to_die) {
> -			object_name = xmalloc(cp-name+1);
> -			strncpy(object_name, name, cp-name);
> -			object_name[cp-name] = '\0';
> -		}
> -		if (!get_sha1_1(name, cp-name, tree_sha1, GET_SHA1_TREEISH)) {
> +		int len = cp - name;
> +		if (!get_sha1_1(name, len, tree_sha1, GET_SHA1_TREEISH)) {
>  			const char *filename = cp+1;
>  			char *new_filename = NULL;
>  
> @@ -1348,8 +1344,8 @@ static int get_sha1_with_context_1(const char *name,
>  			ret = get_tree_entry(tree_sha1, filename, sha1, &oc->mode);
>  			if (ret && only_to_die) {
>  				diagnose_invalid_sha1_path(prefix, filename,
> -							   tree_sha1, object_name);
> -				free(object_name);
> +							   tree_sha1,
> +							   name, len);
>  			}
>  			hashcpy(oc->tree, tree_sha1);
>  			strncpy(oc->path, filename,
> @@ -1360,7 +1356,7 @@ static int get_sha1_with_context_1(const char *name,
>  			return ret;
>  		} else {
>  			if (only_to_die)
> -				die("Invalid object name '%s'.", object_name);
> +				die("Invalid object name '%.*s'.", len, name);
>  		}
>  	}
>  	return ret;

Re: [PATCH] sha1_name: pass object name length to diagnose_invalid_sha1_path()

[René Scharfe]

Am 17.03.2013 08:10, schrieb Junio C Hamano:
>> @@ -1158,16 +1159,16 @@ static void diagnose_invalid_sha1_path(const char *prefix,
>>   		if (!get_tree_entry(tree_sha1, fullname,
>>   				    sha1, &mode)) {
>>   			die("Path '%s' exists, but not '%s'.\n"
>> -			    "Did you mean '%s:%s' aka '%s:./%s'?",
>> +			    "Did you mean '%.*s:%s' aka '.*%.*s:./%s'?",
>
> Will squash an obvious fix in and apply.

Did I try to make a point there?  Certainly not.  It seems I need to go 
back to http://vim-adventures.com/.

Thank you for spotting this!
René