git-push: forced update of tag shows unabbreviated SHA1

git-push: forced update of tag shows unabbreviated SHA1

[Johannes Sixt]

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

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

[Junio C Hamano]

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.

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)

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

[Johannes Sixt]

Junio C Hamano schrieb:
> 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.

> 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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

On Thu, Jan 31, 2008 at 02:21:49AM -0800, Junio C Hamano wrote:

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

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

[Jeff King]

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)

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

On Thu, Jan 31, 2008 at 02:38:37AM -0800, Junio C Hamano wrote:

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