Redefine semantics of find_unique_abbrev()

Redefine semantics of find_unique_abbrev()

[Junio C Hamano]

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)

Re: Redefine semantics of find_unique_abbrev()

[Jeff King]

On Fri, Feb 29, 2008 at 05:41:53PM -0800, Junio C Hamano wrote:

> 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:

> --- 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

Re: Redefine semantics of find_unique_abbrev()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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)

Re: Redefine semantics of find_unique_abbrev()

[Jeff King]

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.

-Peff

Re: Redefine semantics of find_unique_abbrev()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 ;-).

[PATCH] Clean up find_unique_abbrev() callers

[Junio C Hamano]

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];

Re: [PATCH] Clean up find_unique_abbrev() callers

[Jeff King]

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