[PATCH/RFC] get_sha1: prefer 40-hex ref name over 40-hex SHA-1

[PATCH/RFC] get_sha1: prefer 40-hex ref name over 40-hex SHA-1

[Nguyễn Thái Ngọc Duy]

The current behavior is inconsistent when passing SHA-1 to get_sha1.
If it's a short sha-1, refs take precedence. "git rev-parse 1234" will
resolve refs/heads/1234 if exists even if there is an unambiguous
SHA-1 starting with 1234. However if it's full SHA-1, the SHA-1 takes
precedence and refs with the same name are ignored.

The former makes more sense than the latter. This patch makes git
check for 40-hex ref names before consider it SHA-1. In future, we may
want to warn ambiguity between refs and SHA-1 (for both full and short
SHA-1).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Some gray area where my "makes more sense" may not actually be common
 sense.

 sha1_name.c                         | 13 +++++++++++--
 t/t1512-rev-parse-disambiguation.sh | 15 +++++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..faf10b4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -436,11 +436,20 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
 	static const char *warn_msg = "refname '%.*s' is ambiguous.";
 	char *real_ref = NULL;
-	int refs_found = 0;
+	int refs_found;
 	int at, reflog_len;
 
-	if (len == 40 && !get_sha1_hex(str, sha1))
+	if (len == 40 && !get_sha1_hex(str, sha1)) {
+		unsigned char ref_sha1[20];
+		refs_found = dwim_ref(str, len, ref_sha1, &real_ref);
+		if (refs_found > 0) {
+			if (warn_ambiguous_refs && refs_found > 1)
+				warning(warn_msg, len, str);
+			hashcpy(sha1, ref_sha1);
+		}
+		free(real_ref);
 		return 0;
+	}
 
 	/* basic@{time or number or -number} format to query ref-log */
 	reflog_len = at = 0;
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 6b3d797..97ff8ac 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -261,4 +261,19 @@ test_expect_success 'rev-parse --disambiguate' '
 	test "$(sed -e "s/^\(.........\).*/\1/" actual | sort -u)" = 000000000
 '
 
+test_expect_success 'rev-parse 20-hex ref' '
+	REF=`git rev-parse HEAD` &&
+	VAL=`echo| git commit-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904` &&
+	git update-ref refs/heads/$REF $VAL &&
+	test `git rev-parse $REF` = $VAL
+'
+
+test_expect_success 'rev-parse ambiguous 20-hex ref' '
+	REF=`git rev-parse HEAD` &&
+	VAL=`echo| git commit-tree -p HEAD 4b825dc642cb6eb9a060e54bf8d69288fbee4904` &&
+	git update-ref refs/tags/$REF $VAL &&
+	test `git rev-parse $REF 2>err` = $VAL &&
+	grep "refname.*ambiguous" err
+'
+
 test_done
-- 
1.8.2.83.gc99314b

Re: [PATCH/RFC] get_sha1: prefer 40-hex ref name over 40-hex SHA-1

[Eric Sunshine]

On Tue, Apr 30, 2013 at 11:01 PM, Nguyễn Thái Ngọc Duy
<pclouds@gmail.com> wrote:
> The current behavior is inconsistent when passing SHA-1 to get_sha1.
> If it's a short sha-1, refs take precedence. "git rev-parse 1234" will
> resolve refs/heads/1234 if exists even if there is an unambiguous
> SHA-1 starting with 1234. However if it's full SHA-1, the SHA-1 takes
> precedence and refs with the same name are ignored.
>
> The former makes more sense than the latter. This patch makes git
> check for 40-hex ref names before consider it SHA-1. In future, we may
> want to warn ambiguity between refs and SHA-1 (for both full and short

s/warn/warn about/

> SHA-1).
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Re: [PATCH/RFC] get_sha1: prefer 40-hex ref name over 40-hex SHA-1

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

>                                              "git rev-parse 1234" will
> resolve refs/heads/1234 if exists even if there is an unambiguous
> SHA-1 starting with 1234. However if it's full SHA-1, the SHA-1 takes
> precedence and refs with the same name are ignored.

That's an important feature for safety.  When a script has created an
object or learned about it some other way, as long as it doesn't
abbreviate its name it can be sure that git commands will not
misunderstand it.

So I think this is a bad change.  Maybe check-ref-format should
reject 40-hexdigit refnames?

[...]
> --- a/t/t1512-rev-parse-disambiguation.sh
> +++ b/t/t1512-rev-parse-disambiguation.sh
> @@ -261,4 +261,19 @@ test_expect_success 'rev-parse --disambiguate' '
>  	test "$(sed -e "s/^\(.........\).*/\1/" actual | sort -u)" = 000000000
>  '
>  
> +test_expect_success 'rev-parse 20-hex ref' '
> +	REF=`git rev-parse HEAD` &&
> +	VAL=`echo| git commit-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904` &&
> +	git update-ref refs/heads/$REF $VAL &&
> +	test `git rev-parse $REF` = $VAL
> +'

This is a good thing to test.  Nit: outside of t0000, please use

	empty_tree=$(git mktree </dev/null) &&

instead of hard-coding the hash.  Otherwise you are making my life
hard when I write md5git. :)  And more importantly, this makes the
meaning of the test easier to understand by reading it.

Thanks,
Jonathan

Re: [PATCH/RFC] get_sha1: prefer 40-hex ref name over 40-hex SHA-1

[Thomas Rast]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> +test_expect_success 'rev-parse 20-hex ref' '
> +	REF=`git rev-parse HEAD` &&
> +	VAL=`echo| git commit-tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904` &&
> +	git update-ref refs/heads/$REF $VAL &&
> +	test `git rev-parse $REF` = $VAL
> +'
> +
> +test_expect_success 'rev-parse ambiguous 20-hex ref' '
> +	REF=`git rev-parse HEAD` &&
> +	VAL=`echo| git commit-tree -p HEAD 4b825dc642cb6eb9a060e54bf8d69288fbee4904` &&
> +	git update-ref refs/tags/$REF $VAL &&
> +	test `git rev-parse $REF 2>err` = $VAL &&
> +	grep "refname.*ambiguous" err
> +'

Shouldn't these say s/20-hex/40-hex/?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH/RFC] get_sha1: prefer 40-hex ref name over 40-hex SHA-1

[Duy Nguyen]

On Thu, May 2, 2013 at 1:43 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyễn Thái Ngọc Duy wrote:
>
>>                                              "git rev-parse 1234" will
>> resolve refs/heads/1234 if exists even if there is an unambiguous
>> SHA-1 starting with 1234. However if it's full SHA-1, the SHA-1 takes
>> precedence and refs with the same name are ignored.
>
> That's an important feature for safety.  When a script has created an
> object or learned about it some other way, as long as it doesn't
> abbreviate its name it can be sure that git commands will not
> misunderstand it.

Then what about abbrev sha-1? You pick up an abbrev sha-1 from "git
log --oneline", do "git show <abbrev>". If you happen to have
refs/heads/<abbrev> then what you see is not what you intend to see.
Warn about ambiguity and move on? Banning all sha1-alike refs seems
unreasonable (or not? I don't know).

> So I think this is a bad change.  Maybe check-ref-format should
> reject 40-hexdigit refnames?

I tried that first. The test suite screamed. Will do that again and
figure out soon.
--
Duy

[PATCH] get_sha1: improve ambiguity warning regarding SHA-1 and ref names

[Nguyễn Thái Ngọc Duy]

When we get 40 hex digits, we immediately assume it's an SHA-1. Warn
about ambiguity if there's also refs/heads/$sha1 (or similar) on system.

When we successfully resolve a ref like "1234abc" and "1234abc"
happens to be valid abbreviated SHA-1 on system, warn also.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Thu, May 2, 2013 at 1:43 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
 > Nguyễn Thái Ngọc Duy wrote:
 >
 >>                                              "git rev-parse 1234" will
 >> resolve refs/heads/1234 if exists even if there is an unambiguous
 >> SHA-1 starting with 1234. However if it's full SHA-1, the SHA-1 takes
 >> precedence and refs with the same name are ignored.
 >
 > That's an important feature for safety.  When a script has created an
 > object or learned about it some other way, as long as it doesn't
 > abbreviate its name it can be sure that git commands will not
 > misunderstand it.
 >
 > So I think this is a bad change.  Maybe check-ref-format should
 > reject 40-hexdigit refnames?

 We can't, at least not in a simple way, because there are 40-hex
 digit refs in refs/replace. I think warning only is a simpler
 approach to this minor issue.

 sha1_name.c                         | 12 ++++++++++--
 t/t1512-rev-parse-disambiguation.sh | 18 ++++++++++++++++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..04a9fbe 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -435,12 +435,18 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
 	static const char *warn_msg = "refname '%.*s' is ambiguous.";
+	unsigned char tmp_sha1[20];
 	char *real_ref = NULL;
 	int refs_found = 0;
 	int at, reflog_len;
 
-	if (len == 40 && !get_sha1_hex(str, sha1))
+	if (len == 40 && !get_sha1_hex(str, sha1)) {
+		refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);
+		if (refs_found > 0 && warn_ambiguous_refs)
+			warning(warn_msg, len, str);
+		free(real_ref);
 		return 0;
+	}
 
 	/* basic@{time or number or -number} format to query ref-log */
 	reflog_len = at = 0;
@@ -481,7 +487,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (!refs_found)
 		return -1;
 
-	if (warn_ambiguous_refs && refs_found > 1)
+	if (warn_ambiguous_refs &&
+	    (refs_found > 1 ||
+	     !get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY)))
 		warning(warn_msg, len, str);
 
 	if (reflog_len) {
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 6b3d797..db22808 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -261,4 +261,22 @@ test_expect_success 'rev-parse --disambiguate' '
 	test "$(sed -e "s/^\(.........\).*/\1/" actual | sort -u)" = 000000000
 '
 
+test_expect_success 'ambiguous 40-hex ref' '
+	TREE=$(git mktree </dev/null) &&
+	REF=`git rev-parse HEAD` &&
+	VAL=$(git commit-tree $TREE </dev/null) &&
+	git update-ref refs/heads/$REF $VAL &&
+	test `git rev-parse $REF 2>err` = $REF &&
+	grep "refname.*${REF}.*ambiguous" err
+'
+
+test_expect_success 'ambiguous short sha1 ref' '
+	TREE=$(git mktree </dev/null) &&
+	REF=`git rev-parse --short HEAD` &&
+	VAL=$(git commit-tree $TREE </dev/null) &&
+	git update-ref refs/heads/$REF $VAL &&
+	test `git rev-parse $REF 2>err` = $VAL &&
+	grep "refname.*${REF}.*ambiguous" err
+'
+
 test_done
-- 
1.8.2.83.gc99314b

Re: [PATCH] get_sha1: improve ambiguity warning regarding SHA-1 and ref names

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

>  > That's an important feature for safety. When a script has created an
>  > object or learned about it some other way, as long as it doesn't
>  > abbreviate its name it can be sure that git commands will not
>  > misunderstand it.
>  >
>  > So I think this is a bad change.
> ...
>  static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>  {
>  	static const char *warn_msg = "refname '%.*s' is ambiguous.";
> +	unsigned char tmp_sha1[20];
>  	char *real_ref = NULL;
>  	int refs_found = 0;
>  	int at, reflog_len;
>  
> -	if (len == 40 && !get_sha1_hex(str, sha1))
> +	if (len == 40 && !get_sha1_hex(str, sha1)) {
> +		refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);
> +		if (refs_found > 0 && warn_ambiguous_refs)
> +			warning(warn_msg, len, str);

The warning is issued at the right spot from the codeflow's point of
view, but it is very likely that the user did not even mean to use
the str in question as a 'refname'. The warning message we see above
is not appropriate for this case, is it?

> +		free(real_ref);
>  		return 0;
> +	}
>  
>  	/* basic@{time or number or -number} format to query ref-log */
>  	reflog_len = at = 0;
> @@ -481,7 +487,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>  	if (!refs_found)
>  		return -1;
>  
> -	if (warn_ambiguous_refs && refs_found > 1)
> +	if (warn_ambiguous_refs &&
> +	    (refs_found > 1 ||
> +	     !get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY)))
>  		warning(warn_msg, len, str);

Ditto for the case in which (refs_found <= 1) and get_short_sha1()
finds str as a short object name.


> diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
> index 6b3d797..db22808 100755
> --- a/t/t1512-rev-parse-disambiguation.sh
> +++ b/t/t1512-rev-parse-disambiguation.sh
> @@ -261,4 +261,22 @@ test_expect_success 'rev-parse --disambiguate' '
>  	test "$(sed -e "s/^\(.........\).*/\1/" actual | sort -u)" = 000000000
>  '
>  
> +test_expect_success 'ambiguous 40-hex ref' '
> +	TREE=$(git mktree </dev/null) &&
> +	REF=`git rev-parse HEAD` &&
> +	VAL=$(git commit-tree $TREE </dev/null) &&
> +	git update-ref refs/heads/$REF $VAL &&
> +	test `git rev-parse $REF 2>err` = $REF &&
> +	grep "refname.*${REF}.*ambiguous" err
> +'
> +
> +test_expect_success 'ambiguous short sha1 ref' '
> +	TREE=$(git mktree </dev/null) &&
> +	REF=`git rev-parse --short HEAD` &&
> +	VAL=$(git commit-tree $TREE </dev/null) &&
> +	git update-ref refs/heads/$REF $VAL &&
> +	test `git rev-parse $REF 2>err` = $VAL &&
> +	grep "refname.*${REF}.*ambiguous" err
> +'
> +
>  test_done

[PATCH v2] get_sha1: warn about full or short object names that look like refs

[Nguyễn Thái Ngọc Duy]

When we get 40 hex digits, we immediately assume it's an SHA-1. This
is the right thing to do because we have no way else to specify an
object. If there is a ref with the same object name, it will be
ignored. Warn the user about this case because the ref with full
object name is likely a mistake, for example

    git checkout -b $empty_var $(git rev-parse something)

advice.object_name_warning is not documented because frankly people
should not be aware about it until they encounter this situation.

While at there, warn about ambiguation with abbreviated SHA-1 too.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 advice.c                            |  2 ++
 advice.h                            |  1 +
 sha1_name.c                         | 25 +++++++++++++++++++++++--
 t/t1512-rev-parse-disambiguation.sh | 18 ++++++++++++++++++
 4 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/advice.c b/advice.c
index 780f58d..22abde9 100644
--- a/advice.c
+++ b/advice.c
@@ -12,6 +12,7 @@ int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
 int advice_implicit_identity = 1;
 int advice_detached_head = 1;
+int advice_object_name_warning = 1;
 
 static struct {
 	const char *name;
@@ -29,6 +30,7 @@ static struct {
 	{ "resolveconflict", &advice_resolve_conflict },
 	{ "implicitidentity", &advice_implicit_identity },
 	{ "detachedhead", &advice_detached_head },
+	{ "object_name_warning", &advice_object_name_warning },
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index fad36df..24d5420 100644
--- a/advice.h
+++ b/advice.h
@@ -15,6 +15,7 @@ extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
 extern int advice_implicit_identity;
 extern int advice_detached_head;
+extern int advice_object_name_warning;
 
 int git_default_advice_config(const char *var, const char *value);
 void advise(const char *advice, ...);
diff --git a/sha1_name.c b/sha1_name.c
index 95003c7..502d107 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -435,12 +435,31 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
 static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 {
 	static const char *warn_msg = "refname '%.*s' is ambiguous.";
+	static const char *object_name_msg = N_(
+	"Git normally never creates a ref that ends with 40 hex characters\n"
+	"because it will be ignored when you just specify 40-hex. These refs\n"
+	"may be created by mistake. For example,\n"
+	"\n"
+	"  git checkout -b $br $(git rev-parse ...)\n"
+	"\n"
+	"where \"$br\" is somehow empty and a 40-hex ref is created. Please\n"
+	"examine these refs and maybe delete them. Turn this message off by\n"
+	"running \"git config advice.object_name_warning false\"");
+	unsigned char tmp_sha1[20];
 	char *real_ref = NULL;
 	int refs_found = 0;
 	int at, reflog_len;
 
-	if (len == 40 && !get_sha1_hex(str, sha1))
+	if (len == 40 && !get_sha1_hex(str, sha1)) {
+		refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);
+		if (refs_found > 0 && warn_ambiguous_refs) {
+			warning(warn_msg, len, str);
+			if (advice_object_name_warning)
+				fprintf(stderr, "%s\n", _(object_name_msg));
+		}
+		free(real_ref);
 		return 0;
+	}
 
 	/* basic@{time or number or -number} format to query ref-log */
 	reflog_len = at = 0;
@@ -481,7 +500,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
 	if (!refs_found)
 		return -1;
 
-	if (warn_ambiguous_refs && refs_found > 1)
+	if (warn_ambiguous_refs &&
+	    (refs_found > 1 ||
+	     !get_short_sha1(str, len, tmp_sha1, GET_SHA1_QUIETLY)))
 		warning(warn_msg, len, str);
 
 	if (reflog_len) {
diff --git a/t/t1512-rev-parse-disambiguation.sh b/t/t1512-rev-parse-disambiguation.sh
index 6b3d797..db22808 100755
--- a/t/t1512-rev-parse-disambiguation.sh
+++ b/t/t1512-rev-parse-disambiguation.sh
@@ -261,4 +261,22 @@ test_expect_success 'rev-parse --disambiguate' '
 	test "$(sed -e "s/^\(.........\).*/\1/" actual | sort -u)" = 000000000
 '
 
+test_expect_success 'ambiguous 40-hex ref' '
+	TREE=$(git mktree </dev/null) &&
+	REF=`git rev-parse HEAD` &&
+	VAL=$(git commit-tree $TREE </dev/null) &&
+	git update-ref refs/heads/$REF $VAL &&
+	test `git rev-parse $REF 2>err` = $REF &&
+	grep "refname.*${REF}.*ambiguous" err
+'
+
+test_expect_success 'ambiguous short sha1 ref' '
+	TREE=$(git mktree </dev/null) &&
+	REF=`git rev-parse --short HEAD` &&
+	VAL=$(git commit-tree $TREE </dev/null) &&
+	git update-ref refs/heads/$REF $VAL &&
+	test `git rev-parse $REF 2>err` = $VAL &&
+	grep "refname.*${REF}.*ambiguous" err
+'
+
 test_done
-- 
1.8.2.83.gc99314b