threads / discuss / 11805

git-push: forced update of tag shows unabbreviated SHA1

Subject: git-push: forced update of tag shows unabbreviated SHA1

## tl;dr

9 messages between Jan 31, 2008 and Jan 31, 2008.

replies: 8people: 3as markdown or json

Johannes Sixt· Jan 31, 2008, 09:27 UTC · lore
This is just a cosmetical flaw:

When a tag is changed to point to a new commit, then the tag is pushed to a repo that still has the old tag, it is correctly pushed, but the old SHA1 is reported with all 40 digits:

# make upstream repo $ mkdir A && cd A $ git init $ echo a > a; git add a; git commit -m a

# clone, modify, push $ cd .. $ git clone A B $ cd B $ echo b > a; git commit -a -m b $ git push file://$PWD/../A

# tag old state in upstream $ cd ../A $ git tag -m T T HEAD~1

# tag new state in clone (same name) and force-push tag
$ cd ../B
$ git tag -m Tclone T
$ git push -f file://$PWD/../A refs/tags/*:refs/tags/*
Counting objects: 1, done.
Unpacking objects: 100% (1/1), done.
Writing objects: 100% (1/1), 160 bytes, done.
Total 1 (delta 0), reused 0 (delta 0)
To file:///home/jsixt/tmp/foo/B/../A
 + 639669ce44f84417f30842c622064827dda01461...475e55f T -> T (forced update)
Notice that the original SHA1 is not abbreviated.
-- Hannes
Junio C Hamano· Jan 31, 2008, 09:37 UTC · re: Johannes Sixt · lore

Re: git-push: forced update of tag shows unabbreviated SHA1

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 6 quoted lines
> This is just a cosmetical flaw:
> ...
> To file:///home/jsixt/tmp/foo/B/../A
>  + 639669ce44f84417f30842c622064827dda01461...475e55f T -> T (forced update)
>
> Notice that the original SHA1 is not abbreviated.

I suspect that is because you do not _have_ the original object, so there is no uniquely usable abbreviation to name the object in your repository.

This obviously is not tested at all (not even compile tested), but I think it would show you what is going on.

---
 builtin-send-pack.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8afb1d0..9c558ee 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -265,7 +265,7 @@ 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 abbrev ? abbrev : "<you do not have it>";
 }
 
 static void print_ok_ref_status(struct ref *ref)
Johannes Sixt· Jan 31, 2008, 10:01 UTC · re: Junio C Hamano · lore

Re: git-push: forced update of tag shows unabbreviated SHA1

Junio C Hamano schrieb:
Show 12 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> This is just a cosmetical flaw:
>> ...
>> To file:///home/jsixt/tmp/foo/B/../A
>>  + 639669ce44f84417f30842c622064827dda01461...475e55f T -> T (forced update)
>>
>> Notice that the original SHA1 is not abbreviated.
> 
> I suspect that is because you do not _have_ the original object,
> so there is no uniquely usable abbreviation to name the object
> in your repository.
Yes, you are right. This also happens for a commit.
Show 18 quoted lines
> This obviously is not tested at all (not even compile tested),
> but I think it would show you what is going on.
> 
> ---
>  builtin-send-pack.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index 8afb1d0..9c558ee 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -265,7 +265,7 @@ 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 abbrev ? abbrev : "<you do not have it>";
>  }

No, that would be information hiding. I prefer an unabbreviated name. Nevertheless, if we know the name, we could be able to find a suitable abbreviation. But it's really not *that* important...

-- Hannes
Jeff King· Jan 31, 2008, 10:06 UTC · re: Johannes Sixt · lore

Re: git-push: forced update of tag shows unabbreviated SHA1

On Thu, Jan 31, 2008 at 10:27:43AM +0100, Johannes Sixt wrote:
> When a tag is changed to point to a new commit, then the tag is pushed to
> a repo that still has the old tag, it is correctly pushed, but the old
> SHA1 is reported with all 40 digits:

As Junio noted, this is because find_unique_abbrev returns NULL for objects we don't have. Actually, it is somewhat worse -- we return an erroneous abbreviation in the rare case that we are trying to find the abbreviation for something we don't have, but for which we do have a matching abbreviation. For example, if we have

  1234567890123456789012345678901234567890

then for every 12345678* that we don't have, we will claim the correct abbreviation is 1234568.

In practice, I doubt this is a problem.

But getting back to your point: yes, I agree it is a little ugly. Rewriting find_unique_abbrev would be necessary for fixing it, and I'm not sure it is worth the trouble.

-Peff
Junio C Hamano· Jan 31, 2008, 10:21 UTC · re: Jeff King · lore

Re: git-push: forced update of tag shows unabbreviated SHA1

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> ... then for every 12345678* that we don't have, we will claim
> the correct abbreviation is 1234568.
>
> In practice, I doubt this is a problem.
>
> But getting back to your point: yes, I agree it is a little ugly.
> Rewriting find_unique_abbrev would be necessary for fixing it, and I'm
> not sure it is worth the trouble.

I think that needs to be done carefully. I recall some callers do expect it to return NULL for nonexistant objects, so the bug you noted above as "rare case" may need to be fixed, which I think is more important than coming up with a potentially too short abbreviation.

Jeff King· Jan 31, 2008, 10:39 UTC · re: Junio C Hamano · lore

Re: git-push: forced update of tag shows unabbreviated SHA1

On Thu, Jan 31, 2008 at 02:21:49AM -0800, Junio C Hamano wrote:
Show 5 quoted lines
> I think that needs to be done carefully.  I recall some callers
> do expect it to return NULL for nonexistant objects, so the bug
> you noted above as "rare case" may need to be fixed, which I
> think is more important than coming up with a potentially too
> short abbreviation.

The patch I just posted just figures out ahead of time whether the object exists. We can easily have it default to returning NULL but allow a special flag for the new behavior. Something like:

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8afb1d0..454ad8f 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -263,9 +263,8 @@ 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_flags(sha1, DEFAULT_ABBREV,
+			FIND_UNIQUE_ABBREV_MISSING);
 }
 
 static void print_ok_ref_status(struct ref *ref)
diff --git a/cache.h b/cache.h
index 7a38511..0bcbf54 100644
--- a/cache.h
+++ b/cache.h
@@ -383,7 +383,14 @@ extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)
 extern char *sha1_file_name(const unsigned char *sha1);
 extern char *sha1_pack_name(const unsigned char *sha1);
 extern char *sha1_pack_index_name(const unsigned char *sha1);
-extern const char *find_unique_abbrev(const unsigned char *sha1, int);
+
+#define FIND_UNIQUE_ABBREV_MISSING 1
+extern const char *find_unique_abbrev_flags(const unsigned char *sha1, int len, int flags);
+static inline const char *find_unique_abbrev(const unsigned char *sha1, int len)
+{
+	return find_unique_abbrev_flags(sha1, len, 0);
+}
+
 extern const unsigned char null_sha1[20];
 static inline int is_null_sha1(const unsigned char *sha1)
 {
diff --git a/sha1_name.c b/sha1_name.c
index a99aff3..666d36c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -190,12 +190,15 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 	return status;
 }
 
-const char *find_unique_abbrev(const unsigned char *sha1, int len)
+const char *find_unique_abbrev_flags(const unsigned char *sha1, int len,
+		int flags)
 {
 	int status, missing;
 	static char hex[41];
 
 	missing = !has_sha1_file(sha1);
+	if (missing && !(flags & FIND_UNIQUE_ABBREV_MISSING))
+		return NULL;
 	memcpy(hex, sha1_to_hex(sha1), 40);
 	if (len == 40 || !len)
 		return hex;
Jeff King· Jan 31, 2008, 10:27 UTC · re: Jeff King · lore

Re: git-push: forced update of tag shows unabbreviated SHA1

On Thu, Jan 31, 2008 at 05:06:25AM -0500, Jeff King wrote:
> But getting back to your point: yes, I agree it is a little ugly.
> Rewriting find_unique_abbrev would be necessary for fixing it, and I'm
> not sure it is worth the trouble.

Actually, it looks like we already handle a similar case: the null sha1, so the change isn't that big (and the null sha1 case folds nicely into the "missing" case). With the (lightly tested) patch below, find_unique_abbrev _always_ returns an answer, and for missing objects will show enough characters that it doesn't match any existing object.

diff --git a/sha1_name.c b/sha1_name.c
index 13e1164..c0e6af1 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -192,26 +192,24 @@ 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 (!status ||
-		    (is_null && status != SHORT_NAME_AMBIGUOUS)) {
+		if ((!missing && !status) ||
+		    (missing && 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)
Junio C Hamano· Jan 31, 2008, 10:38 UTC · re: Jeff King · lore

Re: git-push: forced update of tag shows unabbreviated SHA1

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> On Thu, Jan 31, 2008 at 05:06:25AM -0500, Jeff King wrote:
>
>> But getting back to your point: yes, I agree it is a little ugly.
>> Rewriting find_unique_abbrev would be necessary for fixing it, and I'm
>> not sure it is worth the trouble.
>
> Actually, it looks like we already handle a similar case: the null sha1,
> so the change isn't that big (and the null sha1 case folds nicely into
> the "missing" case).

Heh, "Make sure to abbreviate something that does not match" --- I forgot about that trick I did looooooooong time ago ;-).

I like the idea.  Does it not break anything?
Jeff King· Jan 31, 2008, 10:41 UTC · re: Junio C Hamano · lore

Re: git-push: forced update of tag shows unabbreviated SHA1

On Thu, Jan 31, 2008 at 02:38:37AM -0800, Junio C Hamano wrote:
Show 8 quoted lines
> > Actually, it looks like we already handle a similar case: the null sha1,
> > so the change isn't that big (and the null sha1 case folds nicely into
> > the "missing" case).
> 
> Heh, "Make sure to abbreviate something that does not match" ---
> I forgot about that trick I did looooooooong time ago ;-).
> 
> I like the idea.  Does it not break anything?

No idea. :) I will hold onto it and submit a cleaned up version post-1.5.4.

-Peff

← back to recent threads