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

2013-05-07 Thread Junio C Hamano 
Nguyễn Thái Ngọc Duy   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)" = 0
>  '
>  
> +test_expect_success 'ambiguous 40-hex ref' '
> + TREE=$(git mktree  + REF=`git rev-parse HEAD` &&
> + VAL=$(git commit-tree $TREE  + 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  + REF=`git rev-parse --short HEAD` &&
> + VAL=$(git commit-tree $TREE  + git update-ref refs/heads/$REF $VAL &&
> + test `git rev-parse $REF 2>err` = $VAL &&
> + grep "refname.*${REF}.*ambiguous" err
> +'
> +
>  test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-05-03 Thread 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 
---
 On Thu, May 2, 2013 at 1:43 AM, Jonathan Nieder  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)" = 0
 '
 
+test_expect_success 'ambiguous 40-hex ref' '
+   TREE=$(git mktree err` = $REF &&
+   grep "refname.*${REF}.*ambiguous" err
+'
+
+test_expect_success 'ambiguous short sha1 ref' '
+   TREE=$(git mktree err` = $VAL &&
+   grep "refname.*${REF}.*ambiguous" err
+'
+
 test_done
-- 
1.8.2.83.gc99314b

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html