threads / discuss / 12390

Redefine semantics of find_unique_abbrev()

Subject: Redefine semantics of find_unique_abbrev()

## tl;dr

7 messages between Mar 1, 2008 and Mar 2, 2008.

replies: 6people: 2as markdown or json

Junio C Hamano· Mar 1, 2008, 01:41 UTC · lore

The function returned NULL when no object that matches the name was found, but that made the callers more complicated, as nobody used that NULL return as an indication that no object with such a name exists. They (at least the careful ones) instead took the full 40-hexdigit and used in such a case, and the careless ones segfaulted.

With this "git rev-parse --short 5555555555555555555555555555555555555555" would stop segfaulting.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-reset.c     |    4 ----
 builtin-send-pack.c |    4 +---
 diff.c              |    2 --
 sha1_name.c         |    8 +++-----
 4 files changed, 4 insertions(+), 14 deletions(-)
diff --git a/builtin-reset.c b/builtin-reset.c
index af0037e..be58a8b 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -66,10 +66,6 @@ static void print_new_head_line(struct commit *commit)
 	const char *hex, *dots = "...", *body;
 
 	hex = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	if (!hex) {
-		hex = sha1_to_hex(commit->object.sha1);
-		dots = "";
-	}
 	printf("HEAD is now at %s%s", hex, dots);
 	body = strstr(commit->buffer, "\n\n");
 	if (body) {
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index b0cfae8..930e0fb 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -264,9 +264,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
 
 static const char *status_abbrev(unsigned char sha1[20])
 {
-	const char *abbrev;
-	abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
-	return abbrev ? abbrev : sha1_to_hex(sha1);
+	return find_unique_abbrev(sha1, DEFAULT_ABBREV);
 }
 
 static void print_ok_ref_status(struct ref *ref)
diff --git a/diff.c b/diff.c
index ad16164..b80f656 100644
--- a/diff.c
+++ b/diff.c
@@ -2581,8 +2581,6 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)
 		return sha1_to_hex(sha1);
 
 	abbrev = find_unique_abbrev(sha1, len);
-	if (!abbrev)
-		return sha1_to_hex(sha1);
 	abblen = strlen(abbrev);
 	if (abblen < 37) {
 		static char hex[41];
diff --git a/sha1_name.c b/sha1_name.c
index 9d088cc..05c2e7a 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -202,16 +202,14 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
 	while (len < 40) {
 		unsigned char sha1_ret[20];
 		status = get_short_sha1(hex, len, sha1_ret, 1);
-		if (!status ||
-		    (is_null && status != SHORT_NAME_AMBIGUOUS)) {
+		if ((!is_null && !status) ||
+		    (is_null && status == SHORT_NAME_NOT_FOUND)) {
 			hex[len] = 0;
 			return hex;
 		}
-		if (status != SHORT_NAME_AMBIGUOUS)
-			return NULL;
 		len++;
 	}
-	return NULL;
+	return hex;
 }
 
 static int ambiguous_path(const char *path, int len)
Jeff King· Mar 1, 2008, 05:06 UTC · re: Junio C Hamano · lore

Re: Redefine semantics of find_unique_abbrev()

On Fri, Feb 29, 2008 at 05:41:53PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> The function returned NULL when no object that matches the name
> was found, but that made the callers more complicated, as nobody
> used that NULL return as an indication that no object with such
> a name exists.  They (at least the careful ones) instead took
> the full 40-hexdigit and used in such a case, and the careless
> ones segfaulted.
> 
> With this "git rev-parse --short 5555555555555555555555555555555555555555"
> would stop segfaulting.

I have been meaning to clean up and submit a similar patch from the 1.5.4 freeze period. However, your patch will always print the 40-hexdigit version, which looks quite ugly in status output. Instead, we can do much better by finding the longest subsequence we _do_ know about, and adding one digit to it.

So you get:
  # this is the current master, abbreviated
  $ git rev-parse --short 97b97c58e609a1cd23b3c2514f41cdb0405870ee
  97b97c5
  # this is an object we don't have, but similar to that
  $ git rev-parse --short 97b97c58e609a1cd23b3c2514f41cdb0405870ff
  97b97c58e609a1cd23b3c2514f41cdb0405870f
  # and less similar means we can abbreviate more
  $ git rev-parse --short 97b97c58e609a1cd23b3c2514fffffffffffffff
  97b97c58e609a1cd23b3c2514ff
Implementation is below:
Show 20 quoted lines
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -202,16 +202,14 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
>  	while (len < 40) {
>  		unsigned char sha1_ret[20];
>  		status = get_short_sha1(hex, len, sha1_ret, 1);
> -		if (!status ||
> -		    (is_null && status != SHORT_NAME_AMBIGUOUS)) {
> +		if ((!is_null && !status) ||
> +		    (is_null && status == SHORT_NAME_NOT_FOUND)) {
>  			hex[len] = 0;
>  			return hex;
>  		}
> -		if (status != SHORT_NAME_AMBIGUOUS)
> -			return NULL;
>  		len++;
>  	}
> -	return NULL;
> +	return hex;
>  }

All we have to do is treat the "missing" case like we treat "is_null" (and in fact, is_null is just a specialized case of something we don't have). So replace this hunk with:

diff --git a/sha1_name.c b/sha1_name.c
index 05c2e7a..9c71d1b 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -192,18 +192,18 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 
 const char *find_unique_abbrev(const unsigned char *sha1, int len)
 {
-	int status, is_null;
+	int status, missing;
 	static char hex[41];
 
-	is_null = is_null_sha1(sha1);
+	missing = !has_sha1_file(sha1);
 	memcpy(hex, sha1_to_hex(sha1), 40);
 	if (len == 40 || !len)
 		return hex;
 	while (len < 40) {
 		unsigned char sha1_ret[20];
 		status = get_short_sha1(hex, len, sha1_ret, 1);
-		if ((!is_null && !status) ||
-		    (is_null && status == SHORT_NAME_NOT_FOUND)) {
+		if ((!missing && !status) ||
+		    (missing && status == SHORT_NAME_NOT_FOUND)) {
 			hex[len] = 0;
 			return hex;
 		}


-Peff
Junio C Hamano· Mar 2, 2008, 07:35 UTC · re: Jeff King · lore

Re: Redefine semantics of find_unique_abbrev()

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> I have been meaning to clean up and submit a similar patch from the
> 1.5.4 freeze period. However, your patch will always print the
> 40-hexdigit version, which looks quite ugly in status output. Instead,
> we can do much better by finding the longest subsequence we _do_ know
> about, and adding one digit to it.

Here is what I ended up with. Instead of saying "missing", I said "exists", which I think makes the logic easier to read, at least to me.

That is, "for objects we have, make sure it uniquely identifies, otherwise, make sure the phoney name is long enough such that it would not name any existing object".

---
 sha1_name.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 9d088cc..8358ba2 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -192,26 +192,25 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 
 const char *find_unique_abbrev(const unsigned char *sha1, int len)
 {
-	int status, is_null;
+	int status, exists;
 	static char hex[41];
 
-	is_null = is_null_sha1(sha1);
+	exists = has_sha1_file(sha1);
 	memcpy(hex, sha1_to_hex(sha1), 40);
 	if (len == 40 || !len)
 		return hex;
 	while (len < 40) {
 		unsigned char sha1_ret[20];
 		status = get_short_sha1(hex, len, sha1_ret, 1);
-		if (!status ||
-		    (is_null && status != SHORT_NAME_AMBIGUOUS)) {
+		if (exists
+		    ? !status
+		    : status == SHORT_NAME_NOT_FOUND) {
 			hex[len] = 0;
 			return hex;
 		}
-		if (status != SHORT_NAME_AMBIGUOUS)
-			return NULL;
 		len++;
 	}
-	return NULL;
+	return hex;
 }
 
 static int ambiguous_path(const char *path, int len)
Jeff King· Mar 2, 2008, 07:42 UTC · re: Junio C Hamano · lore

Re: Redefine semantics of find_unique_abbrev()

On Sat, Mar 01, 2008 at 11:35:11PM -0800, Junio C Hamano wrote:
> That is, "for objects we have, make sure it uniquely identifies,
> otherwise, make sure the phoney name is long enough such that it would not
> name any existing object".

I think your logic is correct, and I think naming it 'exists' is more readable (I don't have a tendency not to double-negate).

But...
Show 8 quoted lines
> -		if (!status ||
> -		    (is_null && status != SHORT_NAME_AMBIGUOUS)) {
> +		if (exists
> +		    ? !status
> +		    : status == SHORT_NAME_NOT_FOUND) {
>  			hex[len] = 0;
>  			return hex;
>  		}

Maybe it is just me, but I find the ternary operator here reduces readability. I would have liked the more verbose:

  if ((exists && !status) ||
      (!exists && status == SHORT_NAME_NOT_FOUND)) {
But now I am just painting your bikeshed.
-Peff
Junio C Hamano· Mar 2, 2008, 08:11 UTC · re: Jeff King · lore

Re: Redefine semantics of find_unique_abbrev()

Jeff King <peff@peff.net> writes:
Show 27 quoted lines
> On Sat, Mar 01, 2008 at 11:35:11PM -0800, Junio C Hamano wrote:
>
>> That is, "for objects we have, make sure it uniquely identifies,
>> otherwise, make sure the phoney name is long enough such that it would not
>> name any existing object".
>
> I think your logic is correct, and I think naming it 'exists' is more
> readable (I don't have a tendency not to double-negate).
>
> But...
>
>> -		if (!status ||
>> -		    (is_null && status != SHORT_NAME_AMBIGUOUS)) {
>> +		if (exists
>> +		    ? !status
>> +		    : status == SHORT_NAME_NOT_FOUND) {
>>  			hex[len] = 0;
>>  			return hex;
>>  		}
>
> Maybe it is just me, but I find the ternary operator here reduces
> readability. I would have liked the more verbose:
>
>   if ((exists && !status) ||
>       (!exists && status == SHORT_NAME_NOT_FOUND)) {
>
> But now I am just painting your bikeshed.

Heh, the ternary is a mini "if-then-else" by itself. Turn your head sideways (just like you do when you meet a smiley) and the parse tree will jump at you ;-).

Junio C Hamano· Mar 2, 2008, 07:49 UTC · re: Jeff King · lore

[PATCH] Clean up find_unique_abbrev() callers

Now find_unique_abbrev() never returns NULL, there is no need for callers to prepare for seeing NULL and fall back to giving the full 40 hexdigits.

While we are at it, drop "..." in the "git reset" output that reports the location of the new HEAD, between the abbreviated commit object name and the one line commit summary. Because we are always showing the HEAD (which cannot be missing!), we never had a case where we show the full 40 hexdigits that is not followed by three dots, and these three dots were stealing 3 columns from the precious horizontal screen real estate out of 80 that can better be used for the one line commit summary.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-reset.c     |    8 ++------
 builtin-send-pack.c |    4 +---
 diff.c              |    2 --
 3 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/builtin-reset.c b/builtin-reset.c
index af0037e..bb3e192 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -63,14 +63,10 @@ static int reset_index_file(const unsigned char *sha1, int is_hard_reset)
 
 static void print_new_head_line(struct commit *commit)
 {
-	const char *hex, *dots = "...", *body;
+	const char *hex, *body;
 
 	hex = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	if (!hex) {
-		hex = sha1_to_hex(commit->object.sha1);
-		dots = "";
-	}
-	printf("HEAD is now at %s%s", hex, dots);
+	printf("HEAD is now at %s", hex);
 	body = strstr(commit->buffer, "\n\n");
 	if (body) {
 		const char *eol;
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index b0cfae8..930e0fb 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -264,9 +264,7 @@ static void print_ref_status(char flag, const char *summary, struct ref *to, str
 
 static const char *status_abbrev(unsigned char sha1[20])
 {
-	const char *abbrev;
-	abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
-	return abbrev ? abbrev : sha1_to_hex(sha1);
+	return find_unique_abbrev(sha1, DEFAULT_ABBREV);
 }
 
 static void print_ok_ref_status(struct ref *ref)
diff --git a/diff.c b/diff.c
index ad16164..b80f656 100644
--- a/diff.c
+++ b/diff.c
@@ -2581,8 +2581,6 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)
 		return sha1_to_hex(sha1);
 
 	abbrev = find_unique_abbrev(sha1, len);
-	if (!abbrev)
-		return sha1_to_hex(sha1);
 	abblen = strlen(abbrev);
 	if (abblen < 37) {
 		static char hex[41];
Jeff King· Mar 2, 2008, 07:51 UTC · re: Junio C Hamano · lore

Re: [PATCH] Clean up find_unique_abbrev() callers

On Sat, Mar 01, 2008 at 11:49:10PM -0800, Junio C Hamano wrote:
>  3 files changed, 3 insertions(+), 11 deletions(-)
I like it. :)
-Peff

← back to recent threads